Skip to content

fix(server): restarts continue every Codex thread, not just the first - #15820

Open
Mnigos wants to merge 2 commits into
pingdotgg:mainfrom
Mnigos:restart-continuation-shared-sessions
Open

Mnigos wants to merge 2 commits into
pingdotgg:mainfrom
Mnigos:restart-continuation-shared-sessions

Conversation

@Mnigos

@Mnigos Mnigos commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #15749

Problem

If shutdown preparation does not finish before the launcher force-kills the server, startup recovery records the restart continuations. It reconciles threads one at a time and, for each one, marks every live session it is bound to as stopped. Codex and OpenCode 2 threads all share one session per provider instance (provider-session:provider-instance:<id>:shared). The first thread reconciled on that session, often an older idle thread, stopped it. Every later thread then read the session as stopped, and restartContinuationRun refused a live turn on a stopped session. So at most one thread per shared session was continued, and none if an idle thread came first. Claude was not affected because each Claude thread has its own session.

Change

During one recovery pass, ProviderRuntimeRecoveryService.reconcile remembers each session as the pass first read it. Continuation eligibility is judged against that state, which means "was the session live when the server went down", rather than against the state a previous thread wrote during the same pass. Session stops, per-thread fresh reads and RestartContinuation.ts are unchanged. A session that was already stopped or error before the pass still blocks continuation.

The triage comment on the issue names two directions. This is the first one: decide eligibility before any thread stops a shared session. The second, not stopping a shared session while other bound threads still have running runs, would change the session lifecycle; the outcome for the user is the same either way.

Verification

New tests in ProviderRuntimeRecoveryPerformance.test.ts use the real SQLite stores. They seed one older idle thread and three running Codex-style threads, all on one shared session, then run startup recovery:

Test case Base efecd3cf8b This branch
startup recovery continues every running thread on a shared provider session Fail: 0 of 3 continued Pass: 3 of 3, session stopped once
startup recovery does not continue threads whose shared session was already stopped Pass Pass
restart recovery records one continuation per active thread and is idempotent (existing, one session per thread) Pass, ~1.2–1.4 s Pass, ~1.2–1.3 s
Gate Result
vp test run on the recovery, restart-continuation, projection-recovery, runtime-layer and replay-recovery suites (12 files), before the rebase 193 passed, 1 skipped (bench matrix)
(cd apps/server && vp test run src/orchestration-v2/ProviderRuntimeRecoveryPerformance.test.ts src/orchestration-v2/ProviderRuntimeRecoveryService.test.ts src/orchestration-v2/RestartContinuation.test.ts) on d4cd794bfe (rebased onto fe417dff4c) 43 passed, 1 skipped (bench matrix)
T3_BENCH_RECOVERY=1 benchmark matrix, run during independent review 4 passed, all six scale scenarios; it has no latency threshold
(cd apps/server && vp run typecheck) Exit 0; no error TS or warning TS
vp lint / vp fmt --check on the two changed files Pass
knip --workspace apps/server --exports Pass

Server-only change; there is no UI, so no screenshots.

Limitations / not checked: no real service restart and no real Codex or OpenCode run, so native resumption of several threads on one shared session is taken from the code path (ProviderTurnStartService reopens the stopped session per native thread), not observed. A shared session with one running thread and one sibling whose provider turn was not running is not covered by a test; the unchanged eligibility check still declines that sibling. A second crash midway through recovery is a pre-existing limit of this pass-local view. The launcher's fixed 5 s grace (TERMINATE_GRACE_MS), which is why the startup path runs at all, is unchanged and out of scope.

This touches the same recovery loop as #15786 (a different bug); whichever lands second needs a small rebase.

Implemented with Claude Opus 5.5, verified with GPT-6 Astra, coordinated by Claude Fable 5.1 in Claude Code.

Startup recovery reconciles threads one at a time and marks each bound provider
session as stopped. Codex and OpenCode 2 threads share one session per provider
instance, so after the first thread on it was reconciled, every later thread
read the session as stopped and was refused a restart continuation.

Continuation eligibility is now judged against each session as the recovery
pass first read it, before any thread in the pass stopped it. A session that
was already stopped or in error before the pass still blocks continuation.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 5, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at d4cd794

Macroscope's review found this PR approvable — This is a narrowly scoped restart-recovery bug fix that preserves initial shared-session state so all eligible Codex threads can receive continuation effects. It adds targeted coverage for shared live sessions and preserves the existing behavior for sessions already stopped before recovery.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 42a34683-326d-40f6-92ba-94630b15f3d3

