Repository navigation
fix(scheduler): stop auto-resume from wiping resume_attempts counter - #1187
Conversation
Root cause: `defaultResumeAgent` in scheduler-daemon.ts shelled out to
`genie agent resume <id>`, which invoked `resumeAgent` in agents.ts and
unconditionally reset `resumeAttempts` to 0. The scheduler's pre-spawn
increment (0→1) was wiped one tick later by the CLI's unconditional
reset, so the counter was stuck at 0 forever and the exhaustion check
at scheduler-daemon.ts:873 (`attempts >= maxAttempts`) was unreachable.
Dead agents were retried every ~60s with no circuit breaker; `genie ls`
showed `error (0/3 resumes)` while the scheduler log showed
`agent_resume_attempted resume_attempts: 1` every minute.
Fix (Option A — parameterize the reset):
- `resumeAgent(agent, opts)` in agents.ts: new `opts.resetAttempts`,
default true. Manual CLI UX (`genie agent resume <id>`) is unchanged
— the operator still gets a fresh retry budget.
- `handleWorkerResume` threads a `noResetAttempts` option through to
`resumeAgent` / `resumeAllAgents`.
- `genie agent resume` commander: new `--no-reset-attempts` flag.
- `defaultResumeAgent` in scheduler-daemon.ts: shells out with
`--no-reset-attempts`. The scheduler now owns the counter end-to-end.
- `attemptAgentResume` success path: explicit reset to 0 so a healthy
resumed agent doesn't carry stale "2/3" state into the next failure.
Regression tests (4 new in scheduler-daemon.test.ts):
- failed attempts accumulate the counter (pre-fix stuck at 0)
- exhaustion check fires on re-entry with attempts=maxAttempts
- success path explicitly resets counter to 0
- scheduler-owned counter invariant: increment not wiped by boundary
Updated 2 pre-existing tests to reflect the new two-write contract
(pre-spawn increment + post-success reset).
Reference: PR #1181 fixed the concurrency-cap inflation that was also
masking this; this PR fixes the counter persistence that PR #1181 did
not address.
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (3)
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.
Code Review
This pull request fixes a bug in the agent resume logic where the resumeAttempts counter was being reset prematurely, leading to infinite retry loops. The changes introduce a --no-reset-attempts flag for the CLI to allow the scheduler to manage the counter lifecycle, ensuring it increments correctly and only resets upon a successful resume. Feedback was provided regarding a potential command injection vulnerability and performance issues in src/lib/scheduler-daemon.ts caused by using execSync with template literals; switching to spawnSync with an argument array is recommended.
| execSync(`genie agent resume ${agentId} --no-reset-attempts`, { | ||
| encoding: 'utf-8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
The use of execSync with a template literal for agentId introduces a potential command injection vulnerability if agentId contains shell metacharacters (such as single quotes). Additionally, execSync blocks the Node.js event loop, which can degrade the performance and responsiveness of the scheduler daemon. Using spawnSync (or ideally spawn async) with an array of arguments is safer as it avoids shell parsing issues and is generally more robust. The suggested fix uses a dynamic import to align with repository patterns for managing module dependencies.
const { spawnSync } = await import('node:child_process');
const result = spawnSync('genie', ['agent', 'resume', agentId, '--no-reset-attempts'], {
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'pipe'],
});
if (result.status !== 0) throw new Error('Resume failed');References
- Use dynamic imports (require() or import()) to break circular dependency cycles that would otherwise occur at module load time.
… re-processing)
Two related bugs, same architectural shape — terminal-state agents that the
scheduler kept re-processing every cycle.
Bug A: Rows in `state='error', auto_resume=true, claude_session_id=null`
were dropped by the resumable filter (no session id to resume), but stayed
in `auto_resume=true` forever — misleading `genie ls` ("auto-resume: on")
and cluttering the worker list. Observed live on felipe's machine: 9 wedged
rows (8 genie-docs directory placeholders + 2 omni workers that died before
capturing a Claude session).
Fix: new `reconcileUnresumable` pass wired into `startAgentResumeTimer`
between `reconcileDeadPaneZombies` and `runAgentRecoveryPass`. Flips those
rows to `auto_resume=false` and logs `agent_marked_unresumable`.
Bug B: The `attempts >= maxAttempts` early-exit in `attemptAgentResume`
logged `agent_resume_exhausted` and returned, but did NOT persist
`auto_resume=false`. Next scheduler tick (60s later) re-entered the same
branch and re-logged the same event — the agent was never re-attempted,
yet the log stayed polluted with redundant exhaustion events forever.
Fix: both exhaustion branches (pre-attempt early-exit + post-attempt final
failure) now call `deps.updateAgent(agentId, { autoResume: false })` before
logging. The next cycle's `attemptAgentResume` short-circuits at the
`autoResume === false` check with `agent_resume_skipped` (debug level),
ending the every-60s spam.
Prior art: PR #1181 (zombie concurrency cap), PR #1187 (counter
persistence). This closes the "terminal states should be terminal" theme.
Staged follow-ups (out of scope):
- dir:-row state mutation root cause (separate /trace)
- omni session-capture gap (separate /trace)
Regression tests (4 new + 1 updated in scheduler-daemon.test.ts):
- reconcileUnresumable flips null-session error rows to auto_resume=false
- reconcileUnresumable leaves valid-session + already-disabled + healthy rows untouched
- attemptAgentResume exhaustion branch persists auto_resume=false + logs event
- Subsequent runAgentRecoveryPass tick does not re-log agent_resume_exhausted
- Existing `exhaustion check fires on re-entry` test updated to assert the new
terminal-flag write (autoResume=false, no counter mutation on early-exit)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y spam Two retry-forever spam bugs in the observability loops — both follow the pattern we've been closing all session: recoverable-looking error on a fast-polling path with no circuit breaker → same log line every 30s. Bug A — filewatch FK violation (session-filewatch.ts) Orphan subagent JSONLs (parent session not in PG sessions table, e.g. SDK agents) made ingestFileFull raise FK constraint 23503. The catch block logged but did NOT advance offsetCache, so every subsequent fs.watch event retried the same file at offset 0 → same FK error → same log. Forever. Fix: detect FK (code 23503 or "foreign key constraint" in message), pin offsetCache to Infinity AND add the session to an unrecoverable set so future events short-circuit before touching ingest. Non-FK errors (connection reset, deadlock) still preserve retry semantics. Bug B — inbox-watcher missing workingDir (inbox-watcher.ts) checkInboxes iterates teams with unread messages. If workingDir is missing from a team's config, it warned every poll cycle — same 3-4 teams re-warning every 30s forever (live-observed on felipe). Fix: mirror the existing spawnFailures rate-limit — new noWorkingDirWarned Map warns once per team, silences for 1 hour, then re-fires (so configs that get populated upstream naturally recover). Pattern continuity with #1187/#1192/#1195 — same shape, same fix class: observability path sees recoverable-looking condition → no circuit breaker → floods logs. Staged follow-up (out of scope): separate /trace needed upstream on why teams register without workingDir (team-config creation bug). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tracer summary
Scheduler's
attemptAgentResumeincrementsresumeAttempts0→1 in PG, thenshells out to
genie agent resume <id>which entersresumeAgentinagents.tsand unconditionally doesregistry.update({ resumeAttempts: 0 })—wiping the increment within the same tick. Counter was stuck at 0 forever,
which meant the exhaustion check at
scheduler-daemon.ts:873(
attempts >= maxAttempts) was unreachable. Dead zombie agents were retriedevery ~60s forever with no circuit breaker;
genie lsshowederror (0/3 resumes)while the scheduler log showedagent_resume_attempted resume_attempts: 1every minute.The line 2376 reset was designed for manual
genie agent resumeinvocations — a fresh retry budget on human intervention — but the scheduler
reused the same CLI entrypoint, so the manual-only reset fired on auto-resume
too.
Before / after behavior contract
attempts = 1attempts = 1genie resumeattempts = 0--no-reset-attemptskeepsattempts = 1attempts = 0(stuck)attempts = 1(persists)attempts = 0(stuck; retry forever)attempts = 3→'exhausted'→ circuit breakerattempts = 0(CLI reset)attempts = 0(explicit reset on success path)genie agent resume <id>attempts = 0(CLI reset)attempts = 0(CLI reset — unchanged UX)Changes
src/term-commands/agents.ts—resumeAgentandhandleWorkerResume/resumeAllAgentsaccept anopts.resetAttemptsflag, defaulttrue. Allexisting manual-path callers get the original fresh-retry-budget behavior.
src/term-commands/agent/resume.ts— new--no-reset-attemptscommanderoption plumbed through to
handleWorkerResume.src/lib/scheduler-daemon.tsdefaultResumeAgentshells out with--no-reset-attempts.attemptAgentResumesuccess path: explicitupdateAgent(id, { resumeAttempts: 0 })so a healthy resumed agent doesn'tcarry stale
2/3state.Tests
Existing tests mocked
resumeAgent: async () => trueso they never exercisedthe reset path. Four new regression tests in
src/lib/__tests__/scheduler-daemon.test.ts:failed attempts accumulate the counter (pre-fix stuck at 0)— 3consecutive failed auto-resumes, counter progression 0→1→2→3→exhausted.
exhaustion check fires on re-entry with attempts=maxAttempts— thepreviously-unreachable early-exit branch is now reachable.
success path explicitly resets counter to 0— verifies the newtwo-write contract (pre-spawn increment + post-success reset).
scheduler-owned counter: increment not wiped by resumeAgent boundary—direct invariant regression test for the original bug.
Two pre-existing tests updated to reflect the new two-write contract on the
success path (
agentUpdatesgoes from length 1 to length 2).Validation
Scope
Does not touch migrations,
buildWorkerStatusMap, or the ls displayrendering — those were correct; only the PG column value was lying.
Related
fix(scheduler): exclude NULL-state + dead-pane zombies from resume concurrency cap) is merged and addresses a different symptom (the142-agent concurrency-cap inflation that was silently blocking every
auto-resume attempt). This PR fixes the counter-persistence bug that fix(scheduler): exclude NULL-state + dead-pane zombies from resume concurrency cap #1181
did not address.
Test plan
bun run checkpasses@next, watch an error-state agent'scounter tick 1/3 → 2/3 → 3/3, confirm
agent_resume_exhaustedin~/.genie/logs/scheduler.logafter the third attempt.🤖 Generated with Claude Code