From 428b85924b0cb2aad3dba4e3055b55a59e6ddfc3 Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Thu, 20 Aug 2026 14:06:07 +0100 Subject: [PATCH 1/3] fix(compute): recover Error-phase sandboxes on gateway startup Sandboxes whose container exited on its own (e.g. SIGTERM from a Podman or Docker machine restart) were left stuck in the Error phase even though the container could be restarted. The startup sweep skipped every Error-phase sandbox unconditionally. Include Error-phase sandboxes in the startup sweep when their Ready condition reason marks a container exit or stop. If the driver restarts the container, move the sandbox back to Provisioning with a Resumed condition; if the container is gone or the start fails, leave the Error state untouched. Genuine (non-container-exit) errors are still skipped. Container-exit restart is handled by the drivers' existing idempotent start_sandbox path, which already restarts stopped/exited containers. The ContainerExited/ContainerStopped condition reasons are promoted to shared constants in openshell-core so the gateway and drivers agree on the recovery signal. Fixes #2179 Signed-off-by: Ian Miller --- crates/openshell-core/src/driver_utils.rs | 12 + crates/openshell-driver-docker/src/lib.rs | 7 +- crates/openshell-driver-podman/src/watcher.rs | 3 +- crates/openshell-server/src/compute/mod.rs | 264 ++++++++++++++++-- 4 files changed, 264 insertions(+), 22 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 8f88c0cd4a..595983dd15 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -45,6 +45,18 @@ pub fn openshell_sandbox_label_selector() -> String { format!("{LABEL_MANAGED_BY}={LABEL_MANAGED_BY_VALUE},{LABEL_SANDBOX_ID}") } +// --------------------------------------------------------------------------- +// Sandbox condition reason strings set by compute drivers. +// --------------------------------------------------------------------------- + +/// Ready-condition reason when a container exits on its own (e.g. SIGTERM +/// from a machine restart, OOM kill, application crash). +pub const CONDITION_EXITED: &str = "ContainerExited"; + +/// Ready-condition reason when a container is explicitly stopped via the +/// runtime API (e.g. `podman stop`, gateway-initiated shutdown). +pub const CONDITION_STOPPED: &str = "ContainerStopped"; + // --------------------------------------------------------------------------- /// Path to the sandbox supervisor binary inside the container image. diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 33acf1a2c6..f91afb09aa 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -3116,7 +3116,12 @@ fn container_ready_condition( ("False", "ContainerPaused", "Container is paused", false) } ContainerSummaryStateEnum::EXITED => { - ("False", "ContainerExited", "Container exited", false) + ( + "False", + openshell_core::driver_utils::CONDITION_EXITED, + "Container exited", + false, + ) } ContainerSummaryStateEnum::DEAD => ("False", "ContainerDead", "Container is dead", false), } diff --git a/crates/openshell-driver-podman/src/watcher.rs b/crates/openshell-driver-podman/src/watcher.rs index 3e98d16271..1268646485 100644 --- a/crates/openshell-driver-podman/src/watcher.rs +++ b/crates/openshell-driver-podman/src/watcher.rs @@ -26,8 +26,7 @@ use tracing::{debug, info, warn}; // Condition reason constants shared across event-building paths. const CONDITION_RUNNING: &str = "ContainerRunning"; const CONDITION_STARTING: &str = "ContainerStarting"; -const CONDITION_EXITED: &str = "ContainerExited"; -const CONDITION_STOPPED: &str = "ContainerStopped"; +use openshell_core::driver_utils::{CONDITION_EXITED, CONDITION_STOPPED}; pub type WatchStream = Pin> + Send>>; diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 30a1303bd5..e605c3b8fd 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -2145,8 +2145,11 @@ impl ComputeRuntime { /// /// `StartSandbox` is idempotent, so call it for every persisted phase that /// requires running compute for drivers that request gateway-managed - /// lifecycle. Stable stopped, deleting, and error states are deliberately - /// left alone. + /// lifecycle. Stable stopped and deleting states are deliberately left + /// alone. Error-phase sandboxes are included only when their Ready + /// condition indicates a container exit or stop (e.g. after a Podman/Docker + /// machine restart): if the container still exists it is restarted and the + /// sandbox is moved back to `Provisioning`; otherwise it stays in `Error`. /// /// Should be called once at gateway startup, before watchers spawn, /// so the watch loop sees the post-start state on its first poll. @@ -2159,6 +2162,7 @@ impl ComputeRuntime { let sandbox_ids = self.list_persisted_sandbox_ids("gateway startup").await?; let mut started = 0usize; + let mut recovered = 0usize; let mut missing = 0usize; let mut failed = 0usize; @@ -2179,7 +2183,9 @@ impl ComputeRuntime { }; let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); - if !sandbox_phase_should_be_running(phase) { + let recoverable_error = + phase == SandboxPhase::Error && is_recoverable_error_reason(&sandbox); + if !sandbox_phase_should_be_running(phase) && !recoverable_error { continue; } @@ -2201,13 +2207,22 @@ impl ComputeRuntime { .await { Ok(_) => { + let did_recover = if recoverable_error { + self.clear_recoverable_error(&sandbox).await + } else { + false + }; info!( sandbox_id = %sandbox.object_id(), sandbox_name = %sandbox.object_name(), ?phase, + recovered = did_recover, "Started sandbox during gateway startup" ); started += 1; + if did_recover { + recovered += 1; + } } Err(err) if err.code() == Code::NotFound => { // Backend resource is gone but the store still @@ -2220,12 +2235,14 @@ impl ComputeRuntime { sandbox_name = %sandbox.object_name(), "Cannot start sandbox: backend resource is missing" ); - self.mark_sandbox_error( - &sandbox, - "BackendResourceMissing", - "Sandbox compute resource disappeared while the gateway was offline", - ) - .await; + if !recoverable_error { + self.mark_sandbox_error( + &sandbox, + "BackendResourceMissing", + "Sandbox compute resource disappeared while the gateway was offline", + ) + .await; + } missing += 1; } Err(err) => { @@ -2235,15 +2252,17 @@ impl ComputeRuntime { error = %err, "Failed to start sandbox during gateway startup" ); - self.mark_sandbox_error( - &sandbox, - "StartFailed", - &format!( - "Failed to start sandbox during gateway startup: {}", - err.message() - ), - ) - .await; + if !recoverable_error { + self.mark_sandbox_error( + &sandbox, + "StartFailed", + &format!( + "Failed to start sandbox during gateway startup: {}", + err.message() + ), + ) + .await; + } failed += 1; } } @@ -2252,6 +2271,7 @@ impl ComputeRuntime { if started > 0 || missing > 0 || failed > 0 { info!( started, + recovered, missing_backend = missing, failed, "Sandbox start sweep complete" @@ -2421,6 +2441,48 @@ impl ComputeRuntime { } } + /// Clear a recoverable `Error` state after the underlying container has + /// been restarted during the startup sweep. Moves the sandbox back to + /// `Provisioning` with a `Resumed` Ready condition. Returns `true` if the + /// store update succeeded. + async fn clear_recoverable_error(&self, sandbox: &Sandbox) -> bool { + let _guard = self.sync_lock.lock().await; + let sandbox_id = sandbox.object_id().to_string(); + match self + .store + .update_message_cas::(&sandbox_id, 0, |s| { + s.set_phase(SandboxPhase::Provisioning as i32); + let name = s.object_name().to_string(); + upsert_ready_condition( + &mut s.status, + &name, + SandboxCondition { + r#type: "Ready".to_string(), + status: "False".to_string(), + reason: "Resumed".to_string(), + message: "Sandbox recovered during gateway startup".to_string(), + last_transition_time: String::new(), + }, + ); + }) + .await + { + Ok(updated) => { + self.sandbox_index.update_from_sandbox(&updated); + self.sandbox_watch_bus.notify(&sandbox_id); + true + } + Err(err) => { + warn!( + sandbox_id = %sandbox_id, + error = %err, + "Failed to clear sandbox error state during startup resume" + ); + false + } + } + } + async fn lease_coordinator(self: Arc, mut shutdown_rx: watch::Receiver) { use lease::{LEASE_ACQUIRE_INTERVAL, LEASE_TTL, ReconcilerLease}; @@ -4046,6 +4108,19 @@ fn sandbox_phase_should_be_running(phase: SandboxPhase) -> bool { ) } +/// Error-phase sandboxes are only eligible for startup recovery when their +/// Ready condition reason indicates a container exit or stop — i.e. the +/// runtime went away (machine restart, daemon restart) rather than a genuine +/// application or infrastructure failure. +fn is_recoverable_error_reason(sandbox: &Sandbox) -> bool { + use openshell_core::driver_utils::{CONDITION_EXITED, CONDITION_STOPPED}; + sandbox + .status + .as_ref() + .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) + .is_some_and(|c| c.reason == CONDITION_EXITED || c.reason == CONDITION_STOPPED) +} + fn is_terminal_failure_reason(reason: &str) -> bool { let reason = reason.to_ascii_lowercase(); let transient_reasons = [ @@ -5199,6 +5274,19 @@ mod tests { assert_eq!(status.exit_code, Some(1)); } + fn error_sandbox_record(id: &str, name: &str, reason: &str) -> Sandbox { + let mut sandbox = sandbox_record(id, name, SandboxPhase::Error); + let status = sandbox.status.get_or_insert_with(Default::default); + status.conditions.push(SandboxCondition { + r#type: "Ready".to_string(), + status: "False".to_string(), + reason: reason.to_string(), + message: String::new(), + last_transition_time: String::new(), + }); + sandbox + } + fn ssh_session_record(id: &str, sandbox_id: &str) -> SshSession { SshSession { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { @@ -8789,11 +8877,29 @@ mod tests { ("sb-stopping", "stopping", SandboxPhase::Stopping), ("sb-stopped", "stopped", SandboxPhase::Stopped), ("sb-deleting", "deleting", SandboxPhase::Deleting), - ("sb-error", "error", SandboxPhase::Error), ] { let sandbox = sandbox_record(id, name, phase); runtime.store.put_message(&sandbox).await.unwrap(); } + // A non-recoverable error is skipped; a container-exit error is retried. + runtime + .store + .put_message(&error_sandbox_record( + "sb-error-perm", + "error-perm", + "BackendResourceMissing", + )) + .await + .unwrap(); + runtime + .store + .put_message(&error_sandbox_record( + "sb-error-exit", + "error-exit", + "ContainerExited", + )) + .await + .unwrap(); runtime.start_persisted_sandboxes().await.unwrap(); @@ -8806,6 +8912,7 @@ mod tests { assert_eq!( called_ids, vec![ + "sb-error-exit".to_string(), "sb-prov".to_string(), "sb-ready".to_string(), "sb-unknown".to_string(), @@ -8975,6 +9082,125 @@ mod tests { } } + #[tokio::test] + async fn start_persisted_sandboxes_recovers_error_phase_when_container_exists() { + let driver = ControlledDriver::new(); + // Default start outcome is Ok: the container is restarted successfully. + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; + + let sandbox = error_sandbox_record("sb-err-recover", "recover", "ContainerExited"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + assert_eq!(driver.start_calls(), 1); + + let stored = runtime + .store + .get_message::("sb-err-recover") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Provisioning + ); + let ready = stored + .status + .as_ref() + .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) + .expect("Ready condition present"); + assert_eq!(ready.reason, "Resumed"); + } + + #[tokio::test] + async fn start_persisted_sandboxes_leaves_error_when_container_missing() { + let driver = ControlledDriver::new(); + driver.set_start_outcome(ControlledLifecycleOutcome::NotFound); + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; + + let sandbox = error_sandbox_record("sb-err-gone", "gone", "ContainerExited"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + // Recovery was attempted, but the error state is preserved untouched. + assert_eq!(driver.start_calls(), 1); + + let stored = runtime + .store + .get_message::("sb-err-gone") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + let ready = stored + .status + .as_ref() + .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) + .expect("Ready condition present"); + assert_eq!(ready.reason, "ContainerExited"); + } + + #[tokio::test] + async fn start_persisted_sandboxes_leaves_error_when_start_fails() { + let driver = ControlledDriver::new(); + driver.set_start_outcome(ControlledLifecycleOutcome::Error("runtime angry")); + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; + + let sandbox = error_sandbox_record("sb-err-fail", "fail", "ContainerStopped"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + assert_eq!(driver.start_calls(), 1); + + let stored = runtime + .store + .get_message::("sb-err-fail") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + let ready = stored + .status + .as_ref() + .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) + .expect("Ready condition present"); + assert_eq!(ready.reason, "ContainerStopped"); + } + + #[tokio::test] + async fn start_persisted_sandboxes_skips_non_recoverable_error() { + let driver = ControlledDriver::new(); + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; + + let sandbox = error_sandbox_record("sb-err-perm", "perm", "BackendResourceMissing"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + // A non-container-exit error is never retried. + assert_eq!(driver.start_calls(), 0); + + let stored = runtime + .store + .get_message::("sb-err-perm") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + } + #[test] fn build_platform_config_inverts_user_namespaces_to_host_users() { use prost_types::value::Kind; From b58ecee398d26a49816edb74852a74c90cddb8f0 Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Tue, 25 Aug 2026 12:54:36 +0100 Subject: [PATCH 2/3] fix(compute): keep ordinary container exits terminal during startup recovery Startup recovery treated every non-OOM container exit as recoverable, lumping application crashes and non-zero exits together with machine/daemon-restart signal kills under the shared `ContainerExited` Ready-condition reason. On the next gateway startup it restarted those crashed workloads and replaced the terminal error with `Resumed`, erasing the failure signal and repeatedly reviving crash-prone sandboxes. Introduce a distinct `ContainerRuntimeRestart` reason for containers terminated by an external signal (exit 137/143 = SIGKILL/SIGTERM), which is the signature of a machine/daemon restart. The Podman inspect-based classification emits it; the startup recovery gate now recovers only `ContainerRuntimeRestart` and `ContainerStopped`. Ordinary `ContainerExited` records stay terminal, so genuine failures keep their error signal. Because only the new reason is recoverable and older gateways never persisted it, exits recorded before this change also stay terminal after an upgrade. Add regression coverage: a generic `ContainerExited` error is not relaunched, a signal-kill classifies as `ContainerRuntimeRestart`, and the startup sweep recovers only the runtime-restart record. Signed-off-by: Ian Miller --- crates/openshell-core/src/driver_utils.rs | 16 +++- crates/openshell-driver-docker/src/lib.rs | 14 ++-- crates/openshell-driver-podman/src/watcher.rs | 60 +++++++++++--- crates/openshell-server/src/compute/mod.rs | 78 +++++++++++++++---- 4 files changed, 136 insertions(+), 32 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 595983dd15..f912a6af90 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -49,10 +49,22 @@ pub fn openshell_sandbox_label_selector() -> String { // Sandbox condition reason strings set by compute drivers. // --------------------------------------------------------------------------- -/// Ready-condition reason when a container exits on its own (e.g. SIGTERM -/// from a machine restart, OOM kill, application crash). +/// Ready-condition reason when a container exits on its own. +/// +/// Covers an ordinary application exit or crash (exit 0, a non-zero error code, +/// or an uncaught fault). This is a terminal reason: gateway startup does NOT +/// auto-restart it, so a genuine failure keeps its error signal instead of +/// being relaunched. pub const CONDITION_EXITED: &str = "ContainerExited"; +/// Ready-condition reason when a container was terminated by an external signal. +/// +/// SIGKILL/SIGTERM (exit 137/143) is what a Podman/Docker machine or daemon +/// restart does to running containers. Distinct from `CONDITION_EXITED` so +/// gateway startup can recover machine-restart victims while leaving ordinary +/// application exits terminal. +pub const CONDITION_RUNTIME_RESTART: &str = "ContainerRuntimeRestart"; + /// Ready-condition reason when a container is explicitly stopped via the /// runtime API (e.g. `podman stop`, gateway-initiated shutdown). pub const CONDITION_STOPPED: &str = "ContainerStopped"; diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index f91afb09aa..0fa87808d9 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -3115,14 +3115,12 @@ fn container_ready_condition( ContainerSummaryStateEnum::PAUSED => { ("False", "ContainerPaused", "Container is paused", false) } - ContainerSummaryStateEnum::EXITED => { - ( - "False", - openshell_core::driver_utils::CONDITION_EXITED, - "Container exited", - false, - ) - } + ContainerSummaryStateEnum::EXITED => ( + "False", + openshell_core::driver_utils::CONDITION_EXITED, + "Container exited", + false, + ), ContainerSummaryStateEnum::DEAD => ("False", "ContainerDead", "Container is dead", false), } } diff --git a/crates/openshell-driver-podman/src/watcher.rs b/crates/openshell-driver-podman/src/watcher.rs index 1268646485..0c94bf72b2 100644 --- a/crates/openshell-driver-podman/src/watcher.rs +++ b/crates/openshell-driver-podman/src/watcher.rs @@ -26,7 +26,9 @@ use tracing::{debug, info, warn}; // Condition reason constants shared across event-building paths. const CONDITION_RUNNING: &str = "ContainerRunning"; const CONDITION_STARTING: &str = "ContainerStarting"; -use openshell_core::driver_utils::{CONDITION_EXITED, CONDITION_STOPPED}; +use openshell_core::driver_utils::{ + CONDITION_EXITED, CONDITION_RUNTIME_RESTART, CONDITION_STOPPED, +}; pub type WatchStream = Pin> + Send>>; @@ -444,15 +446,29 @@ fn condition_from_state(state: &ContainerState) -> DriverCondition { }, "created" => ("False", "ContainerCreated", String::new()), "exited" | "stopped" => { - let msg = if state.oom_killed { - "Container was killed by the OOM killer".to_string() - } else { - format!("Container exited with code {}", state.exit_code) - }; - let reason = if state.oom_killed { - "OOMKilled" + // Exit codes 137 (128+SIGKILL) and 143 (128+SIGTERM) mean the + // container was terminated by an external signal rather than + // exiting on its own — the signature of a machine/daemon restart + // killing running containers. Those are recoverable at gateway + // startup; ordinary application exits (0, non-zero, faults) are not. + let (reason, msg) = if state.oom_killed { + ( + "OOMKilled", + "Container was killed by the OOM killer".to_string(), + ) + } else if matches!(state.exit_code, 137 | 143) { + ( + CONDITION_RUNTIME_RESTART, + format!( + "Container terminated by signal (exit code {})", + state.exit_code + ), + ) } else { - CONDITION_EXITED + ( + CONDITION_EXITED, + format!("Container exited with code {}", state.exit_code), + ) }; ("False", reason, msg) } @@ -599,6 +615,32 @@ mod tests { assert!(cond.message.contains("code 1")); } + #[test] + fn condition_signal_kill_is_runtime_restart() { + // 137 (128+SIGKILL) and 143 (128+SIGTERM) are external terminations — + // the signature of a machine/daemon restart. They classify as + // recoverable `ContainerRuntimeRestart`, distinct from an ordinary + // application exit. + for exit_code in [137, 143] { + let state = ContainerState { + status: "exited".to_string(), + running: false, + exit_code, + oom_killed: false, + health: None, + started_at: None, + finished_at: Some("2026-04-14T12:30:00Z".to_string()), + }; + let cond = condition_from_state(&state); + assert_eq!(cond.status, "False"); + assert_eq!( + cond.reason, "ContainerRuntimeRestart", + "exit code {exit_code} should classify as runtime restart" + ); + assert!(cond.message.contains(&format!("code {exit_code}"))); + } + } + #[test] fn short_id_truncates() { assert_eq!(short_id("abc123def456789"), "abc123def456"); diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index e605c3b8fd..73fe2ff2e5 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -2147,9 +2147,11 @@ impl ComputeRuntime { /// requires running compute for drivers that request gateway-managed /// lifecycle. Stable stopped and deleting states are deliberately left /// alone. Error-phase sandboxes are included only when their Ready - /// condition indicates a container exit or stop (e.g. after a Podman/Docker - /// machine restart): if the container still exists it is restarted and the - /// sandbox is moved back to `Provisioning`; otherwise it stays in `Error`. + /// condition indicates the runtime went away underneath a running container + /// — a signal-kill from a machine/daemon restart or an explicit runtime + /// stop. If the container still exists it is restarted and the sandbox is + /// moved back to `Provisioning`; otherwise it stays in `Error`. Ordinary + /// application exits and crashes stay terminal and are not relaunched. /// /// Should be called once at gateway startup, before watchers spawn, /// so the watch loop sees the post-start state on its first poll. @@ -4109,16 +4111,19 @@ fn sandbox_phase_should_be_running(phase: SandboxPhase) -> bool { } /// Error-phase sandboxes are only eligible for startup recovery when their -/// Ready condition reason indicates a container exit or stop — i.e. the -/// runtime went away (machine restart, daemon restart) rather than a genuine -/// application or infrastructure failure. +/// Ready condition reason indicates the runtime went away underneath a running +/// container — a machine/daemon restart that terminated it by signal +/// (`CONDITION_RUNTIME_RESTART`) or an explicit runtime stop +/// (`CONDITION_STOPPED`). Ordinary application exits (`CONDITION_EXITED`, which +/// covers crashes and non-zero exits) stay terminal so a genuine failure keeps +/// its error signal instead of being relaunched on every gateway startup. fn is_recoverable_error_reason(sandbox: &Sandbox) -> bool { - use openshell_core::driver_utils::{CONDITION_EXITED, CONDITION_STOPPED}; + use openshell_core::driver_utils::{CONDITION_RUNTIME_RESTART, CONDITION_STOPPED}; sandbox .status .as_ref() .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) - .is_some_and(|c| c.reason == CONDITION_EXITED || c.reason == CONDITION_STOPPED) + .is_some_and(|c| c.reason == CONDITION_RUNTIME_RESTART || c.reason == CONDITION_STOPPED) } fn is_terminal_failure_reason(reason: &str) -> bool { @@ -8881,7 +8886,9 @@ mod tests { let sandbox = sandbox_record(id, name, phase); runtime.store.put_message(&sandbox).await.unwrap(); } - // A non-recoverable error is skipped; a container-exit error is retried. + // Terminal errors are skipped: a backend-missing error and an ordinary + // container exit (crash) both stay in Error. Only a signal-kill from a + // machine/daemon restart is retried. runtime .store .put_message(&error_sandbox_record( @@ -8900,6 +8907,15 @@ mod tests { )) .await .unwrap(); + runtime + .store + .put_message(&error_sandbox_record( + "sb-error-restart", + "error-restart", + "ContainerRuntimeRestart", + )) + .await + .unwrap(); runtime.start_persisted_sandboxes().await.unwrap(); @@ -8912,7 +8928,7 @@ mod tests { assert_eq!( called_ids, vec![ - "sb-error-exit".to_string(), + "sb-error-restart".to_string(), "sb-prov".to_string(), "sb-ready".to_string(), "sb-unknown".to_string(), @@ -9088,7 +9104,7 @@ mod tests { // Default start outcome is Ok: the container is restarted successfully. let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; - let sandbox = error_sandbox_record("sb-err-recover", "recover", "ContainerExited"); + let sandbox = error_sandbox_record("sb-err-recover", "recover", "ContainerRuntimeRestart"); runtime.store.put_message(&sandbox).await.unwrap(); runtime.start_persisted_sandboxes().await.unwrap(); @@ -9119,7 +9135,7 @@ mod tests { driver.set_start_outcome(ControlledLifecycleOutcome::NotFound); let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; - let sandbox = error_sandbox_record("sb-err-gone", "gone", "ContainerExited"); + let sandbox = error_sandbox_record("sb-err-gone", "gone", "ContainerRuntimeRestart"); runtime.store.put_message(&sandbox).await.unwrap(); runtime.start_persisted_sandboxes().await.unwrap(); @@ -9142,7 +9158,7 @@ mod tests { .as_ref() .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) .expect("Ready condition present"); - assert_eq!(ready.reason, "ContainerExited"); + assert_eq!(ready.reason, "ContainerRuntimeRestart"); } #[tokio::test] @@ -9201,6 +9217,42 @@ mod tests { ); } + #[tokio::test] + async fn start_persisted_sandboxes_leaves_generic_container_exit_terminal() { + // A container that exited on its own — an ordinary application crash or + // non-zero exit — is stored as Error with `ContainerExited`. Startup + // recovery must NOT relaunch it: doing so would erase the failure + // signal and repeatedly revive a crash-prone workload. Only a + // signal-kill (`ContainerRuntimeRestart`) from a machine/daemon restart + // is recoverable. + let driver = ControlledDriver::new(); + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "podman").await; + + let sandbox = error_sandbox_record("sb-err-crash", "crash", "ContainerExited"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + assert_eq!(driver.start_calls(), 0); + + let stored = runtime + .store + .get_message::("sb-err-crash") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + let ready = stored + .status + .as_ref() + .and_then(|s| s.conditions.iter().find(|c| c.r#type == "Ready")) + .expect("Ready condition present"); + assert_eq!(ready.reason, "ContainerExited"); + } + #[test] fn build_platform_config_inverts_user_namespaces_to_host_users() { use prost_types::value::Kind; From b778f01f568f747923c8c1b4cf1c252e6d4d80e0 Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Tue, 25 Aug 2026 13:15:48 +0100 Subject: [PATCH 3/3] fix(docker): reclassify signal-killed containers as runtime restart Mirror the Podman classification in the Docker driver so that externally signal-killed containers (exit 137/143, non-OOM) are persisted with the ContainerRuntimeRestart ready reason and become eligible for startup recovery, while ordinary exits and OOM kills stay terminal. current_snapshots now inspects EXITED containers to obtain the exit code and OOM flag, then applies the shared exit classification. Without the inspect step the list-summary path lacks an exit code, so the recovery gate could never fire for Docker. Adds unit tests covering signal-kill reclassification (137/143), ordinary exit staying terminal (exit 1), and OOM staying terminal despite exit 137. Signed-off-by: Ian Miller --- crates/openshell-driver-docker/src/lib.rs | 78 +++++++++++++++---- crates/openshell-driver-docker/src/tests.rs | 85 +++++++++++++++++++++ 2 files changed, 149 insertions(+), 14 deletions(-) diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 0fa87808d9..0e189ce216 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -24,10 +24,10 @@ use openshell_core::config::{ }; use openshell_core::driver_mounts; use openshell_core::driver_utils::{ - LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, - LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, SUPERVISOR_IMAGE_BINARY_PATH, - extract_first_tar_entry, supervisor_image_should_refresh, temp_extract_container_name, - validate_linux_elf_binary, write_cache_binary_atomic, + CONDITION_EXITED, CONDITION_RUNTIME_RESTART, LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, + LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, + SUPERVISOR_IMAGE_BINARY_PATH, extract_first_tar_entry, supervisor_image_should_refresh, + temp_extract_container_name, validate_linux_elf_binary, write_cache_binary_atomic, }; use openshell_core::gpu::{ CdiGpuDefaultSelector, CdiGpuInventory, CdiGpuSelectionError, driver_gpu_requirements, @@ -688,10 +688,37 @@ impl DockerComputeDriver { async fn current_snapshots(&self) -> Result, Status> { let containers = self.list_managed_container_summaries().await?; - let container_sandboxes = containers - .iter() - .filter_map(sandbox_from_container_summary) - .collect::>(); + let mut container_sandboxes = Vec::with_capacity(containers.len()); + for summary in &containers { + let Some(mut sandbox) = sandbox_from_container_summary(summary) else { + continue; + }; + // Docker's list summary carries no exit code, so an exited + // container is reported as the generic terminal `ContainerExited`. + // Inspect it to tell a machine/daemon-restart signal kill apart + // from an ordinary application exit, mirroring the Podman driver, + // so startup recovery can revive restart victims while leaving + // crashes terminal. + if summary.state == Some(ContainerSummaryStateEnum::EXITED) + && let Some(container_id) = summary.id.as_deref() + { + match self.docker.inspect_container(container_id, None).await { + Ok(inspected) => { + if let Some(state) = inspected.state.as_ref() { + apply_docker_exit_classification(&mut sandbox, state); + } + } + Err(err) => { + debug!( + container_id, + error = %err, + "Could not inspect exited Docker container to classify its exit" + ); + } + } + } + container_sandboxes.push(sandbox); + } let mut by_id = self.pending_snapshot_map().await; for sandbox in container_sandboxes { by_id.insert(sandbox.id.clone(), sandbox); @@ -3092,6 +3119,34 @@ fn driver_status_from_summary( } } +/// Refine an exited Docker sandbox's `Ready` condition from inspected state. +/// +/// A signal kill (exit 137/143 = SIGKILL/SIGTERM, not OOM) is the signature of +/// a machine/daemon restart terminating a running container. Reclassify it from +/// the generic terminal `ContainerExited` to the recoverable +/// `ContainerRuntimeRestart` so gateway startup can revive it. OOM kills and +/// ordinary application exits stay `ContainerExited` and terminal. +fn apply_docker_exit_classification(sandbox: &mut DriverSandbox, state: &ContainerState) { + if state.oom_killed == Some(true) { + return; + } + let Some(code) = state.exit_code.filter(|&code| matches!(code, 137 | 143)) else { + return; + }; + let Some(condition) = sandbox + .status + .as_mut() + .and_then(|status| status.conditions.iter_mut().find(|c| c.r#type == "Ready")) + else { + return; + }; + if condition.reason != CONDITION_EXITED { + return; + } + condition.reason = CONDITION_RUNTIME_RESTART.to_string(); + condition.message = format!("Container terminated by signal (exit code {code})"); +} + fn container_ready_condition( state: ContainerSummaryStateEnum, ) -> (&'static str, &'static str, &'static str, bool) { @@ -3115,12 +3170,7 @@ fn container_ready_condition( ContainerSummaryStateEnum::PAUSED => { ("False", "ContainerPaused", "Container is paused", false) } - ContainerSummaryStateEnum::EXITED => ( - "False", - openshell_core::driver_utils::CONDITION_EXITED, - "Container exited", - false, - ), + ContainerSummaryStateEnum::EXITED => ("False", CONDITION_EXITED, "Container exited", false), ContainerSummaryStateEnum::DEAD => ("False", "ContainerDead", "Container is dead", false), } } diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index f828120f63..d1899d6fd8 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -2647,3 +2647,88 @@ fn lifecycle_fence_rejects_polled_exit_from_before_restart() { fences.remove("sandbox-1"); assert!(fences.previous_exit("sandbox-1").is_none()); } + +fn exited_sandbox_with_ready_reason(reason: &str) -> DriverSandbox { + DriverSandbox { + id: "sbx-exit".to_string(), + name: "demo".to_string(), + namespace: String::new(), + spec: None, + status: Some(DriverSandboxStatus { + sandbox_name: "demo".to_string(), + instance_id: "container-1".to_string(), + agent_fd: String::new(), + sandbox_fd: String::new(), + conditions: vec![DriverCondition { + r#type: "Ready".to_string(), + status: "False".to_string(), + reason: reason.to_string(), + message: "Container exited".to_string(), + last_transition_time: String::new(), + }], + deleting: false, + }), + workspace: String::new(), + } +} + +fn ready_reason(sandbox: &DriverSandbox) -> &str { + sandbox + .status + .as_ref() + .and_then(|status| status.conditions.iter().find(|c| c.r#type == "Ready")) + .map(|c| c.reason.as_str()) + .expect("Ready condition present") +} + +#[test] +fn docker_signal_kill_reclassified_as_runtime_restart() { + // 137 (128+SIGKILL) and 143 (128+SIGTERM) mark an external termination — + // the signature of a machine/daemon restart — and become recoverable + // `ContainerRuntimeRestart`. + for exit_code in [137, 143] { + let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED); + let state = ContainerState { + status: Some(ContainerStateStatusEnum::EXITED), + oom_killed: Some(false), + exit_code: Some(exit_code), + ..Default::default() + }; + apply_docker_exit_classification(&mut sandbox, &state); + assert_eq!( + ready_reason(&sandbox), + CONDITION_RUNTIME_RESTART, + "exit code {exit_code} should reclassify as runtime restart" + ); + } +} + +#[test] +fn docker_ordinary_exit_stays_terminal() { + // An application exit (non-zero error code) stays `ContainerExited` so its + // failure signal survives instead of being relaunched on startup. + let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED); + let state = ContainerState { + status: Some(ContainerStateStatusEnum::EXITED), + oom_killed: Some(false), + exit_code: Some(1), + ..Default::default() + }; + apply_docker_exit_classification(&mut sandbox, &state); + assert_eq!(ready_reason(&sandbox), CONDITION_EXITED); +} + +#[test] +fn docker_oom_kill_stays_terminal_despite_137() { + // An OOM kill reports exit 137 but must NOT be treated as a recoverable + // restart — it is a genuine failure and stays terminal. + let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED); + let state = ContainerState { + status: Some(ContainerStateStatusEnum::EXITED), + oom_killed: Some(true), + exit_code: Some(137), + ..Default::default() + }; + apply_docker_exit_classification(&mut sandbox, &state); + assert_eq!(ready_reason(&sandbox), CONDITION_EXITED); +}