Skip to content

🤖 refactor: enforce workspace-turn settlement causes (Wave4 PR2) - #4096

Merged
ThomasK33 merged 4 commits into
mainfrom
effect-wave4-pr2-settlement-invariant
Sep 5, 2026
Merged

🤖 refactor: enforce workspace-turn settlement causes (Wave4 PR2)#4096
ThomasK33 merged 4 commits into
mainfrom
effect-wave4-pr2-settlement-invariant

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Sep 5, 2026

Copy link
Copy Markdown
Member

Summary

Make workspace-turn terminal transitions name an internal, validated cause, and pin the synthetic-wake false-settlement regression fixed by #3949. No lifecycle-policy, persisted schema, wire, liveness, or Effect-fiber changes.

  • Cover all 13 central settlement call sites plus the independent explicit-interrupt path with 14 cause kinds. The same synchronous validator runs inside each existing lock before terminal persistence; explicit stop retains its epoch/latch/mirror ordering.
  • Carry the actual superseding input ID for manual supersession. Preserve conservative interruption on unreadable history, missing stream-end, missing anchor, or invalid persisted manual-input ID under a separate cause with an explicit reason; no fabricated manual evidence or extra history reads.
  • Add 19 real-manager/real-store/real-HistoryService regression cases covering pending waiters, disposable cleanup, terminal attention, continuation, malformed causes, and explicit-stop ordering. Reuse the existing test harness without changing existing assertions.

Evidence

  • Exact pre-fix main 3937abd4e96ab7475f5f702bd40118016f83e5c4: behavior-only reproduction compiles and fails at the running assertion: actual interrupted, “Workspace turn superseded by an uncorrelated workspace stream-end.” The new cause assertions are not involved.
  • PR2: 791 tests pass across the manager, TaskService, TaskHandleStore, task tools, and workspace-turn inheritance suites; make static-check and a fresh make build pass on Bun 1.3.5. Detailed logs, reproduction, and isolated live evidence are in the first comment.

Live dogfood limitation: two fresh isolated parent/child monitor attempts failed the requested waiting-through-final scenario: the parent received an early correlated finishReason: "tool-calls" error. With a retained child, the 60-second script then exited successfully and the child completed with the same turn correlation; the handle self-healed to completed after the parent had already failed. No uncorrelated assistant end or new cause assertion failure occurred. The untouched monitor-withdrawal/queue-cut continuation path is documented with screenshots/video in the first comment; per the addendum, this PR does not broaden into that lifecycle-policy fix. Live success is not claimed.

Scope / risk

The Sep 5 addendum supersedes the original single-chokepoint/manual-only/11-caller assumptions. Production is +104 net lines rather than the stale ~40 estimate. The manager owns no AI emitter routing: its regression fixture uses actual creation/admission, the established AI fake with correlated stream state, and the manager's direct event entry point. Existing private-settlement tests receive causes only; all existing assertions remain intact.

The validator is an internal developer contract, not a new authorization or recovery policy. Arbitrary uncorrelated synthetic output is never authoritative completion; readable anchored history without manual supersession stays nonterminal. Unknown-history conservative interruption remains intentional. #3915 remains a soft dependency; the two-week live-host observation and post-wave N20 main-run check remain follow-up signals, not claims established by this PR.


📋 Accepted implementation plan (including superseding Sep 5 addendum)

Effect migration — Wave 4: finish the concurrency/lifecycle core

Bounded wave: 4 PRs (PR 4 optional), explicit STOP criterion, explicit OFF-RAMPs. Plan only; nothing here is implemented.

Review status: Independently reviewed (adversarial Reviewer sub-agent, advisor unavailable): APPROVE WITH REQUIRED EDITS — both edits applied; verified claims: Effect.promise 0-arity thunk allocates no AbortController (internal/effect.js:741–776); closed-scope forkIn+startImmediately runs onInterrupt (2237–2274, 391–409); forkIn observer removes the scope finalizer on exit (2270–2271); Effect.timeoutOrElse exists (Effect.d.ts:7833); 0 line drift at b87f627; 11 settleWorkspaceTurn callers confirmed; 'aborted' ∈ NON_RETRYABLE_STREAM_ERRORS. Line references are to main @ b87f62729.

