Skip to content

fix(orchestration): park stopped delegated completions until resume - #431

Merged
lukemaj merged 12 commits into
mainfrom
fix/422-park-stopped-completions
Oct 18, 2026
Merged

lukemaj merged 12 commits into
mainfrom
fix/422-park-stopped-completions

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

What: Delegated results stay readable while Stopped and are offered on resume through upstream queue semantics.
Why: Completion offers ignored durable Stop and retried rejected work indefinitely; completion-specific hold exceptions then bypassed upstream safety.
So what: The planner reviews this head for merge into main. Held results start with their queue when the user resumes it; deployment and rocky CPU/trace acceptance remain unperformed.

User-visible rule

Results parked during Stop are offered once when the thread resumes. If the thread also has held queued work, the result joins that held queue and starts with it, exactly once, when the user resumes the queue. This is upstream behavior for every queued run, including after a restart.

Mechanism and fix

Choose upstream's completion mailbox offer gate. Parked is derived from existing pending mailbox work plus destination/ancestor retirement, without a separate store or state.

  • Retirement suppresses offers before delivery reads. The final check and nonblocking enqueue share Stop's SQLite BEGIN IMMEDIATE transaction. Consumer eligibility uses upstream dispatchIfCurrent under retirement-owner locks from root to destination; the private exact-fiber/target context permits dispatch in that interval. App-owned clearIfCurrent has no native buffer to clear. Retry/backoff has no retirement-specific branch.
  • Stop cancels an unstarted queued completion's execution while preserving its pending mailbox generation/message identity. Resume offers it through the normal path; message/run ownership and dedupe prevent stale finalization or replay from delivering twice. Started cut wakes retain upstream successor generation behavior.
  • Completion wakes inherit and obey every queue hold. Removed all hold exemptions, held-queue selector/projection exceptions, special startup completion handling and automatic startup scheduling. ProjectionStore.ts and ProviderRuntimeRecoveryService.ts now match origin/main byte for byte. Queue creation, selection and guarded starts use base behavior.
  • Acknowledgement removes only its task and coalesces other eligible pending tasks before deciding that a batch is empty. task_status remains readable while Stopped. Archive/delete disposal and restart cancellation remain.
  • Stop admission precedes Spectrum/feature planning and Wight, keeps retirement refusals retryable without a receipt and preserves base planning/receipt behavior for other commands, and remains checked at commit. Durable subtree Stop still blocks new automatic work.

The earlier forkResumeCommandId, forkResumeTaskIds, snapshots, permission transfers and resume-only release loop remain deleted. No new contract field, migration, version bump or recovery sweep. Register the carried mailbox/continuation hooks and retain the map's #422 PR #431 reference.

Proof

Current head: 3f13cfd3ad05e02e2c54361da9a84492fe8a60a2. 310 tests passed across 17 focused files, including all 49 ThreadStop cases.

Both R4 scenarios reproduced on b15aa0c before deletion: provider-failure holds were bypassed, and preparing-worktree restart failed to retain a completion hold. Retained R3 failed-worktree cases and added the preparing-worktree restart and R4 operational-failure probe; all remain held with zero provider-start effects.

R1 parked sibling, R2 restart-after-resume and R3 startup timing scenarios remain. Their oracle now follows the planner's explicit changed rule: delivery remains present, queued and held; an automatic queue scan does not start it; ordinary queue.resume starts one wake with one provider-start effect. Replaying queue.resume cannot create a second start. No result-loss case was observed. No unique timing scenario was removed. Tests use real SQLite/orchestration/recovery/worker/task_status with Deferred milestones and production startup ordering, without sleeps or polling.

pnpm exec vp test run \
  apps/server/src/orchestration-v2/ThreadLaunchService.test.ts \
  apps/server/src/orchestration-v2/ThreadStop.test.ts \
  apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts \
  apps/server/src/orchestration-v2/ProviderContinuationService.test.ts \
  apps/server/src/childThreads/retirement.test.ts \
  apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts \
  apps/server/src/mcp/QueueResume.integration.test.ts \
  apps/server/src/wight/admission.integration.test.ts \
  apps/server/src/spectrum/humanStopRpc.integration.test.ts \
  apps/server/src/orchestration-v2/RunCompletionReads.test.ts \
  apps/server/src/orchestration-v2/QueuedRunOrder.test.ts \
  packages/contracts/src/orchestrationV2.test.ts \
  apps/server/src/spectrum/Controller.integration.test.ts \
  apps/server/src/spectrum/ReportService.integration.test.ts \
  apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.test.ts \
  apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.regression.test.ts \
  apps/server/src/serverRuntimeStartup.test.ts
pnpm exec vp run --filter t3 --filter @t3tools/contracts typecheck
pnpm exec vp lint apps/server/src/orchestration-v2/{Orchestrator.ts,ThreadStop.test.ts}
pnpm exec vp fmt --check apps/server/src/orchestration-v2/{Orchestrator.ts,ThreadStop.test.ts}
git diff --check
node scripts/fork-features.mjs check
bash scripts/fork-check.sh --base f798cb299ac9eab9d51ef63b5159de14a2b6aa18

Targeted server/contracts typechecks, formatting, diff and fork checks pass (42 features). Lint has only the existing unused layerUnavailable warning. CI owns complete coverage. Required Codex/Grok review of this new SHA belongs to the planner; the implementation writer launched no review rounds and transfers no older verdict.

