diff --git a/.gitkeep b/.gitkeep index 215a936..ec74b41 100644 --- a/.gitkeep +++ b/.gitkeep @@ -1,2 +1,3 @@ # .gitkeep file auto-generated at 2026-06-26T14:21:03.315Z for PR creation at branch issue-144-d8d3fb7dfd56 for issue https://github.com/link-foundation/start/issues/144 -# Updated: 2026-06-26T16:22:21.672Z \ No newline at end of file +# Updated: 2026-06-26T16:22:21.672Z +# Updated: 2026-08-04T03:47:36.189Z \ No newline at end of file diff --git a/README.md b/README.md index b58e153..2cd5cff 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,13 @@ include best-effort `processIds` for tracked wrapper processes and detached screen, tmux, and Docker isolation containers when those native tools can report them. +For detached Docker executions, `oomKilled` is reported as an observation of the +container cgroup flag, not as a verdict: while `docker inspect` still reports the +container as running the status stays `executing`, and once it stops the reported +`exitCode` is the container's real exit code. `137` is only used as a fallback +when the container is gone and neither a stored exit code nor a log footer can be +recovered. + `--upload-log` accepts either an execution UUID or an isolation session name. It looks up the stored `logPath`, installs `gh-upload-log` with Bun or npm if the uploader is missing, and then streams the uploader output directly. diff --git a/js/.changeset/issue-151-oom-observation-not-verdict.md b/js/.changeset/issue-151-oom-observation-not-verdict.md new file mode 100644 index 0000000..2ffbf0b --- /dev/null +++ b/js/.changeset/issue-151-oom-observation-not-verdict.md @@ -0,0 +1,5 @@ +--- +'start-command': patch +--- + +Treat Docker `OOMKilled` as an observation rather than a verdict in `--status` / `--list`: a detached session whose container is still running stays `executing` (with `oomKilled true` alongside), a stopped container reports its real `.State.ExitCode`, and `137` is used only when the container is gone and neither a log footer nor an exit code can be recovered. diff --git a/js/src/lib/status-formatter.js b/js/src/lib/status-formatter.js index 7d9c006..58374c4 100644 --- a/js/src/lib/status-formatter.js +++ b/js/src/lib/status-formatter.js @@ -84,27 +84,36 @@ function readDockerState(record) { * (currently docker via `docker inspect .State.ExitCode`). Returns null when * the backend cannot provide a real code, so callers never surface the `-1` * sentinel for a session whose real exit code is simply not available yet. - * @param {Object} record - Execution record + * A running container has no terminal exit code (docker reports `0` for it), + * so only a stopped container contributes one. + * @param {{running: boolean, exitCode: number|null}|null} dockerState - Inspected state * @returns {number|null} */ -function readBackendExitCode(record) { - const state = readDockerState(record); - return state && !state.running ? state.exitCode : null; +function backendExitCode(dockerState) { + return dockerState && !dockerState.running ? dockerState.exitCode : null; } -function resolveOomExitCode(footerExit, dockerState) { - if (footerExit !== null && footerExit !== undefined) { - return footerExit; +/** + * Reconcile the OOM observation from the stored record and from `docker inspect`. + * + * `State.OOMKilled` is a container-cgroup flag: the kernel sets it when ANY + * process in the cgroup is OOM-killed and it is never cleared for the life of + * the container (moby/moby#47618). It is therefore an *observation*, never a + * verdict about the session (issue #151) — a container that lost one child + * process keeps running and can still exit `0`. Once observed, the flag stays + * `true` for the record. + * @param {Object} record - Execution record + * @param {{oomKilled: boolean|null}|null} dockerState - Inspected state + * @returns {boolean|undefined} Observation, or undefined when nothing is known + */ +function resolveOomObservation(record, dockerState) { + if (record.oomKilled === true || dockerState?.oomKilled === true) { + return true; } - if ( - dockerState && - dockerState.exitCode !== null && - dockerState.exitCode !== undefined && - (!dockerState.running || dockerState.exitCode !== 0) - ) { - return dockerState.exitCode; + if (record.oomKilled === false || dockerState?.oomKilled === false) { + return false; } - return 137; + return undefined; } /** @@ -201,25 +210,18 @@ function enrichDetachedStatus(record) { : dockerState.running : isDetachedSessionAlive(record); + // `oomKilled` is exposed alongside the status, but never decides it (#151). + const oomKilled = resolveOomObservation(record, dockerState); + // Create a shallow copy to avoid mutating the original const cloneRecord = () => { const enriched = Object.create(Object.getPrototypeOf(record)); Object.assign(enriched, record); - return enriched; - }; - - if (record.oomKilled === true || dockerState?.oomKilled === true) { - const enriched = cloneRecord(); - enriched.oomKilled = true; - enriched.status = 'executed'; - if (enriched.exitCode === null || enriched.exitCode === undefined) { - enriched.exitCode = resolveOomExitCode(footerExit, dockerState); - } - if (!enriched.endTime) { - enriched.endTime = new Date().toISOString(); + if (oomKilled !== undefined) { + enriched.oomKilled = oomKilled; } return enriched; - } + }; if (alive === null) { // Liveness is unknown: the backend could not be probed (e.g. a detached @@ -230,22 +232,35 @@ function enrichDetachedStatus(record) { // orchestrators misread as a finished/failed run (issue #136). const isDetached = record.options && record.options.isolationMode === 'detached'; - if (isDetached && record.status === 'executing' && footerExit !== null) { - const enriched = cloneRecord(); - enriched.status = 'executed'; - enriched.exitCode = footerExit; - if (!enriched.endTime) { - enriched.endTime = new Date().toISOString(); + if (isDetached && record.status === 'executing') { + if (footerExit !== null) { + const enriched = cloneRecord(); + enriched.status = 'executed'; + enriched.exitCode = footerExit; + if (!enriched.endTime) { + enriched.endTime = new Date().toISOString(); + } + return enriched; + } + if (oomKilled === true) { + // The container is gone and wrote no footer, so the OOM observation is + // the only evidence left: report the session as finished with the + // conventional SIGKILL code (issue #148). While the container is still + // inspectable this branch is never taken — a live container keeps the + // session `executing` no matter what the cgroup flag says (issue #151). + const enriched = cloneRecord(); + enriched.status = 'executed'; + enriched.exitCode = 137; + if (!enriched.endTime) { + enriched.endTime = new Date().toISOString(); + } + return enriched; } - return enriched; } - return record; + return oomKilled !== undefined ? cloneRecord() : record; } const enriched = cloneRecord(); - if (dockerState?.oomKilled !== null && dockerState?.oomKilled !== undefined) { - enriched.oomKilled = dockerState.oomKilled; - } if (alive && enriched.status === 'executed') { // A live `screen -ls` (or `tmux`/`docker`) session does NOT mean the command @@ -266,11 +281,15 @@ function enrichDetachedStatus(record) { } else if (!alive && enriched.status === 'executing') { // Session ended but record says executing - correct it. Resolve a real exit // code: prefer the log footer, then the backend's own record (e.g. - // `docker inspect .State.ExitCode`), and only fall back to the `-1` sentinel - // as a last resort when no real code can be obtained (issue #136). + // `docker inspect .State.ExitCode`), then `137` when the only evidence left + // is the OOM observation, and only fall back to the `-1` sentinel as a last + // resort when no real code can be obtained (issues #136, #151). enriched.status = 'executed'; if (enriched.exitCode === null || enriched.exitCode === undefined) { - enriched.exitCode = footerExit ?? readBackendExitCode(enriched) ?? -1; + enriched.exitCode = + footerExit ?? + backendExitCode(dockerState) ?? + (oomKilled === true ? 137 : -1); } if (!enriched.endTime) { enriched.endTime = new Date().toISOString(); diff --git a/js/test/isolation.js b/js/test/isolation.js index c57d3ae..3612895 100644 --- a/js/test/isolation.js +++ b/js/test/isolation.js @@ -274,6 +274,13 @@ describe('Isolation Runner Error Handling', () => { console.log(' Skipping: docker not installed'); return; } + // Skip if the daemon is not running - the error will be about the daemon + // instead of the missing image (Windows CI runners ship the docker CLI + // without a running daemon). + if (!require('../src/lib/docker-utils').isDockerAvailable()) { + console.log(' Skipping: docker daemon not running'); + return; + } const result = await runInDocker('echo test', { detached: true }); assert.strictEqual(result.success, false); diff --git a/js/test/session-name-status.js b/js/test/session-name-status.js index ad93b51..f2a519d 100644 --- a/js/test/session-name-status.js +++ b/js/test/session-name-status.js @@ -61,6 +61,38 @@ function createExecutable(filePath, content) { fs.chmodSync(filePath, 0o755); } +// A fake `docker` whose every `inspect` fails, i.e. the container is gone +// (removed) or not visible yet — the "unknown liveness" case of issue #136. +function withFakeDockerMissingContainer(fn) { + const fakeBin = fs.mkdtempSync(path.join(os.tmpdir(), 'fake-docker-gone-')); + const dockerPath = path.join( + fakeBin, + process.platform === 'win32' ? 'docker.cmd' : 'docker' + ); + createExecutable( + dockerPath, + process.platform === 'win32' + ? ['@echo off', 'exit /b 1', ''].join('\r\n') + : ['#!/bin/sh', 'exit 1', ''].join('\n') + ); + + const originalPath = process.env.PATH; + const originalDockerBin = process.env.START_DOCKER_BIN; + process.env.PATH = `${fakeBin}${path.delimiter}${originalPath || ''}`; + process.env.START_DOCKER_BIN = dockerPath; + try { + return fn(); + } finally { + process.env.PATH = originalPath; + if (originalDockerBin === undefined) { + delete process.env.START_DOCKER_BIN; + } else { + process.env.START_DOCKER_BIN = originalDockerBin; + } + fs.rmSync(fakeBin, { recursive: true, force: true }); + } +} + function withFakeDockerInspect(stateLine, fn) { const fakeBin = fs.mkdtempSync(path.join(os.tmpdir(), 'fake-docker-')); const dockerPath = path.join( @@ -677,7 +709,7 @@ describe('Issue #144: detached docker OOMKilled status signal', () => { }); }); -describe('Issue #148: detached docker OOMKilled terminal status', () => { +describe('Issue #148: detached docker OOMKilled terminal status when the container is gone', () => { let store; beforeEach(() => { @@ -692,7 +724,7 @@ describe('Issue #148: detached docker OOMKilled terminal status', () => { cleanupTestDir(); }); - function saveDockerRecord() { + function saveDockerRecord(extra = {}) { const record = new ExecutionRecord({ command: 'sh -c "allocate memory"', logPath: '/tmp/issue-148.log', @@ -701,15 +733,16 @@ describe('Issue #148: detached docker OOMKilled terminal status', () => { isolated: 'docker', isolationMode: 'detached', }, + ...extra, }); store.save(record); return record; } - it('treats oomKilled as terminal even while Docker still reports running', () => { - const record = saveDockerRecord(); + it('makes an OOM-killed session terminal once its container is gone', () => { + const record = saveDockerRecord({ oomKilled: true }); - withFakeDockerInspect('true 137 true', () => { + withFakeDockerMissingContainer(() => { const result = queryStatus(store, record.uuid, 'json'); expect(result.success).toBe(true); const parsed = JSON.parse(result.output); @@ -721,19 +754,124 @@ describe('Issue #148: detached docker OOMKilled terminal status', () => { }); }); - it('uses 137 when oomKilled is terminal but Docker has no terminal exit code yet', () => { + it('uses the container exit code, not 137, for a stopped OOM-flagged container', () => { const record = saveDockerRecord(); + withFakeDockerInspect('false 3 true', () => { + const result = queryStatus(store, record.uuid, 'json'); + expect(result.success).toBe(true); + const parsed = JSON.parse(result.output); + expect(parsed.status).toBe('executed'); + expect(parsed.exitCode).toBe(3); + expect(parsed.oomKilled).toBe(true); + expect(parsed.endTime).toBeTruthy(); + }); + }); +}); + +describe('Issue #151: OOMKilled is an observation, not a verdict', () => { + let store; + let logPath; + + beforeEach(() => { + cleanupTestDir(); + store = new ExecutionStore({ + appFolder: TEST_APP_FOLDER, + useLinks: false, + }); + logPath = path.join(os.tmpdir(), `issue-151-${process.pid}.log`); + fs.rmSync(logPath, { force: true }); + }); + + afterEach(() => { + fs.rmSync(logPath, { force: true }); + cleanupTestDir(); + }); + + function saveDockerRecord(extra = {}) { + const record = new ExecutionRecord({ + command: 'node -e "setTimeout(() => process.exit(0), 600000)"', + logPath, + options: { + sessionName: 'issue151-oom', + isolated: 'docker', + isolationMode: 'detached', + }, + ...extra, + }); + store.save(record); + return record; + } + + it('keeps a running container executing while exposing oomKilled', () => { + const record = saveDockerRecord(); + + // `docker inspect` reports the cgroup OOM flag while the container itself + // is still running: only the container state may decide the status. withFakeDockerInspect('true 0 true', () => { + const result = queryStatus(store, record.uuid, 'json'); + expect(result.success).toBe(true); + const parsed = JSON.parse(result.output); + expect(parsed.status).toBe('executing'); + expect(parsed.exitCode).toBeNull(); + expect(parsed.oomKilled).toBe(true); + expect(parsed.endTime).toBeNull(); + expect(parsed.currentTime).toBeTruthy(); + }); + }); + + it('never synthesizes 137 for a running container that reports exit code 137', () => { + const record = saveDockerRecord(); + + withFakeDockerInspect('true 137 true', () => { + const enriched = enrichDetachedStatus(store.get(record.uuid)); + expect(enriched.status).toBe('executing'); + expect(enriched.exitCode).toBeNull(); + expect(enriched.oomKilled).toBe(true); + }); + }); + + it('reports the container exit code 0 once an OOM-flagged container finishes', () => { + const record = saveDockerRecord(); + + withFakeDockerInspect('false 0 true', () => { const result = queryStatus(store, record.uuid, 'json'); expect(result.success).toBe(true); const parsed = JSON.parse(result.output); expect(parsed.status).toBe('executed'); - expect(parsed.exitCode).toBe(137); + expect(parsed.exitCode).toBe(0); expect(parsed.oomKilled).toBe(true); expect(parsed.endTime).toBeTruthy(); }); }); + + it('prefers the log footer exit code over the OOM fallback when the container is gone', () => { + fs.writeFileSync(logPath, 'Finished: now\nExit Code: 0\n', 'utf8'); + const record = saveDockerRecord({ oomKilled: true }); + + withFakeDockerMissingContainer(() => { + const result = queryStatus(store, record.uuid, 'json'); + expect(result.success).toBe(true); + const parsed = JSON.parse(result.output); + expect(parsed.status).toBe('executed'); + expect(parsed.exitCode).toBe(0); + expect(parsed.oomKilled).toBe(true); + }); + }); + + it('keeps the list output executing for an OOM-flagged running container', () => { + saveDockerRecord(); + + withFakeDockerInspect('true 0 true', () => { + const result = listExecutions(store, 'json'); + expect(result.success).toBe(true); + const parsed = JSON.parse(result.output); + expect(parsed.count).toBe(1); + expect(parsed.executions[0].status).toBe('executing'); + expect(parsed.executions[0].exitCode).toBeNull(); + expect(parsed.executions[0].oomKilled).toBe(true); + }); + }); }); describe('Issue #105: attachCurrentTime for executing status', () => { diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 7b76404..2a9193b 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -437,7 +437,7 @@ checksum = "0c790de23124f9ab44544d7ac05d60440adc586479ce501c1d6d7da3cd8c9cf5" [[package]] name = "start-command" -version = "0.17.2" +version = "0.17.3" dependencies = [ "base64", "chrono", diff --git a/rust/changelog.d/issue-151-oom-observation-not-verdict.md b/rust/changelog.d/issue-151-oom-observation-not-verdict.md new file mode 100644 index 0000000..9292356 --- /dev/null +++ b/rust/changelog.d/issue-151-oom-observation-not-verdict.md @@ -0,0 +1,5 @@ +--- +bump: patch +--- + +Treat Docker OOMKilled as an observation rather than a verdict in `--status` / `--list`: a detached session whose container is still running stays `executing` (with `oomKilled true` alongside), a stopped container reports its real `.State.ExitCode`, and `137` is used only when the container is gone and neither a log footer nor an exit code can be recovered. diff --git a/rust/src/lib/status_formatter.rs b/rust/src/lib/status_formatter.rs index 2eb38a9..073813a 100644 --- a/rust/src/lib/status_formatter.rs +++ b/rust/src/lib/status_formatter.rs @@ -87,8 +87,10 @@ fn read_docker_state(record: &ExecutionRecord) -> Option { /// (currently docker via `docker inspect .State.ExitCode`). Returns None when /// the backend cannot provide a real code, so callers never surface the `-1` /// sentinel for a session whose real exit code is simply not available yet. -fn read_backend_exit_code(record: &ExecutionRecord) -> Option { - let state = read_docker_state(record)?; +/// A running container has no terminal exit code (docker reports `0` for it), +/// so only a stopped container contributes one. +fn backend_exit_code(docker_state: Option) -> Option { + let state = docker_state?; if state.running { None } else { @@ -96,18 +98,26 @@ fn read_backend_exit_code(record: &ExecutionRecord) -> Option { } } -fn resolve_oom_exit_code(footer_exit: Option, docker_state: Option) -> i32 { - if let Some(code) = footer_exit { - return code; +/// Reconcile the OOM observation from the stored record and from `docker inspect`. +/// +/// `State.OOMKilled` is a container-cgroup flag: the kernel sets it when ANY +/// process in the cgroup is OOM-killed and it is never cleared for the life of +/// the container (moby/moby#47618). It is therefore an *observation*, never a +/// verdict about the session (issue #151) — a container that lost one child +/// process keeps running and can still exit `0`. Once observed, the flag stays +/// `true` for the record. +fn resolve_oom_observation( + record: &ExecutionRecord, + docker_state: Option, +) -> Option { + let inspected = docker_state.and_then(|state| state.oom_killed); + if record.oom_killed == Some(true) || inspected == Some(true) { + return Some(true); } - if let Some(state) = docker_state { - if let Some(code) = state.exit_code { - if !state.running || code != 0 { - return code; - } - } + if record.oom_killed == Some(false) || inspected == Some(false) { + return Some(false); } - 137 + None } /// Check if a detached isolation session is still running @@ -184,20 +194,15 @@ pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { None }; - if record.oom_killed == Some(true) - || docker_state.and_then(|state| state.oom_killed) == Some(true) - { + // `oomKilled` is exposed alongside the status, but never decides it (#151). + let oom_killed = resolve_oom_observation(record, docker_state); + let clone_record = || { let mut enriched = record.clone(); - enriched.oom_killed = Some(true); - enriched.status = ExecutionStatus::Executed; - if enriched.exit_code.is_none() { - enriched.exit_code = Some(resolve_oom_exit_code(footer_exit, docker_state)); - } - if enriched.end_time.is_none() { - enriched.end_time = Some(chrono::Utc::now().to_rfc3339()); + if oom_killed.is_some() { + enriched.oom_killed = oom_killed; } - return enriched; - } + enriched + }; let alive = if is_detached_docker { docker_state.map(|state| state.running) @@ -217,23 +222,37 @@ pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { // finished/failed run (issue #136). let is_detached = record.options.get("isolationMode").and_then(|v| v.as_str()) == Some("detached"); - if is_detached && record.status == ExecutionStatus::Executing && footer_exit.is_some() { - let mut enriched = record.clone(); - enriched.status = ExecutionStatus::Executed; - enriched.exit_code = footer_exit; - if enriched.end_time.is_none() { - enriched.end_time = Some(chrono::Utc::now().to_rfc3339()); + if is_detached && record.status == ExecutionStatus::Executing { + if footer_exit.is_some() { + let mut enriched = clone_record(); + enriched.status = ExecutionStatus::Executed; + enriched.exit_code = footer_exit; + if enriched.end_time.is_none() { + enriched.end_time = Some(chrono::Utc::now().to_rfc3339()); + } + return enriched; + } + if oom_killed == Some(true) { + // The container is gone and wrote no footer, so the OOM + // observation is the only evidence left: report the session + // as finished with the conventional SIGKILL code (issue + // #148). While the container is still inspectable this + // branch is never taken — a live container keeps the session + // `executing` no matter what the cgroup flag says (#151). + let mut enriched = clone_record(); + enriched.status = ExecutionStatus::Executed; + enriched.exit_code = Some(137); + if enriched.end_time.is_none() { + enriched.end_time = Some(chrono::Utc::now().to_rfc3339()); + } + return enriched; } - return enriched; } - return record.clone(); + return clone_record(); } }; - let mut enriched = record.clone(); - if let Some(oom_killed) = docker_state.and_then(|state| state.oom_killed) { - enriched.oom_killed = Some(oom_killed); - } + let mut enriched = clone_record(); if alive && enriched.status == ExecutionStatus::Executed { // A live `screen -ls` (or `tmux`/`docker`) session does NOT mean the command @@ -252,13 +271,19 @@ pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { } else if !alive && enriched.status == ExecutionStatus::Executing { // Session ended but record says executing - correct it. Resolve a real // exit code: prefer the log footer, then the backend's own record (e.g. - // `docker inspect .State.ExitCode`), and only fall back to the `-1` - // sentinel as a last resort when no real code can be obtained (issue #136). + // `docker inspect .State.ExitCode`), then `137` when the only evidence + // left is the OOM observation, and only fall back to the `-1` sentinel + // as a last resort when no real code can be obtained (issues #136, #151). enriched.status = ExecutionStatus::Executed; if enriched.exit_code.is_none() { enriched.exit_code = Some( footer_exit - .or_else(|| read_backend_exit_code(&enriched)) + .or_else(|| backend_exit_code(docker_state)) + .or(if oom_killed == Some(true) { + Some(137) + } else { + None + }) .unwrap_or(-1), ); } diff --git a/rust/tests/status_formatter.rs b/rust/tests/status_formatter.rs index 12d7d10..752fba3 100644 --- a/rust/tests/status_formatter.rs +++ b/rust/tests/status_formatter.rs @@ -4,7 +4,7 @@ use serde_json::Value; use start_command::{ - attach_current_time, format_record, format_record_as_links_notation, + attach_current_time, enrich_detached_status, format_record, format_record_as_links_notation, format_record_as_links_notation_with_current_time, format_record_as_text, format_record_as_text_with_current_time, format_record_list, format_record_with_current_time, list_executions, query_status, ExecutionRecord, ExecutionRecordOptions, ExecutionStatus, @@ -304,14 +304,39 @@ fn write_fake_docker(fake_dir: &Path, state_line: &str) -> PathBuf { } } -fn with_fake_docker_inspect(state_line: &str, run: F) { +/// A fake `docker` whose every `inspect` fails, i.e. the container is gone +/// (removed) or not visible yet — the "unknown liveness" case of issue #136. +fn write_missing_container_docker(fake_dir: &Path) -> PathBuf { + #[cfg(windows)] + { + let script = ["@echo off", "exit /b 1", ""].join("\r\n"); + let docker_path = fake_dir.join("docker.cmd"); + std::fs::write(&docker_path, script).unwrap(); + docker_path + } + + #[cfg(not(windows))] + { + use std::os::unix::fs::PermissionsExt; + + let script = ["#!/bin/sh", "exit 1", ""].join("\n"); + let docker_path = fake_dir.join("docker"); + std::fs::write(&docker_path, script).unwrap(); + let mut permissions = std::fs::metadata(&docker_path).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&docker_path, permissions).unwrap(); + docker_path + } +} + +fn with_fake_docker(write_docker: impl FnOnce(&Path) -> PathBuf, run: F) { static FAKE_DOCKER_ENV_LOCK: OnceLock> = OnceLock::new(); let _guard = FAKE_DOCKER_ENV_LOCK .get_or_init(|| Mutex::new(())) .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); let fake_dir = TempDir::new().unwrap(); - let docker_path = write_fake_docker(fake_dir.path(), state_line); + let docker_path = write_docker(fake_dir.path()); let original_path = std::env::var_os("PATH"); let original_docker_bin = std::env::var_os("START_DOCKER_BIN"); let mut paths = vec![fake_dir.path().to_path_buf()]; @@ -337,6 +362,14 @@ fn with_fake_docker_inspect(state_line: &str, run: F) { } } +fn with_fake_docker_inspect(state_line: &str, run: F) { + with_fake_docker(|dir| write_fake_docker(dir, state_line), run); +} + +fn with_fake_docker_missing_container(run: F) { + with_fake_docker(write_missing_container_docker, run); +} + #[test] fn docker_oom_killed_is_exposed_in_status_and_list_output() { let temp_dir = TempDir::new().unwrap(); @@ -377,18 +410,23 @@ fn docker_oom_killed_is_exposed_in_status_and_list_output() { }); } +/// Issue #148: an OOM-killed session whose container is already gone and which +/// wrote no `Exit Code:` footer must still become terminal, with the +/// conventional SIGKILL code as the last-resort fallback. #[test] -fn docker_oom_killed_forces_terminal_status_even_when_container_reports_running() { +fn docker_oom_killed_is_terminal_once_the_container_is_gone() { let temp_dir = TempDir::new().unwrap(); let store = ExecutionStore::with_options(ExecutionStoreOptions { app_folder: Some(temp_dir.path().to_path_buf()), use_links: Some(false), verbose: false, }); - let record = docker_record(); + let mut record = docker_record(); + record.log_path = "/nonexistent-issue148.log".to_string(); + record.oom_killed = Some(true); store.save(&record).unwrap(); - with_fake_docker_inspect("true 137 true", || { + with_fake_docker_missing_container(|| { let json_result = query_status(Some(&store), "issue144-rust", Some("json")); assert!(json_result.success); let parsed: Value = serde_json::from_str(&json_result.output.unwrap()).unwrap(); @@ -400,8 +438,11 @@ fn docker_oom_killed_forces_terminal_status_even_when_container_reports_running( }); } +/// Issue #151: `State.OOMKilled` is a container-cgroup flag that is never +/// cleared, so it must stay an observation — only the container's own state may +/// decide `status` / `exitCode`. #[test] -fn docker_oom_killed_uses_137_when_running_state_has_no_terminal_exit_code() { +fn docker_oom_killed_keeps_running_container_executing() { let temp_dir = TempDir::new().unwrap(); let store = ExecutionStore::with_options(ExecutionStoreOptions { app_folder: Some(temp_dir.path().to_path_buf()), @@ -412,16 +453,90 @@ fn docker_oom_killed_uses_137_when_running_state_has_no_terminal_exit_code() { store.save(&record).unwrap(); with_fake_docker_inspect("true 0 true", || { + let json_result = query_status(Some(&store), "issue144-rust", Some("json")); + assert!(json_result.success); + let parsed: Value = serde_json::from_str(&json_result.output.unwrap()).unwrap(); + assert_eq!(parsed["status"], "executing"); + assert_eq!(parsed["exitCode"], Value::Null); + assert_eq!(parsed["oomKilled"], true); + assert_eq!(parsed["endTime"], Value::Null); + assert!(parsed.get("currentTime").is_some()); + + let list_result = list_executions(Some(&store), Some("json")); + assert!(list_result.success); + let listed: Value = serde_json::from_str(&list_result.output.unwrap()).unwrap(); + assert_eq!(listed["executions"][0]["status"], "executing"); + assert_eq!(listed["executions"][0]["exitCode"], Value::Null); + assert_eq!(listed["executions"][0]["oomKilled"], true); + }); +} + +#[test] +fn docker_oom_killed_never_synthesizes_137_for_a_running_container() { + let temp_dir = TempDir::new().unwrap(); + let store = ExecutionStore::with_options(ExecutionStoreOptions { + app_folder: Some(temp_dir.path().to_path_buf()), + use_links: Some(false), + verbose: false, + }); + let record = docker_record(); + store.save(&record).unwrap(); + + with_fake_docker_inspect("true 137 true", || { + let enriched = enrich_detached_status(&store.get("issue144-rust").unwrap()); + assert_eq!(enriched.status, ExecutionStatus::Executing); + assert_eq!(enriched.exit_code, None); + assert_eq!(enriched.oom_killed, Some(true)); + }); +} + +#[test] +fn docker_oom_killed_uses_the_container_exit_code_when_it_stops() { + let temp_dir = TempDir::new().unwrap(); + let store = ExecutionStore::with_options(ExecutionStoreOptions { + app_folder: Some(temp_dir.path().to_path_buf()), + use_links: Some(false), + verbose: false, + }); + let record = docker_record(); + store.save(&record).unwrap(); + + with_fake_docker_inspect("false 3 true", || { let json_result = query_status(Some(&store), "issue144-rust", Some("json")); assert!(json_result.success); let parsed: Value = serde_json::from_str(&json_result.output.unwrap()).unwrap(); assert_eq!(parsed["status"], "executed"); - assert_eq!(parsed["exitCode"], 137); + assert_eq!(parsed["exitCode"], 3); assert_eq!(parsed["oomKilled"], true); assert!(parsed.get("endTime").is_some()); }); } +#[test] +fn docker_oom_killed_prefers_the_log_footer_over_the_137_fallback() { + let temp_dir = TempDir::new().unwrap(); + let log_path = temp_dir.path().join("issue-151.log"); + std::fs::write(&log_path, "Finished: now\nExit Code: 0\n").unwrap(); + let store = ExecutionStore::with_options(ExecutionStoreOptions { + app_folder: Some(temp_dir.path().to_path_buf()), + use_links: Some(false), + verbose: false, + }); + let mut record = docker_record(); + record.log_path = log_path.to_string_lossy().to_string(); + record.oom_killed = Some(true); + store.save(&record).unwrap(); + + with_fake_docker_missing_container(|| { + let json_result = query_status(Some(&store), "issue144-rust", Some("json")); + assert!(json_result.success); + let parsed: Value = serde_json::from_str(&json_result.output.unwrap()).unwrap(); + assert_eq!(parsed["status"], "executed"); + assert_eq!(parsed["exitCode"], 0); + assert_eq!(parsed["oomKilled"], true); + }); +} + // ===== Issue #105: currentTime in formatter output ===== fn create_executing_record() -> ExecutionRecord {