0. Thesis check (coordinator's judgment vs. evidence)

Thesis: Effect's payoff in this app is structured concurrency + interruption-safe lifecycles in the orchestration core (still Promise + AbortController).

Verdict: holds for the stream engine; only half-holds for turn handles.

  • Stream engine — holds. ServiceContainer.dispose() never stops or awaits in-flight streams (serviceContainer.ts:478–537 has no streamManager step); an in-flight stream dies with the process and is recovered on next load from partial.json (≤ 500 ms stale, PARTIAL_WRITE_THROTTLE_MS, streamManager.ts:776). AppFiberScope exists precisely for this and has no occupant. A supervised per-stream fiber is the right tool.
  • Turn handles — half-holds. The 7× "superseded by an uncorrelated workspace stream-end" false-settle is a correlation-predicate bug (interruptWorkspaceTurnFromUncorrelatedStreamEnd, workspaceTurnManager.ts:4220–4307: any uncorrelated stream-end after the prompt index settles the handle interrupted), not a Promise-vs-fiber structure bug. Turn handles are persisted records (taskHandleStore.upsertWorkspaceTurn) spanning multiple streams (tool-call continuations are deferred via hasSameTurnContinuation, :4491) and surviving restarts; a fiber/Deferred can only model the in-process waiter and would not fix correlation. Open PR [task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 fixes the predicate in Promise idiom and is Codex-green. Wave 4's turn-handle PR therefore becomes "codify the settlement invariant + prove the class is gone", not "fiberize handles" (D5 below).

Corrected baseline numbers (measured this workspace): 46/470 src/node non-test files import effect (coordinator said 35); 113 direct Effect.run* sites outside di/ in 16 files; 226 Effect.gen; 9 TaggedError classes; effect 4.0.0-rc.112, @orpc/* 1.14.11; effect v4 is not GA (rc line still current).

1. Verified current state (evidence the design rests on)

Stream engine (streamManager.ts)
  • startStream (:4723–4901): per-workspace mutex → new AbortController() + linkAbortSignal (:4771–4772) → resourceScope = Scope.makeUnsafe() (:4777) → temp-dir Effect.acquireRelease (:4802–4824) → createStreamAtomicallystreamText (:2244, abortSignal: abortController.signal :2250) → registered in workspaceStreams (:2463) → streamInfo.processingPromise = this.processStreamWithCleanup(...) fire-and-forget (:4876–4882) → returns Ok({ messageId, completion }).
  • processStreamWithCleanup (:3331–4089, plain async): while(true) retry loop; for await (part of fullStream) (:3358–3837) with abort check at loop head (:3361); post-loop if (!signal.aborted) gate (:3849) → completion path (deletePartial :3981, updateHistory :3989, recordSessionUsage :4001, state = COMPLETED :4017, emit stream-end :4023, terminalCompletion :4024); error path → handleStreamFailure (:4094–4112) → persistStreamError writes error partial; finally (:4052–4088): release MCP lease, Effect.runFork(Scope.close(resourceScope)) (:4064–4066), unlink abort, workspaceStreams.delete, eventSpine.emit("stream.end"), completionController.settle.
  • Cancellation: stopStream (:5043–5111) → cancelStreamSafely (:1766–1800): if (state === COMPLETED) { await processingPromise; return }state = STOPPINGflushPartialWriteabortController.abort()cleanupAbortedStream (:1828–1951): await processingPromise → usage → writePartial (:1876–1910) → emitStreamAbortsettle({status:"aborted"}). No completed-guard after the await (verified :1838–1951): a cancel landing between :3849 and :4017 re-writes partial.json after deletePartial and emits stream-abort after stream-end (pre-existing window; dispose() will widen its exposure). cancelStreamSafely is also not idempotent for concurrent callers (only COMPLETED is checked).
  • AIService on stream-abort (aiService.ts:355–377): abandonPartial ? deletePartial : commitPartial → deletePartial (fire-and-forget listener).
  • Crash recovery: HistoryService.commitPartial (historyService.ts:1963–2061) — strips error metadata, hasCommitWorthyParts, stale-epoch check, update-or-append by historySequence, delete partial; invoked from agentSession.init (:5002), aiService.streamMessage (:886), stream-abort (:364), duplicateWorkspace.
  • StreamAbortReason = "user" | "startup" | "system" (src/common/orpc/schemas/stream.ts:295).
  • Pinned seams: chaos test Reflect.set(streamManager, "tokenTracker" | "createStreamResult") (streamManager.chaos.test.ts:130–134, 238–242); streamManager.test.ts Reflect.set on processStreamWithCleanup (:2787), createStreamAtomically (:2783), createTempDirForStream, cleanupStreamTempDir, Reflect.get on workspaceStreams, schedulePartialWrite, …; modelOnlyNotifications.test.ts calls processStreamWithCleanup directly (:93, :187); aiService.test.ts spies startStream, generateStreamToken, createTempDirForStream, isResponseIdLost. Constructor: (historyService, sessionUsageService?, getProvidersConfig?, eventSink = noop, runner = defaultEffectRunner) (:801–813); effectRunner used at :1152, :1154, :1169 only.
  • Every stream event carries workspaceId + messageId; stream-end/stream-abort/error carry metadata.muxMetadata when the prompt had it.
DI / shutdown / startup
  • AppFiberScopeLive is in CoreLive's runtimeSeams (di/layers/core.ts:644), so both roots have it; StreamManagerLive (core.ts:226–238, stage S2b) already yields EffectRunnerTag. CLI cleanup lists include appFiberScope.close (cli/run.ts:1579, cli/workflow.ts:286).
  • Bounds: APP_FIBER_SCOPE_CLOSE_TIMEOUT_MS = 2000, APP_RUNTIME_DISPOSE_TIMEOUT_MS = 2000; outer budgets are 5000 ms on both desktop (desktop/main.ts:1297 Promise.race vs setTimeout(5000)) and xum server (cli/server.ts:236–243 force-exit). The scope bound cannot grow without changing outer budgets.
  • rc.112 semantics verified in node_modules/effect/dist/internal/effect.js:2264: forkIn registers a scope finalizer and removes it when the fiber completes (no leak), and interrupts immediately if the scope is already closed (streams starting mid-shutdown fail closed). Effect.promise(evaluate: (signal) => PromiseLike), Effect.onInterrupt, Effect.forkIn(_, scope, { startImmediately? }), Stream.toAsyncIterableWith(context), Stream.provideContext all exist.
  • ServiceContainer.initialize() (serviceContainer.ts:297–362): six awaited initialize()s wrapped in recordStep (durations only, no catch, no timeout) + three sync start()s + two fire-and-forget sweeps. Failure handling: desktop Startup Failed dialog + app.quit() (desktop/main.ts:1249–1265); cli/server.ts:136 uncontained; ACP serverConnection.ts:205–216 dispose + rethrow; tests/ipc/setup.ts:85 no catch. No outer timeout anywhere.
  • streamBridge.subscriptionIterable (orpc/streamBridge.ts:176) → Stream.toAsyncIterable(...) on the global runtime, 19 call sites in routerSubscriptions.ts; heartbeat via Effect.sleep in forkScoped (:145–152). streamBridge.test.ts has 11 real-time waits, but only 3 are clock-bound (:207 1 ms initial delay, :241 heartbeat 10 ms, :255 10 ms laziness); 8 are waitFor(listenerCount…) readiness polls that TestClock cannot replace.
Turn handles + open PRs
  • Handle record { handleId "wst_…", ownerWorkspaceId, workspaceId, turnId, messageId, status, attentionPolicy, disposableWorkspace }; prompt carries muxMetadata: { type:"workspace-turn-task", taskHandleId, ownerWorkspaceId, turnId } (workspaceTurnManager.ts:1420). TaskService forwards aiService stream-end/stream-abort/error to finalizeWorkspaceTurnFromStreamEnd (:4442–4544): correlated branch matches record.workspaceId && record.turnId (:4472); uncorrelated branch (metadata == null, not agentId === "compact") → interruptWorkspaceTurnFromUncorrelatedStreamEnd → settles interrupted whenever streamEndIndex >= promptIndex (:4293–4305). Producers of such uncorrelated ends: bash-monitor wake continuations, child terminal-attention deliveries, heartbeat, peer messages, parent auto-resume.
  • Cascade: disposable child → cleanupDisposableWorkspaceTurnworkspaceService.remove(…, true) kills its background processes; persistent child → parent sees interruptedtask_stopbackgroundProcessManager.stopMonitor(…, "canceled"). This is the observed "monitors died afterwards".
  • Settlement chokepoint: settleWorkspaceTurn(params) (:2085), 11 callers, guarded by workspaceTurnSettlementLocks.withLock(handleId); waiters in pendingWorkspaceTurnWaitersByHandleId with setTimeout timeouts (:2425–2496).
  • [task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 "preserve turns across synthetic wake ends" (coadler): rewrites the uncorrelated branch — walks history from the turn anchor to the stream-end and settles only if a manual child input intervened (isManualChildWorkspaceInput); otherwise ignores the end. Touches :281–295, :4217–4355 + tests (+286/−31). Codex: "Didn't find any major issues" + clean security on f9baa2fc9. mergeable: MERGEABLE, but Test / Unit and Codex Comments red, 19 commits behind main.
  • [task-service] 🤖 fix: correlate workspace-turn liveness #3915 "correlate workspace-turn liveness" (coadler): creation reservations + getWorkspaceTurnLiveness/getWorkspaceTurnRuntimeActivity (identity-matches the active stream's muxMetadata against the record) for staleness/capacity. Touches :442–486, :1298, :2573, :3761, :3800–4064 (+494/−60). BLOCKED, latest Codex review has open findings, Test / Unit red, 19 behind.
  • Together they are the identity-correlated model the coordinator wants: [task-service] 🤖 fix: correlate workspace-turn liveness #3915 = identity-correlated liveness, [task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 = identity-gated settlement.

2. Design decisions

D1 — Fibers WRAP the AbortController; they do not replace it.
The AI SDK is cancelled only via AbortSignal; the for await loop, soft-interrupt at step boundaries, retry/fallback re-creation of streamResult, and ~30 abort touchpoints (#4032) all key off the signal. Converting the 750-line loop to Stream.fromAsyncIterable + fiber interruption would touch hundreds of WorkspaceStreamInfo transitions and break the processStreamWithCleanup/createStreamResult spy seams. Instead: the fiber is the ownership/supervision unit; the signal stays the cancellation transport. The dual-cancellation glue #4032 feared is confined to one point — the supervisor's onInterrupt — which routes through the existing user-stop path (cancelStreamSafely), so shutdown ≡ "user pressed stop" semantically (partial flushed with usage, stream-abort emitted, completion settles aborted, AIService commits the partial).

D2 — Supervisor topology: one supervisor fiber per stream in AppFiberScope, wrapping the already-started processingPromise.
streamInfo.processingPromise = this.processStreamWithCleanup(...) stays byte-identical (sync-start preserved; Reflect.set(processStreamWithCleanup) seam preserved; cleanupAbortedStream's await processingPromise unchanged). Immediately after it:

// startStream, after processingPromise is assigned (unsupervised path unchanged when no scope)
this.superviseEngine(typedWorkspaceId, streamInfo);

private superviseEngine(workspaceId: WorkspaceId, streamInfo: WorkspaceStreamInfo): void {
  if (this.engineScope === undefined) return;               // direct construction / CLI tests: today's behavior
  assert(streamInfo.engineFiber === undefined, "engine already supervised");
  // Zero-arity thunk on purpose: rc.112 allocates an internal AbortController only
  // when `evaluate.length !== 0`; the stream's own controller stays the sole signal.
  const supervisor = Effect.promise(() => streamInfo.processingPromise).pipe(
    Effect.onInterrupt(() =>
      Effect.uninterruptible(   // explicit, per house doctrine (finalizers are already uninterruptible)
        Effect.promise(async () => this.cancelStreamSafely(workspaceId, streamInfo, "system"))
      )
    ),
    Effect.catchDefect((d) => Effect.sync(() => log.warn("[stream] engine supervisor defect", { workspaceId, error: d })))
  );
  streamInfo.engineFiber = this.effectRunner.runSync(
    Effect.forkIn(supervisor, this.engineScope, { startImmediately: true })
  );
}
  • Effect.promise is interruptible while suspended (internal/effect.js:741–801, Async op); onInterrupt = onErrorFilter(causeFilterInterruptors, …) (:1762); forkIn registers fiberInterrupt(fiber) as the scope finalizer (:2264–2275), fiberInterrupt awaits the fiber (:635–642), and parallel scopeClose awaits all finalizers via fiberAwaitAll (:1590–1601) → closeScopeBounded at dispose step 2 gives "interrupt and await" while historyService/sessionUsage/eventSink → AIService → bridge servers are still alive (bridges stop in step 3, so clients receive stream-abort).
  • Normal completion: fiber exits → forkIn's observer removes the scope finalizer (verified) → no per-stream residue.
  • Stream started after step 2: forkIn on a closed scope calls fiber.interruptUnsafe synchronously and returns the fiber (:2272–2274, runSync does not defect). With startImmediately: true, forkUnsafe runs child.evaluate synchronously (:2233–2247) up to the Effect.promise Async op (:772–801), so the fiber is suspended (_running=false) when the interrupt lands and interruptUnsafe (:391–409) unwinds the stack through the onInterrupt handler → the stream is aborted as system (fail-closed during shutdown). Verified in rc.112 internals (effect.js:2233–2247, 391–409); pin with a test ("stream started after scope close is aborted") so an RC bump cannot silently change it.
  • "system" is semantically exact: "user"/"startup" suppress next-startup recovery (retryEligibility.ts:114–118, 284–287), "system" marks an involuntary backend interruption (as taskService.ts:8100, 8223 use it). No in-session retry loop is possible: the stream-abort handler (agentSession.ts:6010) routes { type: "aborted" } to retryManager.handleStreamFailure, and "aborted" is in NON_RETRYABLE_STREAM_ERRORS (retryEligibility.ts:49–59, 106) → retryManager.ts:99–104 abandons immediately, never schedules a fiber. Dogfooding still checks the restart UX (the recovered partial is shown as interrupted; note whether any next-startup recovery re-sends — same class as today's system aborts from taskService).
  • engineScope arrives as an optional 6th constructor parameter (engineScope?: Scope.Closeable), wired from AppFiberScopeTag in StreamManagerLive (core.ts:226). Default undefined keeps every direct-construction test and aiService.ts:174 path identical (I4). AppFiberScopeLive already sits beneath S2b in runtimeSeams, so no staging change (I6).
  • Abort reason: reuse "system" — no wire/schema change; UI copy for system already exists.
  • Pending-start window (pendingStreamStarts, before registration) is not supervised: nothing is persisted for it yet, and stopStream already aborts pending controllers. Documented, not fixed.

D3 — Fix the two adjacent cancel races in the same PR (closely-related bugs, not deferrals).
(a) cleanupAbortedStream: after await processingPromise, if streamInfo.terminalCompletion !== undefined (completed/failed while the cancel was in flight) → return without abort bookkeeping (prevents partial.json resurrection after deletePartial and a stream-abort after stream-end). (b) cancelStreamSafely (:1766): latch a per-stream cancelPromise so concurrent cancellers (user stop racing dispose) join one cleanup → exactly one stream-abort, one settle. Zero-suspension requirement: the latch must be checked and assigned synchronously at function entry, before any await (the current first await is flushPartialWrite at :1789) — otherwise racing callers can both enter cleanupAbortedStream. Shape:

if (streamInfo.cancelPromise) return streamInfo.cancelPromise;
streamInfo.cancelPromise = (async () => {
  /* existing body, unchanged */
})();
return streamInfo.cancelPromise;

