fix(server): preserve completed runs when accepting child deliveries#25667

merged6

main

fix/issue-25000

View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

Accepting a delegated-completion delivery (notification.delivery.accept) plans a run.updated for the parent run from a read taken while the parent is waiting. If checkpoint capture completes that run before accept commits, the stale write puts it back to waiting with no completedAt or checkpointId, stranding queued work behind an already checkpointed turn.

Change

Main already merges delegated-completion bookkeeping into the current stored run for acknowledge and dispose (updateDelegatedCompletionOnlyRunId on EventSink.commitCommand, from #25672). Delivery acceptance now uses the same hook: dispatchNotificationAccepted returns the parent run id, so the commit keeps the run's current lifecycle and checkpoint fields and takes only delegatedCompletion from the event. Server-only; no client or contract changes.

Fixes pingdotgg/t3code#25757

Related: pingdotgg/t3code#25000 was first linked here, but its run stayed running after late Codex command events, which this change does not touch.

Takeover

Taken over from @juliusmarminge. Main landed #25672 while this PR was open, with a run-id-scoped version of the same merge; the PR's boolean updateDelegatedCompletionOnly on commitCommand and its EventSink refactor were dropped and the fix was ported onto that API (3 lines in Orchestrator.ts). The standalone regression test was folded into #25672's race table as an accept row, which also asserts the task ends delivered. The historical, re-assembled proof was replaced by fresh runs at this head.

Evidence

Server-only change: no render paths, so no visual rows (fixture matrix). Full write-up: proof.md.

Row	Base `401b01747c`	Head `fdcf43b781`	Log
R1 race test over base implementation	accept row fails: `status: 'waiting'`, `completedAt: null`, `checkpointId: null`; 30 others pass	—	r1-base.log
R2 `DelegatedCompletionDelivery.test.ts`	—	31/31 pass (independent rerun identical)	r2-head-verbose.log, rerun
R3 real Codex runtime, fresh home	—	child delivered (seq 93), accept receipt committed (seq 94), parent and follow-up complete with ready checkpoints (seq 110, 136)	r3-transcript.txt, runtime.log

Environment: fs-19 Linux x86_64, Codex CLI 0.163.0 (gpt-6.1-sol) through the CLIProxyAPI hub, T3CODE_DEV_SEED=0 with a fresh --home-dir; synthetic data only. R1/R2 prove the forced read/commit interleaving; R3 proves the accept path integrates end to end (the race itself cannot be forced through supported UI/RPC).

What else changed: nothing user-visible beyond the parent run keeping its completed status and checkpoint.

Scripts (uploaded, not committed): rpc-driver.mjs, run-tests.sh, server-control.py, gate.py, collect-evidence.py, all logs.

Verification log

Supersession check: the PR's regression test fails on main 5522180329 (expected 'waiting' to equal 'completed'), so #25672 did not cover accept.
Tests: DelegatedCompletionDelivery.test.ts 31 → 31 (PR's standalone test folded into the existing race table). Reverting only the Orchestrator.ts change fails exactly the accept row.
vp run check:changed at fdcf43b781: format, lint, knip, graphql, typecheck (3 packages), tests (3 files) pass.
Review round 1 (Codex gpt-6.1-sol, Claude Opus 5.5): both MERGE, no findings; SENSITIVE no, DECISION no.
Proof audit: pass 1 matrix R1–R3 required; pass 2 SUFFICIENT.
Unverified: none. Claude-provider runtime run excluded (no Claude OAuth on the VM; the fix is provider-independent persistence).
Models and harnesses: Claude Opus 5.5 through Claude Code; GPT-6.1-sol through Codex; orchestrated in T3 Code.

Original description by @juliusmarminge

Problem

Accepting a delegated completion can plan a run update while its parent is waiting, then commit after checkpoint capture completes the run. The stale update restores waiting, stranding queued work behind an already checkpointed turn.

Change

Apply the existing delegated-completion-only transactional merge to delivery acceptance. Preserve the current run lifecycle and checkpoint fields while updating delivery bookkeeping in both the event log and projection.

Scope and approval

Originally linked to https://github.com/pingdotgg/t3code/issues/25000

GitCafe: https://git.cafe/pingdotgg/t3code/issues/25000

Triaged bug via the automated issue flow. Server-only persistence fix; no client or contract changes.

Verification

Regression test keeps a checkpoint completion committed while accepting a delivery: failed before the fix with actual waiting, expected completed; passes with the fix and preserves the checkpoint ID.
vp test run apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts: 29 passed.
After merging main at head de5ee510e2: focused suite passed (29 tests), vp run check:changed passed (42 tests).
Both retained independent reviewers returned MERGE.
Real Linux web/server run with Codex CLI 0.163.0: parent checkpointed while a command remained outstanding; child completion delivery, manual follow-up, late command completion and notification runs all settled. Late-command-only behavior also settled on main. The focused test proves the persistence race; exact causality of the reported production incident remains unconfirmed.
Recovered runtime transcript and test provenance. Historical evidence was recovered after the isolated database was deleted; no fresh runtime execution is claimed here.
Proof matrix: server-only fix, zero visual rows. No render paths changed.

Models and harnesses: Claude Opus 5.5 through Claude; GPT-6.1-sol through Codex.

🤖 Generated with Claude Code

Checks

1 failing · 22 successful · 16 skipped

Some checks failed

Failing1

Agent context budgetGitHub
Failed

Added 3 commits2d ago

test(server): reproduce issue 25000 run stuck waiting after delivery accept8cc9783
fix(server): delivery acceptance no longer reverts a completed run to waiting8cfa0bb
Merge main into fix/issue-25000de5ee51

MmacroscopeappBotapproved these changes · review dismissed2d ago

MmacroscopeappBot2d ago

Approvability

Verdict: Approved at fdcf43b

Macroscope's review found this PR approvable — This is a focused server bug fix that prevents stale child-delivery acceptance events from reverting completed parent runs, while preserving delivery updates through the existing transactional merge path. Regression coverage explicitly exercises the acceptance race and verifies completion and checkpoint data remain intact.

^{You can add or adjust custom eligibility rules. Learn more.}

Ggithub-actionsBot2d ago

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	+7 B (+0.1%)	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	+7 B (+0.6%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.9 KiB	20.9 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	2	2	0 (0.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: 401b017 · PR result: fdcf43b · 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.

CcoderabbitaiBot2d ago

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: fix/issue-25000
File: .coderabbit.config.ts
Commit: fdcf43b
Layers: Repository TS config
Review profile: CHILL
Plan: Team
Run ID: 12afd70a-f657-4742-8eb0-9f43c3656017

External image Commits

Reviewing files that changed from the base of the PR and between de5ee510e2cc34865b25adae30d09343df7bd961 and fdcf43b781a36813afeab8547a25adbde0c65b17.

External image Files selected for processing: 2

apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts
apps/server/src/orchestration-v2/Orchestrator.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

The orchestrator now carries the delegated parent run ID from notification acceptance into the dispatch plan. The delivery race test covers acknowledgement and acceptance when the run settles between command read and commit, and checks the delivery state and settled run fields.

Changes: Delegated delivery races

Layer / File(s)	Summary
Pass the parent run ID and test delivery races `apps/server/src/orchestration-v2/Orchestrator.ts`, `apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts`	Notification acceptance returns the parent run ID, which `dispatchOnce` stores in the dispatch plan. The test covers acknowledgement and acceptance races, checking delivery state and the run’s settled status, completion time, and checkpoint.

Priority: External image Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

pingdotgg/t3code#25662: This PR also addresses delegated-completion delivery races and adds checkpoint-completion regression coverage.
pingdotgg/t3code#25672: This PR uses the same run-ID field and covers overlapping settle-race cases.

Suggested reviewers: scottn-pv

Merge Risk: External image Minimal · up to fdcf4

No actionable merge-blocking issue is identified; the change is ready for normal merge checks.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title clearly and concisely describes the main server-side fix: preserving completed runs when accepting child deliveries. It uses an appropriate conventional commit format.
Linked Issues check	Passed	Issue `#25000` requires settled runs to remain settled after late completion-related updates, while delivery items update and queued work can continue. The change passes the delegated acknowledgement ra…
Out of Scope Changes check	Passed	The changes are limited to the server orchestrator and its regression test. `dispatchNotificationAccepted` returns the parent run ID so `commitCommand` can apply the existing delegated-completion-only…
Description check	Passed	The description is complete and directly matches the change. It explains the problem, implementation, scope, linked issue, takeover context, verification results, test evidence, environment, and known…

Full details: Linked Issues check

Explanation

Issue #25000 requires settled runs to remain settled after late completion-related updates, while delivery items update and queued work can continue. The change passes the delegated acknowledgement race cases and adds the notification acceptance race case. The new test verifies delivered delivery state and preserves completed, completedAt, and checkpointId after settlement. The reported runtime checks also cover follow-up and queue progression.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the server orchestrator and its regression test. dispatchNotificationAccepted returns the parent run ID so commitCommand can apply the existing delegated-completion-only merge. The test covers acknowledgement and notification acceptance. No unrelated client, contract, or UI changes are present.

Full details: Description check

Explanation

The description is complete and directly matches the change. It explains the problem, implementation, scope, linked issue, takeover context, verification results, test evidence, environment, and known limitations. The current section names differ slightly from the template, but the required information is present, including scope approval rationale and verification details.

Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

JJulius Marminge2d ago

The linked-issue check asks for independent evidence of turn settlement, late command handling and queue progression. Those sequences were exercised against the real Linux web/server app: main runtime transcript records late command completions after settled turns and successful follow-up/notification runs; head runtime provenance records parent completion, child delivery, follow-up and late-command settlement. The new regression isolates the separately confirmed delivery-accept/checkpoint persistence race. Exact causality of the original incident remains unconfirmed, as stated in the body. No provider event-handling change is justified by the observed late-command sequence.

The related #25662 covers acknowledgement/disposal/wake-policy writers; this PR covers notification.delivery.accept. Both use the same persistence constraint and may conflict when landed.

JJulius Marminge1d ago

Taking over this one

Added 3 commits1d ago

Merge remote-tracking branch 'origin/main' into pr-25666d4b514e
test(server): fold the takeover's accept race into the acknowledge table34668f0
Merge remote-tracking branch 'origin/main' into pr-25666fdcf43b

MmacroscopeappBotapproved these changes1d ago