diff --git a/js/.changeset/detached-status-killed-record.md b/js/.changeset/detached-status-killed-record.md new file mode 100644 index 0000000..fc95550 --- /dev/null +++ b/js/.changeset/detached-status-killed-record.md @@ -0,0 +1,5 @@ +--- +'start-command': patch +--- + +Fix `--status` for detached executions resurrecting a completed (killed) record. `enrichDetachedStatus()` no longer flips an already-`executed` record back to `executing` (and nulls its exit code) just because `screen -ls`/`tmux`/`docker` still lists a same-named session — a lingering shell can outlive a SIGKILLed command (e.g. OOM, exit 137). The recorded exit code and the `Exit Code:` log footer that `start` itself writes are now treated as authoritative; the record only flips to `executing` when there is no recorded exit code and no terminal footer in the log. diff --git a/js/src/lib/status-formatter.js b/js/src/lib/status-formatter.js index 711712f..04625c1 100644 --- a/js/src/lib/status-formatter.js +++ b/js/src/lib/status-formatter.js @@ -109,10 +109,22 @@ function enrichDetachedStatus(record) { Object.assign(enriched, record); if (alive && enriched.status === 'executed') { - // Session still running but record says executed - correct it - enriched.status = 'executing'; - enriched.exitCode = null; - enriched.endTime = null; + // A live `screen -ls` (or `tmux`/`docker`) session does NOT mean the command + // is still running: a lingering shell can outlive a killed command (e.g. the + // OOM killer sends SIGKILL, exit 137, but the login shell stays up for a + // window after `start` already wrote the terminal footer). The footer/recorded + // exit code is authoritative. Only flip back to 'executing' when there is NO + // recorded terminal exit code AND no `Exit Code:` footer in the log. + const footerExit = readExitCodeFromLog(enriched.logPath); + const hasRecordedExit = + enriched.exitCode !== null && enriched.exitCode !== undefined; + if (!hasRecordedExit && footerExit === null) { + // Session still running and no terminal record - correct it + enriched.status = 'executing'; + enriched.exitCode = null; + enriched.endTime = null; + } + // Otherwise keep the recorded/footer exit code - the command has finished. } else if (!alive && enriched.status === 'executing') { // Session ended but record says executing - correct it enriched.status = 'executed'; diff --git a/js/test/session-name-status.js b/js/test/session-name-status.js index 19397b4..72b824e 100644 --- a/js/test/session-name-status.js +++ b/js/test/session-name-status.js @@ -306,6 +306,126 @@ describe('Issue #101: Detached status enrichment', () => { expect(enriched.endTime).not.toBeNull(); } }); + + // Issue #134: a lingering screen session must NOT resurrect a completed + // (killed, exit 137) record back to 'executing' / null exit code. + describe('Issue #134: completed record with a lingering live session', () => { + const screenAvailable = (() => { + const probe = spawnSync('screen', ['-v'], { encoding: 'utf8' }); + return probe.status === 0 || /Screen version/.test(probe.stdout || ''); + })(); + + let sessionName; + let logPath; + + beforeEach(() => { + if (!screenAvailable) { + return; + } + sessionName = `enrich-134-${process.pid}-${Date.now()}`; + logPath = path.join(TEST_APP_FOLDER, `${sessionName}.log`); + if (!fs.existsSync(TEST_APP_FOLDER)) { + fs.mkdirSync(TEST_APP_FOLDER, { recursive: true }); + } + // Footer exactly as `start` writes it for a SIGKILLed command. + fs.writeFileSync( + logPath, + `Killed\n\n${'='.repeat(50)}\nFinished: 2026-06-14 19:10:49.822\nExit Code: 137\n` + ); + // A shell that outlives the (already-finished) command. + spawnSync('screen', ['-dmS', sessionName, 'sh', '-c', 'sleep 30'], { + encoding: 'utf8', + }); + }); + + afterEach(() => { + if (!screenAvailable) { + return; + } + spawnSync('screen', ['-S', sessionName, '-X', 'quit'], { + stdio: 'ignore', + }); + if (logPath && fs.existsSync(logPath)) { + fs.unlinkSync(logPath); + } + }); + + it('keeps the recorded exit code when the session is still listed', () => { + if (!screenAvailable) { + return; + } + const record = new ExecutionRecord({ + command: 'sleep 60', + logPath, + options: { + sessionName, + isolated: 'screen', + isolationMode: 'detached', + }, + }); + record.complete(137); + + // Sanity: the session must actually be alive for this test to be meaningful. + expect(isDetachedSessionAlive(record)).toBe(true); + + const enriched = enrichDetachedStatus(record); + expect(enriched.status).toBe('executed'); + expect(enriched.exitCode).toBe(137); + expect(enriched.endTime).not.toBeNull(); + }); + + it('honors the log footer exit code even without a recorded exit code', () => { + if (!screenAvailable) { + return; + } + const record = new ExecutionRecord({ + command: 'sleep 60', + logPath, + options: { + sessionName, + isolated: 'screen', + isolationMode: 'detached', + }, + }); + // Record was never reconciled: status 'executed' but exitCode still null. + record.status = 'executed'; + record.exitCode = null; + record.endTime = null; + + expect(isDetachedSessionAlive(record)).toBe(true); + + const enriched = enrichDetachedStatus(record); + // Footer says 137, so it must stay finished, not flip to executing. + expect(enriched.status).toBe('executed'); + }); + + it('flips to executing only when there is no terminal record at all', () => { + if (!screenAvailable) { + return; + } + // Log with NO Exit Code footer and no recorded exit code. + fs.writeFileSync(logPath, 'still running, no footer yet\n'); + const record = new ExecutionRecord({ + command: 'sleep 60', + logPath, + options: { + sessionName, + isolated: 'screen', + isolationMode: 'detached', + }, + }); + record.status = 'executed'; + record.exitCode = null; + record.endTime = null; + + expect(isDetachedSessionAlive(record)).toBe(true); + + const enriched = enrichDetachedStatus(record); + expect(enriched.status).toBe('executing'); + expect(enriched.exitCode).toBeNull(); + expect(enriched.endTime).toBeNull(); + }); + }); }); }); diff --git a/rust/changelog.d/135.md b/rust/changelog.d/135.md new file mode 100644 index 0000000..a2249bb --- /dev/null +++ b/rust/changelog.d/135.md @@ -0,0 +1,5 @@ +--- +bump: patch +--- + +Fixed detached `--status` resurrecting a killed (exit 137) record back to `executing`. The `alive && executed` branch in `enrich_detached_status` now consults the recorded exit code and the `Exit Code:` log footer before flipping, so a lingering shell that outlives a `SIGKILL`ed command no longer reports a completed command as still running. diff --git a/rust/src/lib/status_formatter.rs b/rust/src/lib/status_formatter.rs index 515a5c7..6830fe2 100644 --- a/rust/src/lib/status_formatter.rs +++ b/rust/src/lib/status_formatter.rs @@ -87,10 +87,20 @@ pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { let mut enriched = record.clone(); if alive && enriched.status == ExecutionStatus::Executed { - // Session still running but record says executed - correct it - enriched.status = ExecutionStatus::Executing; - enriched.exit_code = None; - enriched.end_time = None; + // A live `screen -ls` (or `tmux`/`docker`) session does NOT mean the command + // is still running: a lingering shell can outlive a killed command (e.g. the + // OOM killer sends SIGKILL, exit 137, but the login shell stays up for a + // window after `start` already wrote the terminal footer). The footer/recorded + // exit code is authoritative. Only flip back to "executing" when there is NO + // recorded terminal exit code AND no `Exit Code:` footer in the log. + let footer_exit = read_exit_code_from_log(&enriched.log_path); + if enriched.exit_code.is_none() && footer_exit.is_none() { + // Session still running and no terminal record - correct it + enriched.status = ExecutionStatus::Executing; + enriched.exit_code = None; + enriched.end_time = None; + } + // Otherwise keep the recorded/footer exit code - the command has finished. } else if !alive && enriched.status == ExecutionStatus::Executing { // Session ended but record says executing - correct it enriched.status = ExecutionStatus::Executed; diff --git a/rust/tests/session_name_status.rs b/rust/tests/session_name_status.rs index b3aad17..d5ccfa2 100644 --- a/rust/tests/session_name_status.rs +++ b/rust/tests/session_name_status.rs @@ -262,6 +262,156 @@ fn test_enrich_detached_status_marks_dead_session_as_executed() { } } +// ===== Issue #134: lingering live session must not resurrect a completed record ===== + +/// Probe whether `screen` is usable in this environment. +fn screen_available() -> bool { + std::process::Command::new("screen") + .arg("-v") + .output() + .map(|o| { + o.status.success() || String::from_utf8_lossy(&o.stdout).contains("Screen version") + }) + .unwrap_or(false) +} + +/// Start a detached screen session that outlives the (already-finished) command. +/// Returns the session name. Caller must quit it. +fn start_lingering_screen(session_name: &str) { + let _ = std::process::Command::new("screen") + .args(["-dmS", session_name, "sh", "-c", "sleep 30"]) + .output(); +} + +/// Whether the lingering session is observable as alive in this environment. +/// +/// Some instrumented environments (notably `cargo tarpaulin`, which traces +/// every fork via ptrace) disrupt the `screen -dmS` daemon fork, so the +/// session never registers in `screen -ls`. There the sanity precondition for +/// these tests cannot hold, so they skip rather than fail. +fn session_observably_alive(record: &ExecutionRecord) -> bool { + is_detached_session_alive(record) == Some(true) +} + +fn quit_screen(session_name: &str) { + let _ = std::process::Command::new("screen") + .args(["-S", session_name, "-X", "quit"]) + .output(); +} + +#[test] +fn test_enrich_keeps_recorded_exit_code_when_session_lingers() { + if !screen_available() { + return; + } + let temp_dir = TempDir::new().unwrap(); + let session_name = format!("enrich-134-recorded-{}", std::process::id()); + let log_path = temp_dir.path().join(format!("{session_name}.log")); + // Footer exactly as `start` writes it for a SIGKILLed command. + std::fs::write( + &log_path, + "Killed\n\n==================================================\nFinished: 2026-06-14 19:10:49.822\nExit Code: 137\n", + ) + .unwrap(); + + start_lingering_screen(&session_name); + + let mut record = ExecutionRecord::with_options(ExecutionRecordOptions { + command: "sleep 60".to_string(), + log_path: Some(log_path.to_string_lossy().to_string()), + options: Some(make_isolation_options(&session_name, "screen", "detached")), + ..Default::default() + }); + record.complete(137); + + // Sanity: the session must actually be alive for this test to be meaningful. + if !session_observably_alive(&record) { + quit_screen(&session_name); + return; + } + + let enriched = enrich_detached_status(&record); + quit_screen(&session_name); + + assert_eq!(enriched.status, ExecutionStatus::Executed); + assert_eq!(enriched.exit_code, Some(137)); + assert!(enriched.end_time.is_some()); +} + +#[test] +fn test_enrich_honors_log_footer_when_no_recorded_exit_code() { + if !screen_available() { + return; + } + let temp_dir = TempDir::new().unwrap(); + let session_name = format!("enrich-134-footer-{}", std::process::id()); + let log_path = temp_dir.path().join(format!("{session_name}.log")); + std::fs::write( + &log_path, + "Killed\n\n==================================================\nFinished: 2026-06-14 19:10:49.822\nExit Code: 137\n", + ) + .unwrap(); + + start_lingering_screen(&session_name); + + // Status 'executed' but exit_code never recorded; the footer is authoritative. + let mut record = ExecutionRecord::with_options(ExecutionRecordOptions { + command: "sleep 60".to_string(), + status: Some(ExecutionStatus::Executed), + log_path: Some(log_path.to_string_lossy().to_string()), + options: Some(make_isolation_options(&session_name, "screen", "detached")), + ..Default::default() + }); + record.exit_code = None; + record.end_time = None; + + if !session_observably_alive(&record) { + quit_screen(&session_name); + return; + } + + let enriched = enrich_detached_status(&record); + quit_screen(&session_name); + + assert_eq!(enriched.status, ExecutionStatus::Executed); +} + +#[test] +fn test_enrich_flips_to_executing_when_no_terminal_record() { + if !screen_available() { + return; + } + let temp_dir = TempDir::new().unwrap(); + let session_name = format!("enrich-134-nofooter-{}", std::process::id()); + let log_path = temp_dir.path().join(format!("{session_name}.log")); + // Log with NO Exit Code footer. + std::fs::write(&log_path, "still running, no footer yet\n").unwrap(); + + start_lingering_screen(&session_name); + + let mut record = ExecutionRecord::with_options(ExecutionRecordOptions { + command: "sleep 60".to_string(), + status: Some(ExecutionStatus::Executed), + log_path: Some(log_path.to_string_lossy().to_string()), + options: Some(make_isolation_options(&session_name, "screen", "detached")), + ..Default::default() + }); + record.exit_code = None; + record.end_time = None; + + if !session_observably_alive(&record) { + quit_screen(&session_name); + return; + } + + let enriched = enrich_detached_status(&record); + quit_screen(&session_name); + + assert_eq!(enriched.status, ExecutionStatus::Executing); + assert_eq!(enriched.exit_code, None); + assert!(enriched.end_time.is_none()); +} + #[test] fn test_get_most_recent_session_name_match() { let (_temp_dir, store) = create_test_store();