Both are ≤ 10 LoC and get behavioral tests.

D4 — Shutdown bound stays 2 s; the finalizer must be fast or abandoned.
Outer budgets are 5 s; 2 s + 2 s already consume 4 s. A flowing stream aborts within one chunk; a wedged provider (no chunks, ignores abort) hits the existing boundedTeardown timeout: warning, continue, process exit — identical to today's outcome. Dogfooding measures the actual [shutdown] AppFiberScope closed { ms } with a live stream.

D5 — Turn handles: codify the settlement invariant; do not fiberize.
Invariant: a workspace-turn handle settles terminally only by (i) a stream terminal event whose muxMetadata correlates {taskHandleId, ownerWorkspaceId, turnId} to the record; (ii) an explicit interrupt (task_stop/interruptWorkspaceTurn); (iii) manual supersession — a manual child input after the turn anchor; (iv) stale-liveness reconciliation. An uncorrelated stream-end is never terminal by itself. #3949 makes (iii) the only uncorrelated outcome; #3915 implements (iv) by identity. Wave 4 adds a cause discriminant to settleWorkspaceTurn (the single chokepoint) with a runtime assertion, plus the regression harness. Rationale for not converting waiters to Deferred/fibers: no behavioral gain, 4.9k-line file, and the coordinator's "settle only on the owning stream's termination" is over-specified — a turn owns several streams.

