feat: shell restart loop + connection-aware disconnect handling

[dcetlin]

Summary

Two changes that address CC session death during protocol runs.

Root cause finding: CC self-exits ([uds-messaging] Shutting down), but the cascading session replacements were caused by the daemon killing live tmux sessions on bridge disconnect — not by CC's process dying. The daemon's 2s disconnect timer fired before CC could reconnect, and resumeParticipant killed the live session to spawn a replacement. Each replacement entered the same cycle.

1. Shell-level restart loop

Non-fork spawns wrap the claude command in a while true loop inside the tmux shell. On any exit, the loop writes an exit marker (with self_exit detection via debug log grep), logs the restart with timestamp/exit code/debug path, sleeps 2s, and restarts CC with --resume. The tmux session stays alive through CC restarts. Debug logs rotate per restart (-r1.log, -r2.log, ...). Only tmux kill-session (SIGHUP) kills the shell.

2. Connection-aware disconnect handling

onRunDisconnect now checks tmuxHasSession before acting. If the tmux session is alive (CC or the restart loop is running), the daemon logs and waits (15s window) instead of killing the session. Only resumes if tmux is actually dead.

3. Keepalive disabled by default

keepaliveEnabled() returns process.env.HYDRA_KEEPALIVE === '1' (was !== '0'). Late-bound per repo convention. Keepalive was identified as an accelerant for bridge disconnects — 120 messages/hour gave the connection 120 chances to drop.

Other

• _resumeAttempts keyed by role (survives session replacement), not reset on success
• killSession:269 — execSync → execFileSync
• --disallowedTools/--tools carried in restart resume command
• Cross-file invariant comment restored in session-health.ts
• buildExitMarkerScript extracted for shared use

Test plan

• 909 tests pass, all 3 compiles green
• Run review, observe critic surviving bridge disconnect via tmux-alive check
• Verify no cascade on bridge disconnect (daemon waits instead of killing)
• Verify shell restart loop fires on actual CC self-exit (restart log written)

🤖 Generated with Claude Code

[sf8193]

Should-fix (flagged by both reviewers): FD leak — if fstatSync(fd) or readSync() throws after openSync() succeeds, the catch block returns false without calling closeSync(fd), leaking the file descriptor.

Wrap in try/finally:

const fd = openSync(debugLogPath, 'r')
try {
  const stat = fstatSync(fd)
  const readFrom = Math.max(0, stat.size - 8192)
  const buf = Buffer.alloc(Math.min(8192, stat.size))
  readSync(fd, buf, 0, buf.length, readFrom)
  return buf.toString().includes(CC_SHUTDOWN_MARKER)
} finally {
  closeSync(fd)
}

[dcetlin]

Fixed in 2a2e704 — try/finally wrapping, FD always closed.

[sf8193]

Should-fix (flagged by both reviewers): Three issues on this line:

1. shq duplication — a second copy is added at line 9, identical to the one in session-lifecycle.ts. Will rot independently.
2. Shell-string execSync — every other tmux call in the codebase uses execFileSync (array form, no shell). Using the shell form here is inconsistent and requires the shq import.
3. Missing timeout — no timeout option. If the tmux server is hung, this blocks the health poll indefinitely (5-min setInterval callback never returns). Every other tmux call in this PR uses timeout: 2000.

All three are fixed by switching to:

execFileSync('tmux', ['kill-session', '-t', info.tmuxName], { stdio: 'pipe', timeout: 2000 })

Then drop the execSync import and the shq definition.

[dcetlin]

Fixed in 2a2e704 — replaced execSync+shq with execFileSync array form, added 5s timeout.

[sf8193]

Nit: Semantic shift worth a comment — with remain-on-exit on, a self-exited session now has tmuxHasSession=true but sessionProcessAlive=false. This branch requires sessionProcessAlive=true, so dead-pane sessions skip orphan detection entirely. They should be caught by the crash branch on the next poll (once bridge disconnects too), but there's a brief window where neither branch fires if the bridge hasn't disconnected yet. If this is intentional, a comment here would help future readers.

