Sign up
View stargazersSign up

fix(server): bound automatic restart recovery#24177

open2
main
GitHub fork: fix/restart-recovery-overload-24059
opened6d agobyZZheruel
13 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Limit automatic restart recovery to one active continuation per environment, so restarting cannot immediately recreate every in-flight workload.

Admission lasts through the resulting run, with pending child recoveries before their parent. Waiting intents stay durable without spending retries; explicit starts and Stop remain available. Persist the existing three-recovery budget across unplanned restarts rather than expiring it after five minutes. Planned shutdowns and completed turns reset it; legacy intents retain their old guard.

Child-result notifications cannot wake an unrecovered cancelled parent. Repeated held deliveries are acknowledged without creating runs or retry loops. A user message or explicit MCP send re-offers saved results.

Verification at e7044817366c on macOS:

  • Regressions reproduced the targeted failures against upstream 66ee628e0875 and the original PR head 7e3babbe997f.
  • vp run check:changed passed formatting, lint, knip, generated-code checks, affected typechecks, and 165 tests. Recorded-provider recovery suites passed 26 tests; filtered Stop/resume/restart coverage passed 23.
  • Additional Codex replay, Claude delivery, selection-restart, and process-lifecycle suites passed 57 tests.
  • Five-thread QA drove the real outbox, worker, and orchestrator with controlled provider starts: all five advanced sequentially, with waiting intents still pending at zero attempts.
  • A live Codex test used disposable T3/provider homes and an empty workspace. Two forced server restarts resumed the gated command, preserved processLossCount: 2, and completed. Disabling recovery prevented automatic continuation; manual resume and Stop worked.
Isolated runtime evidence

The controlled five-thread fixture was removed after capture. Live QA used Node 24.17.0, Codex CLI 0.164.0, and gpt-6.1-sol; only captured disposable server PIDs received signals.

text
1controlled: providerStarts=1 outstanding=1 waiting=4 attempts=[0,0,0,0]
2controlled: providerStarts=5 outstanding=1 waiting=0
3controlled: complete=5 unexpectedStarts=0
4
5live enabled: two SIGKILL/restarts → 3 runs → completed
6live persisted recovery count: 2
7live disabled: cancelled source, automaticContinuations=0
8live manual resume + Stop: interrupted
9live gated-command connections closed after crashes and Stop

All disposable servers stopped. No live T3 state was modified.

Host overload, simulator workloads, and orphaned xcodebuild behavior were not reproduced. The reporter’s exact parent-wake cause remains unverified. This bounds automatic recovery, not ordinary task creation or detached subprocesses. Hosted full CI remains unverified.

Important

One automatic recovery at a time means a long or waiting continuation postpones other automatic recoveries. Maintainer triage and approval of this policy remain outstanding under CONTRIBUTING.md.

Addresses #24059.

Implemented, tested, and reviewed by GPT-6.1-Sol through the Codex harness in T3 Code.

Checks

5 successful · 1 skipped
All checks passed
e704481

Added 2 commits

  • fix(server): bound automatic restart recovery7e3babb
  • fix(server): keep held recovery notifications resumablee704481
CcoderabbitaiBot6d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 66ee628e0875b2b735b199f42e6151e347833637 and e7044817366c2611daa64471bd46ccdbdb9c219f.

📒 Files selected for processing: 13
  • apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts
  • apps/server/src/orchestration-v2/EffectOutbox.ts
  • apps/server/src/orchestration-v2/EffectWorker.test.ts
  • apps/server/src/orchestration-v2/EffectWorker.ts
  • apps/server/src/orchestration-v2/FoundationPersistence.test.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts
  • apps/server/src/orchestration-v2/RestartContinuation.test.ts
  • apps/server/src/orchestration-v2/RestartContinuation.ts
  • apps/server/src/persistence/Migrations.ts
  • apps/server/src/persistence/Migrations/069_RestartRecoveryIndex.ts
  • docs/internals/server-updates.md
  • docs/user/updating.md

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

📝 Walkthrough

Restart recovery now tracks process-loss counts and uses them to control continuation limits. The outbox coordinates recovery admission, child-before-parent ordering, and effect retention. Delegated-completion deliveries remain available while a cancelled parent needs a resume.

