fix(office): stop duplicate and missed children-completed run dispatches (#3620)
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -0,0 +1,421 @@
|
||||
---
|
||||
created: 2026-09-09
|
||||
status: implemented
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-001
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-003
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation Plan: Parent Wake Wave Identity
|
||||
|
||||
## Overview
|
||||
|
||||
Regenerated 2026-09-09 against the frozen spec (round-2 amend). The prior
|
||||
plan under this directory (session `d0842a54`) predates both spec amends: it
|
||||
had one column/encoding, a wider wave-member predicate, one classification
|
||||
site, and no coalescing exclusion or payload-parity obligation. This
|
||||
supersedes it in full.
|
||||
|
||||
Give the `task_children_completed` wake a single durable identity, computed
|
||||
identically by all four producers, persisted on the run in two encodings, and
|
||||
compared by the backstop instead of a timestamp. Implementation order:
|
||||
|
||||
1. **Foundation** (Task 01): the pure wave-string/wave-key derivation and the
|
||||
new wave-member repository read every producer needs. No behavior change.
|
||||
2. **Persistence** (Task 02): the two `runs` columns, the partial unique
|
||||
index, the second unique-violation classifier, and `runs/service`'s
|
||||
coalescing exclusion. Still no producer sets these columns yet, so this is
|
||||
also behavior-inert until Task 03 lands — verified by service-level tests
|
||||
that construct requests directly.
|
||||
3. **Producers**, one work order per identity site, in dependency order
|
||||
forced by the shared engine seam:
|
||||
- Task 03: P1 cascade (`office/scheduler`) — direct insert path, needs its
|
||||
own classification site and its own coalescing guard, and owns the
|
||||
payload-parity read (AC-...-002.16) since it never touches the engine.
|
||||
- Task 04: the engine seam plus P2 edge and P3 backstop, which share it.
|
||||
`OnChildrenCompletedPayload` gains the two encodings; `QueueRunCallback`
|
||||
reads them off the trigger payload it already carries end-to-end
|
||||
(`ActionInput.Payload` is `HandleInput.Payload` verbatim — no new
|
||||
parameter threading through `HandleInput`/`ActionInput`/engine
|
||||
`QueueRunRequest` is needed beyond the two new fields on that one
|
||||
request struct and its `runsServiceEngineAdapter` copy).
|
||||
- Task 05: P4 orchestrator, built on Task 04's seam. No new read; re-sorts
|
||||
the read it already has.
|
||||
4. **Admission** (Task 06): `ListStuckParents` compares wave identity instead
|
||||
of timestamp, once producers actually write it.
|
||||
5. **Closing coverage** (Task 07): the race test, cross-producer parity,
|
||||
coalescing, read-skew, and admission suites that need every producer and
|
||||
the admission rewrite to exist simultaneously.
|
||||
|
||||
Tasks 03, 04 and 05 are not parallel-safe with each other even though they
|
||||
touch different packages: all three add wiring in
|
||||
`internal/backendapp/main.go` (03 wires a new `WorkflowStepGetter` setter for
|
||||
payload parity; 04 changes `runsServiceEngineAdapter`; 05 depends on 04's
|
||||
`OnChildrenCompletedPayload` fields). Land them in order.
|
||||
|
||||
## Scope
|
||||
|
||||
### In scope
|
||||
|
||||
- `internal/office/waveidentity` (new leaf package): `WaveString` and
|
||||
`WaveKey`, pure functions of `(parentID string, memberIDs []string)`
|
||||
(REQ-001).
|
||||
- A new office-repository read returning wave members `(id, state)` ordered
|
||||
by id, applying the shared not-archived/not-ephemeral/not-automation-origin
|
||||
predicate (REQ-001, REQ-002.15).
|
||||
- `runs.wake_wave_key` / `runs.wake_wave_string` columns, partial unique index
|
||||
`idx_run_wake_wave(wake_wave_key, agent_profile_id)`, `models.Run` fields,
|
||||
`CreateRunTx` column list, a second classifier
|
||||
(`IsWakeWaveUniqueViolation`), `runs/service`'s dedupe-on-conflict handling,
|
||||
and its coalescing exclusion for wave-carrying requests/rows (REQ-002).
|
||||
- Threading the two encodings through all four producers, including P1's own
|
||||
`CreateRun` classification site (`office/scheduler.SchedulerService`) and
|
||||
its coalescing guard, and the shared engine seam
|
||||
(`OnChildrenCompletedPayload` → `ActionInput.Payload` → `QueueRunCallback`
|
||||
→ `engine.QueueRunRequest` → `runsServiceEngineAdapter` →
|
||||
`runs/service.QueueRunRequest`) for P2/P3/P4 (REQ-002, REQ-004).
|
||||
- The terminality-confirming wave-member read (AC-...-002.15): P1/P2/P3 read
|
||||
`(id, state)` as their last child-state read before queueing and queue
|
||||
nothing if any member is non-terminal; P4 already satisfies this via
|
||||
`readyChildCompletionRows`.
|
||||
- Payload parity (AC-...-002.16): the cascade resolves the parent's current
|
||||
workflow step, reads its `on_children_completed` `queue_run` action
|
||||
payload (via `engine.CompileStep`, already used by
|
||||
`orchestrator.workflowStore.LoadStep`), and merges it into its own run
|
||||
payload; queues without it, logging the omission, when it cannot resolve.
|
||||
- `internal/office/repository/sqlite.ListStuckParents` rewrite: a
|
||||
dialect-aware wave-string aggregate (new `internal/db/dialect` helper), a
|
||||
wave-member existence gate, and the three-clause, status-qualified
|
||||
admission arm from the system design (REQ-003).
|
||||
- `parent_wake_deduped_total` expvar, incremented at both classification
|
||||
sites.
|
||||
- Regression coverage per the spec's Testing section.
|
||||
|
||||
### Out of scope
|
||||
|
||||
(Mirrors the requirements document's "Out of scope" verbatim; not repeated
|
||||
here beyond a pointer.) Reopen-and-recomplete re-waking; id-set-reuse via
|
||||
unarchive; task `updated_at` stamping; the queued-or-claimed arm's
|
||||
whole-second comparison (`b7e29d7c`); porting `ListStuckParents`' SQLite-only
|
||||
`json_extract`/aggregate expressions elsewhere in the same query to dialect
|
||||
form (`53c24173`, and this work must not widen that gap); `on_children_completed`
|
||||
trigger firing conditions, payload shape, or editor surface; UI.
|
||||
`wakeOperationID` (P2/P3's existing operation id) is **not** deleted or
|
||||
redefined — the round-2 spec keeps every producer's existing identifier
|
||||
untouched, unlike the stale plan this supersedes.
|
||||
|
||||
## Technical approach
|
||||
|
||||
### Foundation: `internal/office/waveidentity` and the wave-member read
|
||||
|
||||
New package, dependency-free beyond `crypto/sha256`, `encoding/hex`,
|
||||
`strings`:
|
||||
|
||||
```go
|
||||
func WaveString(parentID string, memberIDs []string) string
|
||||
func WaveKey(parentID string, memberIDs []string) string // = WaveKey(WaveString(...))
|
||||
```
|
||||
|
||||
`WaveString` = `parentID + "|" + strings.Join(memberIDs, ",")`. `WaveKey` =
|
||||
`"task_children_completed:" + parentID + ":" + hex(sha256(waveString))`.
|
||||
Callers order `memberIDs` ascending by `tasks.id` and pass only wave members;
|
||||
the functions do no I/O and return no error (AC-...-001.1, .2, .10, .11).
|
||||
Sibling of `internal/office/costs/modelsdev`, which `internal/orchestrator`
|
||||
already imports (`orchestrator/model_info.go`) without crossing the
|
||||
`orchestrator`/`office/service` boundary.
|
||||
|
||||
New read, `ListWaveMembers`, in `internal/office/repository/sqlite/blockers.go`
|
||||
beside `ListChildStates` (same file, same `ChildState`-shaped row):
|
||||
|
||||
```go
|
||||
func (r *Repository) ListWaveMembers(ctx context.Context, parentID string) ([]ChildState, error)
|
||||
```
|
||||
|
||||
```sql
|
||||
SELECT id, COALESCE(state, '') AS state FROM tasks
|
||||
WHERE parent_id = ? AND archived_at IS NULL AND is_ephemeral = 0
|
||||
AND COALESCE(origin, '') != 'automation_run'
|
||||
ORDER BY id
|
||||
```
|
||||
|
||||
This is the predicate `task/repository/sqlite.ListChildCompletionRows`
|
||||
already applies (`andNotAutomationOrigin`), reused here because P4 forces the
|
||||
choice (design, "Which children count"). A cross-producer test in Task 01
|
||||
asserts this method and `ListChildCompletionRows` return the same id set for
|
||||
one fixture parent — the equality AC-...-001.8 depends on.
|
||||
|
||||
### Persistence
|
||||
|
||||
- `internal/office/repository/sqlite/base.go`, `createRunTables` (~line
|
||||
329-387): add `wake_wave_key TEXT NOT NULL DEFAULT ''` and
|
||||
`wake_wave_string TEXT NOT NULL DEFAULT ''` to the inline `CREATE TABLE
|
||||
runs`, plus `CREATE UNIQUE INDEX IF NOT EXISTS idx_run_wake_wave ON
|
||||
runs(wake_wave_key, agent_profile_id) WHERE wake_wave_key <> ''` beside
|
||||
`idx_run_idempotency`.
|
||||
- `internal/office/repository/sqlite/base_migrations.go`: new
|
||||
`migrateWakeWaveColumns()`, following `migrateContinuationScope`'s shape
|
||||
(two `r.migrate.Apply` ALTERs + one partial-index `CREATE UNIQUE INDEX IF
|
||||
NOT EXISTS`, no backfill), called from `runMigrations()` next to
|
||||
`migrateParentWakeReceiptColumns()`.
|
||||
- `internal/office/models/models.go`, `Run` struct (~line 381): add
|
||||
`WakeWaveKey string \`db:"wake_wave_key"\`` and `WakeWaveString string
|
||||
\`db:"wake_wave_string"\``.
|
||||
- `internal/runs/repository/sqlite/runs.go`, `CreateRunTx` (~line 31-52): add
|
||||
both columns to the `INSERT INTO runs` list and bound args.
|
||||
- `internal/runs/repository/sqlite/idempotency_violation.go`: add
|
||||
`IsWakeWaveUniqueViolation(err error) bool`, mirroring
|
||||
`IsIdempotencyKeyUniqueViolation` — PostgreSQL: typed `pgErr.ConstraintName
|
||||
== "idx_run_wake_wave"`; SQLite: message substring `"UNIQUE constraint
|
||||
failed: runs.wake_wave_key, runs.agent_profile_id"` (confirm go-sqlite3's
|
||||
exact composite-index message empirically before hardcoding — this is the
|
||||
same risk the superseded plan flagged, now scoped to one new function
|
||||
instead of a rename).
|
||||
- `internal/runs/service/service.go`: `QueueRunRequest` (~line 85-92) gains
|
||||
`WakeWaveKey`, `WakeWaveString string`; `insertRun` (~line 249-274) sets
|
||||
them on the `*models.Run{}` literal; `insertRun`'s `CreateRun` error
|
||||
handling gains a second branch — `IsWakeWaveUniqueViolation(err)` returns a
|
||||
second sentinel (e.g. `errWakeWaveKeyConflict`), and `QueueRun` (~line
|
||||
221-235) maps it to `QueueOutcomeDeduped` exactly like
|
||||
`errIdempotencyKeyConflict`, with no error and no retry
|
||||
(AC-...-002.4/.5/.6). `shouldCoalesceRun` (~line 316-318) gains `&&
|
||||
req.WakeWaveKey == ""` (AC-...-002.14, first half).
|
||||
- `internal/runs/repository/sqlite/runs.go`, `CoalesceRun` (~line 449-488):
|
||||
add `AND (wake_wave_key = '' OR wake_wave_key IS NULL)` — actually the
|
||||
column is `NOT NULL DEFAULT ''`, so `AND wake_wave_key = ''` — to the
|
||||
candidate-row `WHERE`, closing AC-...-002.14's second half (a queued
|
||||
wave-carrying run is never merged into, regardless of which caller
|
||||
invokes `CoalesceRun`).
|
||||
- `internal/office/scheduler/run.go`, `SchedulerService.QueueRun` (~line
|
||||
249-303): the same two guards for P1's own coalesce-and-insert path — skip
|
||||
`ss.repo.CoalesceRun` when a wave key is supplied, and classify
|
||||
`ss.repo.CreateRun`'s error with `IsWakeWaveUniqueViolation` into a nil
|
||||
return + debug log (mirroring the existing `CheckIdempotencyKey` hit
|
||||
handling), not the `enqueue run: %w` wrap that would make .002.4 fail for
|
||||
P1 specifically (this is why Task 03 needs its own classification work,
|
||||
not just Task 02's).
|
||||
- `internal/office/service/wake_metrics.go`: add `parentWakeDedupedTotal =
|
||||
expvar.NewInt("parent_wake_deduped_total")`, incremented at both
|
||||
classification branches above. `runs/service` does not import
|
||||
`office/service`; wire the increment via a small exported callback var (or
|
||||
equivalent seam already used for `parent_wake_*` counters — check
|
||||
`wake_metrics.go`'s existing wiring pattern before choosing) rather than a
|
||||
new cross-package dependency.
|
||||
|
||||
### P1 cascade (Task 03)
|
||||
|
||||
`internal/office/scheduler/reactivity.go`, `cascadeChildrenCompleted`
|
||||
(~line 384-421): keeps `ListChildStates` and its terminal-state loop
|
||||
unchanged (narrowing that read would change cascade's readiness check,
|
||||
forbidden by AC-...-004.7) and adds, after that loop confirms all-terminal:
|
||||
|
||||
1. Call `ListWaveMembers(ctx, task.ParentID)` — the terminality-confirming
|
||||
last read (AC-...-002.15). Any non-terminal member (a race with the
|
||||
`ListChildStates` read) or a read error: queue nothing, return.
|
||||
2. Empty result: queue nothing, return (AC-...-001.7; live once the
|
||||
predicate is narrower than `ListChildStates`, e.g. an all-ephemeral child
|
||||
set).
|
||||
3. Derive `waveidentity.WaveString`/`WaveKey` from the sorted member ids.
|
||||
4. Resolve payload parity: `GetTaskWorkflowStepID(ctx, task.ParentID)`
|
||||
(`office/repository/sqlite/participants.go:55`, already used elsewhere in
|
||||
this package's blast radius) → a new small interface,
|
||||
`WorkflowStepGetter.GetStep(ctx, stepID) (*wfmodels.WorkflowStep, error)`
|
||||
(satisfied by `workflow/service.Service.GetStep`, wired via a new
|
||||
`SchedulerService.SetWorkflowStepGetter` setter, called once at boot next
|
||||
to `SetTaskStarter`/`SetResolver`) → `engine.CompileStep` (already
|
||||
exported, already used by `orchestrator/workflow_store.go:226`) →
|
||||
look up the step's `TriggerOnChildrenCompleted` actions for one of kind
|
||||
`queue_run` with a non-nil `Payload`. Any failure at any of these steps:
|
||||
log the omission at debug and continue without a merged payload — the
|
||||
wake is unconditional (AC-...-004.2) and outranks the optional payload.
|
||||
5. `RunContext` (~line 90-122) gains `WaveKey string`, `WaveString string`
|
||||
(JSON `"-"`, like `IdempotencyKey`) and a merge point for the resolved
|
||||
action payload with the same override precedence `queueRunPayload`
|
||||
applies (workflow-authored keys win over the trigger's own). Since
|
||||
`RunContext` marshals as a fixed struct rather than a generic map, the
|
||||
simplest implementation is: marshal `RunContext` first, then if a merged
|
||||
payload map is non-empty, unmarshal to `map[string]any`, overlay the
|
||||
merged keys, re-marshal — implemented inside `encodeRunContext` or a
|
||||
thin wrapper around it, whichever keeps `QueueRunCtx` readable.
|
||||
6. `QueueRunCtx` → `QueueRun` (~line 246-324) thread `WaveKey`/`WaveString`
|
||||
as **new required parameters** (do not smuggle them through `RunContext`
|
||||
alone into the low-level `QueueRun`, which today takes discrete scalar
|
||||
args) onto the inserted `models.Run{WakeWaveKey, WakeWaveString}`.
|
||||
`childrenCompletedIdempotencyKey` (~line 436-443) is unchanged and stays
|
||||
the run's `idempotency_key` — additive, not replaced (design,
|
||||
"Producers").
|
||||
|
||||
### Engine seam, P2 edge, P3 backstop (Task 04)
|
||||
|
||||
- `internal/workflow/engine/payloads.go`: `OnChildrenCompletedPayload`
|
||||
(~line 38) gains `WaveKey string`, `WaveString string`. Not read by
|
||||
`idempotencyKey` or `queueActionDigest` (phase2_callbacks.go) — those stay
|
||||
untouched, per the design's "narrow seam" decision.
|
||||
- `internal/workflow/engine/adapters.go`, `QueueRunRequest` (~line 45-52):
|
||||
add `WakeWaveKey`, `WakeWaveString string`.
|
||||
- `internal/workflow/engine/phase2_callbacks.go`, `QueueRunCallback.Execute`
|
||||
(~line 81-107): after building `req := QueueRunRequest{...}`, type-assert
|
||||
`in.Payload.(OnChildrenCompletedPayload)` and copy `WaveKey`/`WaveString`
|
||||
onto `req` when present. This is the only engine-package call site that
|
||||
changes — `HandleInput`, `ActionInput`, and `Engine.executeCallback`
|
||||
already carry `Payload any` end-to-end unmodified
|
||||
(`engine.go:429-437` copies `in.Payload` verbatim into `ActionInput`), so
|
||||
no new field needs to thread through those three types.
|
||||
- `internal/backendapp/main.go`, `runsServiceEngineAdapter.QueueRun`
|
||||
(~line 1810-1822): copy `WakeWaveKey`/`WakeWaveString` from the engine's
|
||||
`QueueRunRequest` to the runs-service `QueueRunRequest`.
|
||||
- `internal/office/service/event_subscribers.go`, `queueChildrenCompletedRun`
|
||||
(~line 1116-1151): after `AreAllChildrenTerminal`, add the wave-member read
|
||||
(`ListWaveMembers`) as the terminality confirmation; on empty or
|
||||
non-terminal, queue nothing (return nil, matching the existing
|
||||
`!allDone` early return). `childSetKey`/`wakeOperationID` computation is
|
||||
**unchanged** — the operation id keeps its existing state-inclusive
|
||||
derivation. Build `engine.OnChildrenCompletedPayload{ChildSummaries:
|
||||
summaries, WaveKey: ..., WaveString: ...}` from the new read's sorted ids.
|
||||
- `internal/office/service/scheduler_wake_reconciler.go`,
|
||||
`reconcileOne`/`buildPayload` (~line 93-177): same addition — call
|
||||
`ListWaveMembers` before dispatch (in addition to, not instead of, the
|
||||
existing `GetChildSetKey` re-reads that guard the receipt write — those
|
||||
keep using the state-inclusive key per the design's "Receipts" section),
|
||||
reject on non-terminal/empty, and set `WaveKey`/`WaveString` on the
|
||||
payload `buildPayload` returns. `wakeOperationID` is unchanged.
|
||||
|
||||
### P4 orchestrator (Task 05)
|
||||
|
||||
`internal/orchestrator/event_handlers_children_completed.go`:
|
||||
`childCompletionOperationID` (~line 325-345) is **unchanged** — the
|
||||
operation ledger keeps its existing per-row state/step/terminal/`updated_at`
|
||||
derivation (AC-...-004.3). `childCompletionPayload` (~line 306-316) gains a
|
||||
`parentID` parameter; before building `ChildSummaries` it copies `rows`,
|
||||
sorts the copy ascending by `ID` (the source orders by `created_at, id`), and
|
||||
sets `WaveKey`/`WaveString` from `waveidentity.WaveString/WaveKey(parentID,
|
||||
sortedIDs)`. No new read and no new filter — `readyChildCompletionRows`
|
||||
already rejects `len(rows) == 0` and already establishes terminality from
|
||||
these same rows (AC-...-002.15 satisfied by construction, per the design).
|
||||
|
||||
### Backstop admission (Task 06)
|
||||
|
||||
`internal/office/repository/sqlite/wake_receipts.go`, `ListStuckParents`
|
||||
(~line 101-175):
|
||||
|
||||
- New `internal/db/dialect` helper beside `JSONExtract` (e.g.
|
||||
`OrderedIDConcat(driver, table, parentCol, idCol string) string` or similar
|
||||
— name it to fit the file's existing signature style), branching:
|
||||
SQLite `GROUP_CONCAT(c.id, ',')` fed by an `ORDER BY id` subquery (the
|
||||
`child_set_key` idiom already in this file); PostgreSQL `string_agg(c.id,
|
||||
',' ORDER BY c.id)`. Only this new fragment is dialect-branched — the
|
||||
query's existing `json_extract`/unbranched `GROUP_CONCAT` stay as they are
|
||||
(out of scope per `53c24173`).
|
||||
- Add a wave-string CTE column: `p.id || '|' || COALESCE(<helper output over
|
||||
the wave-member predicate>, '')`, computed for every row (a pre-gate
|
||||
intermediate for a parent with no members, per Terminology).
|
||||
- Add the wave-member existence gate: a second `EXISTS` beside the existing
|
||||
archived-only one, over `archived_at IS NULL AND is_ephemeral = 0 AND
|
||||
COALESCE(origin, '') != 'automation_run'` (AC-...-003.9).
|
||||
- Rewrite the second `NOT EXISTS` arm into three named clauses (design,
|
||||
"Backstop admission"): `status IN ('queued','claimed')` blocks
|
||||
unconditionally; `status IN ('finished','failed','cancelled') AND
|
||||
wake_wave_string = <current wave string>` blocks; `NOT EXISTS (... WHERE
|
||||
wake_wave_key <> '') AND status IN ('finished','failed','cancelled') AND
|
||||
requested_at >= newest_child_updated_at` is the parent-scoped legacy
|
||||
fallback (AC-...-003.5/.6/.10) — the "no keyed run at all for this parent"
|
||||
condition must be evaluated once per parent, not per row, per .003.6.
|
||||
- `StuckParentCandidate` (~line 21-27) gains no new field unless the caller
|
||||
needs the wave string for logging; the comparison lives entirely inside
|
||||
the SQL per AC-...-003.8.
|
||||
|
||||
### Observability
|
||||
|
||||
Already covered under Persistence (`parent_wake_deduped_total`).
|
||||
|
||||
## Tests
|
||||
|
||||
| Acceptance criteria | Test |
|
||||
| --- | --- |
|
||||
| AC-...-001.1-.3, .5, .6, .9-.13 | `internal/office/waveidentity/waveidentity_test.go`: deterministic and order-sensitive per the caller contract; `|`-separated, not NUL; digest-of-string relationship; empty slice documented as never called; no I/O. |
|
||||
| AC-...-001.4, .5 | `internal/office/scheduler/reactivity_children_completed_test.go` (existing terminal-edit and non-state-edit cases) continue to pass unmodified. |
|
||||
| AC-...-001.7, .8 | New cross-producer parity test: `ListWaveMembers` and `ListChildCompletionRows` return the same id set for one fixture parent, including an ephemeral/automation-origin child; an all-excluded child set derives no wave. |
|
||||
| AC-...-002.1-.3, .7-.9, .11-.13 | `internal/runs/service/service_test.go`: `TestQueueRun_DedupesOnWakeWaveKeyIndexRace` (mirrors `TestQueueRun_DedupesOnIdempotencyIndexRace`, ~line 578); a different agent and a different wave-member set each get their own row; retry (`ScheduleRetry`) is untouched by the constraint. |
|
||||
| AC-...-002.4, .5 | `internal/runs/repository/sqlite/idempotency_violation_test.go` (new, pattern from the existing file) for `IsWakeWaveUniqueViolation`; `internal/runs/service/service_postgres_test.go`: Postgres twin of the race test. |
|
||||
| AC-...-002.6 | Same race test asserts no time bound (insert the second request well after the first). |
|
||||
| AC-...-002.10, .002.16 | Payload-parity test: cascade attaches the workflow-authored `queue_run` payload byte-for-byte matching what the engine path would attach for an equivalent step; and still queues without it when the step lookup fails. |
|
||||
| AC-...-002.14 | Coalescing test: a second request for a different parent, same agent, inside the coalescing window, with a wave key, does not merge into the first parent's queued (also wave-keyed) run — two rows exist. |
|
||||
| AC-...-002.15 | Read-skew test: a wave-member read returning one non-terminal member queues nothing and records no identity; a later genuine wave for the same parent is still delivered. |
|
||||
| AC-...-003.1-.10 | `internal/office/repository/sqlite/wake_receipts_test.go`: delivered-and-unchanged wave is not a candidate; non-state child edit after delivery is not a candidate; wave-member change after delivery is a candidate (and an id-set-reuse-to-a-delivered-identity is listed but queues nothing downstream); queued/claimed run blocks regardless of wave; a legacy run with an empty `wake_wave_key` is judged by the timestamp rule, scoped per parent even alongside a keyed run for the same parent; a failed run's wave-scoped block survives an unrelated child edit and is lifted only by a wave-member change; a parent whose only children are excluded is not a candidate. |
|
||||
| AC-...-004.1-.7 | The full race test: cascade (P1) racing one reconciler (P3) tick for the same parent through `office/scheduler`'s own `QueueRun` path (not only `runs/service`), asserting exactly one `runs` row and no failure logged for the loser. Existing `TestWakeOperationID_UnifiedAcrossEdgeAndReconcilerPaths` and the orchestrator's operation-ledger tests continue to pass unmodified, confirming P2/P3's `wakeOperationID` and P4's `childCompletionOperationID` are untouched. |
|
||||
|
||||
## E2E tests
|
||||
|
||||
None. No user-visible control, view, or setting (requirements document,
|
||||
"Out of scope": "User interface"). The effect is observable only as the
|
||||
absence of a duplicate or spurious run in the Office Runs list, which the
|
||||
spec's E2E decision explicitly leaves to Go-level coverage.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [x] [Task 01: Wave identity primitives](task-01-wave-identity-primitives.md)
|
||||
- [x] [Task 02: Persist wave identity and classify its unique violation](task-02-wave-identity-persistence.md)
|
||||
- [x] [Task 03: Wire cascade (P1) onto wave identity](task-03-wire-cascade-producer.md)
|
||||
- [x] [Task 04: Wire engine-routed producers (P2, P3) onto wave identity](task-04-wire-engine-routed-producers.md)
|
||||
- [x] [Task 05: Wire orchestrator (P4) onto wave identity](task-05-wire-orchestrator-producer.md)
|
||||
- [x] [Task 06: Backstop admission compares wave identity](task-06-backstop-admission.md)
|
||||
- [x] [Task 07: Race, parity, and regression coverage](task-07-race-and-regression-coverage.md)
|
||||
|
||||
## Verification results
|
||||
|
||||
All 7 work orders done. Full-package tests pass with `-race` across every
|
||||
touched package (`office/scheduler`, `office/service`,
|
||||
`office/repository/sqlite`, `orchestrator`, `runs/service`,
|
||||
`runs/repository/sqlite`, `task/repository/sqlite`, `office/waveidentity`,
|
||||
`db/dialect`); `golangci-lint --new-from-rev=cd78236315f28982848de4938d56f7722c7f632f`
|
||||
clean on every changed package. See each task file's own Results section
|
||||
for command output and per-task detail. `KANDEV_TEST_POSTGRES_DSN` was not
|
||||
provisioned in this environment, so the gated Postgres twins (Task 06's
|
||||
`OrderedIDConcat`, this initiative's other dialect-sensitive coverage)
|
||||
skipped rather than ran — consistent with the card's instruction not to
|
||||
provision Docker/Postgres here.
|
||||
|
||||
Definition of Done gauntlet: `make fmt`, `make typecheck`, `make lint`
|
||||
(backend 0 issues repo-wide, web eslint clean, lint-architecture clean;
|
||||
lint-harness/lint-specs required Python 3.10+ — this environment's
|
||||
default `python3` is 3.9.6, incompatible with the scripts' `X | None`
|
||||
type-hint syntax; ran clean under `/opt/homebrew/bin/python3.14`
|
||||
instead), `make lint-format`, and `pnpm run i18n:ratchet` (no UI change,
|
||||
as expected for a backend-only card) all pass. `make test` passes for
|
||||
every package this initiative touched; pre-existing failures in
|
||||
unrelated subsystems (npm cache path resolution, worktree/git
|
||||
provisioning, agentctl process management, the dev launcher) reproduce
|
||||
against unmodified code in this sandboxed environment and are unrelated
|
||||
to this change.
|
||||
|
||||
A final pass removed every AC-NN/task-order reference this initiative's
|
||||
comments had accumulated, restating each as a direct invariant per the
|
||||
repo's comment convention.
|
||||
|
||||
## Risks
|
||||
|
||||
- **SQLite composite-unique-index violation message shape.**
|
||||
`IsWakeWaveUniqueViolation`'s SQLite branch needs the exact go-sqlite3
|
||||
message for a two-column unique index, confirmed empirically in Task 02
|
||||
before hardcoding (same risk the superseded plan flagged for a
|
||||
single-column rename; now isolated to one new function).
|
||||
- **Cross-package `parent_wake_deduped_total` increment.** `runs/service`
|
||||
does not import `office/service`; Task 02 picks the least-invasive seam
|
||||
(existing `wake_metrics.go` wiring pattern, if one already crosses this
|
||||
boundary, otherwise a small callback var).
|
||||
- **Payload-parity merge shape.** `RunContext` is a fixed struct, not a
|
||||
generic map, so merging an arbitrary workflow-authored payload into it
|
||||
needs a marshal/overlay/re-marshal step Task 03 must design carefully to
|
||||
avoid disturbing `CoalesceRun`'s existing byte-for-byte payload comparison
|
||||
for non-wave-carrying runs.
|
||||
- **Three sequential, non-parallel producer work orders (03, 04, 05)** share
|
||||
touches to `internal/backendapp/main.go`. Land them one at a time.
|
||||
|
||||
## Open questions
|
||||
|
||||
None outstanding — the frozen spec resolves every prior open question (see
|
||||
the spec's own review history for the closed rounds).
|
||||
@@ -0,0 +1,116 @@
|
||||
---
|
||||
id: "01-wave-identity-primitives"
|
||||
title: "Wave identity primitives"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-001
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.2
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.3
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.5
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.6
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.7
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.8
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.9
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.10
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.11
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-001.13
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 01: Wave identity primitives
|
||||
|
||||
## Summary
|
||||
|
||||
Add the pure two-encoding derivation (`WaveString`, `WaveKey`) as a new leaf
|
||||
package, and add the shared wave-member repository read every producer will
|
||||
use to obtain the ordered id set those functions consume. No caller changes
|
||||
yet — this is foundation only.
|
||||
|
||||
## In scope
|
||||
|
||||
- `internal/office/waveidentity` package: `WaveString(parentID string,
|
||||
memberIDs []string) string` and `WaveKey(parentID string, memberIDs
|
||||
[]string) string`, per the system design's "The two encodings" section
|
||||
(separator `|`, digest prefix `task_children_completed:<parentID>:`,
|
||||
lowercase hex sha256 of the wave string). No I/O, no error return.
|
||||
- `internal/office/repository/sqlite.Repository.ListWaveMembers(ctx,
|
||||
parentID) ([]ChildState, error)` in `blockers.go`, beside `ListChildStates`:
|
||||
`(id, state)` ordered by `id`, filtered to `archived_at IS NULL AND
|
||||
is_ephemeral = 0 AND COALESCE(origin, '') != 'automation_run'` — the same
|
||||
predicate `task/repository/sqlite.ListChildCompletionRows` already applies.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Calling either the derivation or `ListWaveMembers` from any producer
|
||||
(Tasks 03-05).
|
||||
- Persisting the derived values anywhere (Task 02).
|
||||
- The PostgreSQL-dialect wave-string SQL aggregate used by the backstop
|
||||
(Task 06) — that is a separate, SQL-side expression of the same identity,
|
||||
not this Go package.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- `waveidentity.WaveKey(p, ids) == waveidentity.WaveKey(p, ids)` for any
|
||||
fixed input (determinism); different `ids` slices (same length, different
|
||||
order) that represent a *different* set produce a different key, but two
|
||||
calls with the *same* already-sorted slice never differ regardless of the
|
||||
slice's original pre-sort arrival order (callers, not the function, sort).
|
||||
- `ListWaveMembers` and `task/repository/sqlite.ListChildCompletionRows`
|
||||
return the identical set of ids for one fixture parent with a mix of
|
||||
ordinary, archived, ephemeral, and automation-origin children.
|
||||
- `go vet ./internal/office/waveidentity/... ./internal/office/repository/sqlite/...`
|
||||
is clean.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/office/waveidentity/... -run . -v
|
||||
go test ./internal/office/repository/sqlite/... -run TestListWaveMembers -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/office/waveidentity/waveidentity.go` (new)
|
||||
- `internal/office/waveidentity/waveidentity_test.go` (new)
|
||||
- `internal/office/repository/sqlite/blockers.go`
|
||||
- `internal/office/repository/sqlite/blockers_test.go` (or a new
|
||||
`wave_members_test.go` if the existing file is near its line-count limit)
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Risks
|
||||
|
||||
None — pure functions and a single new read-only query, no behavior change
|
||||
for any existing caller.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- Requirements: REQ-OFFICE-WAKE-WAVE-IDENTITY-001, full text.
|
||||
- System design: "Wave identity" and "The two encodings" sections.
|
||||
- `internal/office/repository/sqlite/blockers.go` (`ListChildStates` for the
|
||||
row shape and query idiom).
|
||||
- `internal/task/repository/sqlite/task.go` (`ListChildCompletionRows`,
|
||||
`andNotAutomationOrigin`) for the predicate this must match exactly.
|
||||
- `internal/office/costs/modelsdev` as the precedent leaf-package shape
|
||||
`internal/orchestrator` already imports.
|
||||
|
||||
## Results
|
||||
|
||||
Done. `internal/office/waveidentity` (`WaveString`, `WaveKey`) plus
|
||||
`Repository.ListWaveMembers` (excludes archived, ephemeral, and
|
||||
automation-origin children, ordered ascending by id) landed in
|
||||
`feat(office): add wave identity derivation and wave-member read`.
|
||||
`go test ./internal/office/waveidentity/... ./internal/office/repository/sqlite/... -run 'WaveIdentity|WaveMembers'` green.
|
||||
@@ -0,0 +1,144 @@
|
||||
---
|
||||
id: "02-wave-identity-persistence"
|
||||
title: "Persist wave identity and classify its unique violation"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-wave-identity-primitives"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.2
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.4
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.5
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.14
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 02: Persist wave identity and classify its unique violation
|
||||
|
||||
## Summary
|
||||
|
||||
Add the two `runs` columns and their partial unique index, thread them
|
||||
through `runs/service`'s request/insert path with dedupe-on-conflict
|
||||
handling and a coalescing exclusion, and add the observability counter. No
|
||||
producer sets these fields yet — verified directly against `runs/service`.
|
||||
|
||||
## In scope
|
||||
|
||||
- `runs.wake_wave_key` / `runs.wake_wave_string` columns (both `TEXT NOT
|
||||
NULL DEFAULT ''`), `idx_run_wake_wave(wake_wave_key, agent_profile_id)
|
||||
WHERE wake_wave_key <> ''`, added to `createRunTables` (fresh schema) and a
|
||||
new replayable `migrateWakeWaveColumns()` migration (existing databases).
|
||||
- `models.Run.WakeWaveKey` / `WakeWaveString` fields; `CreateRunTx`'s column
|
||||
list and bound args.
|
||||
- `internal/runs/repository/sqlite/idempotency_violation.go`:
|
||||
`IsWakeWaveUniqueViolation(err error) bool`.
|
||||
- `internal/runs/service/service.go`: `QueueRunRequest` gains the two
|
||||
fields; `insertRun` sets them on the inserted row; a second sentinel
|
||||
(parallel to `errIdempotencyKeyConflict`) maps
|
||||
`IsWakeWaveUniqueViolation` to `QueueOutcomeDeduped`; `shouldCoalesceRun`
|
||||
excludes wave-carrying requests.
|
||||
- `internal/runs/repository/sqlite/runs.go`, `CoalesceRun`: exclude
|
||||
wave-carrying rows from the merge-target candidate query.
|
||||
- `parent_wake_deduped_total` expvar, incremented at this classification
|
||||
site (the second site, in `office/scheduler`, is Task 03's
|
||||
responsibility — this task only covers the one runs/service exercises).
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Any producer actually populating `WakeWaveKey`/`WakeWaveString` on a
|
||||
request (Tasks 03-05).
|
||||
- `office/scheduler.SchedulerService.QueueRun`'s own classification site and
|
||||
coalescing guard (Task 03) — `runs/service` and the office scheduler's
|
||||
direct-insert path are two separate call sites into `CreateRun`, per the
|
||||
system design's "Failure and recovery" section, and this task only closes
|
||||
the `runs/service` one.
|
||||
- `ListStuckParents` (Task 06).
|
||||
|
||||
## Acceptance
|
||||
|
||||
- A `QueueRun` call with a `WakeWaveKey` set inserts a row with both columns
|
||||
populated; a second call with the identical `(WakeWaveKey,
|
||||
AgentProfileID)` returns `QueueOutcomeDeduped`, not an error, and is not
|
||||
coalesced into instead.
|
||||
- A `QueueRun` call with `WakeWaveKey` set to a *different, already-queued*
|
||||
wave's key is never selected as a `CoalesceRun` merge target by an
|
||||
unrelated wave-carrying request for the same agent+reason inside the
|
||||
coalescing window.
|
||||
- `IsWakeWaveUniqueViolation` returns true for a real SQLite composite
|
||||
unique-index violation on this index (confirmed empirically, not assumed
|
||||
from the single-column message) and for the matching PostgreSQL
|
||||
`pgconn.PgError` shape (gated test, `KANDEV_TEST_POSTGRES_DSN`).
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/office/repository/sqlite/... -run TestMigrate -v
|
||||
go test ./internal/runs/service/... -run TestQueueRun -v
|
||||
go test ./internal/runs/repository/sqlite/... -run TestIsWakeWaveUniqueViolation -v
|
||||
KANDEV_TEST_POSTGRES_DSN=... go test ./internal/runs/service/... -run TestPostgresQueueRun -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/office/repository/sqlite/base.go`
|
||||
- `internal/office/repository/sqlite/base_migrations.go`
|
||||
- `internal/office/repository/sqlite/base_migrations_test.go` (replay
|
||||
coverage, per the backend's table-rebuild/migration testing convention)
|
||||
- `internal/office/models/models.go`
|
||||
- `internal/runs/repository/sqlite/runs.go`
|
||||
- `internal/runs/repository/sqlite/idempotency_violation.go`
|
||||
- `internal/runs/repository/sqlite/idempotency_violation_test.go` (new)
|
||||
- `internal/runs/service/service.go`
|
||||
- `internal/runs/service/service_test.go`
|
||||
- `internal/runs/service/service_postgres_test.go`
|
||||
- `internal/office/service/wake_metrics.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (uses `waveidentity` types only for test fixtures, not production
|
||||
code in this task).
|
||||
|
||||
## Risks
|
||||
|
||||
- **SQLite composite-index message shape** — confirm empirically before
|
||||
hardcoding; a wrong substring silently fails classification and P1/engine
|
||||
producers would see a hard error instead of a dedupe on their second
|
||||
arrival.
|
||||
- **`parent_wake_deduped_total` cross-package increment** — check
|
||||
`wake_metrics.go`'s existing wiring before adding a new seam; prefer
|
||||
reusing whatever pattern already gets a runs/service-side outcome back to
|
||||
office's expvars, if one exists.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Persistence", "Coalescing: the third write path into
|
||||
runs", "Failure and recovery" sections.
|
||||
- `internal/runs/repository/sqlite/idempotency_violation.go` (pattern to
|
||||
mirror).
|
||||
- `internal/office/repository/sqlite/base_migrations.go`
|
||||
(`migrateContinuationScope` as the replayable-migration precedent).
|
||||
- `internal/runs/service/service_test.go:578`
|
||||
`TestQueueRun_DedupesOnIdempotencyIndexRace` (pattern for the new race
|
||||
test written here at the request-construction level; the full
|
||||
cross-producer race test is Task 07's).
|
||||
|
||||
## Results
|
||||
|
||||
Done, across three commits: `feat(runs): add wake_wave_key and
|
||||
wake_wave_string columns to runs` (migration-only index per the
|
||||
schema-init-before-migration rule, `models.Run` fields), `feat(runs):
|
||||
dedupe and skip coalescing for wave-carrying queue requests`
|
||||
(`IsWakeWaveUniqueViolation`, `runs/service` classification +
|
||||
`shouldCoalesceRun` guard, `office/shared.ParentWakeDedupedTotal`), and
|
||||
`test(runs): add Postgres twin for the wake-wave-key dedupe race`
|
||||
(env-gated, skips cleanly without `KANDEV_TEST_POSTGRES_DSN`).
|
||||
`go test ./internal/runs/... ./internal/office/repository/sqlite/...` green.
|
||||
@@ -0,0 +1,157 @@
|
||||
---
|
||||
id: "03-wire-cascade-producer"
|
||||
title: "Wire cascade (P1) onto wave identity"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["01-wave-identity-primitives", "02-wave-identity-persistence"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.4
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.12
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.14
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.15
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.16
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-004.2
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 03: Wire cascade (P1) onto wave identity
|
||||
|
||||
## Summary
|
||||
|
||||
`office/scheduler.cascadeChildrenCompleted` gains the terminality-confirming
|
||||
wave-member read, derives and persists the wave identity on its direct
|
||||
insert, gains its own unique-violation classification and coalescing guard
|
||||
(the runs/service ones from Task 02 do not cover this call site), and
|
||||
resolves the workflow-authored `on_children_completed` payload for parity
|
||||
with the engine-routed producers.
|
||||
|
||||
## In scope
|
||||
|
||||
- `RunContext` gains `WaveKey`, `WaveString string` (JSON `"-"`).
|
||||
- `cascadeChildrenCompleted`: after the existing `ListChildStates`
|
||||
terminal-state loop, read `ListWaveMembers`; queue nothing on a read error
|
||||
or any non-terminal member; queue nothing on an empty result
|
||||
(AC-...-001.7); otherwise derive the wave identity from the sorted ids.
|
||||
- `QueueRunCtx`/`QueueRun` (`office/scheduler/run.go`) take the wave
|
||||
identity through to the inserted `models.Run`, skip
|
||||
`ss.repo.CoalesceRun` when a wave key is present, and classify
|
||||
`ss.repo.CreateRun`'s error with `IsWakeWaveUniqueViolation` into a nil
|
||||
return + debug log (not the existing `enqueue run: %w` wrap) —
|
||||
`runs/service`'s classification from Task 02 does not cover this
|
||||
direct-insert call site.
|
||||
- Payload parity (AC-...-002.16): a new `WorkflowStepGetter` interface
|
||||
(`GetStep(ctx, stepID) (*wfmodels.WorkflowStep, error)`), a
|
||||
`SchedulerService.SetWorkflowStepGetter` setter wired at boot to
|
||||
`workflow/service.Service.GetStep`, and cascade logic that resolves the
|
||||
parent's current step via `GetTaskWorkflowStepID` +
|
||||
`WorkflowStepGetter.GetStep` + `engine.CompileStep`, finds a `queue_run`
|
||||
action on `TriggerOnChildrenCompleted` with a non-nil payload, and merges
|
||||
it into the run's persisted payload with the same override precedence
|
||||
`engine.queueRunPayload` applies. Any failure at any step: log at debug,
|
||||
queue without the merged payload (AC-...-004.2 — the wake is
|
||||
unconditional).
|
||||
- `parent_wake_deduped_total` increment at this task's classification site.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- P2/P3/P4 (Tasks 04, 05).
|
||||
- `runs/service`'s own classification and coalescing guard (Task 02,
|
||||
already done, unrelated call site).
|
||||
- Any change to `childrenCompletedIdempotencyKey` or cascade's existing
|
||||
no-session behavior (AC-...-004.2's first half, already true today).
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Two calls to `cascadeChildrenCompleted` for the same parent and the same
|
||||
terminal child set produce exactly one `runs` row, and the second call
|
||||
logs no failure.
|
||||
- A wave-member read that observes one non-terminal member (simulated via a
|
||||
fake repo) queues nothing; a subsequent call once all members are
|
||||
genuinely terminal queues the wake.
|
||||
- When the parent's current step declares an `on_children_completed`
|
||||
`queue_run` action with a payload, that payload's keys appear in the
|
||||
queued run's persisted payload; when the step lookup fails, the run is
|
||||
still queued without them and a debug log records the omission.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/office/scheduler/... -run TestCascadeChildrenCompleted -v
|
||||
go test ./internal/office/scheduler/... -run TestCascade.*WavePayload -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/office/scheduler/reactivity.go`
|
||||
- `internal/office/scheduler/reactivity_children_completed_test.go`
|
||||
- `internal/office/scheduler/run.go`
|
||||
- `internal/office/scheduler/run_test.go`
|
||||
- `internal/backendapp/main.go` (wire `SetWorkflowStepGetter`)
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (`waveidentity`, `ListWaveMembers`), Task 02 (`models.Run` columns,
|
||||
`IsWakeWaveUniqueViolation`).
|
||||
|
||||
## Risks
|
||||
|
||||
- **Payload merge shape.** `RunContext` is a fixed struct, not a generic
|
||||
map; merging an arbitrary action payload onto it (marshal → overlay →
|
||||
re-marshal, or an equivalent) must not change `encodeRunContext`'s output
|
||||
for the common case where no action payload exists — `CoalesceRun`
|
||||
compares payload bytes for equality-adjacent decisions elsewhere in the
|
||||
codebase, so an unconditional shape change here is a regression risk even
|
||||
though `CoalesceRun` itself is skipped for wave-carrying requests.
|
||||
- **`GetTaskWorkflowStepID` reads the parent's *current* step at wake time,
|
||||
not at any earlier decision point** — this is intentional (the design
|
||||
states this resolution "reads workflow configuration, not child task
|
||||
state," so it may run after the wave-member read), but do not repurpose
|
||||
this value for anything identity-bearing; it is payload content only.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Producers" (P1 cascade), "Wake equivalence between
|
||||
producers" sections.
|
||||
- `internal/workflow/engine/phase2_callbacks.go` (`queueRunPayload`,
|
||||
`QueueRunCallback.Execute`) for the precedence and shape to match.
|
||||
- `internal/orchestrator/workflow_store.go:217-232` (`LoadStep`) for the
|
||||
`GetStep` + `engine.CompileStep` pattern.
|
||||
- `internal/office/repository/sqlite/participants.go:55`
|
||||
(`GetTaskWorkflowStepID`).
|
||||
|
||||
## Results
|
||||
|
||||
Done, in `feat(office): wire the cascade producer onto wave identity`.
|
||||
`cascadeChildrenCompleted` now calls `resolveWaveIdentity` (its own
|
||||
terminality-confirming `ListWaveMembers` read) and `resolveWaveActionPayload`
|
||||
(step lookup + `engine.CompileStep`) after the existing `ListChildStates`
|
||||
loop; `RunContext` carries `WaveKey`/`WaveString`/`ExtraPayload` (all
|
||||
`json:"-"`); `queueRun` (the shared body behind `QueueRun`/`QueueRunCtx`)
|
||||
skips `CoalesceRun` and classifies `IsWakeWaveUniqueViolation` when a wave
|
||||
key is present. `SetWorkflowStepGetter` wired in
|
||||
`internal/backendapp/main.go` to `services.Workflow` alongside the
|
||||
existing `SetWorkflowEngineDispatcher` call.
|
||||
|
||||
One deviation from the original scope note: rather than changing the
|
||||
public `QueueRun`/`QueueRunCtx` signatures (`QueueRun` implements
|
||||
`shared.RunQueuer`, used by other non-wave callers), the wave fields route
|
||||
through a new unexported `queueRun` helper both public methods call.
|
||||
|
||||
`go test ./internal/office/scheduler/...` green (18 new/changed tests
|
||||
across `run_test.go` and `reactivity_children_completed_wave_test.go`,
|
||||
plus `newReactivityTestRepo`'s ad hoc `tasks` fixture gained
|
||||
`archived_at`/`is_ephemeral`/`origin` columns so `ListWaveMembers` can run
|
||||
against it). `golangci-lint run ./internal/office/scheduler/...
|
||||
./internal/backendapp/... --new-from-rev=cd78236315f28982848de4938d56f7722c7f632f`
|
||||
clean.
|
||||
@@ -0,0 +1,135 @@
|
||||
---
|
||||
id: "04-wire-engine-routed-producers"
|
||||
title: "Wire engine-routed producers (P2, P3) onto wave identity"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["01-wave-identity-primitives", "02-wave-identity-persistence"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.12
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.15
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-004.6
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 04: Wire engine-routed producers (P2, P3) onto wave identity
|
||||
|
||||
## Summary
|
||||
|
||||
Open the narrow engine seam that lets `OnChildrenCompletedPayload` carry the
|
||||
two encodings from trigger dispatch through to the inserted run, then wire
|
||||
P2 (`queueChildrenCompletedRun`) and P3 (`ParentWakeReconciler.reconcileOne`)
|
||||
to read the wave members and populate that payload. `wakeOperationID`
|
||||
(P2/P3's existing operation id) is unchanged.
|
||||
|
||||
## In scope
|
||||
|
||||
- `engine.OnChildrenCompletedPayload` gains `WaveKey`, `WaveString string`.
|
||||
- `engine.QueueRunRequest` (adapters.go) gains the same two fields.
|
||||
- `QueueRunCallback.Execute`: type-assert `in.Payload.(OnChildrenCompletedPayload)`
|
||||
and copy the two fields onto the `QueueRunRequest` it builds, when present.
|
||||
`idempotencyKey`/`queueActionDigest` are unchanged.
|
||||
- `runsServiceEngineAdapter.QueueRun` (`backendapp/main.go`) copies the two
|
||||
fields into the `runs/service.QueueRunRequest` it constructs.
|
||||
- P2 (`event_subscribers.go`, `queueChildrenCompletedRun`): after
|
||||
`AreAllChildrenTerminal`, add the `ListWaveMembers` terminality
|
||||
confirmation (queue nothing on a non-terminal member, an empty set, or a
|
||||
read error); build `OnChildrenCompletedPayload` with `WaveKey`/`WaveString`
|
||||
from the sorted member ids. `childSetKey`/`wakeOperationID` computation is
|
||||
unchanged.
|
||||
- P3 (`scheduler_wake_reconciler.go`, `reconcileOne`/`buildPayload`): same
|
||||
addition. The existing `GetChildSetKey`/`GetChildSetKeyTx` re-reads that
|
||||
guard the receipt write are unchanged (state-inclusive key, different
|
||||
purpose).
|
||||
|
||||
## Out of scope
|
||||
|
||||
- P1 (Task 03) and P4 (Task 05).
|
||||
- `ListStuckParents` (Task 06).
|
||||
- Any change to `wakeOperationID`, `formatChildSetKey`, or
|
||||
`parent_child_wake_receipts` semantics (AC-...-004.6).
|
||||
|
||||
## Acceptance
|
||||
|
||||
- A run queued via P2 for a parent whose children are all terminal carries
|
||||
the correct `WakeWaveKey`/`WakeWaveString` on the persisted row.
|
||||
- A run queued via P3 for the same parent and wave derives the identical
|
||||
`WakeWaveKey` as P2 would for that parent (byte-for-byte), so
|
||||
`idx_run_wake_wave` actually collapses a P2/P3 race for the same wave
|
||||
(proven fully in Task 07; this task's own test proves derivation parity,
|
||||
not the race itself).
|
||||
- A wave-member read observing a non-terminal member (P2 or P3) queues
|
||||
nothing and dispatches no engine trigger.
|
||||
- `TestWakeOperationID_UnifiedAcrossEdgeAndReconcilerPaths` continues to
|
||||
pass unmodified.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/workflow/engine/... -run TestQueueRunCallback -v
|
||||
go test ./internal/office/service/... -run TestQueueChildrenCompletedRun -v
|
||||
go test ./internal/office/service/... -run TestParentWakeReconciler -v
|
||||
go test ./internal/office/service/... -run TestWakeOperationID_UnifiedAcrossEdgeAndReconcilerPaths -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/workflow/engine/payloads.go`
|
||||
- `internal/workflow/engine/adapters.go`
|
||||
- `internal/workflow/engine/phase2_callbacks.go`
|
||||
- `internal/workflow/engine/phase8_test.go` (or a new test file for the
|
||||
`QueueRunCallback` wave-key copy)
|
||||
- `internal/backendapp/main.go`
|
||||
- `internal/office/service/event_subscribers.go`
|
||||
- `internal/office/service/scheduler_wake_reconciler.go`
|
||||
- `internal/office/service/scheduler_wake_reconciler_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (`waveidentity`, `ListWaveMembers`), Task 02 (persistence, so the
|
||||
copied fields land somewhere real).
|
||||
|
||||
## Risks
|
||||
|
||||
- **Shared engine-seam files.** This task and Task 05 both touch
|
||||
`payloads.go`/`phase2_callbacks.go`'s type assertion; land this one first
|
||||
so Task 05 builds on an existing seam rather than duplicating it.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Producers" (P2 edge, P3 backstop), "The engine seam is
|
||||
narrow" paragraph under "The wave-member read is the last read, and it
|
||||
carries state".
|
||||
- `internal/workflow/engine/engine.go:417-443` (`executeCallback`) —
|
||||
confirms `Payload any` already flows `HandleInput` → `ActionInput`
|
||||
unmodified, so no change is needed there.
|
||||
|
||||
## Results
|
||||
|
||||
Done, in `feat(office): wire the engine-routed producers onto wave
|
||||
identity`. `OnChildrenCompletedPayload`/`engine.QueueRunRequest` gained
|
||||
`WaveKey`/`WaveString`; `QueueRunCallback.Execute` copies them via a new
|
||||
`waveIdentityPayload` helper (empty for every trigger but
|
||||
`on_children_completed`); `runsServiceEngineAdapter.QueueRun` forwards them
|
||||
into `runs/service.QueueRunRequest.WakeWaveKey`/`WakeWaveString`. P2 and P3
|
||||
share a new `office/service.resolveWaveIdentity` helper (same terminality-
|
||||
confirming shape as Task 03's `office/scheduler` one — duplicated rather
|
||||
than shared across packages, consistent with this codebase's existing
|
||||
`runsServiceEngineAdapter` precedent for small intentional duplication
|
||||
across package boundaries).
|
||||
|
||||
`go test ./internal/office/... ./internal/workflow/... ./internal/runs/...
|
||||
./internal/backendapp/...` and `golangci-lint run
|
||||
./internal/workflow/engine/... ./internal/office/service/...
|
||||
./internal/backendapp/... --new-from-rev=cd78236315f28982848de4938d56f7722c7f632f`
|
||||
both clean.
|
||||
@@ -0,0 +1,117 @@
|
||||
---
|
||||
id: "05-wire-orchestrator-producer"
|
||||
title: "Wire orchestrator (P4) onto wave identity"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["01-wave-identity-primitives", "04-wire-engine-routed-producers"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-004.3
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 05: Wire orchestrator (P4) onto wave identity
|
||||
|
||||
## Summary
|
||||
|
||||
`processOnChildrenCompleted` already reads exactly the wave-member rows
|
||||
(`ListChildCompletionRows` applies the same predicate `ListWaveMembers`
|
||||
does) and already confirms terminality on them before evaluating actions.
|
||||
It needs no new read — only a re-sort by id (its source orders by
|
||||
`created_at` first) and to set the two encodings on the
|
||||
`OnChildrenCompletedPayload` the Task 04 seam now carries through.
|
||||
|
||||
## In scope
|
||||
|
||||
- `childCompletionPayload` (`event_handlers_children_completed.go`) gains a
|
||||
`parentID string` parameter; before building `ChildSummaries`, sort a copy
|
||||
of `rows` ascending by `ID` and derive `WaveKey`/`WaveString` from that
|
||||
sorted id list. `childCompletionOperationID` is unchanged (keeps ordering
|
||||
by the rows' existing `created_at` order and its full
|
||||
state/step/terminal/`updated_at` derivation).
|
||||
- Update `childCompletionPayload`'s one call site in
|
||||
`evaluateChildrenCompleted` to pass `parent.ID`.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- P1/P2/P3 (Tasks 03, 04).
|
||||
- `readyChildCompletionRows`, `allChildrenTerminal`,
|
||||
`annotateTerminalChildSteps` — unchanged; they already establish
|
||||
terminality on the rows the wave identity is derived from
|
||||
(AC-...-002.15 satisfied by construction).
|
||||
- Any change to the operation ledger, `EvaluateOnly` transition lifecycle,
|
||||
or reopen-driven step transition (AC-...-004.3).
|
||||
|
||||
## Acceptance
|
||||
|
||||
- For a fixed parent and child set, P4's derived `WakeWaveKey` is
|
||||
byte-identical to what P1/P2/P3 would derive for the same parent and
|
||||
wave-member set (proven together with Task 07's cross-producer test, but
|
||||
this task's own unit test covers P4's re-sort in isolation: feed rows in
|
||||
`created_at` order that differ from `id` order and assert the derived key
|
||||
matches the id-sorted expectation).
|
||||
- `TestProcessOnChildrenCompleted_DedupedAcrossReinit` (or the equivalent
|
||||
existing operation-ledger test) continues to pass unmodified.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/orchestrator/... -run TestProcessOnChildrenCompleted -v
|
||||
go test ./internal/orchestrator/... -run TestChildCompletionPayload -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/orchestrator/event_handlers_children_completed.go`
|
||||
- `internal/orchestrator/event_handlers_children_completed_test.go` (or
|
||||
wherever the existing P4 tests live — confirm exact file before editing)
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (`waveidentity`), Task 04 (the engine seam this payload now flows
|
||||
through — `OnChildrenCompletedPayload`'s new fields and
|
||||
`QueueRunCallback.Execute`'s type assertion must exist first).
|
||||
|
||||
## Risks
|
||||
|
||||
None beyond the shared-seam ordering already called out in Task 04 — this
|
||||
task is a small, additive change to one existing function.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Producers" (P4 orchestrator) section, "The wave-member
|
||||
read is the last read, and it carries state" (P4 paragraph).
|
||||
- `internal/task/repository/sqlite/task.go:2055-2070`
|
||||
(`ListChildCompletionRows`) for the `created_at, id` ordering this task
|
||||
must correct for.
|
||||
|
||||
## Results
|
||||
|
||||
Done, in `feat(orchestrator): derive wave identity in the children-completed
|
||||
producer`. `childCompletionPayload(parentID string, rows
|
||||
[]models.ChildCompletionRow)` derives `WaveKey`/`WaveString` via a new
|
||||
`childCompletionWaveIdentity` helper (copies row ids into a fresh slice,
|
||||
`sort.Strings`, then `waveidentity.WaveKey`/`WaveString` — never mutates
|
||||
`rows`, so `childCompletionOperationID`'s created_at-ordered derivation,
|
||||
called earlier in `processOnChildrenCompleted`, is unaffected regardless of
|
||||
call order). No new read: `readyChildCompletionRows` already applies the
|
||||
same predicate `ListWaveMembers` does and already confirms terminality
|
||||
before `rows` reaches `evaluateChildrenCompleted`; it also already gates on
|
||||
`len(rows) == 0`, so `childCompletionPayload` never runs against zero wave
|
||||
members in production (AC-...-001.7 holds by construction, no extra guard
|
||||
needed).
|
||||
|
||||
`go test ./internal/orchestrator/...` (full package, ~84s) and
|
||||
`golangci-lint run ./internal/orchestrator/...
|
||||
--new-from-rev=cd78236315f28982848de4938d56f7722c7f632f` both clean.
|
||||
`TestProcessOnChildrenCompleted_DedupedAcrossReinit` passes unmodified.
|
||||
@@ -0,0 +1,177 @@
|
||||
---
|
||||
id: "06-backstop-admission"
|
||||
title: "Backstop admission compares wave identity"
|
||||
status: done
|
||||
wave: 4
|
||||
depends_on: ["01-wave-identity-primitives", "02-wave-identity-persistence"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-003
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.1
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.2
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.3
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.4
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.5
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.6
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.8
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.9
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-003.10
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 06: Backstop admission compares wave identity
|
||||
|
||||
## Summary
|
||||
|
||||
Rewrite `ListStuckParents`' staleness arm to compare the parent's current
|
||||
wave string (computed in SQL) against `runs.wake_wave_string`, add a
|
||||
wave-member existence gate so a parent with no possible wave is never
|
||||
listed, and keep a parent-scoped timestamp fallback for pre-upgrade rows
|
||||
that carry no wave identity at all.
|
||||
|
||||
## In scope
|
||||
|
||||
- New `internal/db/dialect` helper for an ordered, comma-joined id
|
||||
aggregate (SQLite `GROUP_CONCAT` over an ordered subquery; PostgreSQL
|
||||
`string_agg(... ORDER BY ...)`), added beside `JSONExtract`.
|
||||
- `ListStuckParents`: a wave-string CTE column
|
||||
(`p.id || '|' || COALESCE(<helper>, '')`) over the wave-member predicate.
|
||||
- A new `EXISTS` gate (wave-member predicate) beside the existing
|
||||
archived-only `EXISTS`, removing a parent with no wave members from
|
||||
candidacy (AC-...-003.9). Additive and narrowing only.
|
||||
- Rewrite the second `NOT EXISTS` arm into three clauses: queued/claimed
|
||||
blocks unconditionally; a terminal run whose `wake_wave_string` matches
|
||||
the parent's current one blocks; only when the parent has **no**
|
||||
`task_children_completed` run with `wake_wave_key <> ''` at all does the
|
||||
existing `requested_at >= newest_child_updated_at` timestamp rule apply
|
||||
(evaluated once per parent, not per row — AC-...-003.6).
|
||||
|
||||
## Out of scope
|
||||
|
||||
- The first `NOT EXISTS`/receipt-comparison arm (unchanged — receipts stay
|
||||
state-inclusive, AC-...-004.6).
|
||||
- Porting this query's existing `json_extract`/unbranched `GROUP_CONCAT` to
|
||||
dialect form (`53c24173` — do not widen or close that gap here; only the
|
||||
new wave-string fragment is dialect-branched).
|
||||
- The whole-second comparison in the queued-or-claimed arm (`b7e29d7c`).
|
||||
|
||||
## Acceptance
|
||||
|
||||
- A parent with a delivered, unchanged wave is not a candidate; a
|
||||
non-state child edit after delivery does not make it one; a real
|
||||
wave-member change does.
|
||||
- A parent whose only children are archived/ephemeral/automation-origin is
|
||||
never listed.
|
||||
- A parent with a pre-upgrade terminal run (`wake_wave_key = ''`) is judged
|
||||
by the timestamp rule; once any keyed run exists for that parent, the
|
||||
timestamp rule is never consulted for it again, even for an older
|
||||
pre-upgrade row on the same parent.
|
||||
- A failed/cancelled run whose wave string matches the current one keeps
|
||||
blocking across an unrelated child edit and is unblocked only by a
|
||||
wave-member change.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/db/dialect/... -run TestOrderedIDConcat -v
|
||||
go test ./internal/office/repository/sqlite/... -run TestListStuckParents -v
|
||||
KANDEV_TEST_POSTGRES_DSN=... go test ./internal/office/repository/sqlite/... -run TestListStuckParents -v
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/db/dialect/json.go` (or a new file beside it for the aggregate
|
||||
helper, if it doesn't fit `json.go`'s theme)
|
||||
- `internal/db/dialect/dialect_test.go`
|
||||
- `internal/office/repository/sqlite/wake_receipts.go`
|
||||
- `internal/office/repository/sqlite/wake_receipts_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (wave-member predicate, matched exactly), Task 02 (`wake_wave_key`/
|
||||
`wake_wave_string` columns must exist to query against).
|
||||
|
||||
## Risks
|
||||
|
||||
- **Ordering divergence between Go and PostgreSQL collation** — the
|
||||
requirements' AC-...-001.13 names this as a silent, PostgreSQL-only
|
||||
failure mode. The gated Postgres test must assert byte-identical ordering
|
||||
with a fixture using real UUID-shaped ids, not sequential test ids that
|
||||
would happen to sort the same everywhere.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Backstop admission" section in full, including its
|
||||
worked SQL fragments.
|
||||
- `internal/office/repository/sqlite/wake_receipts.go` (current
|
||||
`ListStuckParents`, `child_set_key` aggregate idiom to mirror for the new
|
||||
wave-string aggregate).
|
||||
- `internal/db/dialect/json.go` (signature/doc-comment style to match).
|
||||
|
||||
## Results
|
||||
|
||||
Done, in `feat(office): compare wave identity in the backstop reconciler`.
|
||||
|
||||
`internal/db/dialect/aggregate.go` (new file, not `json.go` — a distinct
|
||||
aggregate-vs-extraction theme) adds `OrderedIDConcat(driver, where string)
|
||||
string`, hardcoded to `tasks.id` per "simplest implementation that fully
|
||||
meets the current requirement" rather than a generic group-concat builder.
|
||||
SQLite orders via an `ORDER BY` inside the `GROUP_CONCAT` subquery;
|
||||
PostgreSQL via `string_agg(... ORDER BY ...)` in the aggregate call itself.
|
||||
`TestOrderedIDConcat_FragmentShape` checks both dialects' exact fragment
|
||||
text; `TestOrderedIDConcat_SQLite_OrdersAscendingByID` and its
|
||||
`KANDEV_TEST_POSTGRES_DSN`-gated Postgres twin insert UUID-shaped ids out
|
||||
of order and assert the concatenated result is byte-identical and
|
||||
ascending across both dialects, per the Risks section's collation-drift
|
||||
warning.
|
||||
|
||||
`ListStuckParents` gained a `wave_string` CTE column
|
||||
(`p.id || '|' || COALESCE(OrderedIDConcat(...), '')`) over the wave-member
|
||||
predicate (not archived, not ephemeral, not automation-origin — matching
|
||||
`ListWaveMembers`), plus a second `EXISTS` gate applying that same
|
||||
predicate to remove parents with no possible wave from candidacy
|
||||
(AC-...-003.9), added beside — not replacing — the existing archived-only
|
||||
`EXISTS`. The second `NOT EXISTS` arm is now: queued/claimed blocks
|
||||
unconditionally; a terminal run whose `wake_wave_string` matches the
|
||||
parent's current one blocks (across any unrelated, non-wave-member child
|
||||
edit, unblocked only by a genuine wave-member change); a separate
|
||||
`AND (EXISTS(...) OR NOT EXISTS(...))` clause keeps the pre-upgrade
|
||||
timestamp fallback but scopes it to the parent as a whole
|
||||
(`wake_wave_key <> ''` for *any* row for that parent, any status, any
|
||||
wave), not per-row — so a parent's first wave-keyed run retires the
|
||||
timestamp rule for it permanently, even against an older, still-recent
|
||||
pre-upgrade row for the same parent (AC-...-003.6).
|
||||
|
||||
New tests in `wake_receipts_wave_identity_test.go`:
|
||||
`TestListStuckParents_WaveMatchBlocksAcrossNonMemberChildEdit_UnblockedOnWaveMemberChange`
|
||||
(table-driven over finished/failed/cancelled — delivered-unchanged-wave not
|
||||
a candidate, an automation-origin child completing doesn't unblock it, a
|
||||
real wave-member joining does),
|
||||
`TestListStuckParents_ExcludesParentWithOnlyNonWaveMemberChildren`
|
||||
(ephemeral-only, automation-origin-only, and mixed non-member-only parents
|
||||
are all excluded, with a control candidate), and
|
||||
`TestListStuckParents_KeyedRunRetiresTimestampFallbackForParent` (a recent
|
||||
pre-upgrade terminal run that would otherwise still block under the
|
||||
timestamp rule is bypassed once any wave-keyed run — even for an unrelated,
|
||||
stale wave — exists for the parent).
|
||||
|
||||
All 8 pre-existing `TestListStuckParents_*` tests (and their subtests)
|
||||
pass unmodified, confirming the rewrite is backward-compatible with every
|
||||
scenario that predates wave identity (their fixture runs never set
|
||||
`wake_wave_key`, so they exercise the compatibility/timestamp-fallback
|
||||
path exactly as before).
|
||||
|
||||
`go build ./...`, `go test ./internal/office/repository/sqlite/...
|
||||
./internal/db/dialect/...` (full packages), and `golangci-lint run
|
||||
./internal/office/repository/sqlite/... ./internal/db/dialect/...
|
||||
--new-from-rev=cd78236315f28982848de4938d56f7722c7f632f` all clean. The
|
||||
gated `TestOrderedIDConcat_Postgres_OrdersAscendingByID` skips on this
|
||||
runner (no `KANDEV_TEST_POSTGRES_DSN`), consistent with not provisioning
|
||||
Docker/Postgres for this card.
|
||||
@@ -0,0 +1,197 @@
|
||||
---
|
||||
id: "07-race-and-regression-coverage"
|
||||
title: "Race, parity, and regression coverage"
|
||||
status: done
|
||||
wave: 5
|
||||
depends_on:
|
||||
- "02-wave-identity-persistence"
|
||||
- "03-wire-cascade-producer"
|
||||
- "04-wire-engine-routed-producers"
|
||||
- "05-wire-orchestrator-producer"
|
||||
- "06-backstop-admission"
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-003
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.3
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.14
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.15
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-002.16
|
||||
- AC-OFFICE-WAKE-WAVE-IDENTITY-004.1
|
||||
system_design:
|
||||
- ../../specs/office/system-design/parent-wake-wave-identity.md
|
||||
---
|
||||
|
||||
# Task 07: Race, parity, and regression coverage
|
||||
|
||||
## Summary
|
||||
|
||||
Close the loop with the tests that need every producer and the rewritten
|
||||
admission query to exist at once: the cascade-vs-reconciler race through
|
||||
`office/scheduler`'s own queue path, full cross-producer derivation parity,
|
||||
the coalescing exclusion end-to-end, and the read-skew guarantee.
|
||||
|
||||
## In scope
|
||||
|
||||
- **Race test:** cascade (P1) racing one `ParentWakeReconciler` tick for the
|
||||
same parent and wave, driven through `office/scheduler.QueueRun` (not only
|
||||
`runs/service`) — the two classification sites this initiative added.
|
||||
Asserts exactly one `runs` row and that the losing side logs no failure.
|
||||
Mirrors `TestQueueRun_DedupesOnIdempotencyIndexRace`
|
||||
(`runs/service/service_test.go:578`) and its Postgres twin.
|
||||
- **Cross-producer parity:** for one fixture parent and child set (including
|
||||
an ephemeral and an automation-origin child), all four producers derive
|
||||
byte-identical `WakeWaveKey`/`WakeWaveString`.
|
||||
- **Coalescing:** a second wave-carrying request for a *different* parent,
|
||||
same target agent, inside the coalescing window, does not merge into the
|
||||
first parent's queued (also wave-carrying) run — two rows exist, each with
|
||||
its own identity.
|
||||
- **Read skew:** a wave-member read that observes a non-terminal member
|
||||
queues nothing; a later read of the same parent, once genuinely all
|
||||
terminal, is delivered normally (not permanently suppressed).
|
||||
- **Payload parity end-to-end:** the cascade's queued run and an
|
||||
equivalent engine-routed run for a step with the same
|
||||
`on_children_completed` `queue_run` payload carry the same resolved
|
||||
payload keys.
|
||||
- Confirm the "MUST NOT be weakened" tests
|
||||
(`reactivity_children_completed_test.go`,
|
||||
`scheduler_wake_reconciler_identity_test.go`) still pass unmodified.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Any further production code change — this task is test-only. A finding
|
||||
here that requires a production fix routes back to the relevant earlier
|
||||
work order, not a change made inline in this one.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- The race test fails on the pre-Task-03/04 code (verify by temporarily
|
||||
reverting to confirm it actually exercises the new constraint, then
|
||||
restore) and passes on the finished tree.
|
||||
- All new tests pass on both SQLite and PostgreSQL where a dialect twin is
|
||||
required.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/office/scheduler/... ./internal/office/service/... \
|
||||
./internal/office/repository/sqlite/... ./internal/orchestrator/... \
|
||||
./internal/runs/service/... ./internal/runs/repository/sqlite/... \
|
||||
-race -count=1
|
||||
KANDEV_TEST_POSTGRES_DSN=... go test ./internal/runs/service/... \
|
||||
./internal/office/repository/sqlite/... -run Postgres -count=1
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `internal/office/service/scheduler_wake_reconciler_test.go` (or a new
|
||||
shared-harness test file, since the race test needs both a direct
|
||||
`office/scheduler.QueueRun` caller and a `ParentWakeReconciler` against
|
||||
one database)
|
||||
- `internal/runs/service/service_test.go`
|
||||
- `internal/runs/service/service_postgres_test.go`
|
||||
- A new cross-producer parity test file (exact location depends on which
|
||||
package can import all four producers' derivation call sites without a
|
||||
cycle — likely a `_test.go` file in `internal/office/service` with build
|
||||
tags or fakes for the orchestrator piece, confirmed at implementation
|
||||
time)
|
||||
|
||||
## Dependencies
|
||||
|
||||
Tasks 02 through 06 — this is the closing verification pass.
|
||||
|
||||
## Risks
|
||||
|
||||
- **Test-only task that could hide a production gap.** If the race test
|
||||
cannot actually be made to race deterministically (e.g. both paths commit
|
||||
through the same test transaction manager without true concurrency), the
|
||||
test would pass without proving anything. Use `synctest` or explicit
|
||||
goroutine synchronization (start both inserts, block one at a lock point,
|
||||
release both) per the backend's testing conventions — not
|
||||
`time.Sleep`-based interleaving.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- System design: "Testing" section in full.
|
||||
- Requirements document: "REQUIRED TESTS" section of the task's Kandev
|
||||
running plan (this initiative's own card), which enumerates the same
|
||||
suite from the review history.
|
||||
- `internal/runs/service/service_test.go:578` and
|
||||
`service_postgres_test.go:27` as the race-test pattern.
|
||||
|
||||
## Results
|
||||
|
||||
Done, in `test(office): close race, parity, and regression coverage for
|
||||
wave identity`.
|
||||
|
||||
Most of this task's Acceptance items were already covered by Tasks 02-05's
|
||||
own tests, confirmed unmodified in this pass:
|
||||
|
||||
- **Coalescing:** `TestQueueRunCtx_WaveCarryingRequest_NotCoalesced`
|
||||
(`office/scheduler/run_test.go`, Task 03) and
|
||||
`TestQueueRun_WakeWaveKey_DifferentAgentBothQueued`
|
||||
(`runs/service/service_test.go`, Task 02).
|
||||
- **Read skew:** `TestResolveWaveIdentity_NonTerminalMember_ReturnsNotOK`
|
||||
(`office/scheduler/reactivity_children_completed_wave_test.go`, Task 03).
|
||||
- **Payload parity:** `TestCascadeChildrenCompleted_PayloadParity_*`
|
||||
(same file, Task 03).
|
||||
- **Per-producer derivation parity:**
|
||||
`TestQueueRunCallback_OnChildrenCompletedCopiesWaveIdentity` (Task 04),
|
||||
`TestChildrenCompletedWaveIdentity_IdenticalAcrossEdgeAndReconcilerPaths`
|
||||
(Task 04, P2 vs P3), `TestChildCompletionPayload_MatchesCascadeDerivationForSameParentAndSet`
|
||||
(Task 05, P4 vs `waveidentity` directly).
|
||||
- **"MUST NOT be weakened" regression suites:**
|
||||
`reactivity_children_completed_test.go` (5 pre-existing tests) and
|
||||
`scheduler_wake_reconciler_identity_test.go`
|
||||
(`TestParentWakeReconciler_*`, 7 tests) both pass unmodified.
|
||||
|
||||
This task added the two genuinely new closing pieces:
|
||||
|
||||
- `internal/office/scheduler/wave_identity_race_test.go`:
|
||||
`TestWaveIdentityRace_CascadeVsEngineRoutedPath_CollapsesToOneRun` races
|
||||
P1's direct-insert path (`SchedulerService.QueueRunCtx` -> `queueRun` ->
|
||||
`ss.repo.CreateRun`, classified by
|
||||
`runssqlite.IsWakeWaveUniqueViolation`) against an engine-routed path
|
||||
(`runsservice.Service.QueueRun` -> `insertRun` -> `CreateRunTx`,
|
||||
classified by Task 02's `errWakeWaveKeyConflict` handling) for the same
|
||||
wave identity and target agent, through two goroutines started together
|
||||
via a shared barrier channel (not a sequential call pair) — both
|
||||
`office/scheduler.SchedulerService` and `runsservice.Service` are wired
|
||||
against the exact same in-memory SQLite DB via
|
||||
`officeRepo.RunsRepository()`, so the two classification sites this
|
||||
initiative added contend on the real `idx_run_wake_wave` index.
|
||||
Confirmed the test has teeth: temporarily gave the engine-routed request
|
||||
a different `WakeWaveKey` and watched the assertion fail (`runs = 2, want
|
||||
exactly 1`), then reverted before committing. Passes reliably across
|
||||
repeated `-race -count=10` runs.
|
||||
- `internal/task/repository/sqlite/wave_member_parity_test.go`:
|
||||
`TestListWaveMembersMatchesListChildCompletionRows` builds one shared
|
||||
`*sqlx.DB`, initializes both `task/repository/sqlite.NewWithDB` (owns the
|
||||
`tasks` table in production) and `office/repository/sqlite.NewWithDB`
|
||||
against it, seeds one parent with ordinary/ephemeral/automation-origin/
|
||||
archived children, and asserts `ListWaveMembers` (P1/P2/P3's read) and
|
||||
`ListChildCompletionRows` (P4's read, id-sorted the way
|
||||
`childCompletionPayload` sorts it) return the identical wave-member id
|
||||
set — the AC-...-001.8 cross-producer predicate-equivalence proof Task
|
||||
01's plan text called for but that file's own tests didn't yet include.
|
||||
|
||||
`go build ./...` clean.
|
||||
`go test ./internal/office/scheduler/... ./internal/office/service/...
|
||||
./internal/office/repository/sqlite/... ./internal/orchestrator/...
|
||||
./internal/runs/service/... ./internal/runs/repository/sqlite/...
|
||||
./internal/task/repository/sqlite/... -race -count=1` — all packages pass.
|
||||
`golangci-lint run ./internal/office/scheduler/...
|
||||
./internal/task/repository/sqlite/...
|
||||
--new-from-rev=cd78236315f28982848de4938d56f7722c7f632f` clean. The
|
||||
Postgres-gated command in Verification was not run — no
|
||||
`KANDEV_TEST_POSTGRES_DSN` provisioned in this environment, consistent
|
||||
with the card's own instruction not to stand up Docker/Postgres here;
|
||||
every dialect-sensitive change (Task 06's `OrderedIDConcat`) already has
|
||||
its own gated Postgres twin from that task.
|
||||
@@ -0,0 +1,332 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
created: 2026-09-08
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Parent Wake Wave Identity Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
When every direct child of a parent task reaches a terminal state, Kandev wakes
|
||||
the parent's agent with a `task_children_completed` run. Four independent
|
||||
producers can emit that wake for the same parent at the same moment, and each
|
||||
identifies the completion event differently today. The consequences are opposite
|
||||
failures of one missing primitive:
|
||||
|
||||
- **Duplicate wake.** Two producers describing the same completion event mint
|
||||
structurally different run identities, so the run queue's uniqueness constraint
|
||||
cannot collapse them. The parent agent runs twice, pays twice, and re-reads the
|
||||
same child results.
|
||||
- **Spurious wake.** The backstop decides "has this parent already been woken for
|
||||
its current children" by comparing timestamps, and every task write stamps
|
||||
`updated_at` — so editing a finished child's title, priority, or labels makes a
|
||||
delivered wake look stale and queues a needless autonomous run.
|
||||
|
||||
This capability defines a single **completion wave identity**: one value naming
|
||||
*which completion event* a wake belongs to, computed the same way by every
|
||||
producer, persisted on the run, and compared instead of a timestamp.
|
||||
|
||||
Office owns this contract because the outcome is an Office wakeup policy
|
||||
decision: whether an autonomous `task_children_completed` run enters the Office
|
||||
run queue. It reads and constrains, but does not own, the workflow engine's
|
||||
`on_children_completed` trigger and operation ledger and the task system's
|
||||
parent/child relationship and `updated_at` stamping. The task system's
|
||||
[subtask completion trigger](../../tasks/requirements/subtask-completion-trigger.md)
|
||||
still owns when the trigger fires and what its payload contains; this capability
|
||||
constrains only the identity carried by the resulting run.
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Completion wave:** the event of a parent's full set of wave members being
|
||||
terminal at once. Iterative delegation produces one wave per round.
|
||||
- **Wave member:** a direct child task of the parent that counts towards its
|
||||
parent's completion wave: `archived_at` unset, `is_ephemeral` false, and
|
||||
`origin` not `automation_run`. Children failing any of these are excluded from
|
||||
wave identity throughout. This is the predicate the orchestrator's existing
|
||||
children-completed readiness query applies; the design records why that makes it
|
||||
the only one all four producers can share.
|
||||
- **Wave identity:** the value naming a completion wave. Equal values mean the
|
||||
same wave. It has the two encodings below, which are two spellings of one
|
||||
identity rather than two identities.
|
||||
- **Wave string:** the canonical encoding of a wave identity: the parent id and
|
||||
its wave members' ids in ascending `tasks.id` order, joined by a fixed
|
||||
separator. It is what the backstop's candidate query compares, so it must be
|
||||
derivable with plain SQL string aggregation. A parent with no wave members has
|
||||
no wave and no wave string; an SQL expression of the same shape evaluated per
|
||||
row, before that parent is known to have any member, is an intermediate, not a
|
||||
wave string, until the parent passes the wave-member gate.
|
||||
- **Wave key:** the fixed-length digest of the wave string, and the encoding the
|
||||
run queue's uniqueness constraint indexes, where an unbounded value would
|
||||
eventually exceed an index entry limit.
|
||||
- **Producer:** any code path that can cause a `task_children_completed` run to
|
||||
be queued for a parent. Four exist; the design enumerates them.
|
||||
- **Target agent:** the agent profile a queued run is addressed to. One wave can
|
||||
legitimately fan out to several when a workflow step configures it.
|
||||
- **Terminal state:** the child task states that count as done, as already
|
||||
defined by the `on_children_completed` trigger.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-WAKE-WAVE-IDENTITY-001: One canonical completion-wave identity
|
||||
|
||||
**Intent:** A wave must have exactly one name, so that agreement between
|
||||
producers is a property of the definition rather than of four code paths
|
||||
happening to match.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.1:** Wave identity shall be a pure function
|
||||
of the parent task id and the set of its wave-member child task ids. No other
|
||||
input shall affect it.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.2:** Child ids shall enter the derivation
|
||||
ordered ascending by `tasks.id`, which is unique, so no tiebreak column is
|
||||
required and none shall be introduced. A producer holding rows in any other
|
||||
order shall re-sort first; arrival order is not sufficient.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.3:** Children that are not wave members
|
||||
shall be excluded from the derivation. A child entering or leaving wave
|
||||
membership — by archiving, unarchiving, or being created ephemeral or
|
||||
automation-origin — shall change the wave identity only when it changes the
|
||||
wave-member id set.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.4:** When a child moves between two terminal
|
||||
states, `CANCELLED` to `COMPLETED` say, the wave identity shall not change.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.5:** When any child field other than wave
|
||||
membership changes — title, description, priority, labels, assignee, workflow
|
||||
step, `updated_at` — the wave identity shall not change.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.6:** When a wave member is added or removed,
|
||||
the wave identity shall change.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.7:** A parent with no wave members has no
|
||||
wave, and no `task_children_completed` run shall be queued for it.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.8:** For the same parent and wave-member id
|
||||
set, every producer shall derive a byte-identical wave string and wave key from
|
||||
the same wave-member predicate; a producer whose existing row source applies a
|
||||
different predicate shall be brought onto the shared one.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.9:** Wave identity shall not depend on the
|
||||
wake payload: child-summary truncation, a missing child summary, and missing or
|
||||
present pull-request enrichment shall not change it.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.10:** Wave identity derivation shall have no
|
||||
failure mode of its own: it shall be computable from values the caller already
|
||||
holds, without a further read that can error.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.11:** The wave string and the wave key shall
|
||||
be two encodings of one identity: the key shall be a pure function of the
|
||||
string, so equal strings always yield equal keys, and both shall be derived once
|
||||
per queued run from the same member-id set rather than computed independently at
|
||||
two call sites.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.12:** The backstop's candidate query shall
|
||||
derive the current wave string using only string aggregation available on both
|
||||
SQLite and PostgreSQL; no hash or digest shall be required inside SQL on either
|
||||
dialect.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-001.13:** The ascending `tasks.id` order in .2
|
||||
shall be the same total order in application code and in SQL on both dialects.
|
||||
It rests on task ids being lowercase hexadecimal UUIDs, for which byte order and
|
||||
collation order coincide, and shall not depend on any database locale or
|
||||
collation setting.
|
||||
|
||||
### REQ-OFFICE-WAKE-WAVE-IDENTITY-002: At most one wake per wave per target agent
|
||||
|
||||
**Intent:** Producers race by design — the reconciler exists precisely because
|
||||
an edge-triggered dispatch can be lost — so correctness must come from a durable
|
||||
constraint the second arrival cannot bypass, not from producers avoiding each
|
||||
other.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.1:** Every queued `task_children_completed`
|
||||
run shall durably record the wave identity it was queued for, in both encodings.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.2:** The run queue shall enforce durable
|
||||
uniqueness over (wave key, target agent).
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.3:** When two producers concurrently queue a
|
||||
run for the same wave and target agent, exactly one run row shall exist
|
||||
afterwards.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.4:** The producer whose insert loses that
|
||||
race shall observe a deduplicated outcome, not an error; the wake shall not be
|
||||
logged as a failure or retried. This holds for every producer, including one
|
||||
inserting directly rather than through the shared run-admission service —
|
||||
reaching the queue by an unclassified path is not an exemption.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.5:** Deduplication shall behave identically
|
||||
on SQLite and PostgreSQL.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.6:** Deduplication shall not be time-bounded:
|
||||
a second attempt at the same wave shall be deduplicated however long after the
|
||||
first it arrives.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.7:** When one wave fans out to two distinct
|
||||
target agents, each shall receive exactly one run.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.8:** A run for a different wave of the same
|
||||
parent shall not be suppressed by .2. A parent that completes wave 1, is given
|
||||
new children, and completes wave 2 shall be woken for both.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.9:** Runs of every other reason shall be
|
||||
unaffected: recording no wave identity leaves a run outside this constraint
|
||||
entirely.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.10:** The wake the surviving run delivers
|
||||
shall be equivalent to the wake the losing producer would have delivered. No
|
||||
producer shall depend on being the winner, and no producer's wake shall carry
|
||||
context that another producer's wake for the same wave omits. The obligation
|
||||
binds whenever two producers would queue for the same wave *and* the same
|
||||
target agent, which is when one of the two is suppressed; producers queueing to
|
||||
different target agents each deliver their own run and are not compared.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.11:** Retrying an already-queued run shall
|
||||
not be rejected by .2: a retry continues an existing run and is not a second
|
||||
wake for the wave.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.12:** When a producer cannot read the
|
||||
parent's wave members, it shall queue no run and record no wave identity; the
|
||||
backstop shall deliver that wake later.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.13:** When one wave fans out across several
|
||||
workflow participants resolving to the same target agent profile, that profile
|
||||
shall receive exactly one run, not one per participant. Holding two
|
||||
roles is not a second wave.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.14:** A run's recorded wave identity shall
|
||||
always describe the wake that run delivers. Where the run queue would otherwise
|
||||
merge a later request into a queued run by replacing that run's payload, a
|
||||
request carrying a wave identity shall not be merged into any run, and a queued
|
||||
run carrying one shall not be merged into; both shall be admitted independently
|
||||
and reconciled only by .2. One consequence is accepted: two wakes for *different*
|
||||
parents addressed to the same target agent inside the merge window now produce
|
||||
two runs, where one previously absorbed and discarded the other.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.15:** A producer shall record a wave
|
||||
identity only for a wave-member set it has observed entirely terminal in a
|
||||
single read. That read shall be the producer's last read of child task state
|
||||
before queueing and shall yield each member's state as well as its id; if any
|
||||
member is not terminal in it, the producer shall queue no run and the backstop
|
||||
shall deliver the wake later. Recording an identity for a set never observed
|
||||
simultaneously terminal would permanently suppress the real wave, because .6
|
||||
makes deduplication unbounded.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-002.16:** A producer that queues a wake without
|
||||
consulting the workflow engine shall attach the same workflow-authored action
|
||||
payload the engine would attach for the parent's current workflow step, so that
|
||||
.10 holds for a workflow that authors one. Where it cannot be resolved, the
|
||||
producer shall still queue the wake without it and record the omission: the wake
|
||||
outranks an optional payload, and that wake is unconditional under .004.2.
|
||||
|
||||
### REQ-OFFICE-WAKE-WAVE-IDENTITY-003: Backstop admission compares waves, not time
|
||||
|
||||
**Intent:** The reconciler must re-deliver a genuinely lost wake and not
|
||||
re-deliver a genuinely delivered one. Deciding that by timestamp makes the answer
|
||||
depend on unrelated edits; deciding it by wave identity makes it depend on the
|
||||
thing the wake is about.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.1:** When a terminal
|
||||
`task_children_completed` run exists for the parent whose recorded wave identity
|
||||
equals the parent's current one, the parent shall not be a candidate for
|
||||
re-delivery.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.2:** When a wake has been delivered for the
|
||||
current wave and a user then edits a terminal child's title, priority, or
|
||||
labels, the parent shall not become a candidate and no run shall be queued.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.3:** When a wake has been delivered and the
|
||||
parent's wave-member set subsequently changes to one whose wave identity has not
|
||||
already been queued for that parent and target agent, the parent shall become a
|
||||
candidate and a run for the new wave shall be queued. A change returning the
|
||||
parent to an already-queued identity is the id-set-reuse exclusion under *Out of
|
||||
scope*, suppressed by .002.6, and is not required to be re-delivered.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.4:** While a `task_children_completed` run
|
||||
for the parent is queued or claimed, the parent shall not be a candidate,
|
||||
regardless of which wave that run was queued for.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.5:** A parent with no
|
||||
`task_children_completed` run carrying a wave identity shall continue to be
|
||||
judged by the timestamp rule its runs were written under, so an upgrade queues
|
||||
no re-wake storm.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.6:** Once any `task_children_completed` run
|
||||
carrying a wave identity exists for a parent, that parent's admission shall be
|
||||
decided by wave identity alone and .5 shall never be consulted for it again,
|
||||
whichever of its runs is oldest. The compatibility rule is scoped to the parent,
|
||||
not the individual run row.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.7:** A parent whose assignee cannot accept a
|
||||
run, and a parent whose delivery evidence is missing, shall be admitted or
|
||||
rejected exactly as today. This capability changes only how delivery *for the
|
||||
current wave* is recognised, on the terms .10 states.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.8:** Admission shall decide staleness inside
|
||||
the candidate query. A parent permanently ineligible because its current wave
|
||||
was already delivered shall not be returned and then rejected in the caller,
|
||||
where it would occupy a row of the query's fixed result limit on every tick.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.9:** A parent with no wave members shall not
|
||||
be returned as a candidate: no producer will ever queue for it (.001.7), so
|
||||
returning it creates exactly the permanently rejected candidate .8 forbids. This
|
||||
gate shall only remove candidates, never admit one not admitted today.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-003.10:** Each admission clause shall name the
|
||||
run statuses it applies to, and those sets shall be the ones in force today: a
|
||||
queued or claimed run blocks under .4; a finished, failed, or cancelled run is
|
||||
the delivery evidence .1 and .5 compare; a run of any status counts when deciding
|
||||
whether a parent has left the compatibility rule in .5. A failed or cancelled
|
||||
wake shall therefore keep blocking its parent and, because wave identity no
|
||||
longer moves on an unrelated child edit, shall block that wave permanently rather
|
||||
than until the next such edit. This is intended: the runtime contract already
|
||||
requires an explicit user retry after a terminal execution failure, and the
|
||||
edit-driven re-arming it replaces was an artefact of the churn being removed.
|
||||
The escape hatches are an explicit retry and any wave-member change.
|
||||
|
||||
### REQ-OFFICE-WAKE-WAVE-IDENTITY-004: Every producer, no other behaviour change
|
||||
|
||||
**Intent:** A constraint one producer can route around is decorative; at the same
|
||||
time, three of the four carry behaviour unrelated to wave identity that must
|
||||
survive untouched.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.1:** All four producers shall record the
|
||||
canonical wave identity on every run they queue.
|
||||
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.2:** The Office scheduler cascade shall
|
||||
continue to queue a wake for a parent with no active workflow session, and its
|
||||
wake shall not become conditional on the engine accepting a trigger.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.3:** The orchestrator's workflow operation
|
||||
ledger identifier shall not change, and parent step transitions driven by
|
||||
`on_children_completed`, including on a reopened and recompleted child set,
|
||||
shall fire exactly as today.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.4:** A non-Office workflow that configures
|
||||
`on_children_completed` shall observe no behaviour change other than those
|
||||
.002.7, .002.13 and .002.14 define.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.5:** Upgrading shall require no backfill of
|
||||
historical runs and shall queue no run as a direct consequence of the upgrade.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.6:** The parent wake receipt shall keep its
|
||||
state-inclusive child-set key and its delivery-evidence fields. This capability
|
||||
adds a wave identity to runs; it does not redefine the receipt.
|
||||
- **AC-OFFICE-WAKE-WAVE-IDENTITY-004.7:** The definition of when a parent's
|
||||
children are *terminal enough* for a wake shall not change. Which children count
|
||||
towards it, and the deliberate archived-child divergence between the
|
||||
edge-triggered readiness check and the backstop's candidate query, are outside
|
||||
this capability. Readiness and wave identity may disagree about which children
|
||||
they count, and the wave-member predicate shall not be propagated into any
|
||||
terminality predicate. Exactly two readiness-adjacent additions are in scope and
|
||||
both only remove wakes: .001.7 with .003.9, and .002.15.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Re-waking a parent after a child is reopened and completed again.** A child
|
||||
going `COMPLETED` → `IN_PROGRESS` → `COMPLETED` presents an identical id set and
|
||||
an identical state set at both wave moments, so no function of the current child
|
||||
set distinguishes them. Closing this needs a monotone per-child completion epoch
|
||||
on `tasks`, a separate capability with a different owner, filed as a follow-up.
|
||||
Parent *step transitions* on reopen are unaffected, per .004.3; only the
|
||||
redundant run is suppressed.
|
||||
|
||||
- **Re-waking a parent whose wave-member id set returns to a previously delivered
|
||||
value.** A child that leaves the wave-member set and later rejoins it — plainly,
|
||||
archived then unarchived — returns the identity to a delivered value, which the
|
||||
unbounded deduplication in .002.6 suppresses. Same root limit and same deferred
|
||||
follow-up as the bullet above; .003.3 is qualified to match. Exposure is small:
|
||||
unarchiving is deliberate and rare, and any later membership change still wakes
|
||||
the parent.
|
||||
|
||||
- **The task system's `updated_at` stamping.** Every task write stamps it
|
||||
unconditionally and will continue to. This capability removes the wake
|
||||
pipeline's dependence on it rather than changing it.
|
||||
|
||||
- **The whole-second comparison in the backstop's in-flight run arm**, tracked as
|
||||
its own card. This capability replaces only the *staleness* arm; the
|
||||
queued-or-claimed arm and its resolution behaviour are untouched.
|
||||
|
||||
- **The backstop candidate query's SQLite-only SQL.** Its aggregate child-set
|
||||
expression and raw JSON extraction have no PostgreSQL branch; tracked as its own
|
||||
follow-up. Work here shall not widen that gap — new SQL carries a dialect branch
|
||||
— but closing it is not this capability's outcome.
|
||||
|
||||
- **Any change to the `on_children_completed` trigger's firing conditions,
|
||||
payload shape, or workflow-editor surface.** Those belong to the task system's
|
||||
subtask completion trigger contract. Carrying an extra value alongside a trigger
|
||||
to the action it dispatches, and *reading* an already-authored action payload,
|
||||
are not changes to that contract and are not excluded.
|
||||
|
||||
- **User interface.** No new control, view, or setting. The effect is visible
|
||||
only as the absence of a duplicate or spurious run.
|
||||
@@ -0,0 +1,553 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-001
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-002
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-003
|
||||
- REQ-OFFICE-WAKE-WAVE-IDENTITY-004
|
||||
---
|
||||
|
||||
# Parent Wake Wave Identity System Design
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
This design gives the `task_children_completed` wake a single durable identity,
|
||||
persisted on the run row, produced by every producer, and compared by the backstop's
|
||||
candidate query in place of a timestamp. Office owns the outcome: whether an
|
||||
autonomous run enters the Office run queue. Adjacent contracts it reads and
|
||||
constrains but does not own:
|
||||
|
||||
- `internal/workflow/engine` — the `on_children_completed` trigger, the action path
|
||||
turning it into a `queue_run`, the idempotency key it synthesises, and the operation
|
||||
ledger. This design adds a value travelling beside that key.
|
||||
- `internal/orchestrator` — the task-state and terminal-step handlers dispatching the
|
||||
same trigger for every workflow, Office or not.
|
||||
- `internal/task/repository/sqlite` — `tasks` and its `archived_at`, `is_ephemeral`,
|
||||
`origin` and `updated_at` columns, plus `IsFromOfficePredicate`.
|
||||
- `internal/runs/service` — run admission, the idempotency window, coalescing, and
|
||||
unique-violation classification.
|
||||
- `internal/db/dialect` — the expression helpers new cross-dialect SQL goes through.
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Requirement | Design section |
|
||||
| --- | --- |
|
||||
| `REQ-OFFICE-WAKE-WAVE-IDENTITY-001` | [Wave identity](#wave-identity) |
|
||||
| `REQ-OFFICE-WAKE-WAVE-IDENTITY-002` | [Persistence](#persistence), [Failure and recovery](#failure-and-recovery) |
|
||||
| `REQ-OFFICE-WAKE-WAVE-IDENTITY-003` | [Backstop admission](#backstop-admission) |
|
||||
| `REQ-OFFICE-WAKE-WAVE-IDENTITY-004` | [Producers](#producers) |
|
||||
|
||||
## Current state
|
||||
|
||||
Four producers can queue a `task_children_completed` run for one parent, each
|
||||
verified at `cd7823631`.
|
||||
|
||||
| Producer | Entry point | Fires on | Route to the queue | Identity it uses today |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| P1 cascade | `office/scheduler/reactivity.go` `cascadeChildrenCompleted` | inline from `ApplyTaskMutation`, via the dashboard status update | direct `QueueRun` | `childrenCompletedIdempotencyKey`: parent, agent, digest of child ids, straight to `runs.idempotency_key` |
|
||||
| P2 edge | `office/service/event_subscribers.go` `queueChildrenCompletedRun` | `events.TaskMoved` into a done-category step | workflow engine | `wakeOperationID`: parent, digest of child ids, wrapped by the engine's `idempotencyKey` |
|
||||
| P3 backstop | `office/service/scheduler_wake_reconciler.go` | cron sweep over `ListStuckParents` | workflow engine | same `wakeOperationID` as P2 |
|
||||
| P4 orchestrator | `orchestrator/event_handlers_children_completed.go` `processOnChildrenCompleted` | task state → terminal, and terminal step moves | workflow engine | `childCompletionOperationID`: parent, then per child id, state, step, terminal flag and `updated_at` at ns resolution |
|
||||
|
||||
P4 is not gated on Office: it reaches any parent with an active session whose step
|
||||
configures `on_children_completed`, which shipped `office-default` does with
|
||||
`reason: task_children_completed`. Its `EvaluateOnly` call suppresses persistence and
|
||||
the step transition but still evaluates actions, so its `queue_run` reaches the queue.
|
||||
|
||||
### The four child-set sources disagree, three ways
|
||||
|
||||
The fact the design turns on, and wider than an archived-child mismatch:
|
||||
|
||||
| Source | Feeds | `archived_at` | `is_ephemeral` | automation origin | ordered by |
|
||||
| --- | --- | --- | --- | --- | --- |
|
||||
| `ListChildStates` (`office/repository/sqlite/blockers.go`) | P1 | not filtered | not filtered | not filtered | `id` |
|
||||
| `GetChildSetKey` (`office/repository/sqlite/wake_receipts.go`) | P2, P3 | `IS NULL` | not filtered | not filtered | `id` |
|
||||
| `ListChildCompletionRows` (`task/repository/sqlite/task.go`) | P4 | `IS NULL` | `= 0` | excluded | `created_at`, then `id` |
|
||||
| `ListStuckParents` child aggregate (`wake_receipts.go`) | admission | `IS NULL` | not filtered | not filtered | `id` |
|
||||
|
||||
Two consequences. Telling a producer to reuse "the rows it already has" gives four
|
||||
different answers, so the constraint would be decorative for at least one. And
|
||||
`ORDER BY id` is *not* what every existing query uses — P4's source orders by
|
||||
`created_at` first, so reusing its rows in arrival order derives a different string
|
||||
from the same set. Separately, the backstop's staleness arm compares
|
||||
`runs.requested_at` against `MAX(tasks.updated_at)` over children while
|
||||
`updateTaskTx` stamps `updated_at` on every write regardless of which column changed:
|
||||
symptom B's root.
|
||||
|
||||
## Wave identity
|
||||
|
||||
### Which children count
|
||||
|
||||
The wave-member predicate is the one `ListChildCompletionRows` already applies: not
|
||||
archived, not ephemeral, not automation-origin. P4 forces the choice: P1–P3 can each be narrowed cheaply, but P4's row set is the one its
|
||||
readiness check consumes, so widening it would change *when the
|
||||
`on_children_completed` trigger fires*, a task-system contract that is out of scope.
|
||||
It is the only predicate all four can share without moving a readiness boundary, and
|
||||
it is right on its merits: ephemeral and automation-origin tasks are machinery,
|
||||
not delegated work.
|
||||
|
||||
Two consequences:
|
||||
|
||||
- **Readiness and identity now count different children, deliberately.** A parent with
|
||||
a non-terminal ephemeral child is still not *ready* (readiness arms are untouched,
|
||||
per .004.7) even though that child is not part of its wave. Do not "fix" this by
|
||||
propagating the wave-member predicate into a readiness predicate.
|
||||
- **Narrowing cannot split a wave.** Excluding a child can only merge two identities
|
||||
that differed by an invisible row, never make one wave look like two, so the change
|
||||
direction is safe for duplicate wakes.
|
||||
|
||||
The predicate is expressed once, as a shared SQL fragment in the office repository;
|
||||
`task/repository/sqlite`'s `andNotAutomationOrigin` is the precedent.
|
||||
|
||||
### The two encodings
|
||||
|
||||
```text
|
||||
waveString(parentID, memberIDs) = parentID + "|" + strings.Join(memberIDs, ",")
|
||||
|
||||
waveKey(parentID, memberIDs) =
|
||||
"task_children_completed:" + parentID + ":" +
|
||||
lowerhex(sha256(waveString(parentID, memberIDs)))
|
||||
```
|
||||
|
||||
`memberIDs` are the ids of the parent's wave members, ascending by `tasks.id`.
|
||||
|
||||
Both are pure functions of the same two inputs, with no I/O and no error return,
|
||||
which is what .001.10 requires of the *derivation* — the function, not the caller: a
|
||||
producer may still need a read to obtain the member ids, and .002.12 and .002.15
|
||||
govern that read.
|
||||
|
||||
Why two encodings rather than one. **The backstop must compare in SQL** (.003.8) — a
|
||||
candidate rejected in Go after the SELECT occupies a row of the query's `LIMIT` every
|
||||
tick and starves an actionable parent behind it — and SQL here cannot hash, since
|
||||
`mattn/go-sqlite3` registers no custom functions and there is no `pgcrypto` in the
|
||||
tree, so the compared encoding has to be the plain string. **The uniqueness
|
||||
constraint must be bounded** — a PostgreSQL btree entry caps near 2704 bytes, so a
|
||||
parent with roughly seventy children would push an unhashed key past it and fail the
|
||||
insert outright, so the indexed encoding has to be the digest. Neither job can use
|
||||
the other's encoding, so both are persisted, derived together from one member-id
|
||||
slice at one call site per producer.
|
||||
|
||||
Choices a builder would otherwise have to invent, fixed here:
|
||||
|
||||
- **The separator is `|`, deliberately not a NUL byte.** PostgreSQL `text` cannot
|
||||
contain `U+0000` at all, so a NUL-separated string is unstorable there. A UUID
|
||||
holds only hex digits and dashes, so `|` cannot occur inside an id.
|
||||
- **Ordering is `ORDER BY id` ascending**, and a producer whose rows arrive
|
||||
otherwise re-sorts first. `tasks.id` is the primary key, so the order is total and
|
||||
there is no tiebreak to choose.
|
||||
- **That order must be the *same* in Go and in SQL** (.001.13). Go sorts by byte;
|
||||
PostgreSQL `ORDER BY c.id` sorts by database collation, which for most locales is
|
||||
not byte order. Task ids are `uuid.New().String()` — lowercase hex, dashes at fixed
|
||||
positions — and for that alphabet the two coincide, so no `COLLATE` clause is
|
||||
needed. This is a premise worth stating because its failure is invisible: SQLite
|
||||
(BINARY) never diverges, while on PostgreSQL a divergence would make every parent
|
||||
look permanently stale and the sweep re-wake continuously. A non-UUID id source is
|
||||
what would break it, and the cross-dialect test is what would catch it.
|
||||
- **The digest covers ids only** — state, workflow step, terminal flag and
|
||||
`updated_at` all excluded.
|
||||
- **The parent id appears in the wave string and again as a readable prefix on the
|
||||
wave key.** The prefix keeps the stored value diagnosable; its presence in the
|
||||
digest input stops two parents with an identical child set colliding.
|
||||
- **An empty `memberIDs` slice has no wave.** Producers return before deriving, so
|
||||
neither function is called with one and no key of that shape is written.
|
||||
|
||||
### Where the derivation lives
|
||||
|
||||
A small leaf package holding the two pure functions and nothing else, imported by
|
||||
`office/scheduler`, `office/service` and `orchestrator`. The constraint is *not* that
|
||||
`office/scheduler` may not import `office/service` — it already does
|
||||
(`executor_resolver.go`, `wake_payload.go`, `run.go`). It is the reverse:
|
||||
`office/service` must not import `office/scheduler`, because that edge closes a cycle,
|
||||
which rules out the scheduler as the shared home. `orchestrator` importing a leaf
|
||||
office package is established practice — `orchestrator/model_info.go` imports
|
||||
`internal/office/costs/modelsdev`.
|
||||
|
||||
## Persistence
|
||||
|
||||
### Two columns on `runs`
|
||||
|
||||
```sql
|
||||
ALTER TABLE runs ADD COLUMN wake_wave_key TEXT NOT NULL DEFAULT '';
|
||||
ALTER TABLE runs ADD COLUMN wake_wave_string TEXT NOT NULL DEFAULT '';
|
||||
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS idx_run_wake_wave
|
||||
ON runs(wake_wave_key, agent_profile_id)
|
||||
WHERE wake_wave_key <> '';
|
||||
```
|
||||
|
||||
`wake_wave_key` carries the digest and is indexed; `wake_wave_string` carries the
|
||||
plain string the backstop compares. Both are written in the same insert from one
|
||||
derivation, so they cannot disagree; a test asserts `wake_wave_key == waveKey` of the
|
||||
inputs that produced `wake_wave_string`.
|
||||
|
||||
Applied through the replayable `r.migrate.Apply` path the other office column
|
||||
migrations use (`runs.continuation_scope` in `base_migrations.go` is the model) and
|
||||
added inline to the fresh-schema `CREATE TABLE` in `office/repository/sqlite/base.go`
|
||||
so a new database and an upgraded one converge. The office repository creates `runs`,
|
||||
so this is its migration to own, and both statements are written for both dialects.
|
||||
They run after `runs` exists and none rewrites a row; index creation cannot fail on a
|
||||
pre-existing duplicate pair, because both columns are new and default to empty, so no
|
||||
existing row is inside the partial index.
|
||||
|
||||
The columns live on `runs` rather than in a claim table because the constraint must
|
||||
be enforced in the same transaction as the run insert, and that insert is a single
|
||||
statement; a claim table would need a caller-owned transaction threaded from the
|
||||
office producers through the engine's action dispatch into the runs service, and no
|
||||
seam supports that.
|
||||
|
||||
Both columns carry a value only for `task_children_completed` runs; every other run
|
||||
leaves them empty and, the index being partial, sits outside the constraint.
|
||||
|
||||
`(wake_wave_key, agent_profile_id)` rather than `wake_wave_key` alone: the engine can
|
||||
fan one trigger out to several targets, which is why the receipt records an operation
|
||||
id rather than one delivered run id. Constraining the pair suppresses a second run to
|
||||
the same agent for one wave while leaving a genuine fan-out to *different* agents
|
||||
intact. Per .002.13, a fan-out across participants resolving to the *same* profile
|
||||
collapses to one run: an agent holding two roles is not two waves.
|
||||
|
||||
### Coalescing: the third write path into `runs`
|
||||
|
||||
`runs` is written three ways, not two, and the third is easy to miss because it is an
|
||||
UPDATE. `runs/service.QueueRun` consults `CoalesceRun` **before** `insertRun`, and
|
||||
`CoalesceRun` merges the request into an existing `queued` row for the same agent,
|
||||
reason, and task bucket inside a time window, incrementing `coalesced_count` and
|
||||
**replacing that row's payload**. `shouldCoalesceRun` excludes only
|
||||
`task_comment:`-prefixed keys, so `task_children_completed` is eligible.
|
||||
|
||||
When both payloads contain a task ID, `CoalesceRun` compares those IDs for every
|
||||
reason. A request for a different parent therefore remains a separate row. Wave
|
||||
requests skip this update path and use the durable wave identity index instead.
|
||||
|
||||
Note what is *not* the mechanism: two engine-path producers deriving the same
|
||||
`wakeOperationID` are already collapsed upstream by `CheckIdempotencyKey`. Coalescing
|
||||
still handles different keys when the agent, reason, and task bucket match.
|
||||
|
||||
The decision (.002.14): **a request carrying a wave identity is never merged, and a
|
||||
queued run carrying one is never merged into.** The guard is on the wave columns, not
|
||||
the reason string, so a future wave-keyed reason is covered without a second edit.
|
||||
Both requests are then reconciled by `idx_run_wake_wave`, the stronger mechanism
|
||||
anyway: exact and unbounded, where coalescing is approximate and windowed.
|
||||
|
||||
Different parents addressed to the same agent inside the window stay in separate rows
|
||||
because their task buckets differ. Requests for the same task can still coalesce when
|
||||
they do not carry a wave identity.
|
||||
|
||||
### Existing identifiers are untouched
|
||||
|
||||
`childrenCompletedIdempotencyKey` (P1), `wakeOperationID` (P2, P3) and
|
||||
`childCompletionOperationID` (P4) keep their definitions and values, and
|
||||
`runs.idempotency_key` and `idx_run_idempotency` are unchanged.
|
||||
|
||||
The wave key is *purely additive* — a new column beside whatever identity a producer
|
||||
already computed, never a replacement — which keeps the operation ledger stable,
|
||||
keeps every `parent_child_wake_receipts` row already written meaningful, and removes
|
||||
any historical-operation-id migration question. None of that would hold if
|
||||
`wakeOperationID` were redefined to be the wave key, since the wave-member predicate
|
||||
is narrower than `GetChildSetKey`'s. The two
|
||||
constraints answer different questions — "same dispatch" and "same wave" — and only
|
||||
the second can be computed identically by a direct caller and by the engine.
|
||||
|
||||
### Receipts
|
||||
|
||||
`parent_child_wake_receipts` is unchanged and `GetChildSetKey` keeps its predicate and
|
||||
state-inclusive format: the backstop uses that key for the two re-reads that keep a
|
||||
concurrent child update from being masked by a previous generation's receipt, and that
|
||||
check wants to notice a state change. `wakeOperationID` being unchanged,
|
||||
`delivery_operation_id` keeps its meaning.
|
||||
|
||||
## Producers
|
||||
|
||||
All four derive from the wave-member predicate and pass the resulting pair onto
|
||||
the run they queue.
|
||||
|
||||
- **P1 cascade.** Keeps `ListChildStates` and its terminal-state loop as they are —
|
||||
narrowing that read would change cascade's readiness check, which .004.7 forbids —
|
||||
and gains a separate wave-member read. `RunContext` gains wave-key and wave-string
|
||||
fields beside its `IdempotencyKey`, carried through `SchedulerService.QueueRun` into
|
||||
the run row. Its no-session fallback is untouched: it still queues directly and
|
||||
never consults the engine (.004.2).
|
||||
- **P2 edge and P3 backstop.** Both gain the same wave-member read.
|
||||
`GetChildSetKey` is *not* reused for the derivation — wider predicate,
|
||||
state-inclusive — and stays where it is, serving receipts.
|
||||
- **P4 orchestrator.** No new read and no new filter: its rows are already exactly
|
||||
the wave members. It must re-sort by `id` before deriving, because
|
||||
`ListChildCompletionRows` orders by `created_at` first.
|
||||
`childCompletionOperationID` is unchanged, so the operation ledger, the
|
||||
`EvaluateOnly` transition lifecycle, and the reopen-driven step transition all
|
||||
behave as today.
|
||||
|
||||
### The wave-member read is the last read, and it carries state
|
||||
|
||||
Every producer reaches the queue through several separate, untransacted reads: a
|
||||
readiness check, for P2 and P3 a receipt re-read, and now the wave-member read.
|
||||
Nothing orders them, so a child can be added, archived, or leave a terminal state
|
||||
between two. Unconstrained, that records a run against a member set that was **never
|
||||
simultaneously terminal**, and because deduplication is unbounded (.002.6) that
|
||||
identity suppresses the real wave *forever* — a lost wake, strictly worse than the
|
||||
duplicate this design removes.
|
||||
|
||||
The rule that closes it (.002.15) needs no new transaction seam:
|
||||
|
||||
- the wave-member read returns `(id, state)` per member ordered by `id`, not ids
|
||||
alone;
|
||||
- it is the producer's **last** read of *child task state* before queueing;
|
||||
- if any returned member is non-terminal, the producer queues nothing and returns.
|
||||
|
||||
The recorded identity is then always derived from one statement's rows that the same
|
||||
statement showed entirely terminal. A change landing *after* the read is harmless: it
|
||||
moves the wave identity, so the backstop sees a mismatch and queues the new wave
|
||||
normally. Only the never-terminal set had to be excluded.
|
||||
|
||||
The confirmation can only reject, never admit, which keeps it inside .004.7 — the
|
||||
earlier readiness gates are untouched and strictly wider, so a set that passes them
|
||||
and fails this defers to the backstop. P4 satisfies it by construction:
|
||||
`ListChildCompletionRows` returns `state` and `readyChildCompletionRows` establishes
|
||||
terminality from those same rows.
|
||||
|
||||
For P3 this also settles which value is authoritative: `ListStuckParents`' SQL wave
|
||||
string is an **admission filter only**, never written to a run, and every producer
|
||||
writes the identity from its own final wave-member read. A candidate row whose wave
|
||||
string has gone stale needs no special handling — the producer's read is fresher and
|
||||
its terminality confirmation decides.
|
||||
|
||||
A producer deriving an **empty** member set queues nothing and returns (.001.7). This
|
||||
is live, not theoretical, now the predicate is narrower than the readiness checks: a
|
||||
parent whose only child is ephemeral or automation-origin passes P1's
|
||||
`AreAllChildrenTerminal` gate and its `ListChildStates` loop, and only the
|
||||
wave-member read reveals there is no wave. P4 already guards it
|
||||
(`readyChildCompletionRows` rejects `len(rows) == 0`); P1, P2 and P3 gain the same.
|
||||
|
||||
P1, P2 and P3 read through one new office-repository method returning wave-member
|
||||
`(id, state)` ordered by `id`. Because P4 derives from `ListChildCompletionRows`
|
||||
instead, a test asserts the two SQL sources return an identical id set for the same
|
||||
parent — the equality .001.8 rests on, and the one place two queries must be kept in
|
||||
step by hand.
|
||||
|
||||
The engine seam is narrow: both encodings accompany an `on_children_completed`
|
||||
trigger to the `queue_run` action and are written to the run row, carried by
|
||||
`OnChildrenCompletedPayload` (`workflow/engine/payloads.go`). They do not participate
|
||||
in `idempotencyKey`, `queueActionDigest`, or the operation ledger —
|
||||
`queueActionDigest` keys off the workflow-authored *action* payload, not the trigger
|
||||
payload.
|
||||
|
||||
## Backstop admission
|
||||
|
||||
`ListStuckParents` computes the parent's current wave string in SQL alongside the
|
||||
existing state-inclusive `child_set_key`, over the wave-member predicate:
|
||||
|
||||
```text
|
||||
p.id || '|' || COALESCE(<ordered group concat of wave-member c.id>, '')
|
||||
```
|
||||
|
||||
The aggregate needs a dialect branch, added to `internal/db/dialect` beside
|
||||
`JSONExtract`: SQLite's `GROUP_CONCAT(c.id, ',')` takes ordering from an ordered
|
||||
subquery, the idiom the existing `child_set_key` aggregate uses; PostgreSQL's
|
||||
`string_agg(c.id, ',' ORDER BY c.id)` takes it inside the aggregate. Writing one and
|
||||
hoping is how ordering silently differs between dialects.
|
||||
|
||||
That expression is evaluated for **every** CTE row, including a parent with no wave
|
||||
members, where it yields a well-formed `"<parentID>|"` — a pre-gate intermediate, not
|
||||
a wave string, since per the requirements' *Terminology* a parent with no wave
|
||||
members has neither. Nothing compares it, because the gate below removes the row
|
||||
first; note that a gate testing the assembled expression for emptiness would exclude
|
||||
nothing, as it is never empty.
|
||||
|
||||
The gate is a separate `EXISTS` over the wave-member predicate, beside the existing
|
||||
`EXISTS`:
|
||||
|
||||
```sql
|
||||
AND EXISTS (
|
||||
SELECT 1 FROM tasks c
|
||||
WHERE c.parent_id = p.id
|
||||
AND c.archived_at IS NULL
|
||||
AND c.is_ephemeral = 0
|
||||
AND COALESCE(c.origin, '') != 'automation_run'
|
||||
)
|
||||
```
|
||||
|
||||
Without it, a parent whose only child is ephemeral or automation-origin would be
|
||||
listed every tick, be found to have no wave, queue nothing, and be listed again — the
|
||||
permanently rejected candidate .003.8 prevents. The existing
|
||||
`EXISTS (... archived_at IS NULL)` arm keeps its archived-only predicate and is not
|
||||
replaced: the gate is additive and narrowing, so it can only remove candidates, never
|
||||
admit one not admitted today, which keeps it inside .004.7.
|
||||
|
||||
The second `NOT EXISTS` arm becomes three clauses, each naming its run statuses
|
||||
explicitly per .003.10, and each status set is the one in force today:
|
||||
|
||||
- `status IN ('queued','claimed')` blocks regardless of wave — unchanged, so the
|
||||
sweep never races an in-flight wake;
|
||||
- `status IN ('finished','failed','cancelled')` blocks when its `wake_wave_string`
|
||||
equals the parent's current wave string;
|
||||
- **only when the parent has no `task_children_completed` run carrying a wave
|
||||
identity at all, in any status**, a terminal run blocks under the existing
|
||||
`requested_at >= newest_child_updated_at` rule.
|
||||
|
||||
The second clause keeps its three-status set deliberately, and that is the one place
|
||||
this design changes *when* a wake is re-delivered. Today a failed or cancelled wake
|
||||
blocks only until some child's `updated_at` overtakes `requested_at`, so any
|
||||
unrelated edit silently re-arms it; comparing wave identity removes that churn, so a
|
||||
failed wake blocks its wave until the wave changes. `ListStuckParents`' doc comment
|
||||
already states the contract — a terminal failure stays terminal, needs an explicit
|
||||
user retry, and must not become a cron retry loop — so this is the correct direction.
|
||||
The escape hatches are an explicit retry and any wave-member change.
|
||||
|
||||
The third clause is the compatibility path for pre-change rows. Its guard is
|
||||
status-agnostic on purpose — it asks about *vintage*, not delivery, and a keyed run
|
||||
still `queued` blocks through the first clause anyway — and it is scoped to the
|
||||
**parent**, not the row (.003.6): evaluating per row would let a parent's
|
||||
pre-upgrade row keep answering long after a keyed run existed. "Carrying a wave
|
||||
identity" is tested as `wake_wave_key <> ''`, the same predicate the partial index
|
||||
uses, so a row is inside or outside both the constraint and the compatibility path
|
||||
together. The path is bounded and self-clearing: an unrelated edit can still make a
|
||||
pre-upgrade parent look stale and queue one wake, that wake writes a keyed row, and
|
||||
the parent is judged by wave identity ever after. `newest_child_updated_at` stays
|
||||
solely for this clause.
|
||||
|
||||
The reconciler's two re-reads keep using `GetChildSetKey`: they guard against a child
|
||||
change between the SELECT and the dispatch, and the state-inclusive key is the
|
||||
stricter guard.
|
||||
|
||||
One asymmetry follows from unbounded deduplication and is why .003.3 is qualified
|
||||
rather than absolute. Admission can make a parent a candidate again on any
|
||||
wave-member change, but the constraint still refuses an insert for an identity
|
||||
already queued, so a set that changes and changes *back* — archive, then unarchive —
|
||||
lists the parent and queues nothing. That is the id-set-reuse exclusion: admission
|
||||
promises candidacy, not delivery.
|
||||
|
||||
## Wake equivalence between producers
|
||||
|
||||
AC-OFFICE-WAKE-WAVE-IDENTITY-002.10 requires the surviving run to deliver the same
|
||||
wake whichever producer won. It binds only where one run is actually suppressed —
|
||||
same wave *and* same target agent — so a workflow whose `on_children_completed`
|
||||
action targets `workspace.ceo_agent` while the cascade wakes the parent's assignee
|
||||
raises no equivalence question. Two payload sources have to agree for the cases that
|
||||
do collide.
|
||||
|
||||
**Child summaries: equal, from current storage.** Prompt assembly calls
|
||||
`enrichChildrenContext`, which reads `GetChildSummaries` from the task repository.
|
||||
The four producers do not need to write a `children` payload key. Each surviving run
|
||||
therefore receives the current stored child summaries when the scheduler builds the
|
||||
prompt.
|
||||
|
||||
**Workflow-authored action payload: NOT equal today; this design fixes it.**
|
||||
`queueRunPayload` copies `in.Action.QueueRun.Payload` onto the run verbatim, so every
|
||||
engine-path producer carries whatever the step's `on_children_completed` `queue_run`
|
||||
action declares, and the cascade — never consulting the engine — has no way to. The
|
||||
shipped `office-default` action declares none, so nothing diverges today; but the
|
||||
workflow editor allows one, and the moment a workflow authors a payload while
|
||||
targeting the parent's primary agent, the delivered context depends on which producer
|
||||
won — and suppressing the loser is what makes that observable.
|
||||
|
||||
So, per .002.16, the cascade resolves the parent's current workflow step, reads that
|
||||
step's `on_children_completed` `queue_run` action payload, and merges it into its own
|
||||
run payload with the precedence `queueRunPayload` applies. The comment-specific fields
|
||||
that function also merges do not apply to this trigger and are not reproduced. That
|
||||
resolution reads workflow configuration, not child task state, so it may sit after the
|
||||
wave-member read without violating .002.15.
|
||||
|
||||
Its failure behaviour is deliberately *not* the wave-member read's: if the authored
|
||||
payload cannot be resolved, the cascade still queues the wake without it and logs the
|
||||
omission. Its unconditional wake for a parent with no active workflow session is a
|
||||
contract in its own right (.004.2) and outranks an optional payload. The degradation
|
||||
is named rather than left to be discovered.
|
||||
|
||||
## Failure and recovery
|
||||
|
||||
- **Unique violation on insert — two classification sites, not one.**
|
||||
`IsIdempotencyKeyUniqueViolation`
|
||||
(`runs/repository/sqlite/idempotency_violation.go`) is deliberately scoped to
|
||||
`idx_run_idempotency` by name, so it gains an explicit sibling for
|
||||
`idx_run_wake_wave` rather than being widened to "any unique violation". The sibling
|
||||
matches PostgreSQL's `pgconn.PgError` code `23505` with constraint name
|
||||
`idx_run_wake_wave`, and SQLite's composite message `UNIQUE constraint failed:
|
||||
runs.wake_wave_key, runs.agent_profile_id` — a different shape from the
|
||||
single-column message the existing constant matches.
|
||||
|
||||
Both queue paths must call it, and this is what a single-site fix gets wrong.
|
||||
`office/repository/sqlite.Repository` *embeds* `*runssqlite.Repository`, so P1 and
|
||||
the engine path call the very same `CreateRun`, but only `runs/service` wraps it
|
||||
with classification: `office/scheduler.SchedulerService.QueueRun` calls `CreateRun`
|
||||
directly and wraps the error as `enqueue run: %w`, logged by its caller as
|
||||
`reactivity run failed`. Left alone, P1 losing a wave race logs a failure, which
|
||||
.002.4 forbids — and P1 is the producer the race test drives. So (1)
|
||||
`runs/service`'s `insertRun`/`QueueRun` gains a second sentinel beside
|
||||
`errIdempotencyKeyConflict` and returns `QueueOutcomeDeduped`, covering P2–P4; and
|
||||
(2) `office/scheduler.SchedulerService.QueueRun` classifies the `CreateRun` error
|
||||
and returns `nil` with a debug log, as it already treats a `CheckIdempotencyKey`
|
||||
hit, covering P1.
|
||||
- **Lost dispatch.** Unchanged: the backstop re-derives readiness from current state
|
||||
every tick and simply recognises delivery by wave rather than timestamp.
|
||||
- **Retry.** `ScheduleRetry` updates in place and does not insert, so the constraint
|
||||
is never consulted on a retry (.002.11). A future retry-by-reinsert would have to
|
||||
carry both columns forward or clear them.
|
||||
- **Coalescing.** Excluded for wave-carrying requests and rows, per *Coalescing*
|
||||
above — the counterpart of the retry note: both are UPDATE paths, and one moving a
|
||||
payload without moving the identity breaks .002.14.
|
||||
- **Wave-member set changes mid-flight.** Handled by the terminality confirmation on
|
||||
the final read, not by locking. No producer records an identity for a set it did
|
||||
not observe entirely terminal, so unbounded deduplication can never suppress a wave
|
||||
that was not delivered.
|
||||
- **Wave-member read fails.** The producer does not dispatch (.002.12). P2 logs at
|
||||
warn and defers to the backstop; P3 retries next tick; P1 already returns without
|
||||
queueing on a failed child read and does the same here. P4 cannot hit this case.
|
||||
- **Upgrade.** No backfill. Existing rows keep the empty defaults, stay outside the
|
||||
partial index, and are judged by the parent-scoped compatibility clause.
|
||||
|
||||
## Security
|
||||
|
||||
No new trust boundary: both encodings contain only task ids the same caller already
|
||||
reads, and neither is user-supplied or rendered.
|
||||
|
||||
## Observability
|
||||
|
||||
The existing `parent_wake_*` expvars and their paired `wake.metric.*` logs are
|
||||
unchanged. One counter is added, `parent_wake_deduped_total`, incremented when a
|
||||
`task_children_completed` insert is rejected by `idx_run_wake_wave`, at both
|
||||
classification sites so a P1 dedupe is as visible as an engine-path one. It is the
|
||||
only direct evidence the constraint is doing work, since a run that never exists
|
||||
leaves no other trace. Parent task id is unbounded and is not a label,
|
||||
matching the existing counters; it goes on the structured log.
|
||||
|
||||
## Testing
|
||||
|
||||
- **Derivation:** terminal-to-terminal edit stable; non-state edit stable; member
|
||||
added or removed changes it; archived, ephemeral and automation-origin children
|
||||
excluded; independent of insertion order; empty set never keyed; wave key is the
|
||||
digest of the wave string.
|
||||
- **Cross-producer:** all four derive identical encodings for one parent and child
|
||||
set, including a parent with an ephemeral or automation-origin child and P4's rows
|
||||
in `created_at` order. The new wave-member method and `ListChildCompletionRows`
|
||||
return the same id set.
|
||||
- **Dialect:** the wave-string aggregate is byte-identical on SQLite and PostgreSQL,
|
||||
ordering included.
|
||||
- **Race:** cascade against one backstop tick yields exactly one run row and the
|
||||
loser returns success without logging a failure, mirroring the runs service's
|
||||
idempotency-index race tests and their PostgreSQL twin. The cascade case must run
|
||||
through `office/scheduler`'s own queue path, not only `runs/service` — those are the
|
||||
two classification sites.
|
||||
- **Coalescing:** a second request for a *different* parent, same agent, inside the
|
||||
window does not merge into the first parent's queued run; two runs exist, each
|
||||
carrying its own identity, neither differing from the wake it delivers.
|
||||
- **Read skew:** a wave-member read returning a non-terminal member queues nothing
|
||||
and records no identity, and a later genuine wave for that parent is still
|
||||
delivered — the case otherwise suppressed permanently.
|
||||
- **Admission:** a non-state child edit after delivery lists no candidate; a
|
||||
wave-member change lists one; a parent whose only run predates the change still
|
||||
follows the timestamp rule; a parent holding both a pre-upgrade and a keyed run is
|
||||
judged by wave identity alone. A failed run whose wave string equals the current one
|
||||
keeps blocking across an unrelated child edit, and a wave-member change unblocks it.
|
||||
- **Gate:** a parent whose only children are ephemeral or automation-origin is not a
|
||||
candidate and no producer queues for it; a parent that is a candidate today stays
|
||||
one whenever it has a wave member.
|
||||
- **Payload parity:** the cascade attaches the workflow-authored
|
||||
`on_children_completed` action payload when the step declares one, matching the
|
||||
engine path byte-for-byte, and still queues the wake without it when it cannot be
|
||||
resolved.
|
||||
- The two identity tests pinning the id-only decision, and the cascade's
|
||||
children-completed tests, must continue to pass unmodified.
|
||||
|
||||
## Related decisions
|
||||
|
||||
- [ADR-0015](../../../decisions/0015-explicit-completion-signal-for-auto-advance.md)
|
||||
Reference in New Issue
Block a user