diff --git a/js/.changeset/issue-148-detached-docker-oom-status.md b/js/.changeset/issue-148-detached-docker-oom-status.md new file mode 100644 index 0000000..6589446 --- /dev/null +++ b/js/.changeset/issue-148-detached-docker-oom-status.md @@ -0,0 +1,5 @@ +--- +'start-command': patch +--- + +Treat detached Docker sessions with `oomKilled` as terminal in status output, using Docker's exit code when available and 137 as the OOM fallback. diff --git a/js/src/lib/status-formatter.js b/js/src/lib/status-formatter.js index f40e9ad..7d9c006 100644 --- a/js/src/lib/status-formatter.js +++ b/js/src/lib/status-formatter.js @@ -62,6 +62,23 @@ function inspectDockerState(sessionName) { }; } +function isDetachedDockerRecord(record) { + const opts = record.options || {}; + return ( + opts.isolated === 'docker' && + opts.isolationMode === 'detached' && + Boolean(opts.sessionName) + ); +} + +function readDockerState(record) { + const opts = record.options || {}; + if (opts.isolated !== 'docker' || !opts.sessionName) { + return null; + } + return inspectDockerState(opts.sessionName); +} + /** * Best-effort terminal exit code reported by the isolation backend itself * (currently docker via `docker inspect .State.ExitCode`). Returns null when @@ -71,21 +88,23 @@ function inspectDockerState(sessionName) { * @returns {number|null} */ function readBackendExitCode(record) { - const opts = record.options || {}; - if (opts.isolated !== 'docker' || !opts.sessionName) { - return null; - } - const state = inspectDockerState(opts.sessionName); + const state = readDockerState(record); return state && !state.running ? state.exitCode : null; } -function readDockerOomKilled(record) { - const opts = record.options || {}; - if (opts.isolated !== 'docker' || !opts.sessionName) { - return null; +function resolveOomExitCode(footerExit, dockerState) { + if (footerExit !== null && footerExit !== undefined) { + return footerExit; + } + if ( + dockerState && + dockerState.exitCode !== null && + dockerState.exitCode !== undefined && + (!dockerState.running || dockerState.exitCode !== 0) + ) { + return dockerState.exitCode; } - const state = inspectDockerState(opts.sessionName); - return state ? state.oomKilled : null; + return 137; } /** @@ -172,8 +191,15 @@ function readExitCodeFromLog(logPath) { * @returns {Object} Possibly updated execution record */ function enrichDetachedStatus(record) { - const alive = isDetachedSessionAlive(record); const footerExit = readExitCodeFromLog(record.logPath); + const dockerState = isDetachedDockerRecord(record) + ? readDockerState(record) + : null; + const alive = isDetachedDockerRecord(record) + ? dockerState === null + ? null + : dockerState.running + : isDetachedSessionAlive(record); // Create a shallow copy to avoid mutating the original const cloneRecord = () => { @@ -182,6 +208,19 @@ function enrichDetachedStatus(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(); + } + return enriched; + } + if (alive === null) { // Liveness is unknown: the backend could not be probed (e.g. a detached // docker container that is not visible yet on a slow Docker-in-Docker host, @@ -204,9 +243,8 @@ function enrichDetachedStatus(record) { } const enriched = cloneRecord(); - const oomKilled = readDockerOomKilled(enriched); - if (oomKilled !== null) { - enriched.oomKilled = oomKilled; + if (dockerState?.oomKilled !== null && dockerState?.oomKilled !== undefined) { + enriched.oomKilled = dockerState.oomKilled; } if (alive && enriched.status === 'executed') { diff --git a/js/test/session-name-status.js b/js/test/session-name-status.js index 377702b..ad93b51 100644 --- a/js/test/session-name-status.js +++ b/js/test/session-name-status.js @@ -677,6 +677,65 @@ describe('Issue #144: detached docker OOMKilled status signal', () => { }); }); +describe('Issue #148: detached docker OOMKilled terminal status', () => { + let store; + + beforeEach(() => { + cleanupTestDir(); + store = new ExecutionStore({ + appFolder: TEST_APP_FOLDER, + useLinks: false, + }); + }); + + afterEach(() => { + cleanupTestDir(); + }); + + function saveDockerRecord() { + const record = new ExecutionRecord({ + command: 'sh -c "allocate memory"', + logPath: '/tmp/issue-148.log', + options: { + sessionName: 'issue148-oom', + isolated: 'docker', + isolationMode: 'detached', + }, + }); + store.save(record); + return record; + } + + it('treats oomKilled as terminal even while Docker still reports running', () => { + const record = saveDockerRecord(); + + withFakeDockerInspect('true 137 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.oomKilled).toBe(true); + expect(parsed.endTime).toBeTruthy(); + expect(parsed.currentTime).toBeUndefined(); + }); + }); + + it('uses 137 when oomKilled is terminal but Docker has no terminal exit code yet', () => { + const record = saveDockerRecord(); + + 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('executed'); + expect(parsed.exitCode).toBe(137); + expect(parsed.oomKilled).toBe(true); + expect(parsed.endTime).toBeTruthy(); + }); + }); +}); + describe('Issue #105: attachCurrentTime for executing status', () => { it('should add currentTime to serialization when status is executing', () => { const record = new ExecutionRecord({ diff --git a/rust/Cargo.lock b/rust/Cargo.lock index e4539dd..7b76404 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -437,7 +437,7 @@ checksum = "0c790de23124f9ab44544d7ac05d60440adc586479ce501c1d6d7da3cd8c9cf5" [[package]] name = "start-command" -version = "0.17.1" +version = "0.17.2" dependencies = [ "base64", "chrono", diff --git a/rust/changelog.d/issue-148-detached-docker-oom-status.md b/rust/changelog.d/issue-148-detached-docker-oom-status.md new file mode 100644 index 0000000..80fe7f3 --- /dev/null +++ b/rust/changelog.d/issue-148-detached-docker-oom-status.md @@ -0,0 +1,5 @@ +--- +bump: patch +--- + +Treat detached Docker sessions with OOMKilled as terminal in status output, using Docker's exit code when available and 137 as the OOM fallback. diff --git a/rust/src/lib/status_formatter.rs b/rust/src/lib/status_formatter.rs index 36839df..2eb38a9 100644 --- a/rust/src/lib/status_formatter.rs +++ b/rust/src/lib/status_formatter.rs @@ -14,6 +14,7 @@ use std::fs; use std::process::Command; /// Live state of a detached docker container by name. +#[derive(Clone, Copy)] struct DockerState { running: bool, exit_code: Option, @@ -64,16 +65,30 @@ fn inspect_docker_state(session_name: &str) -> Option { }) } +fn is_detached_docker_record(record: &ExecutionRecord) -> bool { + record.options.get("isolated").and_then(|v| v.as_str()) == Some("docker") + && record.options.get("isolationMode").and_then(|v| v.as_str()) == Some("detached") + && record + .options + .get("sessionName") + .and_then(|v| v.as_str()) + .is_some() +} + +fn read_docker_state(record: &ExecutionRecord) -> Option { + if record.options.get("isolated")?.as_str()? != "docker" { + return None; + } + let session_name = record.options.get("sessionName")?.as_str()?; + inspect_docker_state(session_name) +} + /// Best-effort terminal exit code reported by the isolation backend itself /// (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 { - if record.options.get("isolated")?.as_str()? != "docker" { - return None; - } - let session_name = record.options.get("sessionName")?.as_str()?; - let state = inspect_docker_state(session_name)?; + let state = read_docker_state(record)?; if state.running { None } else { @@ -81,12 +96,18 @@ fn read_backend_exit_code(record: &ExecutionRecord) -> Option { } } -fn read_docker_oom_killed(record: &ExecutionRecord) -> Option { - if record.options.get("isolated")?.as_str()? != "docker" { - return None; +fn resolve_oom_exit_code(footer_exit: Option, docker_state: Option) -> i32 { + if let Some(code) = footer_exit { + return code; } - let session_name = record.options.get("sessionName")?.as_str()?; - inspect_docker_state(session_name)?.oom_killed + if let Some(state) = docker_state { + if let Some(code) = state.exit_code { + if !state.running || code != 0 { + return code; + } + } + } + 137 } /// Check if a detached isolation session is still running @@ -156,9 +177,36 @@ fn read_exit_code_from_log(log_path: &str) -> Option { /// the session is still running, returns a copy with status "executing". pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { let footer_exit = read_exit_code_from_log(&record.log_path); + let is_detached_docker = is_detached_docker_record(record); + let docker_state = if is_detached_docker { + read_docker_state(record) + } else { + None + }; - let alive = match is_detached_session_alive(record) { - Some(v) => v, + if record.oom_killed == Some(true) + || docker_state.and_then(|state| state.oom_killed) == Some(true) + { + 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()); + } + return enriched; + } + + let alive = if is_detached_docker { + docker_state.map(|state| state.running) + } else { + is_detached_session_alive(record) + }; + + let alive = match alive { + Some(value) => value, None => { // Liveness is unknown: the backend could not be probed (e.g. a // detached docker container that is not visible yet on a slow @@ -183,7 +231,7 @@ pub fn enrich_detached_status(record: &ExecutionRecord) -> ExecutionRecord { }; let mut enriched = record.clone(); - if let Some(oom_killed) = read_docker_oom_killed(&enriched) { + if let Some(oom_killed) = docker_state.and_then(|state| state.oom_killed) { enriched.oom_killed = Some(oom_killed); } @@ -771,12 +819,8 @@ pub fn query_status( #[cfg(test)] mod tests { use super::*; - use crate::execution_store::{ExecutionRecordOptions, ExecutionStoreOptions}; + use crate::execution_store::ExecutionRecordOptions; use serde_json::json; - use std::collections::HashMap; - use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe}; - use std::path::{Path, PathBuf}; - use tempfile::TempDir; fn executing_record() -> ExecutionRecord { ExecutionRecord::with_options(ExecutionRecordOptions { @@ -793,99 +837,6 @@ mod tests { }) } - fn docker_record() -> ExecutionRecord { - let mut options = HashMap::new(); - options.insert( - "sessionName".to_string(), - Value::String("issue144-oom".to_string()), - ); - options.insert("isolated".to_string(), Value::String("docker".to_string())); - options.insert( - "isolationMode".to_string(), - Value::String("detached".to_string()), - ); - - ExecutionRecord::with_options(ExecutionRecordOptions { - command: "sh -c 'exit 0'".to_string(), - uuid: Some("issue144-rust".to_string()), - log_path: Some("/tmp/issue144.log".to_string()), - options: Some(options), - ..Default::default() - }) - } - - fn write_fake_docker(fake_dir: &Path, state_line: &str) -> PathBuf { - #[cfg(windows)] - { - let script = [ - "@echo off", - "if not \"%1\"==\"inspect\" exit /b 1", - "echo %3 | findstr /C:\"State.Pid\" >nul", - "if %errorlevel%==0 (", - " echo fake-container-id 4321", - " exit /b 0", - ")", - &format!("echo {}", state_line), - "exit /b 0", - "", - ] - .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", - "[ \"$1\" = \"inspect\" ] || exit 1", - "case \"$3\" in", - " *State.Pid*) echo \"fake-container-id 4321\" ;;", - &format!(" *) echo \"{}\" ;;", state_line), - "esac", - "", - ] - .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_inspect(state_line: &str, run: F) { - let fake_dir = TempDir::new().unwrap(); - let docker_path = write_fake_docker(fake_dir.path(), state_line); - 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()]; - if let Some(existing) = original_path.as_ref() { - paths.extend(std::env::split_paths(existing)); - } - let joined = std::env::join_paths(paths).unwrap(); - std::env::set_var("PATH", &joined); - std::env::set_var("START_DOCKER_BIN", &docker_path); - let result = catch_unwind(AssertUnwindSafe(run)); - if let Some(path) = original_path { - std::env::set_var("PATH", path); - } else { - std::env::remove_var("PATH"); - } - if let Some(path) = original_docker_bin { - std::env::set_var("START_DOCKER_BIN", path); - } else { - std::env::remove_var("START_DOCKER_BIN"); - } - if let Err(payload) = result { - resume_unwind(payload); - } - } - #[test] fn links_notation_indents_nested_process_id_arrays() { let process_ids = json!({ @@ -914,44 +865,4 @@ mod tests { output ); } - - #[test] - fn docker_oom_killed_is_exposed_in_status_and_list_output() { - 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 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"], "executed"); - assert_eq!(parsed["exitCode"], 0); - assert_eq!(parsed["oomKilled"], true); - - let links_result = query_status(Some(&store), "issue144-rust", Some("links-notation")); - assert!(links_result.success); - assert!(links_result.output.unwrap().contains(" oomKilled true")); - - let text_result = query_status(Some(&store), "issue144-rust", Some("text")); - assert!(text_result.success); - assert!(text_result - .output - .unwrap() - .contains("OOM Killed: true")); - - 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["count"], 1); - assert_eq!(listed["executions"][0]["status"], "executed"); - assert_eq!(listed["executions"][0]["exitCode"], 0); - assert_eq!(listed["executions"][0]["oomKilled"], true); - }); - } } diff --git a/rust/tests/status_formatter.rs b/rust/tests/status_formatter.rs index e5dd11e..12d7d10 100644 --- a/rust/tests/status_formatter.rs +++ b/rust/tests/status_formatter.rs @@ -2,6 +2,7 @@ //! //! Tests for execution record formatting in various output formats. +use serde_json::Value; use start_command::{ attach_current_time, format_record, format_record_as_links_notation, format_record_as_links_notation_with_current_time, format_record_as_text, @@ -9,6 +10,10 @@ use start_command::{ list_executions, query_status, ExecutionRecord, ExecutionRecordOptions, ExecutionStatus, ExecutionStore, ExecutionStoreOptions, }; +use std::collections::HashMap; +use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe}; +use std::path::{Path, PathBuf}; +use std::sync::{Mutex, OnceLock}; use tempfile::TempDir; fn create_test_record() -> ExecutionRecord { @@ -234,6 +239,189 @@ fn test_list_executions_no_store() { assert!(result.error.unwrap().contains("tracking is disabled")); } +fn docker_record() -> ExecutionRecord { + let mut options = HashMap::new(); + options.insert( + "sessionName".to_string(), + Value::String("issue144-oom".to_string()), + ); + options.insert("isolated".to_string(), Value::String("docker".to_string())); + options.insert( + "isolationMode".to_string(), + Value::String("detached".to_string()), + ); + + ExecutionRecord::with_options(ExecutionRecordOptions { + command: "sh -c 'exit 0'".to_string(), + uuid: Some("issue144-rust".to_string()), + log_path: Some("/tmp/issue144.log".to_string()), + options: Some(options), + ..Default::default() + }) +} + +fn write_fake_docker(fake_dir: &Path, state_line: &str) -> PathBuf { + #[cfg(windows)] + { + let script = [ + "@echo off", + "if not \"%1\"==\"inspect\" exit /b 1", + "echo %3 | findstr /C:\"State.Pid\" >nul", + "if %errorlevel%==0 (", + " echo fake-container-id 4321", + " exit /b 0", + ")", + &format!("echo {}", state_line), + "exit /b 0", + "", + ] + .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", + "[ \"$1\" = \"inspect\" ] || exit 1", + "case \"$3\" in", + " *State.Pid*) echo \"fake-container-id 4321\" ;;", + &format!(" *) echo \"{}\" ;;", state_line), + "esac", + "", + ] + .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_inspect(state_line: &str, 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 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()]; + if let Some(existing) = original_path.as_ref() { + paths.extend(std::env::split_paths(existing)); + } + let joined = std::env::join_paths(paths).unwrap(); + std::env::set_var("PATH", &joined); + std::env::set_var("START_DOCKER_BIN", &docker_path); + let result = catch_unwind(AssertUnwindSafe(run)); + if let Some(path) = original_path { + std::env::set_var("PATH", path); + } else { + std::env::remove_var("PATH"); + } + if let Some(path) = original_docker_bin { + std::env::set_var("START_DOCKER_BIN", path); + } else { + std::env::remove_var("START_DOCKER_BIN"); + } + if let Err(payload) = result { + resume_unwind(payload); + } +} + +#[test] +fn docker_oom_killed_is_exposed_in_status_and_list_output() { + 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 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"], "executed"); + assert_eq!(parsed["exitCode"], 0); + assert_eq!(parsed["oomKilled"], true); + + let links_result = query_status(Some(&store), "issue144-rust", Some("links-notation")); + assert!(links_result.success); + assert!(links_result.output.unwrap().contains(" oomKilled true")); + + let text_result = query_status(Some(&store), "issue144-rust", Some("text")); + assert!(text_result.success); + assert!(text_result + .output + .unwrap() + .contains("OOM Killed: true")); + + 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["count"], 1); + assert_eq!(listed["executions"][0]["status"], "executed"); + assert_eq!(listed["executions"][0]["exitCode"], 0); + assert_eq!(listed["executions"][0]["oomKilled"], true); + }); +} + +#[test] +fn docker_oom_killed_forces_terminal_status_even_when_container_reports_running() { + 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 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["oomKilled"], true); + assert!(parsed.get("endTime").is_some()); + assert!(parsed.get("currentTime").is_none()); + }); +} + +#[test] +fn docker_oom_killed_uses_137_when_running_state_has_no_terminal_exit_code() { + 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 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"], "executed"); + assert_eq!(parsed["exitCode"], 137); + assert_eq!(parsed["oomKilled"], true); + assert!(parsed.get("endTime").is_some()); + }); +} + // ===== Issue #105: currentTime in formatter output ===== fn create_executing_record() -> ExecutionRecord {