Repository navigation
fix(scheduler): harden recoverOnStartup against per-worker failures - #1163
Conversation
release: cross-team reachback drift fix
`recoverOnStartup` iterated resumable workers and called
`deps.isPaneAlive(worker.paneId)` + `attemptAgentResume` without any
per-worker fault isolation. A single throw — most commonly "error
connecting to /tmp/tmux-UID/<socket>" when the tmux socket is not yet
reachable at daemon boot — aborted the for-loop, and the outer
try/catch in `startDaemon` logged `recovery_error` and moved on.
Observed symptom on a real server reboot:
14:44:09.439 recovery_started
14:44:09.487 recovery_error error="... display-message -t '%5' ...
error connecting to /tmp/tmux-1000/genie
(No such file or directory)"
(session 'genie' was created 30s later at 14:44:39)
Every worker stayed stale with resume_attempts=0 forever — there was no
retry path, and auto-resume is the only code path that would have
revived them.
Fix:
1. Extract the worker-recovery loop into `runAgentRecoveryPass` with a
per-worker try/catch. One worker's failure no longer poisons the
rest. Each per-worker failure emits a structured
`recovery_worker_failed` log line carrying the worker id, daemon
id, and error message.
2. After the initial pass, if any worker failed, schedule a one-shot
retry 60 s later via `setTimeout(...).unref()`. This covers the
boot race where tmux is not yet listening when the daemon starts.
The retry uses the same fault-isolated pass and emits
`recovery_retry_scheduled` / `recovery_retry_completed` /
`recovery_retry_error` events.
3. Surface `failed_agents` in the existing `recovery_completed` log
line so the outcome of the initial pass is observable without
having to correlate scattered events.
Tests: 3 new cases in scheduler-daemon.test.ts covering (a) one worker
throwing does not stop recovery for the next, (b) a retry is scheduled
when any initial failure occurs, and (c) no retry is scheduled when
every worker succeeds. Full file: 62/62 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bdfa1dc2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| setTimeout(async () => { | ||
| try { | ||
| const retry = await runAgentRecoveryPass(deps, daemonId, config); |
There was a problem hiding this comment.
Cancel startup recovery retry when daemon stops
scheduleRecoveryRetry creates a one-shot setTimeout that always runs runAgentRecoveryPass after 60s, but this timer is not tied to daemon lifecycle (no running check and no handle returned for stop() to clear). In startDaemon, stop() clears other timers but cannot cancel this one, so stopping or restarting the daemon within that 60s window still lets the old instance run recovery and potentially resume agents out-of-band under a stale daemon_id, causing duplicate or conflicting recovery actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request enhances the startup recovery process by implementing per-worker fault isolation and a one-shot retry mechanism for failed agent recoveries. These changes ensure that a single failure, such as an unreachable tmux socket, does not abort the entire recovery loop. A review comment identifies a potential issue where the retry timer is not tracked or cleared, which could lead to background tasks executing after the daemon has been shut down.
| error: message, | ||
| }); | ||
| } | ||
| }, RECOVERY_RETRY_DELAY_MS).unref?.(); |
There was a problem hiding this comment.
The retry timer scheduled here is not tracked or cleared when the daemon stops. If startDaemon is stopped shortly after boot, this background task will still execute, potentially attempting to interact with a closed database connection or spawning processes after the daemon has gracefully shut down. Consider returning the timer handle from recoverOnStartup so it can be cleared in the stop() function of startDaemon.
Summary
recoverOnStartupiterates resumable workers and callsdeps.isPaneAlive(worker.paneId)+attemptAgentResumewithout per-worker fault isolation. A single throw — most commonly a tmux socket that is not yet reachable when the daemon boots after a server reboot — aborts the for-loop, the outer try/catch instartDaemonlogsrecovery_error, and every subsequent worker stays stranded withresume_attempts=0forever. Auto-resume is the only code path that would have revived them.Smoking gun from a real server reboot
30-second startup race between
genie serveand the tmux session. The firstisPaneAlivecall threw, aborted recovery for all 9 resumable workers. No retry mechanism existed.Fix
runAgentRecoveryPasswith a per-worker try/catch. One worker's failure no longer poisons the rest. Each failure emits a structuredrecovery_worker_failedlog line with worker id, daemon id, and error message.setTimeout(...).unref(). Covers the boot race where tmux is not yet listening. Retry uses the same fault-isolated pass and emitsrecovery_retry_scheduled/recovery_retry_completed/recovery_retry_errorevents.failed_agentsto the existingrecovery_completedlog so the result of the initial pass is observable in one line.Tests
Three new cases in
scheduler-daemon.test.ts:isPaneAlivethrowing does not stop recovery for the nextFull file: 62/62 pass. Pre-push suite: 2479/2479 pass.
Test plan
devtarget passesrecovery_worker_failed+recovery_retry_scheduledevents in~/.genie/logs/scheduler.log🤖 Generated with Claude Code