D6 — Startup: initialize() stays a Promise facade over a runtime-run startup effect; timeout ⇒ same failure path as a thrown step.
Each step is Effect.tryPromise({ try: async () => step(), catch: identity }).pipe(Effect.timeoutOrElse({ duration: STARTUP_STEP_TIMEOUT_MS, orElse: () => Effect.fail(new StartupStepTimeoutError(name, ms)) })) (timeoutOrElse exists in rc.112, Effect.d.ts:7833; chosen over timeout + catchTag because the step's error channel is unknown, which catchTag cannot narrow). No forkDetach needed: a Promise step keeps running on its own when the waiting fiber times out (not inside an uninterruptible region, so the timeout interrupts the wait directly). StartupStepTimeoutError extends Error with name = "StartupStepTimeoutError" set in the constructor and message "<step> exceeded <ms> ms" (so the desktop dialog's error formatting shows both the class and the step name) → desktop shows it in the existing Startup Failed dialog; CLI/ACP/tests paths unchanged. Step errors keep their identity (v4 runPromise rejects with the raw failure). Downgrading any step to best-effort is a policy change, out of scope (audit of the six implementations: extensionMetadata/telemetry/experiments are local fs, <50 ms; policy has its own 10 s fetch timeout; workspaceService bounds its sync internally; only taskService.initialize — config scan + editConfig + recovery sendMessages — is potentially unbounded). The three start()s stay sync (Effect.sync), the two fire-and-forget sweeps stay outside the effect. stepDurationsMs is preserved.
Abandon-and-quit safety: an abandoned taskService.initialize may be mid-editConfig when the root exits. Parity requirement for PR 3: after a rejected initialize(), every root runs the bounded dispose() before exiting. Verified: desktop already does — services is assigned before the await (main.ts:653–656), the catch calls app.quit(), and the before-quit listener (:1271–1305, guard if (isDisposing || !services) return) races services.dispose() against 5 s; ACP does (serverConnection.ts:205–216); cli/server.ts does not (:133–136 awaited at top level, main().catch at :282 only logs) → PR 3 adds a bounded dispose() there (≤ 10 LoC, same 5 s budget).

D7 — streamBridge: thread the runtime context, not a runner. subscriptionIterable gains context?: Context.Context<never>Stream.toAsyncIterableWith(context); routerSubscriptions passes the handler's "effect/context". Production behavior identical; heartbeat sleeps on the runtime Clock; tests can run the 3 clock-bound waits on TestClock. Honest scope: the 8 readiness polls stay.

3. PRs (ordered by value ÷ risk; each independently mergeable)

PR 1 — StreamManager engine core becomes the first AppFiberScope occupant

Value: high (the only remaining shutdown data-integrity gap; the reason AppFiberScope exists). Risk: medium → low with D1/D2. Net product LoC ≈ +55 (superviseEngine ~25, ctor param/field ~5, engineFiber field ~2, D3 guards ~12, core.ts wiring ~2, doc updates in appRuntime.ts/appFiberScope.ts "occupant" text ~10).

Files: src/node/services/streamManager.ts, src/node/services/di/layers/core.ts, src/node/services/di/appRuntime.ts + appFiberScope.ts (docs), tests below.

Pre-work (before writing product code; each yields a note in the PR body):

  1. Confirm processStreamWithCleanup never rejects (try/catch/finally shape :3331–4089); else the supervisor must fold rejections (it already catchDefects).
  2. Confirm Effect.promise interruption + onInterrupt await ordering under Scope.close in a 20-line probe test (pattern of appFiberScope.test.ts:27–47).
  3. Enumerate abort observers that run after the finalizer resolves (AIService stream-abort listener → commitPartial; agentSession completion continuations) and confirm the durable order (writePartial → commit → deletePartial) makes a mid-flight process.exit recoverable on next load (it is: partial survives until commit completes).
  4. Measure: [shutdown] AppFiberScope closed { ms } with a live stream in the sandbox (D4).
  5. Pin (same probe test): a fiber forked with startImmediately: true into an already-closed scope still runs its onInterrupt finalizer (reviewer-verified in rc.112 internals; the test guards RC bumps).

Acceptance (behavioral tests only):

  • streamManager.test.ts (new cases; existing cases untouched): with engineScope = Scope.makeUnsafe("parallel") and a fake createStreamResult whose fullStream yields one text-delta then blocks until its AbortSignal fires — closeScopeBounded(engineScope) resolves; writePartial was called with the streamed text; exactly one stream-abort (abortReason: "system") and zero stream-end; completion settles {status:"aborted"}; workspaceStreams is empty.
  • Wedged provider (fullStream never yields, ignores abort): closeScopeBounded resolves within the bound, never rejects, warns once (assert the returned promise resolves and no throw; do not assert log text).
  • No-scope construction: identical event sequence to today (guards existing suites; no new assertions needed beyond the unchanged suites passing).
  • D3(a): cancel issued after the loop exits but before COMPLETED → history has exactly one final message, partial.json absent, event order stream-end only.
  • D3(b): stopStream + closeScopeBounded racing on one stream → exactly one stream-abort, one settle.
  • Fiber residue: after 50 completed streams, closeScopeBounded(engineScope) emits zero stream-abort and completes in the same tick class as an empty scope (assert no aborts and workspaceStreams.size === 0).
  • streamManager.chaos.test.ts — existing cases byte-identical; one new fuzz variant constructs with an engine scope and closes it at a random iteration: every stream settles exactly once (count terminal events per messageId ≤ 1, all completion promises settle).
  • serviceContainer.test.ts: "dispose() aborts and awaits an in-flight stream before desktopBridgeServer.stop()" (extend the ordering harness at :295–323); coreServicesRoot.test.ts: xum run cleanup list does the same via appFiberScope.close.