No repo-wide local checks, browser/server launch, live userdata writes, rocky builds/tests, merge or deployment. Local Node 26.8.2 versus requested ^24.13.1 remains a validation limitation. Website/SEO impact: none, server orchestration only.

Integration compatibility

Current origin/main remains 0ad34f2b0dfaac9e0dc54da72b2feb2dd371350b, already the branch base. git merge-tree --write-tree 3f13cfd3ad05e02e2c54361da9a84492fe8a60a2 6e734b07269c9647762478222139f7abe4f37244 exits 0, tree 586b970f06f899e5f7ab228aef523f29177340a8: no textual conflicts with PR #430. Combined-runtime proof is not claimed.

Upstream first

Inspected upstream main 9c161462c94f7d9d6d9169da5c841457cbadf213; searched open PRs and Issues. No same-cause durable-retirement park/resume fix found. Reuse upstream mailbox primitives and queue policy; upstream Stop disposes deliveries and lacks the fork retirement guard. Related PRs #15331 expand disposal, #21164 expose held queues, and #20025 address a separate finalization/checkpoint race. None is ported. Comparison/search record.

Receipt and admission correction after delta review

Early retirement preflight now runs only for message.dispatch, before Wight permits and Spectrum planning. Wight handles only messages. Spectrum handles message.dispatch plus thread.stop and thread.unsettle lifecycle operations; the latter two never had this retirement preflight. All other commands retain base's planning order and commit-time retirement guard.

Reverted the broad non-message rejection-receipt change from 0b5f88b. Retirement-only refusals never persist rejected receipts. Prepared-run.release still records a planning rejection when Stop interrupted its prepared run, exactly as on base. queue.resume refused solely by retirement remains retryable with the same command id after explicit human resume. No hold logic changed.

The new public-dispatch/SQLite regression fails on 0b5f88b because a rejected receipt persists; the repaired tree leaves no receipt on refusal, accepts the same id after resume, and clears the queued run's hold. ThreadLaunchService remains 89/89 green. Combined focused proof: 310 tests pass in 17 files, including 49 ThreadStop cases, with no sleeps or polling.

Run 39140050198 was not solely the launch failure: Test Server 3 had 1 failed / 1370 passed / 2 skipped; Test separately failed the SSH zombie fixture (1 failed / 46 passed), tracked in #439. Check aggregates both. Fork-check FAIL lines are expected-negative fixture output. SSH root cause/flakiness remains unverified; no SSH fix or fresh green CI is claimed.

Elon record

Requirements and who asked: User/#422 require quiet Stop, readable results and once-only delivery. The planner explicitly chose upstream hold inheritance and queue.resume for held work, with zero result loss and preserved operational safety.
Deleted: Non-message early retirement preflight and broad non-message rejection receipts. Every completion hold exemption, held-queue SQL/memory selector exception, startup completion special case and automatic startup promotion. Resume fields/snapshots remain deleted. No replacement flag, state, store or retry exception.
Bottleneck: Fork queue policy diverged from upstream; the existing held queue and explicit user resume already own the decision.
Checked myself: Reproduced the same-id queue.resume receipt failure, inspected Spectrum and Wight command types, restored base planning and retirement refusal semantics, retained the existing launch test, ran 310 tests across 17 focused files and targeted checks, and verified the pushed candidate plus clean merge tree with #430.

Refs #422 (rocky acceptance after deploy keeps the Issue open)

Model: GPT-6.1-Sol. Harness: Codex in T3 Code.

@github-actions github-actions Bot added size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Oct 18, 2026
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-head review found three blocking completion-delivery failures at b937100f9c4a194dd9834e298b61658259f022c8.
Why: Retaining cohorts does not yet preserve delivery across pre-start Stop and held queues, and consumer eligibility is checked outside Stop's serialization boundary.
So what: The implementation writer should fix these paths and provide focused regressions before another exact-head review; this review does not authorize merge or deployment.

Reviewed the entire eight-file diff from dfb9235811e8d9b76f4713c383301e1a220364ce to b937100f9c4a194dd9834e298b61658259f022c8, #422 and every comment, root AGENTS.md, and the surrounding Stop, retirement, mailbox, receipt, task_status, queue and disposal consumers. Verdict: changes required.

  1. [P2] Stop before provider start consumes an unread completion permanently. Location: finalizeDelegatedCompletionDelivery, Orchestrator.ts:11602-11605, reached through the newly retained cohort in holdStoppedThread. Let a completion be dispatched into a new starting wake run, then Stop before its provider-turn.start effect executes. The pre-start interrupt path writes status: "interrupted" and cancels the start effect (lines 10217-10379), so no provider receives the notification. The terminal listener nevertheless marks its claimed tasks delivered, because only cancelled maps back to pending; with no other pending task, it clears cohort.delivery (lines 11607-11654). Explicit resume then has nothing to offer, and later planning refuses to reclaim a delivered task. The result remains readable manually, but the unread result never gets its promised delivery on resume. Preserve unconsumed ownership for this Stop path and cover Stop between wake admission and provider start.

  2. [P2] Explicit message resume can leave parked completions behind a held queue indefinitely. Location: the new resume offer, Orchestrator.ts:12540-12541. Start with queued user work, Stop the parent, and let a child finish while Stopped. Stop retains the queued run with queueHeld: true. An explicit message can start a new parent run and re-offer the completion, but while that resume run is starting the completion takes queue_after_active and inherits queueHeld: true from the older queued run (lines 5385-5391). startNextQueuedRun refuses any held queue (lines 1418-1423), and neither explicit message resume nor receipt replay releases that hold. The child notice therefore never reaches the resumed provider without a separate queue.resume. Similarly, a wake already queued before Stop is held and skipped by the offer gate because its message already exists; a late sibling can be merged into that same held delivery. Ensure explicit message resume actually releases the parked completion without requiring the user to resume unrelated held work. The current new tests have no held queue and assert message/run creation, so they do not cover this failure.

  3. [P2] A consumed offer can still read and dispatch after Stop commits. Location: the new dispatchIfCurrent callback, Orchestrator.ts:1208-1212. The callback reads retirement once, then executes the entire consumer effect without the thread command lock. A reachable ordering is: worker checks eligible; its threads.getThreadRecords yields; thread.stop acquires the thread lock and commits retirement; worker resumes and calls threads.dispatch. Admission correctly rejects that dispatch, but the stopped parent has already incurred the consumer read, rejected dispatch and retry scheduling that fix(orchestration): park delegated completions for Stopped threads and deliver on resume #422 requires suppressing. The retry gate eventually drops this offer, so this finding does not claim an infinite retry chain. Coordinate consumption eligibility with Stop, and cover Stop after eligibility has passed but before dispatch. The new before-stop test starts the worker only after Stop, so it cannot expose this ordering.

