From f48f99f86af38c904aa5a4190e206a91b2fa6241 Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Tue, 14 Jul 2026 19:01:22 +0100 Subject: [PATCH 1/3] feat(podman): add resume_sandbox method to restart exited containers When a Podman machine restarts, sandbox containers exit with SIGTERM (code 143) but remain on disk. This method inspects the container state and calls start_container if the container exists but is not running, enabling the gateway to recover sandboxes without user intervention. Signed-off-by: Ian Miller --- crates/openshell-driver-podman/src/driver.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index cf639fa265..42c92d4f1e 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -693,6 +693,25 @@ impl PodmanComputeDriver { } } + /// Resume an exited sandbox container. Returns `Ok(true)` if the + /// container exists and was (re)started, `Ok(false)` if it is gone. + pub async fn resume_sandbox(&self, sandbox_name: &str) -> Result { + let name = container::container_name(sandbox_name); + let inspect = match self.client.inspect_container(&name).await { + Ok(i) => i, + Err(PodmanApiError::NotFound(_)) => return Ok(false), + Err(e) => return Err(ComputeDriverError::from(e)), + }; + if inspect.state.running { + return Ok(true); + } + match self.client.start_container(&name).await { + Ok(()) => Ok(true), + Err(PodmanApiError::NotFound(_)) => Ok(false), + Err(e) => Err(ComputeDriverError::from(e)), + } + } + /// Fetch a single sandbox by name. pub async fn get_sandbox( &self, From 4150476ec78dd64af39841323fe1dc7886e8fd1e Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Tue, 14 Jul 2026 19:01:35 +0100 Subject: [PATCH 2/3] fix(compute): recover Error-phase sandboxes on gateway startup (#2179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a Podman/Docker machine restart, all sandbox containers exit with SIGTERM (code 143). The gateway marks them Error, and on next startup the resume sweep skipped Error-phase sandboxes entirely — leaving them stuck until the user deleted and recreated them. Now resume_persisted_sandboxes() attempts Error-phase sandboxes too: - Ok(true): container exists and restarted — clear error, set phase to Provisioning so the watch loop promotes to Ready - Ok(false): container gone — leave as Error (no overwrite) - Err: driver failure — leave as Error (no overwrite) Also wires StartupResume for PodmanComputeDriver so the resume sweep runs on Podman-backed gateways (previously Docker-only). Signed-off-by: Ian Miller --- crates/openshell-server/src/compute/mod.rs | 207 +++++++++++++++++---- 1 file changed, 167 insertions(+), 40 deletions(-) diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 3a92cd209a..682fe077af 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -98,6 +98,15 @@ impl StartupResume for DockerComputeDriver { .map_err(|err| err.to_string()) } } +#[tonic::async_trait] +impl StartupResume for PodmanComputeDriver { + async fn resume_sandbox(&self, _sandbox_id: &str, sandbox_name: &str) -> Result { + Self::resume_sandbox(self, sandbox_name) + .await + .map_err(|err| err.to_string()) + } +} + /// Interval between store-vs-backend reconciliation sweeps. const RECONCILE_INTERVAL: Duration = Duration::from_secs(60); @@ -434,12 +443,13 @@ impl ComputeRuntime { let driver = PodmanComputeDriver::new(config) .await .map_err(|err| ComputeError::Message(err.to_string()))?; + let startup_resume: Arc = Arc::new(driver.clone()); let driver: SharedComputeDriver = Arc::new(PodmanDriverService::new(driver)); Self::from_driver( ComputeDriverKind::Podman.as_str().to_string(), driver, None, - None, + Some(startup_resume), None, store, sandbox_index, @@ -739,11 +749,12 @@ impl ComputeRuntime { /// Resume sandboxes whose store records say they should be running. /// Drivers that do not auto-restart compute resources across gateway /// restarts (currently only Docker) implement `StartupResume`. For - /// each sandbox in the store whose phase is not `Deleting` or - /// `Error`, we ask the driver to resume the underlying resource. If - /// the driver reports that the resource no longer exists or fails to - /// start, the sandbox is moved to the `Error` phase so the failure - /// surfaces in the UI. + /// each sandbox in the store whose phase is not `Deleting`, we ask + /// the driver to resume the underlying resource. Error-phase + /// sandboxes are included: if the container still exists (e.g. after + /// a Podman machine restart), it is restarted and the sandbox is + /// moved back to `Provisioning`. If the container is gone or the + /// driver fails, the sandbox stays in `Error`. /// /// Should be called once at gateway startup, before watchers spawn, /// so the watch loop sees the post-resume state on its first poll. @@ -759,6 +770,7 @@ impl ComputeRuntime { .map_err(|e| e.to_string())?; let mut resumed = 0usize; + let mut recovered = 0usize; let mut missing = 0usize; let mut failed = 0usize; @@ -772,40 +784,45 @@ impl ComputeRuntime { }; let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); - if !sandbox_phase_should_be_running(phase) { + if phase == SandboxPhase::Deleting { continue; } + let sandbox_error = phase == SandboxPhase::Error; match resume .resume_sandbox(sandbox.object_id(), sandbox.object_name()) .await { Ok(true) => { + if sandbox_error { + self.clear_sandbox_error(&sandbox).await; + } info!( sandbox_id = %sandbox.object_id(), sandbox_name = %sandbox.object_name(), ?phase, + recovered = sandbox_error, "Resumed sandbox during gateway startup" ); resumed += 1; + if sandbox_error { + recovered += 1; + } } Ok(false) => { - // Backend resource is gone but the store still - // remembers the sandbox. Mark Error so the UI - // surfaces the inconsistency; the reconcile loop - // will eventually prune it after the orphan grace - // period. warn!( sandbox_id = %sandbox.object_id(), sandbox_name = %sandbox.object_name(), "Cannot resume sandbox: backend resource is missing" ); - self.mark_sandbox_error( - &sandbox, - "BackendResourceMissing", - "Sandbox container disappeared while the gateway was offline", - ) - .await; + if !sandbox_error { + self.mark_sandbox_error( + &sandbox, + "BackendResourceMissing", + "Sandbox container disappeared while the gateway was offline", + ) + .await; + } missing += 1; } Err(err) => { @@ -815,12 +832,14 @@ impl ComputeRuntime { error = %err, "Failed to resume sandbox during gateway startup" ); - self.mark_sandbox_error( - &sandbox, - "ResumeFailed", - &format!("Failed to resume sandbox during gateway startup: {err}"), - ) - .await; + if !sandbox_error { + self.mark_sandbox_error( + &sandbox, + "ResumeFailed", + &format!("Failed to resume sandbox during gateway startup: {err}"), + ) + .await; + } failed += 1; } } @@ -829,6 +848,7 @@ impl ComputeRuntime { if resumed > 0 || missing > 0 || failed > 0 { info!( resumed, + recovered, missing_backend = missing, failed, "Sandbox resume sweep complete" @@ -875,6 +895,42 @@ impl ComputeRuntime { } } + async fn clear_sandbox_error(&self, sandbox: &Sandbox) { + 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); + } + Err(err) => { + warn!( + sandbox_id = %sandbox_id, + error = %err, + "Failed to clear sandbox error state during startup resume" + ); + } + } + } + async fn lease_coordinator(self: Arc, mut shutdown_rx: watch::Receiver) { use lease::{LEASE_ACQUIRE_INTERVAL, LEASE_TTL, ReconcilerLease}; @@ -1969,22 +2025,6 @@ fn rewrite_user_facing_conditions(status: &mut Option, spec: Opti } } -/// Phases for which a sandbox should have a running compute resource. -/// `Deleting` and `Error` are intentionally excluded: deletion is in -/// progress, or the sandbox has already failed and should not be -/// silently revived. `Unspecified` is included because it is the proto -/// default value; persisted rows with that value should be reconciled -/// from the live driver state rather than skipped forever. -fn sandbox_phase_should_be_running(phase: SandboxPhase) -> bool { - matches!( - phase, - SandboxPhase::Unspecified - | SandboxPhase::Provisioning - | SandboxPhase::Ready - | SandboxPhase::Unknown - ) -} - fn is_terminal_failure_reason(reason: &str) -> bool { let reason = reason.to_ascii_lowercase(); let transient_reasons = [ @@ -3304,6 +3344,7 @@ mod tests { assert_eq!( called_ids, vec![ + "sb-error".to_string(), "sb-prov".to_string(), "sb-ready".to_string(), "sb-unknown".to_string(), @@ -3395,6 +3436,92 @@ mod tests { ); } + #[tokio::test] + async fn resume_persisted_sandboxes_recovers_error_phase_when_container_exists() { + let resume = Arc::new(RecordingResume::default()); + resume.set_result("sb-err-recover", Ok(true)).await; + let runtime = + test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; + + let sandbox = sandbox_record("sb-err-recover", "recover", SandboxPhase::Error); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.resume_persisted_sandboxes().await.unwrap(); + + assert_eq!(resume.calls().await.len(), 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 resume_persisted_sandboxes_leaves_error_when_container_missing() { + let resume = Arc::new(RecordingResume::default()); + resume.set_result("sb-err-gone", Ok(false)).await; + let runtime = + test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; + + let sandbox = sandbox_record("sb-err-gone", "gone", SandboxPhase::Error); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.resume_persisted_sandboxes().await.unwrap(); + + assert_eq!(resume.calls().await.len(), 1); + + let stored = runtime + .store + .get_message::("sb-err-gone") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + } + + #[tokio::test] + async fn resume_persisted_sandboxes_leaves_error_when_resume_fails() { + let resume = Arc::new(RecordingResume::default()); + resume + .set_result("sb-err-fail", Err("docker broke".to_string())) + .await; + let runtime = + test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; + + let sandbox = sandbox_record("sb-err-fail", "fail", SandboxPhase::Error); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.resume_persisted_sandboxes().await.unwrap(); + + assert_eq!(resume.calls().await.len(), 1); + + let stored = runtime + .store + .get_message::("sb-err-fail") + .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 a705b0e6e71c758fe554f6a63c816baec925871b Mon Sep 17 00:00:00 2001 From: Ian Miller Date: Thu, 16 Jul 2026 11:37:28 +0100 Subject: [PATCH 3/3] fix(compute): restrict recovery to container-exit errors, return CAS status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback from @maxamillion: 1. Only recover Error-phase sandboxes whose Ready condition reason is ContainerExited or ContainerStopped. Sandboxes that failed for terminal reasons (BackendResourceMissing, ResumeFailed, OOM) are left untouched — their original error reason is preserved. 2. clear_recoverable_error() now returns bool so the caller only logs recovered=true and increments the counter after the state transition actually succeeds. CAS failures no longer produce misleading counts. Adds is_recoverable_error_reason() helper and a new test for non-recoverable error sandboxes being skipped. Signed-off-by: Ian Miller --- crates/openshell-core/src/driver_utils.rs | 12 +++ crates/openshell-driver-docker/src/lib.rs | 2 +- crates/openshell-driver-podman/src/watcher.rs | 3 +- crates/openshell-server/src/compute/mod.rs | 87 ++++++++++++++++--- 4 files changed, 88 insertions(+), 16 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 9e4411b2a1..6f6c8c1656 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -27,6 +27,18 @@ pub const LABEL_SANDBOX_NAME: &str = "openshell.ai/sandbox-name"; /// Container/pod label carrying the sandbox namespace. pub const LABEL_SANDBOX_NAMESPACE: &str = "openshell.ai/sandbox-namespace"; +// --------------------------------------------------------------------------- +// 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 06d575a1e1..746cf42b73 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2801,7 +2801,7 @@ 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 54606ea442..3fe500fa5d 100644 --- a/crates/openshell-driver-podman/src/watcher.rs +++ b/crates/openshell-driver-podman/src/watcher.rs @@ -22,8 +22,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 682fe077af..2fcc3f21d3 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -787,25 +787,31 @@ impl ComputeRuntime { if phase == SandboxPhase::Deleting { continue; } - let sandbox_error = phase == SandboxPhase::Error; + let recoverable_error = phase == SandboxPhase::Error + && is_recoverable_error_reason(&sandbox); + if phase == SandboxPhase::Error && !recoverable_error { + continue; + } match resume .resume_sandbox(sandbox.object_id(), sandbox.object_name()) .await { Ok(true) => { - if sandbox_error { - self.clear_sandbox_error(&sandbox).await; - } + 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 = sandbox_error, + recovered = did_recover, "Resumed sandbox during gateway startup" ); resumed += 1; - if sandbox_error { + if did_recover { recovered += 1; } } @@ -815,7 +821,7 @@ impl ComputeRuntime { sandbox_name = %sandbox.object_name(), "Cannot resume sandbox: backend resource is missing" ); - if !sandbox_error { + if !recoverable_error { self.mark_sandbox_error( &sandbox, "BackendResourceMissing", @@ -832,7 +838,7 @@ impl ComputeRuntime { error = %err, "Failed to resume sandbox during gateway startup" ); - if !sandbox_error { + if !recoverable_error { self.mark_sandbox_error( &sandbox, "ResumeFailed", @@ -895,7 +901,7 @@ impl ComputeRuntime { } } - async fn clear_sandbox_error(&self, sandbox: &Sandbox) { + 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 @@ -920,6 +926,7 @@ impl ComputeRuntime { Ok(updated) => { self.sandbox_index.update_from_sandbox(&updated); self.sandbox_watch_bus.notify(&sandbox_id); + true } Err(err) => { warn!( @@ -927,6 +934,7 @@ impl ComputeRuntime { error = %err, "Failed to clear sandbox error state during startup resume" ); + false } } } @@ -2025,6 +2033,19 @@ fn rewrite_user_facing_conditions(status: &mut Option, spec: Opti } } +/// 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 = [ @@ -2433,6 +2454,19 @@ mod tests { sandbox } + 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 { @@ -3326,11 +3360,12 @@ mod tests { ("sb-ready", "ready", SandboxPhase::Ready), ("sb-unknown", "unknown", SandboxPhase::Unknown), ("sb-deleting", "deleting", SandboxPhase::Deleting), - ("sb-error", "error", SandboxPhase::Error), ] { let sandbox = sandbox_record(id, name, phase); runtime.store.put_message(&sandbox).await.unwrap(); } + let error_sb = error_sandbox_record("sb-error", "error", "ContainerExited"); + runtime.store.put_message(&error_sb).await.unwrap(); runtime.resume_persisted_sandboxes().await.unwrap(); @@ -3443,7 +3478,7 @@ mod tests { let runtime = test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; - let sandbox = sandbox_record("sb-err-recover", "recover", SandboxPhase::Error); + let sandbox = error_sandbox_record("sb-err-recover", "recover", "ContainerExited"); runtime.store.put_message(&sandbox).await.unwrap(); runtime.resume_persisted_sandboxes().await.unwrap(); @@ -3475,7 +3510,7 @@ mod tests { let runtime = test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; - let sandbox = sandbox_record("sb-err-gone", "gone", SandboxPhase::Error); + let sandbox = error_sandbox_record("sb-err-gone", "gone", "ContainerExited"); runtime.store.put_message(&sandbox).await.unwrap(); runtime.resume_persisted_sandboxes().await.unwrap(); @@ -3503,7 +3538,7 @@ mod tests { let runtime = test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; - let sandbox = sandbox_record("sb-err-fail", "fail", SandboxPhase::Error); + let sandbox = error_sandbox_record("sb-err-fail", "fail", "ContainerExited"); runtime.store.put_message(&sandbox).await.unwrap(); runtime.resume_persisted_sandboxes().await.unwrap(); @@ -3522,6 +3557,32 @@ mod tests { ); } + #[tokio::test] + async fn resume_persisted_sandboxes_skips_non_recoverable_error() { + let resume = Arc::new(RecordingResume::default()); + let runtime = + test_runtime_with_resume(Arc::new(TestDriver::default()), Some(resume.clone())).await; + + let sandbox = + error_sandbox_record("sb-err-perm", "perm", "BackendResourceMissing"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.resume_persisted_sandboxes().await.unwrap(); + + assert!(resume.calls().await.is_empty()); + + 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;