📥 Commits

Reviewing files that changed from the base of the PR and between fe417df and d4cd794.


📒 Files selected for processing (2)
  • apps/server/src/orchestration-v2/ProviderRuntimeRecoveryPerformance.test.ts
  • apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts

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



📝 Walkthrough

Walkthrough

Startup reconciliation now uses a provider session’s state from before the recovery pass when selecting continuations for threads that share the session. Tests cover ready and stopped shared sessions.

Changes

Shared-session recovery

Layer / File(s) Summary
Preserve session state during reconciliation
apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts
The recovery pass records each provider session’s first-observed state and uses it when selecting continuations during thread reconciliation.
Test shared-session recovery
apps/server/src/orchestration-v2/ProviderRuntimeRecoveryPerformance.test.ts
Scenarios can attach active threads to one ready or stopped session. Tests assert that a ready session is stopped once and produces a continuation for each active thread, while a stopped session produces no additional stop or continuations. Both cases assert that active runs become terminal.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: maria-rcks


Merge Risk | ⚪ Minimal · up to d4cd7

Merge Risk: ⚪ Minimal · up to d4cd7

Shared-session recovery has no identified merge-blocking issue. The examined idle-sibling ordering does not prevent an eligible active run from continuing.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d4cd7

The change restores continuation for eligible threads sharing a session while retaining identity checks, user-stop exclusions, and execution-policy selection. The snapshot lasts only one recovery pass, so interrupted recovery still has limitations.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The expanded automatic-execution scope is additional eligible, previously running sibling threads sharing a provider session. The reviewed path does not select new provider identities or grant additional permissions; turn start continues to resolve policy from the thread and selected model.

Trust Boundaries and Controls

  • observed — The normal startup path completes recovery before starting the effect worker. Commands wait on readiness, and the inspected HTTP middleware waits on the same gate. Recovery failure propagates to failed readiness rather than allowing commands to execute during an incomplete pass.
  • observed — Continuation delivery rereads persisted records and rejects disabled settings, archived or deleted threads, duplicate continuation messages, non-cancelled sources, newer started work, provider-instance changes, prior user interrupt requests, and native maintenance commands.

Resilience and Maintainability Implications

  • observed — Continuation effects use stable source-run identities, and outbox insertion ignores duplicate effect IDs. Delivery also uses stable message and command identities. These controls contain ordinary repeated recovery and delivery without making the entire multi-thread pass atomic.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: restarting continuations for every eligible Codex thread.
Description check ✅ Passed The description clearly covers the problem, change, scope, verification, and limitations. The scope section refers to the issue’s triage comment but does not include the explicit maintainer approval c…
Linked Issues check ✅ Passed Issue #15749 requires startup recovery to continue each eligible running thread after shutdown preparation fails. ProviderRuntimeRecoveryService.reconcile now records each session's state before its…
Out of Scope Changes check ✅ Passed The changes are limited to startup recovery session-state handling and its recovery tests. The test fixture changes support the shared-session scenarios required by issue #15749. The PR does not chang…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

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

@rohan-patnaik

Copy link
Copy Markdown

Independent verification of #15749 on current main (a4c9494b0e3606775cc5fc929fc138399288bd43), with this PR's two-file patch at d4cd794bfe3c4e30c282c23b8e64fa0c8bf4094f applied to a clean checkout:

  • A synthetic two-running-thread shared-session regression fails on unmodified main: only the first thread receives continuation. It passes with this PR.
  • This PR's SQLite fixture (an older idle thread followed by three running threads sharing one session) fails on unmodified main: zero continuations. With the fix, all three continue and the session is stopped exactly once. The already-stopped-session guard also passes.
  • Four focused recovery suites pass: 46 tests passed, one opt-in benchmark matrix skipped. Scoped formatting, lint, server typecheck, and git diff --check pass.

The test port only needed the current-main rename from TestLayer to layerTest at two references. Verification command:

vp test run apps/server/src/orchestration-v2/RestartContinuation.test.ts apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.test.ts apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.regression.test.ts apps/server/src/orchestration-v2/ProviderRuntimeRecoveryPerformance.test.ts

I also tested pre-reading entire recovery projections before reconciliation. Although that restores continuation, the same SQLite fixture emits four session-stop events instead of one. This PR's first-seen session cache avoids that duplication, retains only shared session rows, and keeps other per-thread projection data fresh. This confirms the existing fix covers the independently reproduced case; no competing PR is needed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Restart continuation skips all Codex threads when shutdown preparation does not finish (shared session stopped by first reconciled thread)

2 participants