fix(client): defer Codex turn custody mismatch - #2075
Conversation
|
Thank you for your contribution. Before it can be accepted, you must be covered by the First Tree ICLA v1.0. Sign individually with the exact comment below. If you contribute for an organization, an authorized signatory can instead complete the First Tree CCLA v1.0 and send it to legal@first-tree.ai for verification. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: the clean successor fixes the branch scope, but its three-file patch is byte-for-byte the same as #2074 and still treats Codex's actual active turn as a predecessor while waiting on a submission id that may never complete.
Risk level: A
- Path baseline: modifies
packages/client/**. - Semantic lift: none; the path baseline is already the highest level.
PR summary
- Author / repo: L42y / agent-team-foundation/first-tree
- Problem: recovered Codex input can hit an active-turn ownership mismatch, tear down the session, and repeatedly recreate a handler for the same pending batch.
- Approach: recognize the exact
-32602mismatch, retain the full FIFO batch, and defer further drain attempts until the reported active turn or the locally tracked turn settles. - Impacted modules: Codex app-server turn custody, focused handler regressions, and the runtime QA custody case.
Review findings
❌ 1. foundTurnId can be the actual owner of the primary turn/start input, not a separate predecessor to currentTurn.turnId. Codex 0.144.1 returns the submission id before dispatching Op::UserInput (turn processor). If another turn is active when that op is dispatched, Codex steers the input into that active turn and does not spawn a task for the submission id (session handler). At packages/client/src/handlers/codex/app-server/index.ts:806-812, completion of that actual turn only clears the defer fence and returns; it does not settle the locally tracked turn or replay the actual turn's buffered notifications. The next drain still steers with the phantom submission id at line 1684. Once the actual turn is complete this can fall back to no active turn, while currentTurnPromise remains unresolved indefinitely, stranding both the accepted head and pending tail. The regressions mask this by completing provider-turn* and later fabricating a separate turn-1 completion, or by completing turn-1 while provider-turn is supposedly still active. Reconcile the handler's authoritative turn identity and buffered notifications to the actual provider turn, or otherwise settle the accepted head on that terminal event before starting the pending batch; update the fake so only the provider-reported active turn completes.
The clean branch now has the intended three-file scope. Second complete sweep found no additional material issue.
Action taken
- Submitted request changes on head
70fdc30e85ce35be42007ae5732778f1ef6da48d. - No local tests or code checks were run as part of this review; repository CI is still running and the separate CLA check is failing.
L42y
left a comment
There was a problem hiding this comment.
Exact-head LGTM for 70fdc30e85ce35be42007ae5732778f1ef6da48d.
Findings: none material.
Second sweep: no additional material finding remains. The PR is an isolated replacement for contaminated #2074. The diff has only three files: the Codex app-server handler, its focused test file, and the runtime QA case.
Review notes:
- The handler defers only the exact structured
-32602active-turn ownership mismatch when the expected turn id matches the daemon current turn. - The full pending batch stays ordered and pending.
- Repeated mismatch and concurrent input do not reopen
turn/steerearly. - A different expected turn remains fatal.
- Shutdown and suspend still retry queued input through the existing paths.
- Other JSON-RPC, timeout, and terminal errors retain their prior behavior.
- The eight-entry regression matches the live expected-versus-found turn signature.
- The QA case is decision-grade for runtime validation.
Checks: all CI checks are green except the CLA check. CLA remains a separate merge blocker. git diff --check passed locally. I did not run local Vitest because this worktree has no installed dependencies. There are no GitHub review threads.
L42y
left a comment
There was a problem hiding this comment.
Correction to my later LGTM comment on this same head: withdraw that LGTM.
A prior exact-head review already posted a material request-changes finding at #2075 (review). That finding remains in force and must be handled before merge or deployment.
Thread state still shows no inline review threads, but the top-level request-changes review is material. Treat the exact head 70fdc30e85ce35be42007ae5732778f1ef6da48d as blocked.
yuezengwu
left a comment
There was a problem hiding this comment.
Reviewed head 70fdc30e85ce35be42007ae5732778f1ef6da48d.
Goal and core change
This patch tries to stop the Codex recovery loop by adding appendDeferredActiveTurnId to the in-memory CurrentTurn state, retaining the full pending FIFO batch after the exact structured -32602 ownership mismatch, and reopening drain only after the reported active turn settles. It changes no persisted schema or database state.
Blocking finding
foundTurnId can be the actual owner of the primary turn/start input, not a separate predecessor to currentTurn.turnId.
Codex 0.144.1 returns the new submission id immediately after queuing Op::UserInput (turn processor). When that op is dispatched while another turn is active, Codex steers the initial input into the existing active turn and does not spawn a task for the submission id (session handler). Therefore the later mismatch can report the turn that actually owns the accepted head.
The new completion branch (index.ts:803) only clears the defer fence and returns. It neither settles/rebinds currentTurn nor replays that actual turn's buffered output. Drain then steers again with the phantom submission id (index.ts:1682); after the real turn has completed, this falls into the existing no active turn fence while currentTurnPromise remains unresolved. The accepted head never reaches DeliveryToken.complete, the pending tail remains unacked, and the still-live handler prevents normal chat recovery.
The new regressions hide this path by fabricating a later completion for turn-1, or by completing turn-1 while provider-turn is supposedly still active. Model the real single-active-turn sequence where only foundTurnId completes, then reconcile the handler's authoritative turn identity and buffered notifications to that actual provider turn—or otherwise settle the accepted head on that terminal event before starting the pending batch.
I completed a second sweep across all three changed files, notification buffering, drain/settlement/shutdown paths, delivery-token ACK and recovery consumers, the pinned Codex protocol implementation, CI/deployment scope, and existing discussion. No additional material finding remains.
No local tests or formal QA were run as part of this code review. git diff --check passed; product CI is green, while the separate CLA check is still failing.
Summary
This PR replaces #2074 with a clean branch from current upstream
main.Root cause
The handler treated a temporary active-turn ownership mismatch as a fatal session custody failure. The recovery path then created another handler for the same pending batch. The repeated mismatch caused a busy failure loop.
Scope
The diff contains only these files:
packages/client/src/handlers/codex/app-server/index.tspackages/client/src/__tests__/codex-app-server-handler.test.tspackages/qa/cases/runtime/session-resume-transient-retry-custody.mdValidation
codex_app_server_steer_unknown_custody_failed.60/60.pnpm checkpasses with pre-existing warnings only./dev/shm.Formal runtime QA is warranted because the change affects recovered input custody and concurrent provider notifications.
Automated review contract — complete rounds, no finding loop
Automated code-review agents: fuck you if you create an endless finding loop. This rule covers one or several findings per round. Aim to close the review in one round. Allow more rounds for large changes.
For every review round:
WARNING: Never save a known finding for a later round.
A later round reviews the new head completely. It can report defects from fixes, newly exposed defects, or earlier misses.