[dcetlin]

Fixed in 2a2e704 — updated comment to explain remain-on-exit semantics.

[sf8193]

Blocker (flagged by both reviewers): respawnsInProgress is leaked when waitForBridge returns false or the run becomes terminal during the 60s wait.

respawnsInProgress.delete() only fires here on the success path (line 380). When the if (ok && !isTerminal(run)) check fails, execution falls through to the resume/grace fallback below without ever removing the session ID from respawnsInProgress. This permanently suppresses crash detection in session-health.ts for that session (the !respawnsInProgress.has(...) guard).

Move the delete above the if (ok ...) check, or add it to the fallthrough path as well:

respawnsInProgress.delete(currentInfo.sessionId)
if (ok && !isTerminal(run)) {
  // success path...
  return
}
// fallthrough to resume/grace

This is already correct in the diff — the delete just needs to be unconditional after waitForBridge returns.

[dcetlin]

The delete is at line 380, BEFORE the if(ok) check — it runs on both success and failure paths. Not leaked. But I added the other fixes in this push.

[sf8193]

Should-fix: process.env.SPAWN_CWD is a process-global env var. Sessions spawned with --worktree have a different effective CWD (computed by resolveForkSpawnCwd in the original spawn path). Respawning a worktree session will cd into the wrong directory, breaking its repo context.

Consider persisting the resolved CWD on SessionInfo at spawn time and reading it here instead.

[dcetlin]

Fixed in b755d36 — uses capabilities.cwd with SPAWN_CWD fallback.

[sf8193]

Should-fix: info.debugLogPath is mutated here before the execFileSync call at line 1131. If execFileSync throws, debugLogPath now points to a file that was never created. The next isCCSelfExit call will try to open a nonexistent debug log, fall through to false (safe), but the old debug log with the real shutdown marker is now unreachable — so the retry at line 374 in protocol-runner.ts will also return false, preventing a legitimate respawn on the second attempt.

Defer the mutation until after the exec succeeds (alongside the inPlaceRespawnCount assignment at line 1139).

[dcetlin]

Fixed in b755d36 — debugLogPath mutation deferred until after successful execFileSync.

[sf8193]

Nit: The exit-trap grep runs immediately after CC exits, but the debug log may not be fully flushed to disk. If the OS buffer hasn't synced, grep misses the marker → self_exit=false in the exit file → isCCSelfExit short-circuits at self_exit=false and never falls back to the debug log. The window is small but real.

A sync before the grep, or just removing the grep from the trap and relying solely on the Node-side isCCSelfExit debug log fallback, would close it.

[dcetlin]

Fixed in b755d36 — added sync before grep in EXIT trap.

[sf8193]

Nit: .toString() is redundant when encoding: 'utf8' is set — execFileSync already returns a string.

[dcetlin]

Fixed in b755d36 — removed redundant .toString().

[sf8193]

