Skip to content

fix(server): recover Codex sessions after app-server exit - #81

Merged
kalvenschraut merged 3 commits into
rtvisionfrom
feature/codex-app-server-exit-recovery
Oct 6, 2026
Merged

kalvenschraut merged 3 commits into
rtvisionfrom
feature/codex-app-server-exit-recovery

Conversation

@kalvenschraut

@kalvenschraut kalvenschraut commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Problem

When a shared codex app-server exits, 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

  • A test-owned Node child exits with code 0 on SIGTERM. Two subscribers receive the original PID/code error, the cached session is removed, and the next open starts a different live child. Reusing that replacement does not start another process.
  • A protocol regression verifies that process exit fails pending and future requests even while an inbound notification handler is blocked. Late termination observers receive the stored error.
  • Disabling adapter exit propagation makes the recovery regression time out, confirming it detects the original defect.
  • Focused run of protocol.test.ts, client.test.ts, ProviderSessionManager.test.ts, and CodexAdapterV2.test.ts with --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 orchestration and Stop ends a background command no Codex process tracks any more.
  • Server and effect-codex-app-server typechecks passed. Scoped formatting and diff checks passed. Scoped lint reported only two existing unused-variable warnings.
  • CI's Effect checker caught an unknown error type in the new protocol test. The test now discards the unused raw response before Effect.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.
  • A subsequent CI run cleared the protocol diagnostics but reported a chained Effect.provide warning in the recovery test. Its layers now use a single provision. All 45 session-manager tests passed after this correction.
  • CI is green on 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.

@kalvenschraut

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: RTVision/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f1fb4d88-8aef-41f9-8789-f0dd39ccdb85
📥 Commits

Reviewing files that changed from the base of the PR and between 6112578 and b07a7c5.

📒 Files selected for processing (5)
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
  • apps/server/src/orchestration-v2/ProviderSessionManager.test.ts
  • packages/effect-codex-app-server/src/client.ts
  • packages/effect-codex-app-server/src/protocol.test.ts
  • packages/effect-codex-app-server/src/protocol.ts

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


📝 Walkthrough

Walkthrough

The 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.

Changes

Codex process termination

Layer / File(s) Summary
Observe and record process termination
packages/effect-codex-app-server/src/protocol.ts, packages/effect-codex-app-server/src/protocol.test.ts
The protocol observes process exit independently of inbound stream progress. It records the termination error, fails pending requests, and exposes the error through awaitTermination. The test checks that pending and future observers receive the same process-exit error.
Expose termination through the client
packages/effect-codex-app-server/src/client.ts
The client forwards an optional process-exit effect to the protocol and exposes awaitTermination. Child-process clients pass the same termination-error effect for both termination options.
Propagate termination to session streams
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts, apps/server/src/orchestration-v2/ProviderSessionManager.test.ts
The adapter fails its event queue when client termination fails, and reads session events directly from the queue. The integration test checks shared session opens, process-exit errors, the errored session state, and reopening with a new process and runtime.

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
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b07a7

No actionable issue remains identified for this change; it is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the fix: recovering Codex sessions after the app-server exits.
Description check ✅ Passed The description includes all required sections and gives specific details about the problem, implementation, scope, and verification. However, it says no client contracts changed, while the change sum…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

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 +40 B (+0.8%) 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 +40 B (+3.4%) 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: 6112578 · PR result: b07a7c5 · 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.

@kalvenschraut

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@kalvenschraut
kalvenschraut merged commit 260138d into rtvision Oct 6, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 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