Repository navigation
fix(server): recover Codex sessions after app-server exit - #81
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe protocol now observes child-process termination and exposes its stored error. The Codex client and adapter propagate that error to session event streams. Tests cover protocol termination behavior and session recovery after a child process exits. ChangesCodex process termination
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ChildProcess
participant CodexProtocol
participant CodexClient
participant CodexAdapter
participant EventQueue
ChildProcess->>CodexProtocol: processExit effect fails
CodexProtocol->>CodexClient: awaitTermination fails with stored error
CodexClient->>CodexAdapter: termination watcher receives error
CodexAdapter->>EventQueue: fail with ProviderAdapterEventStreamError
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains identified for this change; it is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Problem
When a shared
codex app-serverexits, including a clean exit with code 0, its cached provider session can remain in T3. Every later turn reuses the dead client and fails immediately until the server restarts.Change
Observe child exit independently of stdout processing and expose the stored termination error through the Codex client. Fail the adapter event queue after draining queued events, so the existing session manager releases the runtime, fails its subscribers, and evicts the cached session. The next open creates a fresh child process.
Keep EOF error classification separate from process-exit observation so replay clients retain their existing behavior. No new settings or client contracts are needed.
Scope and approval
This is a focused fix for an obvious lifecycle bug in an existing provider. It connects child termination to the existing runtime-error release path. The protocol and session-manager tests cover that same defect.
Verification
protocol.test.ts,client.test.ts,ProviderSessionManager.test.ts, andCodexAdapterV2.test.tswith--testTimeout 10000: 204 passed, 2 failed. Both failures are existing Stop-test timeouts, reproduced against the original production source:interrupts a completed run's background command through orchestrationandStop ends a background command no Codex process tracks any more.effect-codex-app-servertypechecks passed. Scoped formatting and diff checks passed. Scoped lint reported only two existing unused-variable warnings.unknownerror type in the new protocol test. The test now discards the unused raw response beforeEffect.flip. All 21 protocol tests, the package typecheck, and formatting checks passed after this correction. The local Effect checker could not run because its packaged integration does not support Linux musl; CI owns that verification.Effect.providewarning in the recovery test. Its layers now use a single provision. All 45 session-manager tests passed after this correction.b07a7c587b51aff322fe5ac2a1f6662acef85480, including lint, the Effect-aware typecheck, build, all test jobs, and release smoke. Thread transfer remains within every enforced ceiling on this commit. Platform preview and unchanged mobile-analysis jobs were skipped.The original auto-update trigger was not reproduced. The regression exercises clean child exit and session replacement directly, but does not drive a full active turn. Repo-wide checks and client UI verification were not run.
Astra's independent read-only review gave GO for opening this PR with no reachable blockers on the initial patch. It inspected source and supplied logs; it did not rerun checks. Later CI corrections only change the protocol test's unused success type and combine the recovery test's layer provision.
Implemented with gpt-6.1-sol, high reasoning effort, through the Codex harness.