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/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, 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 3a92cd209a..2fcc3f21d3 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,7 +784,12 @@ 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 recoverable_error = phase == SandboxPhase::Error + && is_recoverable_error_reason(&sandbox); + if phase == SandboxPhase::Error && !recoverable_error { continue; } @@ -781,31 +798,37 @@ impl ComputeRuntime { .await { Ok(true) => { + 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, "Resumed sandbox during gateway startup" ); resumed += 1; + if did_recover { + 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 !recoverable_error { + self.mark_sandbox_error( + &sandbox, + "BackendResourceMissing", + "Sandbox container disappeared while the gateway was offline", + ) + .await; + } missing += 1; } Err(err) => { @@ -815,12 +838,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 !recoverable_error { + self.mark_sandbox_error( + &sandbox, + "ResumeFailed", + &format!("Failed to resume sandbox during gateway startup: {err}"), + ) + .await; + } failed += 1; } } @@ -829,6 +854,7 @@ impl ComputeRuntime { if resumed > 0 || missing > 0 || failed > 0 { info!( resumed, + recovered, missing_backend = missing, failed, "Sandbox resume sweep complete" @@ -875,6 +901,44 @@ impl ComputeRuntime { } } + 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}; @@ -1969,20 +2033,17 @@ 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 - ) +/// 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 { @@ -2393,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 { @@ -3286,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(); @@ -3304,6 +3379,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 +3471,118 @@ 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 = error_sandbox_record("sb-err-recover", "recover", "ContainerExited"); + 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 = error_sandbox_record("sb-err-gone", "gone", "ContainerExited"); + 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 = error_sandbox_record("sb-err-fail", "fail", "ContainerExited"); + 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 + ); + } + + #[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;