Skip to content

Commit babf43a

Browse files
committed
docs(adr): ADR 0017 — fork, never rebind, for per-attempt trace routing
Review found a blocking concurrency flaw in the revised routing design: sharded test attempts run concurrently (Promise.allSettled in runReplayTestShards) under one inherited AsyncLocalStorage request scope, so mutable scope rebinding would let one attempt overwrite or clear another's replay-timing destination after an await — cross-writing or dropping events. Replace rebinding with a journal fork primitive: journal.fork(bindings, fn) runs fn in a new ALS scope object sharing the parent's buffer/phaseCounts/ envelope/sinks but carrying frozen routing bindings. Each attempt wraps its work (including nested replay dispatch) in a fork binding its own trace path; the binding dies with the fork, so no clearing step exists to race. Existing updateDiagnosticsScope rebinds stay confined to sequential request setup, pre-fan-out. Validation gains a concurrent-shard regression proving each replay-timing.ndjson contains only its own attempt's events.
1 parent 3a504a6 commit babf43a

1 file changed

Lines changed: 23 additions & 12 deletions

File tree

‎docs/adr/0017-unified-event-journal.md‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ Normative summary of the proposal; contracts and rationale below.
2929
- Every consumer is a **sink**: explicitly registered at scope construction, invoked synchronously
3030
in registration order, individually best-effort (a sink error is swallowed and must not affect
3131
other sinks or the request). Sinks with dynamic destinations (the per-attempt replay trace) read
32-
routing context that the owning lifecycle binds onto the scope, mirroring today's mid-request
33-
`logPath` rebinding.
32+
**immutable routing bindings carried by a forked child scope** — concurrent sub-work (sharded
33+
test attempts run under `Promise.allSettled`) forks, never rebinds shared scope state.
3434
- **Progress streaming is not journal-owned.** `src/request/progress.ts` and its typed
3535
`RequestProgressEvent` union remain the transport's channel, unchanged.
3636
- **Existing formats do not change, with one declared exception.** `events.ndjson` v1 entries,
@@ -158,12 +158,20 @@ statically enumerable, greppable, no dynamic subscription API. Semantics, normat
158158
the request.
159159
- **Flushing:** flush remains journal-owned (`flushDiagnosticsToSessionFile` semantics unchanged);
160160
sinks that buffer expose flush hooks the journal calls on request finalization and fatal paths.
161-
- **Dynamic destinations:** a sink whose output path is not known at scope construction reads
162-
routing context bound onto the scope by the owning lifecycle. Precedent:
163-
`createRequestExecutionScope` already rebinds `logPath` mid-request. The replay trace sink works
164-
the same way: `prepareReplayTestTimingTrace` binds the current attempt's trace path onto the
165-
scope (and clears it at attempt end); the sink drops trace-kind events while no destination is
166-
bound. Multiple attempts in one request therefore route to their own files, exactly as today.
161+
- **Dynamic destinations — fork, never rebind:** a sink whose output path is not known at scope
162+
construction reads **immutable routing bindings** carried by the current scope. The journal
163+
gains a fork primitive: `journal.fork(bindings, fn)` runs `fn` inside a new AsyncLocalStorage
164+
scope object that shares the parent's buffer, `phaseCounts`, envelope, and sink list, but
165+
carries its own **frozen** bindings. Sharded `test` runs execute attempts concurrently
166+
(`Promise.allSettled` in `runReplayTestShards`, `src/daemon/handlers/session-test.ts`) under one
167+
inherited request scope, so mutating shared scope state to route traces would let one attempt
168+
overwrite or clear another's destination after an `await`. Instead, each attempt's lifecycle
169+
wraps its work — including all nested replay dispatch — in a fork binding that attempt's
170+
`replay-timing.ndjson` path; the binding dies with the fork, so no clearing step exists to race.
171+
The sink drops trace-kind events emitted with no bound destination. The existing
172+
`updateDiagnosticsScope` rebinds (`session`, `logPath` in `createRequestExecutionScope`) remain:
173+
they run once during sequential request setup, before any concurrency fan-out; new routing
174+
context for parallel or attempt-scoped work must use forks.
167175

168176
**Registered sinks:**
169177

@@ -282,7 +290,9 @@ opt-in decision.
282290
no request active.
283291
- Per-attempt routing: a multi-attempt `test` run writes each attempt's trace to its own file with
284292
legacy `type` values; trace-kind events emitted with no bound destination are dropped, not
285-
misrouted.
293+
misrouted. A **concurrent-shard regression** runs sharded attempts in parallel and proves each
294+
`replay-timing.ndjson` contains only its own attempt's events — no cross-writes, no drops —
295+
including events emitted from nested replay work after `await` points.
286296
- Sink isolation: a sink that throws does not affect the buffer, other sinks, or the response;
287297
ordering across sinks is registration order.
288298
- `cost.runnerRoundTrips` parity: the trait-derived set equals the current literal list; the
@@ -307,6 +317,7 @@ Each step lands green and independently useful:
307317
the journal; the `events.ndjson` writer becomes a sink; idle-reap/shutdown finalizers open
308318
session-scoped teardown scopes so out-of-request `action.recorded` events keep flowing; the
309319
`events` command and pagination untouched.
310-
4. **Replay trace as sink** — both trace helpers replaced by one sink with scope-bound per-attempt
311-
routing and legacy `type` mapping; the declared redaction change ships here with its fixture and
312-
changelog entry.
320+
4. **Replay trace as sink** — the journal fork primitive lands with its concurrent-shard
321+
regression; both trace helpers are replaced by one sink with fork-bound per-attempt routing and
322+
legacy `type` mapping; the declared redaction change ships here with its fixture and changelog
323+
entry.

0 commit comments

Comments
 (0)