--- status: historical created: 2026-08-18 archived: 2026-08-18 reason: "Stage-C evidence: the adversarial review of #5991 (the tuning trio T1/T2/T3 + the drain guard) with the builder's disposition; the guard's release point moved off the seal here." --- # Review: stage C tuning trio (T1 / T2 / T3 + drain guard) — `stage-c-tuning` vs `fb2292a24` Reviewed: full `git diff fb2292a24 HEAD` (23 files; production, tests, docs), the attribution report §2–§5, speculation.md §4, serving-loop.md §2/§3, events.md §4, the ledger, and the surrounding code paths the diff depends on (`scheduler/events.ts` dispatch + drop chokepoints, `wake-shaping.ts`, `lineage.ts`, `fan-out.ts`, `wave.ts` seal/commit, `space-server.ts` `#waveCycle`/`park`, `v2.ts` `applySessionSync`/`pull`, `cfc/prepare.ts` `flowLabelWorkExists`/`forEachFlowObservation`, `cell.ts` send). I ran the four new/changed suites (all green on this box), `deno check` on the touched files (clean), and two mutations in scratch copies outside the worktree: the E2 pin goes red with the arrival wake removed (timeout waiting for retirement), and (ii) goes red with the yield observer removed (`wave-commit-rejected` loop, walk never commits). Worktree left untouched (`git status` clean). ## Summary verdict No blocker. The three mechanisms do what the register says they do, the OFF arm is byte-identical on the hot paths (no `await` added anywhere the yielder is absent), the T1 memo is invalidated on every input the probe reads, the arrival trigger is a trigger and not a relaxation, and the pins are non-vacuous (two mutations verified). Two things I would want changed before merge, one of them a real (if low-frequency) correctness residual: 1. **The drain guard releases at SEAL, not at wave commit** — there is a window (wave-detach → next drain's guard check) in which the entry is still pending in the store, its copy has already run, and the guard is open; a re-drain there queues a second copy and the non-idempotent handler runs twice. The window is macrotask-free in the steady state, which is why every measured run shows `processed == appended`, but it opens whenever the drain hits a real await before the entry's guard check (a first-time sidecar/stream fetch for another stream in the same drain, a foreign-engine resolve). events.md's "exactly-once" sentence overstates what the guard provides. (Finding 1.) 2. **The late-echo rule drops the run's writes but not the run's cascade or its enactable effects** — the seal returns `ok`, so lineage confirms and the children queued by the dropped run dispatch, seal echoes under minted (never-terminal) ids with floors above W, and stand exactly the way the parent would have; `deferSealedEffects` still enacts navigateTo for a run whose writes were discarded. Not a regression (children had the same shape before), but the doc sentence "its writes are dropped before any layer is sealed" reads as "the run is inert" and it is not. Needs a stated semantic and probably an owner call. (Finding 2.) The rest is minor: a stale-instance window in the fan-out yield placement, an exception path that can wedge a drain id until park, a post-`await` re-check the late-echo arm should mirror from the closed arm, a CI-flake hazard in T3 test (ii), and a handful of doc/comment inaccuracies (two of them about which mechanism actually explains E2). ## Findings ### 1. MAJOR (latent) — drain in-flight guard released at seal; re-drain window re-opens the 2× dispatch it exists to close - `packages/runner/src/executor/space-server.ts:1996-2001` (release in the queued copy's `onCommit`), `:2049-2052` (belt), `:1951` (the guard); `docs/specs/server-side-execution/events.md:238-249`. - Mechanism: the queued copy's `onCommit` fires from `runFinalCommitCallback` after `tx.commit()` resolves, and for a served handler tx `commit()` resolves at `SpaceServer.seal()` → `wave.seal(tx)` (accumulator accept), NOT at the wave's commit. So the id leaves `#drainInFlight` while the entry's `consequenced` mark is still only in the open wave. The cycle sequence is: settle race cut → `#currentWave = undefined` (space-server.ts:2991) → `await #sealChain` → `commitWave` → post-commit `selectPendingStreamEventDocs` (3028-3039) → `#eventScanOwed` → next cycle's `#drainStreamEvents` → per-doc `await …sync()` (1860) → per-entry guard check (1951). If the copy's dispatch (the start of a scheduler pass — a macrotask) lands after the wave detach and before its guard check, the drain sees the entry pending and not in flight and queues a SECOND copy; both seal, the handler runs twice, the toggle flips back (the click is lost). Steady state: `pull` for an already-watched doc returns on a microtask (v2.ts:3295-3301) and `commitWave`'s awaits are microtask-only, so the window is empty — consistent with every guarded run reading `processed == appended`. It is NOT empty when the same drain has to fetch a doc for real ahead of the entry (a new stream's first event: `enqueueWatchRefresh` → loopback flush timer), or when a foreign-provisioning wave resolves engines. Low frequency, but exactly the non-idempotent-handler class the guard was built for. - Suggested fix: hold the id until a STORE-visible terminal outcome — release from the wave outcome (`consequenceOf` ∪ `requeuedEventIds` after `commitWave`, plus abandon/withdraw), keep the `onFailure`/drop-path releases (those never seal), and make the success-path `onCommit` a no-op for the guard. Cheaper alternative: additionally skip entries whose eventId is sealed into the currently open wave (the accumulator has the contribution contexts). Either way, reword events.md: "at most one queued/held/running copy at a time", not "exactly-once", and name the residual if left. ### 2. MINOR (semantic gap, needs a stated rule) — the late-echo drop leaves the run's cascade and enactable effects alive - `packages/runner/src/speculation/overlay-destination.ts:382-392` (returns `{ ok: {} }`), `:592-653` (`deferSealedEffects` has no late-echo arm); `packages/runner/src/scheduler/lineage.ts:53-90` (ok → lineage confirmed); `packages/runner/src/cell.ts:1889-1940` (a send inside a speculation-stamped run queues a lineage child with a MINTED id). - Scenario: late echo of intent `e` runs (busy worker), calls `send()` → child queued with `mintEventId(link, originTx)` and `parentEventId = e`; the parent seals → dropped at seal → `ok` → lineage confirmed (same-space children do not even wait for it: `processPullQueuedEventDuringExecute` only parks cross-space pending origins) → the child dispatches over served state → seals an event-handler echo whose id is never in `#terminalIntents` → registers → floor = served commit seq > W → stands until the next authored input, hiding the served child consequence. Same symptom the rule targets, one hop down, for every handler that emits. Also: the dropped parent's `navigateTo` effect is enacted by `deferSealedEffects` (converges via the nonce, but it is an optimistic enactment for a run whose writes were discarded — the closed-overlay arm refuses enactment in exactly this shape). - Not a regression (before the rule, the child echo was equally divergent), so MINOR — but the doc/comment claim "its writes are dropped before any layer is sealed" invites reading the run as inert. Options, for the owner: (a) treat a late echo's cascade as jobless too — the child's context carries `parentEventId`, so `#terminalIntents.has(parentEventId)` at the child's seal is a one-line extension (argument: the server's run of `e` produced the authoritative cascade); (b) fail the parent's seal so lineage drops not-yet-dispatched children (but same-space children may already have dispatched, and the caller's onCommit view would show an error for a consequenced intent — the durable-ack coupling in cell.ts reads the echo tx); (c) leave it and state the residual in speculation.md §4. Whichever, add a `deferSealedEffects` arm for dropped late echoes mirroring `#closed` (owned, not enacted). ### 3. MINOR — fan-out yield sits between the instance-set recompute and its use - `packages/runner/src/scheduler/run.ts:592-621` (`toRun` computed at 598, yield at 608-611, `instance = toRun[0]` at 612). - During the macrotask turn a demander can depart (or the ratchet re-key), so `instance` may already have been pruned; `fanOutRunStarted` re-inserts it (fan-out.ts:278-286), the departed instance runs, and its log joins the union subscription until the next run of the node prunes it. Harmless-ish (wasted run, stale trigger reads), but the fix is free: move the `if (ran && …) await turn` above the `serverRunDemandersFor` / `fanOutInstances` recompute so the yield precedes the snapshot it acts on. `startGen` is already read after the yield, which is right. ### 4. MINOR — drain guard: a throw between `add` and `queueEvent` wedges the id until park; a late `onCommit` from a dead tenure can release the NEXT tenure's id - `space-server.ts:1987` (add) … `:2095-2098` (`catch (drainError)` re-arms the rescan but does not `delete`). Anything that throws inside the argument construction (`markRuntimeInjectedEventKeys(carried)`) or in `queueEvent` (`holdShapedEvent`, `queueSchedulerEvent`) leaves the id in the set: every later re-drain skips it, the entry never re-queues, and only park clears it. Add `this.#drainInFlight.delete(entry.eventId)` in the catch. - Cross-tenure: `park()` clears the set (3231) but the old runtime's in-progress execute is not cancelled ("dispose does NOT cancel a run already under way"); a served event dispatched in that final pass fires its `onCommit` later (a rejected direct commit after `clearSealDestination`) and deletes an id the NEW tenure's drain may have just added — reachable only through the park-dispose-timeout path (the abandoned dispose completes late) so NIT-grade, but a one-line tenure check in the closure (`if (this.#runtime === runtime)`) closes it. ### 5. MINOR — late-echo check is not re-validated after `await inner.sealInto(...)` - `overlay-destination.ts:382-392` vs `:518` and the `#closed` re-check at `:543-559`. - Between the synchronous terminal check and the entry's registration there is an await; a terminal signal arriving in that window (a sidecar frame integrating during a `sealInto` that yields a macrotask — today `sealSpaceCommit` is microtask-only, so this is defensive) would run `retireIntent` before the entry exists and the entry would stand — the exact shape the rule closes. Mirror the closed-overlay re-check: after the await, if `#terminalIntents.has(eventId)`, withdraw the collected entries and return ok. ### 6. MINOR — T3 test (ii) is a CI-flake hazard on a loaded box - `packages/runner/test/executor-cooperative-yield.test.ts:262-268` (`renewIntervalMs: 600_000`, `leaseTtlMs: 600`) with `#lastRenewAt` stamped at acquire (`space-server.ts:540`). - The 600-ms TTL clock starts at `lease.acquire()` — before runtime creation, boot serve, and the test's `waitUntil` polls — and nothing renews until the walk's first spent slice (the timer is inert by design). If activation + registration takes > 600 ms, the first `#renewIfDue` finds the row expired → `lease.lost += 1` (blip reacquire) → `expect(lease.lost).toBe(0)` is red for reasons unrelated to the belt. Measured here (instrumented scratch copy): activation 28–101 ms, so ~6× headroom on an unloaded box; the ledger's CI-class boxes run at load 3–7. Either lengthen TTL and the walk together (e.g. TTL 2 s, 60 steps), or issue one explicit renew right before `registerWalk()`, and note the invariant in the test comment. Same family, lower risk: (i)'s `< 600 ms` bound (measured 184 ms) and the unit test's `firedAt < 65 ms` (line 347) — a GC pause on CI can eat 40 ms. ### 7. MINOR — docs/comments: overclaims and an inconsistent E2 attribution - `events.md:245` "This is the mark path's own exactly-once" — the guard gives at-most-one-in-flight copy (Finding 1); "reached its final commit callback" should say the callback fires at SEAL for a served handler, not at the wave commit, since that is precisely where the residual lives. - `overlay-destination.ts:41-58` attributes "the attribution's 48-s lockdown-chip stall" to the arrival-decoupling shape (an EARLY echo, floor ≤ W, retired by the new wake), while `:360-381` attributes the same stall to the LATE echo (floor = served seq 50 > W = 49 — W is the input-batch head, not the derived commit's seq — so no arrival trigger can retire it; only the drop-at-seal or the next input can). Both cannot be the whole story of one stall, and the live re-measurement shows `overlayArrivalSweeps 15–25` and `overlayLateEchoDrops 0`, i.e. the path the code says explains E2 never fired. Pick one attribution per comment, and say in the register that E2's mechanism is inferred (flippedAt=-1 + busy worker), not witnessed. - `overlay-destination.ts:1019-1022` "cost one set lookup per upsert": the observer builds one Set of arrived keys and then does O(entries × writtenDocs) lookups per frame; say that. - `verification-coverage.md` T2 (b): the register presents the late-echo rule as "covered here"; the speculation.md §4 sentence is now normative ("is NOT registered") without a RULED marker, and the dossier called it a candidate rule / owner call — see Questions. ### 8. NIT — observer slot hygiene on the replica - `overlay-destination.ts:1066-1068`, `:1102-1104`: the release closures set the replica's observer field to `undefined` unconditionally; if two overlays ever share a replica (two runtimes on one manager — tests do this shape), the second overwrites the first's wake and the first's close clobbers the second's. Release only if the slot still holds our function. Related: installs are keyed by space but bound to a replica INSTANCE; a provisional-replica replacement (`v2.ts:2056-2078`) orphans the wake and `#arrivalObserverReleases.has(space)` prevents re-install. Both pre-existing shapes for the ack observer; the arrival observer inherits them. ### 9. NIT — the dropped late echo reports a `scheduler.event.commit` success with `writeCount > 0` - `scheduler/events.ts:1399-1451` — the dispatch's telemetry sees `error === undefined` and reports the writes as committed. Cosmetic; a `late-echo-dropped` marker or `writeCount: 0` would keep the telemetry honest. ## What I verified and found sound - **T1**: every input of `flowLabelWorkExists` moves the epoch — `read`/`readOrThrow`/`readValueOrThrow`/`trackReadPaths` → `prepareRead`; `write`/`writeOrThrow`/`writeValuesOrThrow` (per yielded write)/`recordMergeableOp`/`recordSqliteWrite` → `noteWrite`; dereference traces; trigger reads. The probe's other inputs (`getReactivityLog().writes/attemptedWrites`, `getWriteDetails`, `getReadActivities`, `getCfcState().dereferenceTraces/triggerReads`) are all fed by those chokepoints; `labelMetadataObservations`/`structureContainers` are not probe inputs. No read bypasses the wrapper (traverse.ts holds an `IExtendedStorageTransaction`). `TransactionWrapper` is a pure delegate, so `flowLabelWorkExists(this.wrapped)` equals the wrapper's view. Memo stamped after the probe (its own internal-verifier reads bump the epoch), positive verdicts never memoized, gating on `enforcementMode`/`flowLabelsMode` unchanged, `??` on the optional call is correct (a `false` verdict is not nullish). One semantic difference, not a bug: the old second probe could observe metadata that landed between prepare and commit if a caller awaited between them; the memo pins the verdict to the tx's own journal — a doc that changes under a value read conflicts at commit anyway. Tests: mutation-sensitive by construction (computed/memo counts), timing-free. - **T2(a)**: `applySessionSync` collects only forward confirmed-seq moves, fires after notifications, contained; the sweep's replica side effects go through `resolveVerdict` (async) so no synchronous re-entry; predicates unchanged (`W ≥ floor`, arrival, blocked-by-unacked); the scripted pin exercises W`: `queued` at drain time; `marked` when an ACCEPTED event-handler seal for that id lands (SpaceServer.seal's accept hook) or when the drain stages an error/drop notice; released only by the WAVE OUTCOME (`committedEventIds` ∪ `requeuedEventIds` after `commitWave` — every abort arm reports its event-handler contributions as requeued), by a deferral (no mark), by the copy's final callback while still `queued` (an aborted run / name-resolution drop — nothing reached a wave), by a notice that failed to stage/seal, and by park. The `catch (drainError)` releases too (finding 4a). Callbacks are tenure-checked (finding 4b). events.md §4 + the register now say "at most one copy until the store has spoken", not "exactly-once". Verified: the events-down exactly-once pin green (mutation guard-removed → processed 11, red), 6/6 two-browsers runs on the final binary `processed == appended`. 2. **MINOR, late-echo cascade + effects → FIXED (option a).** A child echo whose `parentEventId` is a jobless intent is dropped too, and the dropped run's own id joins the jobless set (grandchildren fold in); `deferSealedEffects` owns and does not enact a dropped late echo's effects (the closed arm's shape). Pinned (scripted: child, grandchild dropped; a live intent's child registers; navigateTo not flushed). Stated in speculation.md §4 / the register as INFERRED mechanism for E2, not witnessed. Owner question 1/2 stays open (ratification of the sentence). 3. **MINOR, fan-out yield placement → RESOLVED by REMOVAL.** Moving the yield ahead of the snapshot (as suggested) reproduced a real double emission (executor-space-server's LT6 early-emit arm: two durable entries) — a macrotask inside the fan-out loop lets a run's own async seal refusal land mid-pass and dirty its instance, which the same pass then re-runs while the refusal's queued retry re-runs it again. The original placement only avoided it with one demander. The fan-out loop keeps its microtask shape (run.ts back to base + a note); the settle-loop yield between ACTIONS stays. Deadline honesty is "within one action" (docs, register, comments updated). 4. **MINOR, wedge/tenure → FIXED** (see 1). 5. **MINOR, post-await re-check → FIXED** (mirrors the closed arm; withdraws collected entries). 6. **MINOR, test (ii) flake hazard → HARDENED**: TTL 900 ms / 45-step walk (1.8 s, ~9× activation headroom); unit bound `firedAt < 90` (the loop's own burn is 100 ms). (i)'s bound unchanged (measured 184 ms vs 600). 7. **MINOR, docs/comments → FIXED**: overlay header now attributes E2 to the late echo (inferred) and the wake to the routine decoupled-arrival shape; events.md wording; the "one set lookup" comment; register states inference. 8. **NIT, observer-slot hygiene → FIXED** (release only if the slot still holds our function; both wakes). 9. **NIT, telemetry → not changed** (cosmetic; noted). Owner questions 1–6: carried into the PR's Flags verbatim.