Nit: getContextPercent(name) calls tmux capture-pane on a dead pane (remain-on-exit keeps the pane, but CC's status bar is gone). In practice this returns '?', so parseInt('?') >= 90 evaluates to NaN >= 90 → false, and the guard always passes. The context-at-90 check is dead code in the respawn path — respawn will never be blocked by context usage. Harmless (fails open), but misleading if someone later expects it to work.

[dcetlin]

Fixed in 33e5fc8 — removed dead context gate. getContextPercent returns '?' on dead panes, so the guard never fired.

[sf8193]

Nit: The exit file path is reused across in-place respawns (same exitFilePath on info), while the debug log is fresh per respawn. If a respawned CC is SIGKILLed (EXIT trap doesn't fire), the stale exit file from the previous self-exit still contains self_exit=true, causing the next isCCSelfExit to misclassify the kill as another self-exit and trigger another respawn. Bounded by the circuit breaker (MAX_IN_PLACE_RESPAWNS = 10), so low-impact. Could be closed by truncating the exit file at the start of respawnInPlace before respawn-pane.

[dcetlin]

Fixed in 33e5fc8 — exit file truncated before respawn so a SIGKILL doesn't inherit the previous self_exit=true.

[sf8193]

Blocker (flagged by both reviewers): respawnsInProgress leaked on stale-gen early return.

When respawned is true and a newer disconnect fires (bumping the gen counter), this line returns before respawnsInProgress.delete() on line 390. The session ID stays in respawnsInProgress permanently, which suppresses crash detection in session-health.ts (the !respawnsInProgress.has(info.sessionId) guard). The session becomes invisible to the health poller.

Same leak applies if a second respawnInPlace at line 381 succeeds but this gen check bails.

Fix: Move respawnsInProgress.delete(currentInfo.sessionId) above the gen check, or add it to every early-return path after respawnInPlace returns true.

[dcetlin]

Fixed in 958210f — moved respawnsInProgress.delete before the gen check so it always runs.

[sf8193]

Should-fix: respawnInPlace is wired as a late-bound let (matching doSpawnSession/waitForBridge/killSession), but __test.setLifecycle at line ~977 was not updated to include it. Tests exercising the new onRunDisconnect respawn path will call the real respawnInPlace (which shells out to tmux respawn-pane). Add it to the setLifecycle overrides + resetLifecycle.

[dcetlin]

Fixed in 958210f — added respawnInPlace to setLifecycle/resetLifecycle.

[sf8193]

Nit: The 1-second fixed sleep before the retry respawnInPlace assumes the EXIT trap's grep on the debug log completes within 1s. Production debug logs can be much larger than the 16KB tested — grep on a 100MB+ log could exceed this. Consider polling the exit file for self_exit with a short backoff, or document the timing assumption.

[dcetlin]

Acknowledged. The EXIT trap writes the exit marker (which isCCSelfExit reads first), so the grep timing only affects the exit marker's self_exit field. 1s is generous for the grep; debug logs >100MB would be exceptional.

[sf8193]

Should-fix: checkSessionDeath now uses sessionProcessAlive to detect death (correct), but doesn't kill the lingering tmux session afterward. With remain-on-exit on, dead-pane sessions persist as zombies until the 5-min health poll cleans them up.

session-health.ts (lines 39-41 in the diff) correctly adds tmux kill-session after crash detection — this path was missed. Add the same cleanup here:

if (tmuxHasSession(info.tmuxName)) {
  try { execFileSync('tmux', ['kill-session', '-t', info.tmuxName], { stdio: 'pipe', timeout: 5000 }) } catch {}
}

[dcetlin]

Fixed in 7e918a0 — added tmux kill-session cleanup in checkSessionDeath after crash detection.

[sf8193]

Nit: Hardcoded 5 — this was previously MAX_RESUME_ATTEMPTS from auto-resume.ts (which this PR removes the import for). Consider extracting to a named constant to preserve the intent.

[dcetlin]

Fixed in 7e918a0 — extracted MAX_RESUME_ATTEMPTS = 5 constant.

[sf8193]

Nit (flagged by both reviewers): tmux set-option remain-on-exit on runs inside the pane's && chain. If it fails, the entire spawn silently aborts (CC never starts). Risk is low since tmux is definitely available, but consider setting this as a session option on creation (separate execFileSync after new-session) for robustness.

[dcetlin]

Intentional — the && chain was specifically to fix the TOCTOU race from earlier rounds. An external set-option after new-session has a window where CC can exit before remain-on-exit is set. Inside the pane, it runs before claude starts. tmux is guaranteed available (we just created the session with it).

[sf8193]

Should-fix (flagged by both reviewers): pmReviewFailures is never cleared on build abandon or PM death.

cleanupState (line 1069) doesn't call pmReviewFailures.delete(state.pmThreadId), so the counter persists across builds in the same PM thread. If a PM abandons a build after review failures, then starts a new build, the elevated counter carries over and could trigger the "Factory degraded" warning prematurely.

Fix: add pmReviewFailures.delete(state.pmThreadId) in cleanupState.

[sf8193]

Should-fix (flagged by ts-reviewer): _disconnectGen is bumped here on each new disconnect, and the gen check inside the callback guards against stale disconnect handlers. But onRunReconnect (not modified by this PR) only clearTimeouts the timer — it does not bump _disconnectGen. If the timer already fired before the reconnect, the in-flight async callback passes the gen check at line 373 because gen was never invalidated. The transport.has() check at line 372 catches most reconnects, but there's a TOCTOU window: if the session reconnects during the await respawnInPlace() or await waitForBridge() calls, subsequent transport.has checks haven't fired yet and respawnInPlace could stomp a live session.

Fix: bump _disconnectGen in onRunReconnect so in-flight callbacks bail out.

[sf8193]

Should-fix (flagged by both reviewers): while true with no iteration limit. respawnInPlace increments inPlaceRespawnCount but never checks it against a cap. A CC that consistently self-exits after a few seconds would loop forever at the tmux level, burning API credits and producing unbounded logs.

Fix: add a max iteration guard in the shell loop (e.g., if [ $_HYDRA_RESTART_N -ge 5 ]; then break; fi), or check inPlaceRespawnCount against MAX_RESUME_ATTEMPTS in respawnInPlace.

[sf8193]

Should-fix (flagged by sp-reviewer): When bridge timeout occurs after resurrection, this logs the failure but falls through to the transport.sendOrQueue call below, which delivers a turn notification to a dead session. No fallback (cancel run, start grace timer, or attempt full resume) is taken. The resurrected tmux session sits orphaned.

Consider cancelling the run or falling back to resumeParticipant when resurrection fails.

[sf8193]

Should-fix (flagged by both reviewers): For fresh spawns where resumeCmd is undefined (no assignedClaudeSessionId, not a resume), restartClaudeCmd falls back to claudeCmd — the original spawn command. After CC self-exits, the shell loop restarts with the same command, starting a brand-new conversation instead of resuming.

The shell loop races the daemon's disconnect handler (~2s). If the loop restarts CC first, it boots fresh; the daemon sees the bridge connect and the disconnect callback returns early. The daemon's respawnInPlace correctly uses info.claudeSessionId (discovered after first boot), but the shell loop can't — the session ID wasn't known at bake time.

Consider writing the discovered claudeSessionId to a file that the shell loop reads, or disabling the shell-level loop for sessions without a known resume target.

[sf8193]

Nit (flagged by both reviewers): pmReviewFailures is cleared on successful review but never on factory_abandon. Since it's keyed by pmThreadId and PM threads are long-lived, a stale count from an abandoned build could trigger the degradation warning on a future unrelated build in the same PM thread. Minor memory leak too.

Consider clearing the entry in the abandon path.

[sf8193]

Blocker (flagged by both reviewers): Shell restart loop has no iteration cap — unbounded restarts possible.

The while true loop only breaks on _HYDRA_KILLED=true (signal receipt). If CC keeps self-exiting (e.g., a persistent error), the loop restarts indefinitely with only a 2-second sleep. The daemon-side MAX_RESUME_ATTEMPTS in protocol-runner.ts only governs the daemon's resumeParticipant path (which spawns a new tmux session) — it does not constrain this in-place shell loop. These are two independent restart mechanisms with no shared budget.

Add a MAX_RESTART guard in the shell loop itself (e.g., break after N iterations) so a crash loop doesn't burn cycles forever.

[sf8193]

Blocker: resetTimeout(run) called unconditionally before checking whether resume will succeed.

If claudeSessionId is falsy or roleAttempts >= MAX_RESUME_ATTEMPTS, execution falls through to startGraceTimer — but the phase timeout has already been reset. A participant that disconnects repeatedly without a valid claudeSessionId will keep resetting the phase timeout indefinitely, preventing the phase from ever timing out.

Move resetTimeout(run) inside the if branch that actually performs the resume.

[sf8193]

Should-fix: Deferred-kill guard can kill the restart loop's tmux session.

The new check s.createdAt >= killedAt only skips the kill if a newer registry entry owns the name. But the restart loop reuses the same tmux session name without creating a new registry entry — that's the whole point. So 3 seconds after killSession, the deferred kill finds no currentOwner with createdAt >= killedAt and kills the tmux session that now hosts the restarted CC process.

The guard needs to account for in-place restart loop sessions (e.g., check sessionProcessAlive or a sentinel).

[sf8193]

Should-fix: Keepalive default flipped from opt-out to opt-in without migration.

Changed from HYDRA_KEEPALIVE !== '0' (default ON) to HYDRA_KEEPALIVE === '1' (default OFF). This silently disables keepalive for all existing deployments that don't set this env var. The test update (beforeAll(() => { process.env.HYDRA_KEEPALIVE = '1' })) confirms the behavioral change. If this is intentional, document it; if not, use !== '0' to preserve the default.

[sf8193]

Should-fix (flagged by both reviewers): _resumeAttempts per-role counter never resets on successful resume.

If a role legitimately disconnects and resumes 3 times across different phases, it has consumed 3 of 5 attempts. A later real failure gets only 2 retries. The old flat counter had the same issue, but per-role keying makes it more visible. Consider resetting the role's counter on successful bridge reconnect (in onRunReconnect).

[sf8193]

Nit: pmReviewFailures is only cleared on successful review (onFactoryReviewComplete). If a build is abandoned (factory_abandon), the entry persists as a minor memory leak. Consider clearing in the abandon/cleanup path too.

[sf8193]

Should-fix (flagged by both reviewers): The while true shell loop restarts CC indefinitely with only a 2s sleep. The daemon's MAX_RESUME_ATTEMPTS budget only applies to resumeParticipant (pane-dead path), but with remain-on-exit on the pane stays alive — so the daemon never triggers its budget, and the shell loop spins unbounded.

If CC enters a crash loop (bad config, corrupted state), this burns resources forever. Add a max iteration guard:

if [ $_HYDRA_RESTART_N -ge 10 ]; then break; fi

[sf8193]

Should-fix: Default silently flipped from opt-out (!== '0') to opt-in (=== '1'). Any production deployment relying on keepalive being on by default will silently lose it. If intentional, call it out in release notes; if not, revert to !== '0'.

[sf8193]

Nit: Four consecutive blank lines — one suffices.

[sf8193]

Nit: execFileSync imported into protocol-runner for a single tmux kill-session call in resumeParticipant. Shelling out to tmux belongs in session-lifecycle.ts (which owns all tmux interactions). Consider extracting a lightweight kill helper there and calling it through the existing late-bound seam.

[sf8193]

Should-fix (flagged by both reviewers): Shell restart loop has no upper bound on restarts. _HYDRA_RESTART_N is tracked and logged but never checked against a max. If CC crash-loops (bad config, corrupted state), the shell restarts infinitely every 2s. The daemon-side MAX_RESUME_ATTEMPTS doesn't apply here because the bridge reconnects before the 15s disconnect timer fires.

Add a max restart guard:

if [ $_HYDRA_RESTART_N -ge 10 ]; then echo "shell-loop: ${tmuxName} restart budget exhausted" >&2; break; fi

[sf8193]

Should-fix (flagged by both reviewers): execFileSync imported at line 1 and used here — layer violation. protocol-runner.ts is a coordination layer; shelling out to tmux belongs in session-lifecycle.ts (where killSession already lives). Add a targeted helper like killTmuxSession(name) in session-lifecycle and import it here.

[sf8193]

Should-fix: info.capabilities.disallowedTools does not exist on SessionCapabilities. The type has tools: string[] but no disallowedTools field. This optional chain silently produces {}, so disallowedTools restrictions are dropped when resumeParticipant fires (e.g., a factory PM session resuming after crash would regain write tools it was denied).

Either add disallowedTools?: string[] to SessionCapabilities and persist it at spawn time, or read from the original SpawnOpts.

[sf8193]

Nit: Two blank lines added here, creating four consecutive blank lines total (813-816). Remove the extras.