Changes: Restart recovery and delegated delivery

Layer / File(s)Summary
Process-loss count and continuation limit
apps/server/src/orchestration-v2/EffectOutbox.ts, apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts, apps/server/src/orchestration-v2/RestartContinuation.ts, apps/server/src/orchestration-v2/EffectWorker.ts, apps/server/src/orchestration-v2/*test.ts, docs/internals/server-updates.md, docs/user/updating.md		Continuation effects carry processLossCount through recovery and execution. Recovery increments the count for unfinished runs, uses it for the continuation limit, and sets it to zero for planned shutdowns. Tests cover the count and limit behavior. The documentation describes the recovery limit and resets.
Recovery admission, ordering, and retention
apps/server/src/orchestration-v2/EffectOutbox.ts, apps/server/src/orchestration-v2/FoundationPersistence.test.ts, apps/server/src/persistence/Migrations.ts, apps/server/src/persistence/Migrations/069_RestartRecoveryIndex.ts, docs/internals/server-updates.md		The outbox limits active continuation recovery, claims child continuation work before parent work, and retains effects referenced by active continuation runs. Tests cover admission, ordering, and pruning. Migration 69 adds a partial index for active restart-continuation runs.
Delegated-completion delivery hold
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts, docs/internals/server-updates.md		Delivery offering and queued-wake dispatch check whether a cancelled parent still needs a resume. User messages, MCP messages, and restart continuations can re-offer the saved delivery. Tests cover repeated wake dispatch and resumption actions.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant ProviderRuntimeRecoveryService
3 participant EffectOutbox
4 participant EffectWorker
5 participant RestartContinuation
6 ProviderRuntimeRecoveryService->>EffectOutbox: enqueue continuation with processLossCount
7 EffectOutbox->>EffectWorker: provide eligible continuation
8 EffectWorker->>RestartContinuation: call continueRestartedRun with processLossCount

Possibly related issues

  • Issue 23871 — The change re-offers saved delegated results after a user message or explicit MCP send.
  • Issue 24059 — The change limits active recovery and carries process-loss counts across restarts.

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to e7044

No unresolved issue identified here prevents merging after normal checks.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description provides detailed problem, change, verification, limitations, and issue context, but it does not use the required section headings. The required scope and approval evidence is incomple…Restructure the description under ## Problem, ## Change, ## Scope and approval, and ## Verification. Add the triaged issue or approval discussion link and the explicit maintainer approval comment, or explain why this focused fix qualifies f…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the primary change: bounding automatic server restart recovery.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description provides detailed problem, change, verification, limitations, and issue context, but it does not use the required section headings. The required scope and approval evidence is incomplete: it references #24059 but states that maintainer triage and approval remain outstanding.

Resolution

Restructure the description under ## Problem, ## Change, ## Scope and approval, and ## Verification. Add the triaged issue or approval discussion link and the explicit maintainer approval comment, or explain why this focused fix qualifies for an exemption. Retain the reported verification results and limitations under ## Verification.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

ZZheruel6d ago
Synced from GitHub

Re the CodeRabbit description-check warning: this is correct. Maintainer triage and approval of the recovery policy remain outstanding; no exemption or maintainer approval is claimed. The PR description explicitly records that requirement. There is no approval comment available to link yet.

Mmaria2d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks @Zheruel. Your persisted process-loss count, planned-shutdown reset, worker forwarding, and protection against outbox pruning address the five-minute recovery loophole.

I rebuilt this as #25602 because this fork has maintainer edits disabled. It keeps those pieces and the three-recovery limit, with live Codex verification of four forced restarts, manual resume, and Stop. Blacksmith check:changed passed, including 125 tests.

I compared your complete diff against the replacement. Environment-wide admission, child-first scheduling, parent-wake suppression, their migration, and their tests/docs were dropped to keep the fix focused on the existing budget. The remaining retention lookup uses the existing thread/status index and also covers queued/preparing continuations.

@Zheruel, parent completion wakes and crashes before a successor run commits remain separate gaps; this does not claim to bound those paths or orphaned subprocesses. Your original PR remains open.