feat: remain-on-exit — retain dead panes for reliable crash detection - #307
feat: remain-on-exit — retain dead panes for reliable crash detection#307dcetlin wants to merge 6 commits into
Conversation
| encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], timeout: 2000, | ||
| }).trim() | ||
| processAlive = pd === '0' | ||
| } catch {} |
There was a problem hiding this comment.
Should-fix: This inlines the same execFileSync('tmux', ['display-message', ...]) logic that sessionProcessAlive() in util.ts already encapsulates. Same duplication exists in sessions.ts:421-426. Call sessionProcessAlive(info.tmuxName) instead — if the parsing semantics ever change (e.g. tmux version differences), these copies will drift.
(flagged by both reviewers)
| // by the bridge-server disconnect handler (3s delay + tmux check). Skip sessions in spawn | ||
| // grace period (bridge needs time to connect). | ||
| if (!crashAlerted.has(info.sessionId) && info.sessionType !== 'thread_guest' && !info.deadAt && (now - info.createdAt > SPAWN_GRACE_MS) && !tmuxHasSession(info.tmuxName) && !transport.has(info.sessionId)) { | ||
| if (!crashAlerted.has(info.sessionId) && info.sessionType !== 'thread_guest' && !info.deadAt && (now - info.createdAt > SPAWN_GRACE_MS) && !sessionProcessAlive(info.tmuxName) && !transport.has(info.sessionId)) { |
There was a problem hiding this comment.
Should-fix: When crash detection fires here, deadAt is set and persisted, but the retained tmux session (kept alive by remain-on-exit) is never killed. Compare with sessions.ts:432 and session-lifecycle.ts:991-993 which both clean up the corpse with kill-session. Without cleanup here, a crashed session that is never resumed and survives daemon restarts will leave a dead tmux session lingering indefinitely.
| if (!info) return null | ||
|
|
||
| const tmuxExists = tmuxHasSession(info.tmuxName) | ||
| const processAlive = sessionProcessAlive(info.tmuxName) |
There was a problem hiding this comment.
Nit: classifyResumeFailure receives processAlive as the tmuxAlive parameter. Previously tmuxAlive meant "tmux session exists"; now it means "process inside the pane is alive" — a different semantic. With remain-on-exit, these can diverge (tmux session exists but process is dead). The classifier still produces correct results because both old-false and new-false lead to 'kill', but the parameter name is now misleading. Consider renaming the parameter to processAlive in classifyResumeFailure to match.
| if (processAlive) { | ||
| delete info.deadAt | ||
| restored++ | ||
| } else { |
There was a problem hiding this comment.
Nit: This kill-session call has no timeout — a hung tmux server would block the event loop. The new sessionProcessAlive() correctly sets timeout: 2000; add the same here and at session-lifecycle.ts:992 for consistency.
2dac548 to
5b0275e
Compare
|
All 4 review items addressed in
943 tests pass (including the updated |
| await fireTransition(run, 'timeout', '', 'backstop timed out') | ||
| }, ms) | ||
| return true | ||
| return false |
There was a problem hiding this comment.
Blocker: backstop return value changed from true to false with no test or comment. When backstopTimer is the only onEnter behavior (e.g. spike's reporting phase), returning false means !handled fires, calling resetTimeout() — which sets up a second timeout timer alongside the one backstopTimer just created. Both target the same phase and both call fireTransition(run, 'timeout', ...), causing a double-fire race. If this change is intentional, it needs either a comment explaining why or resetTimeout suppression in the !handled path.
(flagged by both reviewers)
| if (info && await tryRespawnPane(info)) { | ||
| run.disconnectTimers.delete(sessionId) | ||
| return | ||
| } |
There was a problem hiding this comment.
Blocker: When tryRespawnPane fails (returns false), execution falls through to resumeParticipant() which spawns a completely new session. But respawn-pane -k already killed the dead pane and started a new process inside it. If that new process is still booting when resumeParticipant fires, you have two processes racing for the same tmux session name. On failure, the respawned pane needs to be cleaned up before falling through.
| if (ok) { | ||
| process.stderr.write(`daemon: respawn-pane ${info.tmuxName}: bridge reconnected — recovery successful\n`) | ||
| return true | ||
| } |
There was a problem hiding this comment.
Should-fix: After successful respawn-pane recovery, info.deadAt is never cleared and registry.persist() is never called. If deadAt was set by crash detection before the respawn attempt, the session will appear dead in the dashboard and health polls despite being alive. Neither caller (checkSessionDeath, onRunDisconnect) clears it on a true return either.
| if (ownerIsActor) { | ||
| notifyParticipant(run, run.ownerSessionId, run.protocol.ownerKickoff(run.params)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Should-fix: The new return exits notifyKickoff after sending ownerKickoff to the owner-as-active-actor, but skips the notification loop for non-owner participants. Previously, execution continued and all non-active participants received the onKickoff/fallback notification. Now (e.g. in build protocol where owner=builder=active actor), the critic gets no kickoff notification at all.
| import { registry, threadRegistry, sessionEmoji } from './sessions.js' | ||
| import { transport } from './bridge-transport.js' | ||
| import { formatDuration, tmuxHasSession } from './util.js' | ||
| import { formatDuration, tmuxHasSession, sessionProcessAlive } from './util.js' |
There was a problem hiding this comment.
Nit: sessionProcessAlive is imported but never used in this file — the dashboard still uses tmuxHasSession at the filter. Either switch the dashboard filter to sessionProcessAlive for consistency or remove the unused import.
|
|
||
| export async function tryRespawnPane(info: SessionInfo): Promise<boolean> { | ||
| if (!info.claudeSessionId) return false | ||
| if (sessionProcessAlive(info.tmuxName)) return false |
There was a problem hiding this comment.
Nit: Hardcoded model fallback 'claude-opus-4-6[1m]' — use spawnModel() (already imported from shared/constants.ts) for late-bound defaults per project conventions.
| try { | ||
| execSync(`tmux respawn-pane -k -t ${shq(info.tmuxName)} ${shq(cmd)}`, { stdio: 'pipe', timeout: 5000 }) | ||
| } catch (err) { | ||
| process.stderr.write(`daemon: respawn-pane ${info.tmuxName}: failed: ${err instanceof Error ? err.message : String(err)}\n`) |
There was a problem hiding this comment.
Nit: execSync with string interpolation through shq() — every other tmux call in this file uses execFileSync with an args array, which avoids the shell entirely. respawn-pane takes a shell command as its final arg so the shell may be needed here, but if so add a brief comment explaining why.
(flagged by both reviewers)
|
All 7 review items addressed in
943 tests pass. |
CC's EIO teardown during REPL unmount destroys the tmux pane, making
tmuxHasSession flip to false before the daemon can react. With
remain-on-exit on, the pane is retained after death and #{pane_dead}
distinguishes alive (0) from dead-but-retained (1).
- Add sessionProcessAlive() — checks #{pane_dead} via tmux display-message
- Set remain-on-exit on (window option, -w) at spawn time
- Switch 12 tmuxHasSession sites to sessionProcessAlive where the
question is "is CC running?" (crash detect, orphan detect, thread
guards, fork/resume/respawn guards, codex bootstrap, isAlive)
- Keep tmuxHasSession at 3 sites where "does tmux session exist?" is
correct (peek, snapshot — capture-pane works on retained dead panes)
- Autopsy reports both tmuxExists and processAlive; cleans up retained
panes on resume failure
Experimentally verified: CC exits → pane retained → has-session true,
pane_dead=1. EIO fires in 98.4% of exits (477/485); shell survives in
98.5% of those (470/477). The 7 pane-death cases are the exact scenario
remain-on-exit addresses.
943 tests pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The initial remain-on-exit audit grepped for tmuxHasSession but missed
three inline execFileSync('tmux', ['has-session'...]) probes that bypass
the helper entirely:
- bridge-server.ts:422 (checkSessionDeath) — the primary death detection
path. With a retained pane, has-session succeeds and the entire autopsy
block is skipped. Now checks #{pane_dead} via display-message.
- sessions.ts:423 (daemon restart) — resurrects dead sessions when the
retained pane makes has-session succeed. Now checks pane_dead, and
reaps retained corpses with kill-session so names return to the pool.
- session-lifecycle.ts:499 (spawn cleanup) — reap path lived in a catch
block that never ran with retained panes. Now uses sessionProcessAlive.
Found by adversarial review critic (round 1, §2/§4/§5).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ameter - Replace inline display-message probes in bridge-server.ts and sessions.ts with sessionProcessAlive() calls (review item 1) - Add kill-session cleanup in session-health.ts crash detection so retained corpses don't linger (review item 2 — real bug) - Rename classifyResumeFailure parameter tmuxAlive → processAlive to reflect changed semantics (review item 3) - Add timeout: 2000 to kill-session calls in sessions.ts and session-lifecycle.ts (review item 4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When CC self-exits and the pane is retained (remain-on-exit), try tmux respawn-pane to restart CC in the same pane before falling through to Layer 2 (resumeParticipant, which creates a new session). tryRespawnPane: checks pane is dead but tmux session exists, then runs respawn-pane -k with claude --resume. Waits 10s for bridge reconnection. If the bridge reconnects, recovery is silent — no new session, no name churn, no spawn announcements. If not, falls through to the existing resumeParticipant path. Wired into both paths: - Protocol critics: protocol-runner.ts, before resumeParticipant - Non-protocol sessions: bridge-server.ts checkSessionDeath Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- tryRespawnPane: kill pane on failure before falling through to resumeParticipant (prevents two processes racing for same name) - tryRespawnPane: clear deadAt on successful recovery - tryRespawnPane: use spawnModel() instead of hardcoded model fallback - tryRespawnPane: add comment explaining execSync usage - backstopTimer: restore return true (false caused duplicate timeout alongside the backstop's own timer) - notifyKickoff: remove early return so non-owner participants still get kickoff notifications when ownerKickoff fires - dashboard.ts: remove unused sessionProcessAlive import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
00c59fc to
fdc4111
Compare
…ry path tryRespawnPane inserted a 10-second broken detour on every critic death: wrong --channels flag meant CC never connected its bridge, waitForBridge burned the full timeout, then killed the pane and fell through to Layer 2. Meanwhile the protocol runner's disconnect timer logic was confused by the delay. The review's own critic dying proved this live. Also adds kill-session to checkSessionDeath's death branch so retained corpses (remain-on-exit) are reaped on the primary death path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| delete info.deadAt | ||
| restored++ | ||
| } else { | ||
| try { execFileSync('tmux', ['kill-session', '-t', info.tmuxName], { stdio: 'pipe', timeout: 2000 }) } catch {} |
There was a problem hiding this comment.
Should-fix: sessionProcessAlive() returns false both when the pane is dead (remain-on-exit retained it) AND when the tmux session doesn't exist at all. This kill-session fires unconditionally, spraying failed calls for sessions whose tmux is already gone on every daemon restart. Worse, if a tmux session name was reused between daemon shutdown and restart, this kills the wrong session. Guard with tmuxHasSession() first, matching the pattern in session-health.ts:27-29.
(flagged by both reviewers)
| const info = registry.get(sessionId) | ||
| if (info && !info.deadAt && !tmuxHasSession(info.tmuxName)) { | ||
| if (info && !info.deadAt && !sessionProcessAlive(info.tmuxName)) { | ||
| info.deadAt = Date.now() |
There was a problem hiding this comment.
Should-fix: Both death-detection paths in codex-bootstrap.ts (here and line 85) set deadAt but don't kill the now-retained tmux session. With remain-on-exit on, these zombie panes will accumulate until daemon restart or manual cleanup. Every other death-detection site in this PR (bridge-server, session-health, sessions.ts, session-lifecycle resume) performs the cleanup — codex-bootstrap is the odd one out.
| hasExitMarker: !!(info.exitFilePath && existsSync(info.exitFilePath)), | ||
| hasExitFilePath: !!info.exitFilePath, | ||
| }) | ||
| if (tmuxExists && !processAlive) { |
There was a problem hiding this comment.
Should-fix: This inline kill-session fires unconditionally when tmuxExists && !processAlive, regardless of verdict. Then on kill verdict, killSession() also runs — which likely tries its own tmux cleanup on a now-gone session. If killSession does any tmux-dependent operations (reading final pane state, etc.), that data is already unavailable. Consider either: (a) only doing inline kill when verdict is 'orphan', or (b) moving the cleanup into killSession itself.
| import { join } from 'path' | ||
| import { execSync, execFileSync } from 'child_process' | ||
| import { STATE_DIR } from './config.js' | ||
| import { sessionProcessAlive } from './util.js' |
There was a problem hiding this comment.
Nit: Two separate import statements from './util.js' — merge into one.
| // the pane and tmuxHasSession flips to false before the daemon can react. | ||
| if (tmuxConfirmedAlive) { | ||
| try { | ||
| execFileSync('tmux', ['set-option', '-w', '-t', tmuxName, 'remain-on-exit', 'on'], { stdio: 'pipe' }) |
There was a problem hiding this comment.
Nit: -w scopes remain-on-exit to the window level. If the session ever gets a second window (manual split, plugin), only the original window gets the option. Using -s or omitting the flag (defaults to session when -t targets a session name) would be more robust. Minor because the daemon doesn't create extra windows.
2 participants
Summary
remain-on-exit on, the pane is retained after death and#{pane_dead}distinguishes alive from dead-but-retained.sessionProcessAlive()— checks#{pane_dead}viatmux display-message. Returns false for dead-but-retained panes and missing sessions.tmuxHasSessioncall sites tosessionProcessAlivewhere the question is "is CC running?" (crash detection, orphan detection, thread occupancy guards, fork/resume/respawn guards, codex bootstrap,isAlive()).tmuxHasSessionat 3 sites where "does the tmux session exist?" is correct (peek, snapshot —capture-paneworks on retained dead panes).tmuxExistsandprocessAlive; cleans up retained panes on resume failure.Evidence
Experimentally verified on a live test session:
has-session: FALSE(session destroyed)has-session: TRUE,pane_dead=1Analysis of 485 exit logs: EIO fires in 98.4% of CC exits (477/485). Shell survives the EIO in 98.5% of those (470/477). The 7 pane-death cases are the exact scenario
remain-on-exitaddresses.Test plan
peekandsnapshotwork on dead-but-retained panesresumeandrespawncommands work when pane is dead-but-retained (should not block with "already running")🤖 Generated with Claude Code