Cleared within the inspected scope: terminal task_status acknowledgement still uses its unguarded administrative resolution path; duplicate message/generation validation remains under command serialization; archive and delete retain cohort disposal; retirement preflight and commit guards retain subtree admission and queue-resume refusal; the current-head metadata-only delta adds the two carried upstream files. No #424 recovery-sweep change is requested by these findings.

Limits: read-only source review, with no repository writes, tests, servers, browser use, rocky operations or live userdata access. The reported 103-test run was on a982cc0e11; I verified that the diff from that revision to this candidate consists only of the two allowlist entries, but did not rerun or independently execute that proof. Full CI and post-deployment CPU/trace acceptance remain outside this review. The worktree and live PR head both remained the exact reviewed SHA before publication.

Model: GPT-6.1-Sol. Harness: Codex in T3 Code.

@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 4.9 KiB 5.0 KiB +30 B (+0.6%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB +30 B (+2.5%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.8 KiB 20.9 KiB +41 B (+0.2%) 29.3 KiB ✅
Codex Live turn messages 1 2 +1 (+100.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 0ad34f2 · PR result: 3f13cfd · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: All three blocking findings from the independent review are fixed at 1a3800f576276d1b83368f1e81088a8324298219.
Why: The first implementation preserved mailbox records but did not prove that unread notices survived pre-start Stop or escaped unrelated held work, and its eligibility check was outside command serialization.
So what: Request a fresh exact-head review; no merge or deployment is authorized by these fixes.

  1. Pre-start Stop: Finalization checks the delivery attempt's persisted providerTurnId. An interrupted attempt with no accepted provider turn remains unconsumed, so its tasks get a successor mailbox delivery instead of being marked delivered. No provider history scan is added.
  2. Held queues: Explicit resume clears holds only on already-queued completion runs. The owner labels newly re-offered parked deliveries with resume intent so they do not inherit unrelated holds. Queue readiness and selection permit these unheld completions past held user work, while the user's held messages remain held. Ordinary offers still inherit queue holds. Worktree/disabled-instance/usage-limit guards remain.
  3. Consumption race: The producer's existing dispatchIfCurrent callback holds the actual thread command lock through eligibility, reads and dispatch. A private context value identifies the owning thread and exact Effect fiber, so that same consumer can dispatch without relocking. Child fibers acquire their own locks. The regression queues a concurrent Stop and proves that it cannot retire the thread inside the consumer interval; a later consumer is skipped.

Five regressions failed on the reviewed revision b937100f9c4a194dd9834e298b61658259f022c8: queued delivery stayed queued, pre-start delivery was marked delivered, consumer held no serialization lock, and both stale/late completion cases stuck behind held user work. The repaired focused suite passes 109 tests in 10 files, adding RunCompletionReads.test.ts and QueuedRunOrder.test.ts to the eight files in the PR. Server typecheck passes; targeted lint has only the existing layerUnavailable warning; formatting/diff checks and the 42-feature registry check pass. No wall sleeps/polling, repo-wide checks, or rocky operations.

Model: GPT-6.1-Sol. Harness: Codex in T3 Code.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-head re-review found three blocking defects at 1a3800f576276d1b83368f1e81088a8324298219.
Why: The consumer lock can deadlock during cleanup, resume eligibility depends on which duplicate offer wins, and the lock does not serialize ancestor retirement.
So what: The implementation writer should fix these paths and add focused regressions before another exact-head review; no merge or deployment is authorized.

Verdict: changes required. Reviewed #422 body and all comments, the prior review and author responses, root AGENTS.md, AgentsMD Verification/code-review procedures, the entire nine-file cumulative diff from dfb9235811e8d9b76f4713c383301e1a220364ce, and surrounding mailbox, queue, retirement, receipt, task_status, disposal and command-lock consumers.

  1. [P1] Cleanup reacquires the non-reentrant lock already held by the completion consumer. Location: ProviderContinuationService.ts:280-282, wrapping the existing cleanup at lines 119-122 and 201-205; Orchestrator.ts:1222-1237.

    Reachable trigger A: enqueue an app-owned completion, then archive its non-retired parent before the worker consumes that offer. The outer withCurrentDelegatedOffer acquires the parent's command lock because retirement does not include archive state. The consumer sees archivedAt, then calls request.dispatchIfCurrent(Effect.void) to clear the offer. That callback unconditionally reacquires the same lock. Trigger B: disable the parent's provider before consuming an outstanding completion. The actual ThreadManagementService rejects message.dispatch, the worker acknowledges the tasks, and its cleanup takes the same nested callback. App-owned offers supply no clearIfCurrent, so both fallbacks are reachable. KeyedLock.ts explicitly implements non-reentrant one-permit exclusion; the same-fiber bypass exists only in dispatchWithReceipt, not in this callback. The worker never returns to requests.take, blocking continuation delivery for every thread and keeping this parent's command lock held. Existing disabled-provider tests supply clearIfCurrent, so they miss the real producer shape. Make cleanup safe under the outer consumer interval and cover an actual app-owned offer on both paths.

  2. [P2] A pre-Stop offer consumed after resume can strand the resumed result behind held user work. Location: Orchestrator.ts:5401-5423, with the resume offer at lines 12605-12607.

    Queue unrelated user work and let a child finish, leaving its ordinary intent: "offer" request buffered. Stop the parent, then explicitly resume it before the shared worker consumes that request, for example while the worker is handling another thread. Resume releases only completion runs that already exist and adds a second request for the same identity with intent: "resume". The older FIFO request is now eligible again and wins dispatch while the resume run is starting; its captured ordinary intent makes the newly created completion run inherit queueHeld: true. The newer resume request then sees the existing message in currentDelegatedCompletionDelivery and skips dispatch, so it cannot release that hold. No completion run existed during the atomic unhold step, and the queue selector only bypasses holds for unheld completions. The unread result remains held until a separate queue.resume or another Stop/resume cycle. The new before-stop regression drains the stale request while still Stopped, before resume, so it excludes this ordering. Resume entitlement must survive duplicate/stale offer ordering; add the case that consumes the original request only after resume commits.

  3. [P2] An ancestor Stop still commits inside a descendant completion's eligibility/consumption interval. Location: Orchestrator.ts:1223-1228.

    Let G own delegated child P, and P own a completed delegated child C. The completion consumer for P takes only P's lock, verifies retirement, and yields before its records read or dispatch. thread.stop(G) takes G's separate lock and commits G's retirement token. By retirementState (retirement.ts:32-49), P is already retired by that token, even before the descendant Stop effect runs. G's commit cancels descendant outbox work but does not take P's command lock or cancel this continuation consumer. When the consumer resumes, it reads P's records and attempts message.dispatch after subtree Stop has committed. The retirement guard correctly refuses admission, but the forbidden consumer read/dispatch and retry scheduling have already occurred. Automatic server completions also bypass the extra parent lock used only for explicit user/MCP messages at Orchestrator.ts:12633-12646. The new concurrent-Stop test covers Stop on P itself, so it does not close the ancestor interval. Coordinate the consumer with the retirement owners that can change its eligibility and cover Stop on G after P's eligibility check. This is a completion-eligibility race, not a request to change perf(prism): recover only the changed run instead of sweeping all children on every run update #424's global recovery sweeps.

Cleared within the inspected paths: the pre-start interrupted delivery now uses persisted providerTurnId === null to reserve a successor instead of marking the unread result delivered; same-thread Stop and ordinary consumption share the command lock; the exact fiber check prevents child fibers from using the dispatch bypass; task_status retains its administrative acknowledgement path; archive/delete cohort disposal and commit-time subtree admission remain present. These do not clear the failures above.

Limits: source review only. No file writes, tests/builds, servers, browser use, rocky operations, live userdata access, or merge. I inspected the regression code but did not execute or independently certify the reported 109-test/typecheck/lint results. Full CI and authorized post-deployment trace/CPU acceptance remain separate. The worktree was clean, and both worktree HEAD and the live PR head were the exact reviewed SHA immediately before publication.

Model: GPT-6.1-Sol, configured high effort. Harness: Codex in T3 Code.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: The cleanup, stale-offer and ancestor-Stop findings are repaired at 129fe0f38ef1f66a9109faaa2e06eae88dee0d4e.
Why: Resume permission belongs to the durable delivery, and every ancestor can change eligibility; app-owned cleanup must not reacquire the consumer gate.
So what: A new exact-head review follows; this PR remains unmerged and undeployed.

  • Cleanup: actual producer offers now supply the existing clearIfCurrent hook as a no-op, because their mailbox is durable and they have no sticky adapter buffer. Real worker tests cover archive and real disabled-provider acknowledgement, then prove another command can acquire the parent lock.
  • Stale/resume ordering: remove captured offer/resume intent. An optional resume command receipt lives on the existing delivery, is persisted atomically with explicit resume, and is read under dispatch serialization. The original FIFO offer can win after resume without inheriting unrelated holds; the duplicate skips the existing identity. New generations retain ordinary queue policy unless explicitly resumed.
  • Ancestors: read retirement owners, acquire command locks root-to-target, verify the owner chain again, then check retirement and consume. The ancestor Stop regression verifies both owners are held, queues Stop on the ancestor while the descendant consumes, and checks that retirement cannot change within that interval. Child fibers cannot reuse the dispatch bypass.
  • CI compatibility: early retirement refusal no longer records a rejected receipt, matching the prior commit guard's rollback. Both failing Spectrum report tests reproduce before this correction and pass afterward. No Spectrum implementation change.

Two additional regressions fail on 1a3800 for the intended stale-offer and ancestor-lock failures. Full focused proof is 183 tests in 13 files, with server/contracts typechecks and targeted lint passing (only the existing layerUnavailable warning). PR body has complete commands and current mechanism. Prior failures remain recorded on their reviewed SHAs; no verdict is transferred.

Model: GPT-6.1-Sol. Harness: Codex in T3 Code.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-head review found one blocking resume/finalization ordering defect at 129fe0f38ef1f66a9109faaa2e06eae88dee0d4e.
Why: Explicit resume grants permission to the current delivery generation, but delayed finalization can replace that generation and discard the permission before its unread result is delivered.
So what: The implementation writer should preserve resume eligibility across this unread successor transition and add a focused regression before another exact-head review; no merge or deployment is authorized.

Verdict: changes required. Reviewed #422 body and all comments, all prior PR findings and fix responses, root AGENTS.md, AgentsMD Verification and code-review procedures, the full eleven-file cumulative diff from dfb9235811e8d9b76f4713c383301e1a220364ce, and surrounding command, retirement, mailbox, queue, acknowledgement, cleanup and terminal-listener paths.

  1. [P2] Resume before interrupted-delivery finalization strands the unread successor behind unrelated held work. Locations: resume tagging, Orchestrator.ts:12457-12468, and successor construction, Orchestrator.ts:11674-11687.

    Reachable sequence: dispatch a completion into a starting wake, queue an unrelated user message behind it, then Stop before provider start. Stop interrupts the wake and holds the unrelated message. Before the terminal listener finalizes that interrupted delivery, explicitly resume the parent with a new message. Resume writes forkResumeCommandId onto the old generation; its re-offer skips the already-existing interrupted wake message. The listener then runs: providerTurnId === null correctly keeps the result unread, but nextDelivery is a fresh object with only generation/messageId/taskIds, so it drops the resume receipt. If the resume run is still starting, the successor takes the queue path, fails resumingCompletion at lines 5410-5415, and inherits queueHeld: true from the unrelated user message at lines 5428-5433. After the resume run ends, both queued runs remain held and no notice is delivered. Reconciliation cannot unhold the already-created successor; replaying the resume receipt only re-offers it. Another Stop/resume or a separate queue.resume is required.

    The ordering is allowed by the implementation: Stop commits/publishes events without waiting for finalizeDelegatedCompletionDelivery; the terminal listener processes events sequentially with Stream.runForEach(handleTerminalRun) at lines 12817-12831, and can still be waiting on an earlier unrelated thread's command lock. A regression can hold that earlier listener milestone, perform Stop and explicit resume on the parent, then release finalization while the resume run remains pre-start. The current pre-start regression explicitly calls recoverDelegatedTasks before resume (ThreadStop.test.ts:517-528); its starting variant also has no unrelated held message, so it excludes this failure. Preserve the resume entitlement for already-unread work across this successor transition without granting it to ordinary later results.

Cleared within the inspected paths: actual app-owned offers now supply clearIfCurrent, avoiding the archive/disabled-provider cleanup re-entry; same-generation stale/duplicate offers derive resume permission from durable state; consumption acquires retirement owners root-to-target, checks the chain again and holds those locks through reads/dispatch; the private bypass checks the exact target and fiber; task_status administrative acknowledgement and archive/delete disposal remain; preflight rejection preserves the former no-receipt behavior and commit-time admission remains. No new blocking lock-order cycle was found in the inspected consumers. These observations do not clear the successor defect above, and no #424/#425/#426 recovery-sweep change is requested.

Limits: source review only. No repository writes, test/build execution, servers, browser use, rocky operations, live userdata access or merge. Inspected the added regressions but did not independently execute or certify the reported 183-test proof. Full CI and authorized post-deployment CPU/trace acceptance remain separate. The worktree was clean and both local HEAD and live PR head matched the reviewed SHA immediately before publication. The live PR base has advanced to 0ad34f2b0dfaac9e0dc54da72b2feb2dd371350b; this review uses the explicitly requested fixed base above, not an integration/rebase candidate.

Model: GPT-6.1-Sol, configured high effort. Harness: Codex in T3 Code.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Accepted the delayed-finalization finding and preserved explicit resume eligibility for unread successor deliveries.
Why: Resume can commit before the interrupted wake settles; replacing that delivery discarded its durable receipt and stranded its unread result behind held user work.
So what: The same mailbox gate remains the selected mechanism. The fix carries the existing receipt only when the prior wake was unconsumed; consumed wakes cannot grant permission to later results. Fresh exact-head review follows proof and push.

Requirements and who asked: #422 requires unread parked results to deliver after one explicit resume; round 4 demonstrated a missing settlement ordering.
Deleted: The assumption that delivery generation replacement can discard resume entitlement while its work remains unread. No new store, retry branch, flag or recovery sweep.
Bottleneck: The unread successor constructor dropped durable resume ownership after the explicit resume transaction had already committed.
Checked myself: The focused public-seam regression fails on 129fe0 with queued instead of starting. It uses the existing runtime-reconciliation event marker to defer listener settlement until after resume, then public recovery. With the conditional receipt carry, the successor starts once while unrelated user work stays held; the two directly affected files pass 62 tests. No sleeps or polling. A preliminary fixture's empty Queue.takeAll wait was removed and is not counted as reproduction proof.

Prior review: #431 (comment)

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-head review found no blocking findings at 6ffcc4a308cf39aea7432b7be3092a638d8b1883.
Why: The unread successor now retains its durable resume receipt, closing the outstanding resume-before-finalization defect without transferring that receipt after a consumed wake.
So what: The planner can use this exact-head review for the remaining delivery gates; merge requires human approval, and deployment and rocky acceptance remain unperformed.

Verdict: no blocking findings in the inspected scope. Reviewed the full eleven-file cumulative diff from fixed original base dfb9235811e8d9b76f4713c383301e1a220364ce, surrounding callers and state owners, #422 body and all seven comments, PR body and all comments, all prior findings and repair responses, root AGENTS.md, and the canonical AgentsMD Verification/code-review procedures. GitHub's inline-comment and submitted-review lists were also empty.

Adjudication and inspected invariants:

  • Round 4 successor defect: cleared. In finalization, cancelled wakes and interrupted attempts with persisted providerTurnId === null remain unread. Successor construction carries the existing receipt only for that unread transition. A consumed wake marks its claimed tasks delivered and does not copy the receipt into a successor for pending later results. Fresh delivery construction and provider-acceptance successors likewise do not copy it.
  • The new regression establishes Stop, resume, then finalization using the existing reconciliation marker and public recovery. It asserts the old delivery still owns the receipt before recovery, then observes one successor run starting after the resume run ends while unrelated instructions stay held. Its seam proves orchestration scheduling, not actual provider execution.
  • Earlier cleanup, stale-offer and ancestry findings remain cleared. Actual app-owned offers provide a no-op clearIfCurrent, avoiding cleanup re-entry into the non-reentrant lock. Resume permission is read from the durable current generation, so an older FIFO offer cannot lose it. Consumption acquires retirement owners root-to-target, rereads the chain and retirement state, and holds those locks through consumer reads/dispatch. The private dispatch bypass requires the same target and exact fiber; child fibers do not inherit permission to bypass locking.
  • Producer and retry consumer gates precede delivery-record reads and dispatch for a retired target. Retry/backoff policy has no retirement-specific branch. The queued completion release and selector allow resumed notices past held user work while ordinary offers retain hold inheritance; worktree/instance/usage-limit checks remain in the inspected paths. Message/generation validation and command serialization preserve identity deduplication.
  • Administrative task_status acknowledgement remains available while Stopped. Acknowledgement removes claimed task ownership and cancels empty queued deliveries; archive/delete retain cohort disposal. Retirement preflight still precedes Wight admission and feature interception, preserves refusal without a receipt, and retains commit-time subtree admission and restart cleanup. No perf(prism): recover only the changed run instead of sweeping all children on every run update #424/perf(codex): find and stop repeated identical turn-item writes #425/perf(orchestration): read one run for turn-start status instead of the full recovery projection #426 recovery-sweep implementation or new component is included.

Limits: source review only. I inspected the regressions and reported commands, but did not run tests, builds, lint, formatting, servers or browsers, and do not independently certify the author's 184-test/typecheck claims or the claimed failing baseline run. No worktree mutation, rocky operation, live userdata access, merge or deployment occurred. Full CI and authorized post-deployment CPU/trace acceptance remain separate. The worktree was clean, and local HEAD and live PR head both matched the reviewed SHA immediately before publication. Live main is 0ad34f2b0dfaac9e0dc54da72b2feb2dd371350b; this is the requested fixed-base review, not proof of a rebased or merged candidate.

Model: GPT-6.1-Sol, configured high effort. Harness: Codex in T3 Code. Independent reviewer; not the implementation author.

Keep delegated results in the upstream completion mailbox while durable Stop blocks offers. Reuse producer eligibility for stale offers and retries, re-offer on explicit resume, and check retirement before feature planning.

Co-Authored-By: GPT-6.1-Sol via Codex
Co-Authored-By: GPT-6.1-Sol via Codex
Co-Authored-By: GPT-6.1-Sol via Codex
Serialize mailbox consumption with Stop, retain interrupted notices whose provider never started, and release only resumed completion deliveries past unrelated held work. Cover the three independent review findings at orchestration seams.

Co-Authored-By: GPT-6.1-Sol via Codex
…estry

Keep resume permission on the durable mailbox delivery, serialize its consumer against every retirement owner, and clear app-owned offers without re-entering the lock. Preserve the no-receipt semantics of retirement guard refusal for Spectrum reports.

Co-Authored-By: GPT-6.1-Sol via Codex
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Accept both planner review findings and preserve the same mailbox mechanism while repairing resume ownership and offer serialization.
Why: A delivery receipt alone omits pending siblings, and an eligibility read before asynchronous projection reads leaves an ancestor Stop race.
So what: Snapshot the task identities covered by explicit resume, carry permission only for those unread tasks into successors, and recheck retirement plus enqueue under the existing SQLite write transaction used by Stop commits. The planner owns the next review rounds.

Requirements and who asked: Planner review requires all results already parked at resume to run without releasing ordinary later results, and zero producer offers after ancestor Stop commits; #422 owns both requirements.
Deleted: The assumption that only an unconsumed current wake can own parked work, and the unlocked read-to-offer gap. No new retry rule, store, or global recovery sweep.
Bottleneck: Resume's durable permission needs task identity scope, while the enqueue decision must share serialization with the durable Stop write.
Checked myself: Read both full reviewer probes and the actual successor constructor, resume commit, producer callers and Node SQLite client. All producer callers can already hold the destination lock, so acquiring ancestors there would invert lock order. The existing BEGIN IMMEDIATE transaction serializes the final durable check and nonblocking enqueue against Stop without acquiring another thread lock. Rebased the exclusive branch onto current origin/main 0ad34f2.

@lukemaj
lukemaj force-pushed the fix/422-park-stopped-completions branch from 6ffcc4a to 8bd5d3e Compare October 18, 2026 14:02
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Both P2 planner findings and the P3 map reference are fixed at 8bd5d3e00e2e1d47c86a095d8167b671fa1f75e8.
Why: Resume needs to cover pending siblings by identity, and producer eligibility must remain valid through enqueue when an ancestor stops.
So what: The planner can run required Codex/Grok reviews on this pushed head; the implementation writer started no review rounds.

Resume now captures task IDs and releases that snapshot before later results. Terminal finalization and provider notification acceptance preserve the scoped permission, while late results cannot join it. The final retirement check and nonblocking offer run in Stop's existing SQLite write-transaction interval, avoiding nested ancestor-lock acquisition.

188 focused tests pass across 13 files, including both reviewer probes and three later-result timings. Both original probes failed on the rebased baseline for the reported reasons. Server/contracts targeted typechecks, targeted lint/format, diff and fork checks pass; only the existing unused layerUnavailable warning remains. Full commands are in the PR body.

Rebased onto origin/main 0ad34f2b0dfaac9e0dc54da72b2feb2dd371350b. Exact current head merges without textual conflicts with #430 head 6e734b07269c9647762478222139f7abe4f37244 via git merge-tree (exit 0); combined runtime is not claimed. Canonical #422 handoff is updated. No merge or deployment.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Replace resume-specific permissions with ordinary durable mailbox delivery gated only by retirement.
Why: Resume receipts and task snapshots created additional ownership paths that stranded results after acknowledgement and restart.
So what: Delete both contract fields and all snapshot/release logic. Completion runs never inherit user holds; Stop cancels an unstarted queued wake while retaining its mailbox identity, and normal resume/startup offering retries that identity. Planner-owned review follows the new proof.

Requirements and who asked: The planner explicitly selected one retirement-derived rule, retaining zero offers/dispatches while retired, once-only delivery, task_status readability, and all four reviewer probes.
Deleted: forkResumeCommandId, forkResumeTaskIds, task snapshots, successor permission transfer, separate resume release loops and completion dependence on user queue holds.
Bottleneck: Multiple lifetimes represented one fact: pending mailbox work is deliverable exactly when durable retirement permits it.
Checked myself: Read both full new probes, cancellation/acknowledgement, mailbox dedupe, queue creation/promotion, startup holding and successor ownership. A cancelled unstarted wake can retain the same mailbox ID and be retried through the existing message-update path; no new parked state is needed. Stop's atomic final offer gate and consumer ancestry interval stay.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Accept both round-3 findings and narrow the hold exemption to user-queue pauses while restoring actual startup scheduling.
Why: A single broad completion exception bypassed workspace-failure holds, and an unheld queued completion still needs the normal start path after process recovery.
So what: Restore failed-worktree and provider-start hold behavior, preserve every existing hold during startup, and start eligible queued mailbox completions during the existing delegation startup phase. No resume fields or new queue state are needed; planner-owned review follows.

Requirements and who asked: The planner corrected the over-broad hold rule: completion delivery may bypass the Stop pause inherited from user work, but must obey every operational hold. Startup must actually schedule eligible queued completion work through the existing start path.
Deleted: The exception around worktreeMissingRun, the completion exception in failure-hold application, startup clearing existing completion holds, and the test's manual resumeQueuedRuns call.
Bottleneck: Queue eligibility was confused with readiness to execute, and startup normalization was mistaken for scheduling.
Checked myself: Read both probes, worktree queue creation, startNextQueuedRun guards, startup hold normalization and runOrderedV2StartupPhases. Main remains at 0ad34f2. The startup phase can call the existing start path under its existing destination lock without a new scan or global resume.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Both round-3 findings are fixed at b15aa0cf00289fe1a603a83e83ff5d53c288ff1b; 218 focused tests pass across 16 files.
Why: Completion eligibility must retain operational holds, and production startup must schedule eligible queued work rather than merely normalize its hold flag.
So what: The planner can run required reviews on this pushed head; no implementation-owned review rounds were launched.

Hold rule: completions bypass the user-work pause inherited from Stop. Missing-worktree holds apply at creation; failed/disabled provider-start holds apply to completions exactly as on base. Startup preserves every existing hold, including completion holds, instead of clearing them. It adds its user-work pause only to user work.

The existing delegation startup phase calls startNextQueuedRun under the existing destination lock for persisted queued mailbox work, after retirement permits it. All normal start guards apply. No global queue resume or new scan. The startup regression uses runOrderedV2StartupPhases without resumeQueuedRuns and asserts exactly one pending/running provider-start effect. The failed-worktree probe covers live and startup variants, asserting queued/held state and zero start effects. Both probes failed on 00552dd before repair.

Server/contracts targeted typechecks, touched-file lint/format, diff and fork checks pass; only the existing unused layerUnavailable warning remains. Origin/main stays 0ad34f2, already the branch base. Exact head merges cleanly with #430 at 6e734b0 (git merge-tree exit 0); combined-runtime proof is not claimed. PR body, fork-features and the canonical #422 handoff are updated. No merge or deployment.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

What: Delete all completion-specific queue-hold behavior and restore upstream queue scheduling semantics.
Why: The hold exemption bypassed operational safety and required increasingly broad startup exceptions; both reviewers rejected it.
So what: Parked results are offered once when the thread resumes. If queued work is held, the result joins that held queue and starts with it exactly once when the user resumes the queue, including after restart. Preserve the atomic retirement offer gate, pending mailbox identity, ordinary acknowledgement and pre-interceptor Stop admission.

Requirements and who asked: The planner explicitly chose upstream hold inheritance for every completion and removal of automatic startup promotion; no result may be lost, and operational/provider/worktree holds must stay authoritative.
Deleted: Completion hold inheritance exemptions, held-queue selection/projection exceptions, startup completion hold handling, and the added startup startNextQueuedRun call. No replacement state, flag or retry machinery.
Bottleneck: Queue policy diverged from upstream instead of leaving delivery pending until the user explicitly resumes held work.
Checked myself: Read the Codex R4 probe and all hold-related cumulative edits, confirmed main is unchanged, and built both operational-failure and preparing-worktree restart regressions. The queued result must remain observable and start once only after the ordinary queue.resume command.

Keep early retirement admission before feature planning while recording rejected non-message commands. Prepared launch release keeps its durable receipt when Stop wins workspace preparation.

Co-Authored-By: GPT-6.1-Sol via Codex
Preserve base planning and commit-time retirement refusal semantics for other commands. Keep queue resume retryable with its original command id and retain stopped launch planning rejection receipts.

Co-Authored-By: GPT-6.1-Sol via Codex
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Required reviews complete at exact head 3f13cfd3ad05e02e2c54361da9a84492fe8a60a2.

  • R1-R4 (Codex high, plus Grok R2-R4): stranded results on resume, an ancestor-Stop offer race, acknowledge stranding, restart re-holds, and completions bypassing worktree and operational holds. The planner replaced the per-resume snapshot and hold-exemption designs with upstream queue semantics: a completion wake is an ordinary queued run that obeys every hold; parked means pending delivery while durably retired.
  • R5 (fac743caac): Codex high and Grok APPROVE. CI then failed ThreadLaunchService Stop-during-preparation (early preflight skipped the launch release's rejection receipt).
  • R6 (0b5f88b1f2): Codex high REQUEST CHANGES (same-id queue.resume retry poisoned); Grok not run (provider balance).
  • R7 (this head): early retirement preflight only for message.dispatch; other commands follow base admission and receipt behavior. Codex gpt-6.1-sol high APPROVE (138 tests plus the R6 probe pass). Grok 4.7 medium APPROVE (traced the launch release and queue.resume retry against base).
  • CI on this head: 22 success, 4 skipped; Native fingerprint diff was cancelled and has been rerun.

User-visible rule: results parked during Stop are offered once on resume; if the thread also has held queued work, the result joins that held queue and starts exactly once on queue.resume.

Ready for the final train review and the user's merge decision.

🤖 Claude Opus 5.5 planner via Claude Code in T3 Code

@lukemaj
lukemaj merged commit 24ecfea into main Oct 18, 2026
29 of 30 checks passed
@lukemaj
lukemaj deleted the fix/422-park-stopped-completions branch October 18, 2026 16:40
lukemaj added a commit that referenced this pull request Oct 18, 2026
…431)

* fix(orchestration): park stopped delegated completions until resume

Keep delegated results in the upstream completion mailbox while durable Stop blocks offers. Reuse producer eligibility for stale offers and retries, re-offer on explicit resume, and check retirement before feature planning.

Co-Authored-By: GPT-6.1-Sol via Codex

* docs: register stopped completion delivery fix

Co-Authored-By: GPT-6.1-Sol via Codex

* chore: register carried completion eligibility hooks

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): retain unread completion delivery across stop races

Serialize mailbox consumption with Stop, retain interrupted notices whose provider never started, and release only resumed completion deliveries past unrelated held work. Cover the three independent review findings at orchestration seams.

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): persist completion resume and lock retirement ancestry

Keep resume permission on the durable mailbox delivery, serialize its consumer against every retirement owner, and clear app-owned offers without re-entering the lock. Preserve the no-receipt semantics of retirement guard refusal for Spectrum reports.

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): retain resume eligibility through unread settlement

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): resume parked siblings and serialize completion offers

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): derive parked completions from retirement

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): preserve holds and start recovered completions

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): restore upstream completion queue holds

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): retain stopped launch rejection receipts

Keep early retirement admission before feature planning while recording rejected non-message commands. Prepared launch release keeps its durable receipt when Stop wins workspace preparation.

Co-Authored-By: GPT-6.1-Sol via Codex

* fix(orchestration): limit early Stop admission to messages

Preserve base planning and commit-time retirement refusal semantics for other commands. Keep queue resume retryable with its original command id and retain stopped launch planning rejection receipts.

Co-Authored-By: GPT-6.1-Sol via Codex
(cherry picked from commit 24ecfea)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant