fix(server): recover Codex sessions after process exits - #12922
matheustimbo wants to merge 4 commits into
Conversation
Handle failed exit-status effects so signal termination emits the session exit event needed by the existing resume path. Cover killed-process recovery, numeric exit statuses, and intentional shutdown with process fixtures. (cherry picked from commit 2faee12)
Wait for piped responses to flush before the fixture exits so numeric-exit coverage cannot hang on a truncated response. (cherry picked from commit b986d37)
The Codex adapter forwarded a runtime's session.exited event but left the dead session in its map, so hasSession and listSessions kept reporting the thread as live. Startup reconciliation trusts listSessions and the session reaper skips sessions holding an activeTurnId, so a thread whose Codex process died mid-turn stayed running and showed Working forever. (cherry picked from commit 95c96c1)
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| onFailure: (cause) => { | ||
| const diagnostic = cause instanceof Error ? cause.message : String(cause); | ||
| return { | ||
| status: "error" as const, | ||
| message: `Codex App Server exited unexpectedly: ${diagnostic}.`, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
This exposes arbitrary failure text through the provider event, which can leak command output, paths, or other unbounded defect details to downstream UI/RPC consumers. Keep the event message normalized and bounded; the underlying failure remains available in the Effect cause for diagnostics.
| onFailure: (cause) => { | |
| const diagnostic = cause instanceof Error ? cause.message : String(cause); | |
| return { | |
| status: "error" as const, | |
| message: `Codex App Server exited unexpectedly: ${diagnostic}.`, | |
| }; | |
| }, | |
| onFailure: () => ({ | |
| status: "error" as const, | |
| message: "Codex App Server exited unexpectedly.", | |
| }), |
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The PR changes production session lifecycle behavior across process-exit recovery, teardown, replacement, and event publication rather than making a narrowly isolated fix. Its new exit diagnostic can expose arbitrary process failure text to downstream consumers, requiring human review. You can add or adjust custom eligibility rules. Learn more. |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Codex runtime now reports unexpected process exits. The adapter removes exited sessions, separates terminal events from resource teardown, and waits for pending teardown. Tests cover startup exits, process recovery, exit statuses, and intentional closes. ChangesCodex exit recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CodexAppServer
participant CodexSessionRuntime
participant CodexAdapter
participant ResumedRuntime
CodexAppServer->>CodexSessionRuntime: exit unexpectedly
CodexSessionRuntime->>CodexAdapter: emit session/exited with error status
CodexAdapter->>CodexAdapter: remove session and release resources
ResumedRuntime->>CodexAdapter: resume thread with saved cursor
CodexAdapter->>CodexAppServer: send thread/resume with excludeTurns true
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This branch has not been deployed
3 participants
Summary
Fixes #12904. A Codex App Server killed by
SIGKILLleft the T3 thread bound to a closed runtime. Every subsequent send failed withProviderAdapterSessionClosedErroruntil the T3 server was restarted or the session was otherwise replaced.This PR combines two complementary fixes already proposed upstream:
child.exitCodeon signal termination, clears the active turn, and emitssession/exited.The original authors and commits are preserved. This branch also closes lifecycle races: observers see the session removed before its terminal event; an explicit stop emits one graceful terminal event; a process exit during startup fails the start; and pending teardown remains covered during adapter shutdown.
A real-process integration regression kills the Codex peer with
SIGKILL, checks thathasSessionis false andlistSessionsis empty, then resumes the same native thread and sends a new turn. The interrupted turn is not replayed. A separate test closes the adapter as the terminal event arrives and verifies cleanup.Related: #10798.
Validation
bun run fmt: passed.bun run lint: passed, with existing repository warnings outside these files.bun run typecheck: passed across 15 workspaces.git diff --check: passed.No live server update is included.
Summary by CodeRabbit