Gate suites: streamManager.test.ts, streamManager.chaos.test.ts, streamManager.modelOnlyNotifications.test.ts, aiService.test.ts, agentSession.disposeRace.test.ts, agentSession.sinceReplayContract.test.ts, serviceContainer.test.ts, coreServicesRoot.test.ts, di/*.test.ts, taskService.test.ts, workspaceService.test.ts, turnRequestBuilder.test.ts; make static-check.

House pre-review audits: interruption posture (supervisor's only suspension is the promise; finalizer uninterruptible end-to-end incl. cancelStreamSafelycleanupAbortedStream); no defect escapes (catchDefect on the supervisor; Effect.promise thunks async); spy-seam check (processStreamWithCleanup, createStreamResult, createStreamAtomically, startStream signatures unchanged; constructor arity unchanged, trailing optional); sync-start (processingPromise assigned before fork; runSync(forkIn) completes synchronously); no constructor side-effects added; zero-suspension check on D3(b) latch (cancelPromise checked-and-assigned synchronously at cancelStreamSafely entry, before the first await at :1789; review the diff for any inserted await/lookup ahead of the assignment).

Rollback: revert the core.ts wiring line → engineScope undefined → today's behavior; D3 guards can stay (independent bug fixes).

PR 2 — Turn-settlement invariant + false-settle regression harness (gated on #3949)

Value: high (7× production race). Risk: low. Net product LoC ≈ +40 (WorkspaceTurnSettlementCause union + cause on settleWorkspaceTurn params + assert ~10; 11 call sites × 1–3 lines).

Relationship to open PRs — explicit:

Design: type WorkspaceTurnSettlementCause enumerated from the 11 callers (audited at main @ b87f62729): :1373 creation validation failure; :1476 pre-stream interrupt during launch; :1500/:1518 pre-stream send failure; :3834/:3863 stale-liveness recovery / restart timeout (settleStaleWorkspaceTurn#3915's region); :4301 uncorrelated-stream-end manual supersession (the only uncorrelated settle in the codebase; the path #3949 rewrites); :4529 correlated terminal; :4571 stream-abort; :4675 deferred stream error; :4736 terminal stream error. settleWorkspaceTurn asserts params.cause is a member and, for manual-supersession, that the superseding input's messageId is supplied — turning D5 into an exhaustive Record<Cause, …> check rather than prose, so a future "settle on uncorrelated end" cannot be added without naming (and justifying) a cause.

Acceptance:

  • New workspaceTurnManager.uncorrelatedStreamEnd.test.ts (real WorkspaceTurnManager + TaskHandleStore + fake aiService emitter, following the existing suite's harness): (1) create turn → correlated stream-startsynthetic wake stream on the same child ends uncorrelated after the anchor → handle stays running, waiter unresolved, no disposable cleanup, no terminal attention → correlated stream-endcompleted. (2) same with finishReason:"tool-calls" continuation in between. (3) manual child input between anchor and end → interrupted with cause: manual-supersession. (4) explicit interruptWorkspaceTurninterrupted, cause: explicit-interrupt. Case (1) is the scripted reproduction: it must fail on the pre-[task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 merge-base (run the file from a sibling worktree at git merge-base origin/main <#3949 head>; record the failing assertion in the PR body) and pass after.
  • Existing 113 workspaceTurnManager.test.ts cases and taskService.test.ts turn cases unchanged.

Gate suites: workspaceTurnManager.test.ts, taskService.test.ts, taskHandleStore.test.ts, tools/task*.test.ts; make static-check.

Audits: spy-seam (getWorkspaceTurn, listAllWorkspaceTurns, enqueueTerminalAttention, deliverPersistentChildWorkspaceTurnResult untouched); settlement lock held across the assert; no new suspension inside withLock.

Rollback: revert; the test file stays valid against #3949 alone (drop the cause assertions).

PR 3 — ServiceContainer.initialize() as a runtime-run startup effect with per-step timeouts

Value: medium (a hung taskService.initialize() currently pins the splash screen forever; deterministic TestClock tests of startup). Risk: low–medium. Net product LoC ≈ +80 (step table ~20, timed-step helper ~15, StartupStepTimeoutError ~8, constant ~3, facade ~10, root dispose-on-failure parity ≤ 10, doc update ~10).

Files: serviceContainer.ts, src/constants/terminationTimeouts.ts (keep with the termination constants so the budget doc stays in one place), cli/server.ts (dispose in the startup catch if missing), di/appRuntime.ts doc ("Deliberately not done" → remove the initialize() line; add startup contract).

Design (D6): initialize(): Promise<void>this.runtime.managed.runPromise(this.startupEffect()). startupEffect = Effect.gen over an ordered readonly steps: ReadonlyArray<{ name, run: () => Promise<void> }> (assert names unique); each step: recordStep timing kept, Effect.tryPromise({ try: async () => run(), catch: identity }).pipe(Effect.timeoutOrElse({ duration: STARTUP_STEP_TIMEOUT_MS, orElse: () => Effect.fail(new StartupStepTimeoutError(name, STARTUP_STEP_TIMEOUT_MS)) })). Then Effect.sync for the three start()s; the sweeps remain after runPromise. Constant STARTUP_STEP_TIMEOUT_MS — pre-work measures [startup] <step> { ms } across sandbox cold starts and picks ≥ 10× the slowest observed (propose 60 s; must be generous — a false timeout turns a slow-but-fine start into a crash). Roots dispose after a rejected initialize() (D6 abandon-and-quit safety).

Acceptance (all in serviceContainer.test.ts, TestClock via the existing AppLive spy at :355–395):

  • A step that never resolves → initialize() rejects with StartupStepTimeoutError naming the step after exactly TestClock.adjust(STARTUP_STEP_TIMEOUT_MS); later steps did not run.
  • A rejecting step → initialize() rejects with the same error object (identity), later steps did not run (parity with today).
  • Happy path → stepDurationsMs has all six keys; start()s called once each; second initialize() call behavior unchanged from today (verify whether re-entry is guarded today; preserve).
  • tests/ipc harness and ACP entry still pass unchanged.

Gate suites: serviceContainer.test.ts, coreServicesRoot.test.ts, di/*.test.ts, src/node/acp/*.test.ts, TEST_INTEGRATION=1 bun x jest tests/ipc (smoke subset); make static-check.

Audits: I1 untouched (no layer body changes); I2 (only the composition root touches the runtime); error identity preserved (no wrapping); abandoned-step safety — every root runs bounded dispose() after a rejected initialize() (D6; cli/server.ts gains it in this PR); no sweep moved into the effect; the six-step order and [startup] <step> names unchanged.

Rollback: revert; constant removal.

PR 4 (optional — cut if budget is exhausted) — streamBridge on the runtime context

Value: low (closes the last documented "global runtime" exception; enables TestClock for the heartbeat). Risk: low. Net product LoC ≈ +30 (context? option + toAsyncIterableWith ~8; 19 call sites × 1 line via one shared helper in routerSubscriptions.ts ~3).

Acceptance: the 3 clock-bound waits (:207, :241, :255) run on TestClock; the heartbeat test asserts N heartbeats after TestClock.adjust(N × interval) with zero real time; existing behavioral assertions unchanged; tests/ipc subscription tests pass. Do not rewrite the 8 readiness polls.

Gate suites: streamBridge.test.ts, routerSubscriptions*.test.ts, orpc/*.test.ts, TEST_INTEGRATION=1 bun x jest tests/ipc (subscription subset); make static-check.

Audits: Stream.toAsyncIterableWith preserves double-close safety (pin with the existing test); Cause.Done typing unchanged; no Scope/MemoMap/Scheduler captured (pass the oRPC effect/context, which the DI layer already strips per EffectRunnerLive); context stays optional so direct callers/tests without a runtime keep today's global-runtime path.

Rollback: revert; the optional context default (Context.empty()) is exactly today's toAsyncIterable, so a partial revert of call sites is also safe.

Execution order and size

Net product LoC for the wave ≈ +205 (PR 1 ≈ +55, PR 2 ≈ +40, PR 3 ≈ +80, PR 4 ≈ +30); tests ≈ +600–800. PR 1 starts immediately. PR 2 starts the moment #3949 merges (parallel with PR 1/3 — disjoint files). PR 3 after PR 1 merges (both touch appRuntime.ts docs; PR 3 also touches serviceContainer.ts). PR 4 last, only if PRs 1–3 landed and no OFF-RAMP fired. Each PR: Codex dual review, Codex Comments minimization, merge queue; commit WIP early (/tmp wipes).

4. STOP criterion (measurable) and OFF-RAMPs

Wave 4 is done — and the Effect migration line stops without a new RFC — when all hold:

  1. dispose() awaits in-flight streams: serviceContainer.test.ts ordering test + coreServicesRoot.test.ts pass on main; a sandbox script -f transcript of xum server receiving SIGTERM mid-stream shows stream-abort[shutdown] AppFiberScope closed { ms }[shutdown] desktopBridgeServer.stop, and immediately after exit partial.json is absent while chat.jsonl contains the interrupted assistant message (baseline on main: partial.json present, message absent until next load). { ms } < 2000 in the flowing-stream case.
  2. False-settle class eliminated: the scripted reproduction fails on the pre-[task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 merge-base and passes on main after PR 2; settleWorkspaceTurn rejects any settlement without an enumerated cause; the coordinator's own Mux sessions show zero "superseded by an uncorrelated workspace stream-end" in the two weeks after PR 2 (soft signal, logged in the wave summary).
  3. Startup: timeout and error-identity tests pass under TestClock; [startup] per-step lines unchanged in the sandbox transcript; a throwaway build with the constant set to 1 ms shows Startup failed: StartupStepTimeoutError: <step> exceeded 1 ms and a clean exit.
  4. No new lifecycle flakes: 0 failures attributable to the touched suites across N = 20 consecutive completed Test / Unit runs on main after the last Wave 4 merge — query gh run list --workflow pr.yml --branch main --limit 60 --json databaseId,status,conclusion,event,headSha (note: gh run list --json serializes these fields in lowercase, e.g. {"status":"completed","conclusion":"success"}, unlike statusCheckRollup), keep status === "completed" (pending runs have an empty conclusion, not null), take the newest 20, and for any run with conclusion !== "success" (case-insensitive normalization acceptable) inspect the failing job's log for the touched suite names (job timeout/cancelled from the 15-min budget is not a flake); plus green merge-queue runs for each PR. Any attributable flake → fix or revert before declaring done.

OFF-RAMP (PR 1): fires if pre-work 1–3 shows (a) routing shutdown through cancelStreamSafely cannot preserve crash-recovery semantics without changing cleanupAbortedStream's contract beyond D3, (b) the chaos variant exposes a double-settle not closable by D3(b), or (c) the finalizer cannot fit the 2 s bound for flowing streams. Then: stop PR 1, keep AppFiberScope unoccupied, update appRuntime.ts "Deliberately not done" with the concrete blocker and the measured evidence, land D3 alone as a bug-fix PR. PRs 2–4 are independent and proceed.
OFF-RAMP (PR 3): if error identity or the tests/ipc/ACP paths cannot be preserved, keep initialize() as is and record why.
OFF-RAMP (PR 2): #3949 not merged → hold (see PR 2).

5. Risk register

Risk Likelihood Mitigation
Crash-recovery regression: double commit / partial resurrection when shutdown-abort races completion medium (window widens with dispose()) D3(a) guard + test; commitPartial's historySequence update-or-append is idempotent (historyService.ts:2036–2041)
Provider abort emits an error chunk → error path instead of abort path low identical to today's user-stop path (parity); chaos variant covers hostile streams
Wedged provider pins the 2 s bound → warning every shutdown low boundedTeardown already bounds; transcript measures; no budget change possible (5 s outer)
AIService stream-abort listener (commitPartial) still in flight when process.exit runs low durable order writePartial → commit → deletePartial; next-load recovery; PR 1 transcript checks partial.json is already gone when cli/server.ts logs its final cleanup line before process.exit(0) (:252–266)
Chaos-test seams (createStreamResult, tokenTracker) none if scope-less construction stays default new variant added, old cases untouched
Collision with #3915/#3949 medium PR 2 gated; no edits to their regions; one-line cause: conflicts only
RC churn (rc.113+ renames forkIn/onInterrupt/toAsyncIterableWith) low all Effect imports already in streamManager.ts/streamBridge.ts; pins fixed; GA upgrade is a separate lockstep PR (§6)
Startup false timeout on slow hosts medium if constant too small measure first; ≥ 10× slowest observed; generous default (60 s)
Sync-start assumptions in tests that Reflect.set(processStreamWithCleanup) low promise assigned before fork; forkIn runSync synchronous
shutdown() (desktop second before-quit listener) still does not await streams accepted contract says shutdown() never touches the runtime; desktop's dispose race is the covered path
Streams starting during shutdown covered forkIn on closed scope interrupts immediately → system abort (startImmediately semantics verified in rc.112; pinned by test)
system abort triggers an in-session RetryManager retry during shutdown unreachable (verified) "aborted"NON_RETRYABLE_STREAM_ERRORSretryManager.ts:99–104 abandons; no fiber scheduled
PR 3: abandoned taskService.initialize mid-editConfig when the root exits after a timeout low desktop/ACP already dispose on startup failure; PR 3 adds the missing cli/server.ts dispose; config writes are lock/journal-protected
startImmediately/onInterrupt semantics differ in a later RC low pinned by the probe test in appFiberScope.test.ts style; RC bumps are a separate lockstep PR

6. Standing item — effect v4 GA + @orpc/experimental-effect lockstep (analysis only)

v4 is not GA (rc.112 is current; v3 3.x remains the stable line). No PR this wave. When GA ships: one lockstep PR bumping effect + all @orpc/* (1.14.11 today; check the GA-compatible @orpc/experimental-effect), canary gates = di/*.test.ts, streamBridge.test.ts, streamManager.test.ts, serviceContainer.test.ts, TEST_INTEGRATION=1 bun x jest tests/ipc, make static-check. The Context → ServiceMap rename risk is firewalled: Context.Service tags, Context.omit/get, Layer, ManagedRuntime, TestClock live only under di/ + orpc/effectContext.ts; streamManager.ts/streamBridge.ts use Effect/Scope/Fiber/Exit/Stream/Queue/Cause only. PR 4 adds one Context.Context<never> type reference to streamBridge.ts — keep it as a type-only import so a rename is a one-line fix.

7. Dogfooding (per PR; evidence attached to the PR with gh … --attach)

Common setup: make dev-server-sandbox DEV_SERVER_SANDBOX_ARGS="--clean-projects" (or xum server on a temp XUM_ROOT) with XUM_LOG_LEVEL=debug, run under script -f ~/wave4-scratch/<pr>-<scenario>.log; scratch under $HOME/wave4-scratch/ (never /tmp). Drive the UI with agent-browser (opensnapshot -i → click the explicit "Send message" ref; re-snapshot after typing). Screenshots are primary evidence; record WebM and finalize with ffmpeg -c copy.

  • PR 1: (1) start a long stream (prompt that streams ~30 s), wait 3–5 s, kill -TERM <server pid>; transcript must show the order in STOP Better authentication UX #1 and [shutdown] AppFiberScope closed { ms }; (2) ls <XUM_ROOT>/sessions/<ws>/partial.json (absent) + tail -n 1 chat.jsonl (interrupted assistant message); (3) restart, open the workspace in agent-browser, screenshot the persisted interrupted message; (4) same scenario on main for the baseline diff; (5) xum run Ctrl-C mid-stream transcript (CLI root parity); (6) quality gate between phases: gate suites green before the sandbox run, sandbox evidence before requesting review.
  • PR 2: primary evidence is the regression file run on both the merge-base worktree (failing output) and the branch (passing). Secondary: sandbox parent workspace delegates via task kind=workspace to a child that arms a background bash monitor firing within ~10 s and keeps working ~60 s; screenshot the parent's task result (baseline main: interrupted … uncorrelated workspace stream-end; after: completed) and the child's log lines.
  • PR 3: xum server cold start transcript with [startup] <step> { ms } for the six steps (parity); throwaway worktree build with STARTUP_STEP_TIMEOUT_MS = 1 → transcript of Startup failed: StartupStepTimeoutError … and exit code (do not ship); desktop dialog cannot be shown headless — cite the unchanged desktop/main.ts:1249–1265 catch.
  • PR 4: agent-browser session left idle 60 s with the connection indicator visible (heartbeats keep it green) + screenshot; streamBridge.test.ts on TestClock.

PR 2 implementation addendum — Sep 5, after #3949 merged

This addendum supersedes stale factual assumptions in D5 / PR 2, not their minimal-change goal. Verified base: #3949 merged as 2f99c6a5b027cd09c7574f7af3eeba1cb804ec6a; its pre-merge main parent is 3937abd4e96ab7475f5f702bd40118016f83e5c4. #3915 remains open and is only a soft prerequisite. Proceed with PR 2 without changing its liveness algorithm.

  • Preserve the actual [task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 fallback. It still conservatively interrupts when history cannot be read or the stream-end/turn anchor cannot be found. Such a transition must have a distinct uncorrelated-conservative-fallback cause with a reason (history read failed, missing stream end, or missing turn anchor), never a fabricated manual-input ID. Proven manual-supersession requires the actual superseding input's message ID. Change only evidence plumbing through the existing helper; preserve read count, classification, stale-event behavior and outcomes.
  • There are two settlement paths. settleWorkspaceTurn is the central helper, but explicit interruptWorkspaceTurn also persists terminal state inside its own lock while establishing stop epoch/latch and mirror ordering. Use one small synchronous cause validator inside both existing lock boundaries. Do not route explicit interrupt through the central helper, nest locks, move stop-latch work, or add async work. Enumerate actual current call sites (including newer creation/admission/recovery failures), not the old count of 11.
  • Internal contract only. Causes and their evidence are internal parameters/assertions, not persisted task-handle fields, schemas, wire payloads or migrations. Use exhaustive typing plus runtime validation for missing/invalid causes; add behavioral negative coverage. Keep real HistoryService/TaskHandleStore and established test/spy seams.
  • Honest invariant: an uncorrelated synthetic end is never authoritative completion. With readable, anchored history and no proven manual supersession it remains nonterminal; explicit conservative fallback may interrupt when evidence cannot be established. Keep failure/abort/recovery and explicit-stop semantics unchanged. A concrete missing final-continuation correlation belongs at its dispatch source, not in arbitrary wake-output completion (the fresh [task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 review examined this and accepted the production correlation trace).
  • Reproduction base: the old merge-base origin/main <PR head> recipe is invalid after the prerequisite's additive main sync. Use the exact pre-[task-service] 🤖 fix: preserve turns across synthetic wake ends #3949 main parent above. Run the behavior-only reproduction there (strip only PR2-specific cause assertions if needed); it must compile and fail on the intended premature terminal settlement. The complete harness must pass on the PR2 branch. Add fallback-reason tests and preserve explicit-interrupt side effects/order.
  • Dogfooding: retain §7 PR2's scripted regression evidence plus a freshly built isolated parent/child monitor-wake scenario, with screenshots/video. Do not assume this coordinator's currently running backend has upgraded just because main merged. Report the two-week live-session observation as a soft signal with version/exposure caveats, not as proof supplied by unit tests.

Acceptance remains regression proof + explicit, validated settlement causes without lifecycle-policy changes. Record this reconciliation in the PR description. Advisor reviewed this approach on Sep 5 and recommended preserving the explicit-interrupt path and conservative fallback as separate named causes rather than forcing the old plan's inaccurate single-chokepoint claim.

8. Non-goals (restated; out of this wave)

Typed-error propagation sweep / removing the ~113 facades; converting services to yield*-based Effect services; PubSub for the internal EventEmitter bus; Schema at persistence boundaries; Effect observability; converting sync read paths, AI-SDK per-request callbacks, cross-process lock interiors, or deterministic try-lock funnels; replacing AbortController as the SDK cancellation transport; converting the fullStream loop to Stream; fiberizing turn-handle waiters; downgrading startup steps to best-effort; changing outer quit budgets; supervising the pre-registration stream-start window; shutdown() semantics; the effect GA bump (standing analysis only).


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

Require an internal cause at both existing settlement locks without changing lifecycle policy. Preserve conservative fallback reasons and capture the actual manual superseding input.

---
_Generated with [`mux`](https://github.com/coder/mux) • Model: `coder:openai/gpt-6-astra` • Thinking: `high` • Cost: `$6.94`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=high costs=6.94 -->
Extract the existing manager test harness without changing assertions and
add real-history regressions for synthetic wake ends, continuation,
manual supersession, conservative fallbacks, malformed settlement causes,
and explicit-interrupt authorization and ordering.

The two behavior regressions and 123 existing tests pass on PR1; twelve
cause-contract tests intentionally await the separately implemented PR2
production change. Typecheck, targeted ESLint, and formatting pass.
Supply user-stream-abort evidence at the existing private settlement seam without changing its assertions.
@chatgpt-codex-connector

This comment has been minimized.

@ThomasK33

ThomasK33 commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

PR2 validation and scope ledger

Primary scripted reproduction

Exact pre-#3949 main: 3937abd4e96ab7475f5f702bd40118016f83e5c4 (not a merge-base calculation). A fresh scratch worktree received only the new test file and extracted test helper; its production code was untouched.

bun test src/node/services/workspaceTurnManager.uncorrelatedStreamEnd.test.ts \
  -t '^WorkspaceTurnManager uncorrelated stream-end synthetic end after the turn anchor preserves the pending turn until its correlated final$'

The test compiled and failed at line 173, expected status: running, received status: interrupted, with Workspace turn superseded by an uncorrelated workspace stream-end. No PR2 cause method/type/assertion was involved. Branch proof: all 791 tests / 3,778 assertions / 15 files pass on Bun 1.3.5. This includes all existing manager/TaskService/TaskHandleStore/task-tool tests and agentSession.workspaceTurnInheritance; no existing assertions changed. make static-check and fresh make build passed. A second full targeted run after the baseline comparison also passed.

Review correction 905468a274fe559984e263dd27ffb70eba77466c: Codex P2 exposed malformed persisted manual IDs. Five real-disk cases (missing/null/non-string/empty/whitespace) failed at the assertion before the fix and now pass using the named conservative fallback; the 19-case new suite and all 791 targeted tests are green. The internal malformed-cause rejection stays strict.

Internal cause inventory

Terminal source Cause
Persisted creation/admission failure creation-admission-failure
Deferred agent validation failure creation-validation-failure
Pre-stream cancellation launch-canceled
Accepted pre-stream failure / send failure send-failure (two callers)
Stale record recovered from history stale-history-recovery
Unrecoverable restart stale-restart
Proven manual input after anchor manual-supersession + actual input messageId
Uncorrelated history unavailable uncorrelated-conservative-fallback + history-read-failed, missing-stream-end, missing-turn-anchor, or invalid-manual-input-id
Strictly correlated terminal end correlated-stream-end
User stream abort user-stream-abort
Failed continuation dispatch continuation-failure
Terminal stream error terminal-stream-error
Startup execution-mirror admission failure recovery-admission-failure
Explicit interrupt (independent lock path) explicit-interrupt

13 central callers + explicit interrupt; 14 kinds. Both existing locks call the same synchronous validator before terminal persistence. Manual .some becomes .find solely to capture evidence. Conservative fallback read count, classifier order/truthiness, stop epoch/latch/mirror/waiters/cleanup ordering, and terminal-resettle policy are preserved. Causes are not persisted or exposed on TaskHandle/wire schemas.

Fresh isolated live sandbox — blocked, not passed

Built product commit 9d7504b8b (the two intervening commits at dogfood time were tests-only), separate root/project, loopback server, browser-only interaction; no user's live backend used. Both agents used Exec + coder:openai/gpt-6-astra. The later malformed-ID boundary fix 905468a27 is covered by red/green tests, final static checks, and a fresh build; the failed live scenario was not rerun after that fix.

  1. Disposable child: parent b9a485cceb, child 322b194b6e, handle wst_0587fa2396. Bash started 07:49:11Z and emitted PR2_MONITOR_WAKE at 07:49:19Z. The correlated initial stream ended tool-calls, the parent failed early, and disposable cleanup stopped the script before its 60-second completion.
  2. Retained retry: child e45f097353, handle wst_69f22dc361, turn 52ec629c6a. Script started 07:51:54Z, first await returned running + wake at 07:52:04.927Z, parent failed early around 07:52:09Z. Script exited 07:52:54Z / code 0, child retrieved PR2_WORK_DONE at 07:53:00.181Z and finished PR2_CHILD_COMPLETED around 07:53:05Z. Both assistant ends carry the same workspace-turn correlation; the retained handle then self-healed to completed, too late for the original parent waiter.

The source trace points to output consumption withdrawing an already queued monitor wake while the correlated tool-end cut is in flight (BackgroundProcessManager.getOutputBashMonitorWakeReconcilerWorkspaceService queued-wake cancellation → AgentSession.hasPendingBashMonitorWakeContinuation). This is untouched by PR2; no baseline live replay or universal lifecycle fix is claimed. The precise blocker is preserved rather than changing the addendum's protected classification/liveness/resettle policy. There was no uncorrelated synthetic assistant completion in these live runs, so the deterministic red/green harness remains the primary proof.

Disposable-attempt parent failure:

Disposable-attempt parent reports early correlated tool-calls failure

Retained parent waiting (before early failure):

Retained parent waiting on its child

Retained child later completes the full background work:

Retained child verifies exit zero and PR2_WORK_DONE

Retained attempt, 81.8 seconds:

11-retained-attempt.webm

Final child-result review, 10.9 seconds:

20-final-review.webm

Owned browser/recordings/server are stopped. Full logs, exact red-base output, branch green output, safe histories, and final task records are retained under ~/wave4-scratch/pr2/.

Diff size and remaining limits

File Added Removed Net
workspaceTurnManager.ts (only product file) 111 7 +104
workspaceTurnManager.uncorrelatedStreamEnd.test.ts 486 0 +486
workspaceTurnManager.testHarness.ts (extracted) 246 0 +246
workspaceTurnManager.test.ts 6 236 −230
taskService.test.ts 2 0 +2

Deviation from stale plan estimates: 13 rather than 11 callers; two settlement lock paths rather than one; conservative fallbacks remain; established manager fixture uses direct entry points rather than invented emitter wiring. All were reconciled by the Sep 5 addendum or verified current architecture. The live blocker above is an explicit limitation. #3915 is still soft; unknown-history interruption, unupgraded live-host exposure, two-week session observation, and post-wave N20 main-run observation remain outside claims here.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Please review d899e9cf6d87db887a2dcbaf56f00c9392a37191. PR2 adds internal settlement causes and regression coverage only; the Sep 5 addendum in the body is authoritative. The first comment contains exact pre-fix red/branch green proof (786 tests + static checks), fresh isolated screenshot/video evidence, and the explicit correlated tool-calls live blocker. No lifecycle-policy fix or uncorrelated live-success claim is made.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

Please review d899e9cf6d87db887a2dcbaf56f00c9392a37191. PR2 adds internal settlement causes and regression coverage only; the Sep 5 addendum in the body is authoritative. The first comment contains exact pre-fix red/branch green proof (786 tests + static checks), fresh isolated screenshot/video evidence, and the explicit correlated tool-calls live blocker. No lifecycle-policy fix or uncorrelated live-success claim is made.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d899e9cf6d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/workspaceTurnManager.ts Outdated
Readable history can contain a missing, blank, or non-string manual input ID. Retain conservative interruption with a named invalid-manual-input-id fallback instead of letting the strict internal cause assertion strand waiters. Preserve the actual valid ID, one history read, and existing outcomes.

Five real-disk regressions reproduce the assertion failure before this fix; all791 targeted tests and make static-check pass afterwards.

---
_Generated with [`mux`](https://github.com/coder/mux) • Model: `coder:openai/gpt-6-astra` • Thinking: `high` • Cost: `$20.37`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=high costs=20.37 -->
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Please review updated head 905468a274fe559984e263dd27ffb70eba77466c. Addressed the malformed persisted manual-ID finding with a named conservative fallback and five real-disk regressions. 791 targeted tests / 3,778 assertions and make static-check pass. Replied to and resolved only PRRT_kwDOPxxmWM6fheZO. Scope and the separate live correlated tool-calls limitation remain as documented.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

Please review updated head 905468a274fe559984e263dd27ffb70eba77466c. Addressed the malformed persisted manual-ID finding with a named conservative fallback and five real-disk regressions. 791 targeted tests / 3,778 assertions and make static-check pass. Replied to and resolved only PRRT_kwDOPxxmWM6fheZO. Scope and the separate live correlated tool-calls limitation remain as documented.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector

This comment has been minimized.

@ThomasK33
ThomasK33 added this pull request to the merge queue Sep 5, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 5, 2026
@ThomasK33

Copy link
Copy Markdown
Member Author

Merge-queue retry 1 — unchanged PR head

The first merge-group run 33955400476 failed in Unit because Bun 1.3.5 crashed with a segmentation fault / exit 132, not because a test assertion failed. The crash followed createCodeExecutionTool result-offloading tests; every other code job, including Docker/Windows builds, passed.

Before retrying:

  • The same code_execution.ts and code_execution.test.ts are byte-identical to preceding green main 13835b38277b8d439f59451f3bf3a9e7025cc3a1; its merge-group 33955142625 passed Unit, including that exact test.
  • Local CI-style reproduction, bun test --max-concurrency=1 --coverage --coverage-reporter=lcov src/node/services/tools/code_execution.test.ts, passed 85 tests / 401 assertions on Bun 1.3.5 (~7 s).
  • No source changes or unrelated fixes. PR head remains 905468a274fe559984e263dd27ffb70eba77466c, with both clean reviews and green PR CI. The prior queue entry was removed on failure; re-enqueueing once, not duplicating a live entry or bypassing checks.

Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $20.37

@ThomasK33
ThomasK33 added this pull request to the merge queue Sep 5, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 5, 2026
@ThomasK33

Copy link
Copy Markdown
Member Author

Merge-queue retry 2 — same-SHA rerun is green

The second queue run 33956065597 initially hit Bun 1.3.5 segmentation faults in both the isolated QuickJS process and the main coverage process. All 19 PR2 regression cases passed before the latter crash (as they also did in the first queue run).

Rather than immediately re-enqueueing, I reran only Unit and Required on the exact same failed group SHA de45e245c026776c5a40b3f47d7f487cb0d97d24. Attempt 2 is now completed / success, including both jobs.

The isolated local QuickJS command likewise crashed once, then passed unchanged (83 tests / 408 assertions); the same test on the owned exact pre-#3949 checkout also passed 83/408. Its source/test are unchanged from green preceding main d7743aa413f089b0aa4828cde45556f6fe12fd71 (green group). This supplies a concrete same-SHA green signal before another queue attempt, not an unrelated source or CI workaround.

PR head remains 905468a274fe559984e263dd27ffb70eba77466c; no source changes, force-pushes, extra review triggers, or merge bypasses. Re-enqueueing only after the usual fresh head/review/check/no-existing-entry guards.


Generated with mux • Model: coder:openai/gpt-6-astra • Thinking: high • Cost: $71.94

@ThomasK33
ThomasK33 added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit a8466cd Sep 5, 2026
35 of 38 checks passed
@ThomasK33
ThomasK33 deleted the effect-wave4-pr2-settlement-invariant branch September 5, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant