Repository navigation
Stop repeated recovery of finished workers - #25679
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDispatch completion now settles eligible workers as abandoned at the assignment-settled stage, and database initialization repairs historical settled assignments. Legacy recovery uses execution-host inventory and terminal-absence evidence to classify missing-workspace workers, persists the observed host, and defers unverifiable workers. Automatic retries are scoped to deferred dispatches and avoid empty persistence batches. The change also adds tests, reliability-gate details, and test-harness updates. Priority: ⬆️ High Merge Risk: 🟡 Moderate · up to A live SSH worker could be treated as exited, while workers with deleted workspaces may never recover automatically. Resolve both paths before merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 4 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 25 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
dfad577e-79d1-4436-bb55-1b69897b932d
📒 Files selected for processing (17)
config/reliability-gates.jsoncsrc/main/runtime/orca-runtime-test-scenario-builders.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-05.spec.tssrc/main/runtime/orchestration-message-delivery-identity.test.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-completion.tssrc/main/runtime/orchestration/db/orchestration-db.tssrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-settlement.tssrc/main/runtime/orchestration/worker-dispatch-settlement.test.tssrc/main/runtime/runtime-legacy-worker-missing-workspace-recovery.test.tssrc/main/runtime/runtime-legacy-worker-recovery-test-fixture.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-controller.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-runner.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| reconcile( | ||
| options: LegacyWorkerRecoveryOptions = {} | ||
| ): Promise<LegacyWorkerTerminalRecoveryResult> { | ||
| if (!options.retry) { | ||
| this.cancelScope(options.connectionId ? `ssh:${options.connectionId}` : 'local') | ||
| } | ||
| let resolveResult!: (result: LegacyWorkerTerminalRecoveryResult) => void | ||
| let rejectResult!: (error: unknown) => void | ||
| const result = new Promise<LegacyWorkerTerminalRecoveryResult>((resolve, reject) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,190p' src/main/runtime/runtime-legacy-worker-terminal-recovery-controller.tsRepository: stablyai/orca
Length of output: 5722
🏁 Script executed:
printf '%s\n' '--- runner ---'
sed -n '1,260p' src/main/runtime/runtime-legacy-worker-terminal-recovery-runner.ts
printf '%s\n' '--- types ---'
sed -n '1,240p' src/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts
printf '%s\n' '--- bindings and usages ---'
rg -n -F -- 'new RuntimeLegacyWorkerTerminalRecoveryController' src
rg -n -F -- 'updateRetry(' src/main
rg -n -F -- 'ports.reconcile' src/main/runtime
printf '%s\n' '--- relevant test files ---'
rg -l -F -- 'RuntimeLegacyWorkerTerminalRecoveryController' src test tests 2>/dev/null || true
printf '%s\n' '--- changed hunk versus PR base ---'
git diff --unified=35 d518d4a5466ed5355b1ea3dcce81044f013fd0e3 4a9bf26fd5b930f82b8865ec4e236e018ebc8327 -- src/main/runtime/runtime-legacy-worker-terminal-recovery-controller.tsRepository: stablyai/orca
Length of output: 18105
🏁 Script executed:
printf '%s\n' '--- production controller construction ---'
sed -n '120,225p' src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
printf '%s\n' '--- explicit reconcile callers ---'
rg -n -F -- '.legacyWorkerRecovery.reconcile(' src/main
rg -n -F -- 'legacyWorkerRecovery.reconcile(' src/main
printf '%s\n' '--- retry tests ---'
sed -n '1,280p' src/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.ts
printf '%s\n' '--- runner and retry changed status ---'
git diff --unified=8 d518d4a5466ed5355b1ea3dcce81044f013fd0e3 4a9bf26fd5b930f82b8865ec4e236e018ebc8327 -- src/main/runtime/runtime-legacy-worker-terminal-recovery-controller.ts src/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.tsRepository: stablyai/orca
Length of output: 20716
🏁 Script executed:
printf '%s\n' '--- production controller construction ---'
sed -n '120,225p' src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
printf '%s\n' '--- explicit reconcile callers ---'
rg -n -F -- '.legacyWorkerRecovery.reconcile(' src/main
rg -n -F -- 'legacyWorkerRecovery.reconcile(' src/main
printf '%s\n' '--- retry tests ---'
sed -n '1,280p' src/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.ts
printf '%s\n' '--- controller retry diff ---'
git diff --unified=8 d518d4a5466ed5355b1ea3dcce81044f013fd0e3 4a9bf26fd5b930f82b8865ec4e236e018ebc8327 -- src/main/runtime/runtime-legacy-worker-terminal-recovery-controller.ts src/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.tsRepository: stablyai/orca
Length of output: 20706
🏁 Script executed:
printf '%s\n' '--- bound runtime methods ---'
rg -n -F -- 'reconcileLegacyWorkerTerminals' src/main/runtime
rg -n -F -- 'updateLegacyWorkerTerminalRecoveryRetry' src/main/runtime
printf '%s\n' '--- automation operation ---'
sed -n '175,215p' src/main/runtime/orca-runtime-automation-operations.ts
printf '%s\n' '--- method definitions and owners ---'
rg -n -C 12 -- 'reconcileLegacyWorkerTerminals\s*\(|updateLegacyWorkerTerminalRecoveryRetry\s*\(' src/main/runtimeRepository: stablyai/orca
Length of output: 42194
🏁 Script executed:
nl -ba src/main/runtime/runtime-legacy-worker-terminal-recovery-controller.ts | sed -n '32,112p'
printf '%s\n' '--- runner update ---'
nl -ba src/main/runtime/runtime-legacy-worker-terminal-recovery-runner.ts | sed -n '142,156p'
printf '%s\n' '--- retry contract ---'
nl -ba src/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts | sed -n '17,32p'
printf '%s\n' '--- production bindings and explicit call ---'
nl -ba src/main/runtime/orca-runtime-preserved-branch-cleanup.ts | sed -n '180,189p'
nl -ba src/main/runtime/orca-runtime-automation-operations.ts | sed -n '193,208p'
printf '%s\n' '--- retry test coverage block ---'
nl -ba src/main/runtime/runtime-legacy-worker-terminal-recovery-retry.test.ts | sed -n '89,166p'Repository: stablyai/orca
Length of output: 9510
Reset the scope when the queued explicit pass starts.
When an explicit request arrives during a timer pass, cancelScope() deletes the retry record but does not stop the running pass. If both passes defer a worker, the timer pass recreates the record and arms a retry before the explicit pass runs. The explicit pass then reuses that record, so its fresh six-retry budget can start with one retry already counted. Cancel the scope again when the queued explicit pass starts.
Suggested fix
const run = this.queue.then(async () => {
try {
+ if (!options.retry) {
+ this.cancelScope(options.connectionId ? `ssh:${options.connectionId}` : 'local')
+ }
resolveResult(await runLegacyWorkerTerminalRecovery(this, this.ports, options))![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Assignment settlement at its source — new
worker-dispatch-settlement.tssettles an active worker (starting/ready/start_unknown/stopping/stop_unknown) toabandonedwithstage: 'assignment_settled'when its dispatch iscompleted/failed/circuit_broken, called fromcompleteDispatchandsettleActiveDispatchesForTask, plus a one-shotreconcileSettledWorkerDispatchesrepair at DB open. Preserveslast_error, does not kill a terminal or claim process exit. - Bounded recovery retries —
MAX_LEGACY_WORKER_RECOVERY_RETRIES = 6with capped exponential backoff and anexhaustedflag; a non-retry (startup/reconnect/explicit) reconcile cancels the scope and resets the budget, while the internal timer passesretry: true. Exhaustion leaves unverifiable workers unchanged and logs once per host. - Deleted-workspace and host-routed recovery — the runner now defers foreign/runtime/wrong-connection/unrecoverable-local PTYs up front, turns
selector_not_foundinto a provider keyed by the PTY's own host withworkspace: null, gates oninventory.queriedHostIds.has(hostId), and retires a deleted workspace only on!allLivePtyIds.has(ptyId)or a differing terminal identity. Resolutions carry an optionalhostId. - Durable-write budget — persistence snapshots only hosts whose session actually changed, prefers the owning-provider
hostIdover stale workspace metadata, and flushes whenoriginals.size > 0 || adoptedso unchanged adopted surfaces stay durable without extra rollback copies.
The added tests are meaningful rather than tautological: the settlement suite injects trigger failures to prove whole-transaction rollback and reopens the DB to prove historical repair, the missing-workspace suite exercises the 100-candidate single-inventory budget and the live/unverified/different-incarnation deferrals, and the retry suite advances fake time to pin exactly six retries, budget reset on explicit recovery, and per-host cancellation. I traced the queriedHostIds/allLivePtyIds absence proof against retireOrchestrationAuthorityAbsentFromInventory and the PtyControllerInventory contract comment, and the stopping/stop_unknown → abandoned settlement against the guards in updateTaskStatus, settleWorkerReportInTransaction, and decision-gate-store; the latter cannot reach an active worker through any production caller.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
4a9bf26 to
206248d
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review at 4a9bf26.
- Queued explicit recovery now resets the retry scope —
reconcileruns a secondcancelScopeinside the queued run for non-retry passes, so a timer pass that re-arms after the caller's synchronous cancel cannot survive once the explicit pass actually starts. - Regression test for the queued-reset race — advances fake timers through a hung timer pass, then asserts the re-armed timer was cancelled (
vi.getTimerCount() === 0) and the explicit pass gets a fresh six-retry budget. - Inherited journal fixture repaired —
journal-submission-positions.test.tsnow builds its Codex provider handle withcodexProviderHandle('thread-1')instead of a stale literal, the same fixture repair as#25706.
I traced the race the new commit closes: a timer pass already in flight can call updateRetry after the caller's synchronous cancelScope and re-arm a timer, so the in-queue cancel is genuinely needed, and it does not affect the host-isolation or exhaustion paths. The new test fails without it.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
206248d to
f9edb76
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Keep plain-SSH inventory misses from settling… · orca-runtime-preserved-branch-cleanup.ts:185
src/main/runtime/orca-runtime-preserved-branch-cleanup.ts:185
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep plain-SSH inventory misses from settling prior-relay workers.
When a persisted SSH worker’s workspace resolves while its target is in plain-SSH fallback,
SshPlainShellPtyProvider.probePtyLiveness()returnsfalsefor a prior-relay PTY ID that is not in the new provider’sshellsorlostsets. The new binding turns that result into proven absence. Recovery can then persistexited, retire the saved terminal surface, and reconcile a worker whose prior-relay process is still live. Return unknown for IDs the fallback provider did not own; require an owning-host proof before settling the worker.
🟡 Minor · Deleted-workspace candidates bypass… · runtime-legacy-worker-terminal-recovery-runner.ts:86-95
src/main/runtime/runtime-legacy-worker-terminal-recovery-runner.ts:86-95
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeleted-workspace candidates bypass host-evidence recovery.
When an eligible candidate’s workspace is missing, production resolution propagates
Error('selector_not_found'). This catch only acceptserror.code, so the candidate can fall through todeferredDispatchIdswithout an inventory check. The scheduled retry invokes the same runner and can defer it again while the workspace remains missing.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1fbf8afa-67e2-407a-bbfd-716c4c45bf78
📒 Files selected for processing (6)
config/reliability-gates.jsoncsrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-settlement.tssrc/main/runtime/orchestration/worker-dispatch-repair-safety.test.tssrc/main/runtime/runtime-legacy-worker-missing-workspace-recovery.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-runner.tssrc/shared/startup-line-prompt-carry.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- config/reliability-gates.jsonc
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
ELI5
Finished workers could remain in recovery, causing repeated session copies and saves while the user typed. Finished assignments now leave recovery. Workers whose host becomes available later still recover automatically.
What Changed
Why
#25413 reduced copies but left stale assignments and repeated work. Fixing settlement at its source and limiting each retry to unresolved work removes that cost while preserving recovery after prolonged outages. An arbitrary retry cutoff would require user intervention, so this revision removes it. SQL read failures remain retry failures instead of silently ending recovery.
Linked Issue
Slack incident. Historical sources: #11271 and #22612. #23262 separately retired automatic JSON snapshots and is already merged.
Visual Proof
N/A — background lifecycle and recovery changes; no UI layout change. Hidden Electron restart journeys check rendered worker tabs, retained output, the original process identity, and absence of duplicate launch or interruption.
Testing
Reproduce the worker checks using the commands in
orchestration.worker-recovery-convergenceinconfig/reliability-gates.jsonc. Run the compatibility suite withORCA_BACKGROUND_LAUNCH=1 pnpm test tests/e2e/cross-version-wire --fileParallelism=false --maxWorkers=1.Review
Claude Opus 5.5 reviewed the full PR through three adversarial rounds; the final review is CLEAN for P0/P1 on
9875cf10d104c1ea6b41bf40b076cd6ca62fe37f. It independently passed 124 targeted tests with the repository configuration. Review identified a bounded P2 delay in requested terminal closes; the final change restores their existing retry cadence with an indexed, bounded backlog probe. The final review rechecked full-backlog preservation, release intent, archive protection, host ownership and retry cancellation. The inherited Windows prompt fixture was also aligned with main to remove a duplicate block in the combined merge.Covered transaction rollback, genuine report outcomes, terminal custody, incarnation replacement, host isolation, uncertain SSH evidence, failed durable writes, queued recovery, late availability and cancellation. SQL tests check the actual query plan and stable parameterization across batch sizes. The foreground regression fails with five probes before the correction, then passes with zero recovery probes and one explicit status read. The scoped-query and late-recovery tests also fail with their production corrections reverted. Reverting the worker-first startup query and per-pass workspace reuse causes four failures in query-plan and lookup-count tests. Reverting the later active-worker index and read-only preflight causes two failures: query-plan coverage and a healthy repair under another connection’s writer lock. Old-writer reintroduction and upgrade-index rebuilding are covered.
Rebased onto main's released-protocol correction. Aligned the duplicate-import cleanup with main so the combined merge retains the provider import; fixed the over-budget prompt fixture; retained the released JSON parser as a test-only dependency so historical compatibility sources remain executable. One additive partial index; no table-column, schema-version, execution-command or wire payload changes.
Agent skill upstream boundary
Notes
The reliability gate remains experimental. Native macOS/local/daemon restart behavior and deterministic SSH contracts are covered. Native Linux/Windows desktop and WSL worker journeys, active-assignment SSH absence/deleted-folder recovery, and paired-runtime journeys remain gaps. Historical repair with a live SSH terminal is exercised through restart and reconnect. Completed historical assignments no longer re-adopt an unpersisted terminal surface solely from stale worker bookkeeping; the process and terminal custody are preserved. Recovery inventories omit foreground-agent probes, so badge/title refresh can wait for the next normal inventory. The pre-existing failed-database-open handle cleanup limitation is outside this change. Remaining costs: a one-time index build and repair for older databases, minor index maintenance on worker-state changes, automatic metadata retries for unresolved workers, and delayed cleanup until the owning host can verify process exit. Healthy repair checks inspect only the active-worker index and do not take a writer lock. SSH relay readiness already triggers immediate recovery; the timer remains a fallback. No retry cutoff or age-based discard is introduced. The original customer's typing latency and CI p95 have not been measured; deterministic counts and the synthetic scale measurement cover the identified work.
Checklist