Skip to content

Commit c27a3a3

Browse files
authored
fix(compute): recover Error-phase sandboxes on gateway startup (#2269)
* 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 <milleryan2003@gmail.com> * 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 <milleryan2003@gmail.com> * 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 <milleryan2003@gmail.com> --------- Signed-off-by: Ian Miller <milleryan2003@gmail.com>
1 parent eb15e1a commit c27a3a3

5 files changed

Lines changed: 521 additions & 40 deletions

File tree

crates/openshell-core/src/driver_utils.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,30 @@ pub fn openshell_sandbox_label_selector() -> String {
4040
format!("{LABEL_MANAGED_BY}={LABEL_MANAGED_BY_VALUE},{LABEL_SANDBOX_ID}")
4141
}
4242

43+
// ---------------------------------------------------------------------------
44+
// Sandbox condition reason strings set by compute drivers.
45+
// ---------------------------------------------------------------------------
46+
47+
/// Ready-condition reason when a container exits on its own.
48+
///
49+
/// Covers an ordinary application exit or crash (exit 0, a non-zero error code,
50+
/// or an uncaught fault). This is a terminal reason: gateway startup does NOT
51+
/// auto-restart it, so a genuine failure keeps its error signal instead of
52+
/// being relaunched.
53+
pub const CONDITION_EXITED: &str = "ContainerExited";
54+
55+
/// Ready-condition reason when a container was terminated by an external signal.
56+
///
57+
/// SIGKILL/SIGTERM (exit 137/143) is what a Podman/Docker machine or daemon
58+
/// restart does to running containers. Distinct from `CONDITION_EXITED` so
59+
/// gateway startup can recover machine-restart victims while leaving ordinary
60+
/// application exits terminal.
61+
pub const CONDITION_RUNTIME_RESTART: &str = "ContainerRuntimeRestart";
62+
63+
/// Ready-condition reason when a container is explicitly stopped via the
64+
/// runtime API (e.g. `podman stop`, gateway-initiated shutdown).
65+
pub const CONDITION_STOPPED: &str = "ContainerStopped";
66+
4367
// ---------------------------------------------------------------------------
4468

4569
/// Path to the sandbox supervisor binary inside the container image.

crates/openshell-driver-docker/src/lib.rs

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ use openshell_core::config::{
2626
};
2727
use openshell_core::driver_mounts;
2828
use openshell_core::driver_utils::{
29-
LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME,
30-
LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, SUPERVISOR_IMAGE_BINARY_PATH,
31-
extract_first_tar_entry, supervisor_image_should_refresh, temp_extract_container_name,
32-
validate_linux_elf_binary, write_cache_binary_atomic,
29+
CONDITION_EXITED, CONDITION_RUNTIME_RESTART, LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE,
30+
LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE,
31+
SUPERVISOR_IMAGE_BINARY_PATH, extract_first_tar_entry, supervisor_image_should_refresh,
32+
temp_extract_container_name, validate_linux_elf_binary, write_cache_binary_atomic,
3333
};
3434
use openshell_core::gpu::{
3535
CdiGpuDefaultSelector, CdiGpuInventory, CdiGpuSelectionError, driver_gpu_requirements,
@@ -829,10 +829,37 @@ impl DockerComputeDriver {
829829

830830
async fn current_snapshots(&self) -> Result<Vec<DriverSandbox>, Status> {
831831
let containers = self.list_managed_container_summaries().await?;
832-
let container_sandboxes = containers
833-
.iter()
834-
.filter_map(sandbox_from_container_summary)
835-
.collect::<Vec<_>>();
832+
let mut container_sandboxes = Vec::with_capacity(containers.len());
833+
for summary in &containers {
834+
let Some(mut sandbox) = sandbox_from_container_summary(summary) else {
835+
continue;
836+
};
837+
// Docker's list summary carries no exit code, so an exited
838+
// container is reported as the generic terminal `ContainerExited`.
839+
// Inspect it to tell a machine/daemon-restart signal kill apart
840+
// from an ordinary application exit, mirroring the Podman driver,
841+
// so startup recovery can revive restart victims while leaving
842+
// crashes terminal.
843+
if summary.state == Some(ContainerSummaryStateEnum::EXITED)
844+
&& let Some(container_id) = summary.id.as_deref()
845+
{
846+
match self.docker.inspect_container(container_id, None).await {
847+
Ok(inspected) => {
848+
if let Some(state) = inspected.state.as_ref() {
849+
apply_docker_exit_classification(&mut sandbox, state);
850+
}
851+
}
852+
Err(err) => {
853+
debug!(
854+
container_id,
855+
error = %err,
856+
"Could not inspect exited Docker container to classify its exit"
857+
);
858+
}
859+
}
860+
}
861+
container_sandboxes.push(sandbox);
862+
}
836863
let mut by_id = self.pending_snapshot_map().await;
837864
for sandbox in container_sandboxes {
838865
by_id.insert(sandbox.id.clone(), sandbox);
@@ -3491,6 +3518,34 @@ fn driver_status_from_summary(
34913518
}
34923519
}
34933520

3521+
/// Refine an exited Docker sandbox's `Ready` condition from inspected state.
3522+
///
3523+
/// A signal kill (exit 137/143 = SIGKILL/SIGTERM, not OOM) is the signature of
3524+
/// a machine/daemon restart terminating a running container. Reclassify it from
3525+
/// the generic terminal `ContainerExited` to the recoverable
3526+
/// `ContainerRuntimeRestart` so gateway startup can revive it. OOM kills and
3527+
/// ordinary application exits stay `ContainerExited` and terminal.
3528+
fn apply_docker_exit_classification(sandbox: &mut DriverSandbox, state: &ContainerState) {
3529+
if state.oom_killed == Some(true) {
3530+
return;
3531+
}
3532+
let Some(code) = state.exit_code.filter(|&code| matches!(code, 137 | 143)) else {
3533+
return;
3534+
};
3535+
let Some(condition) = sandbox
3536+
.status
3537+
.as_mut()
3538+
.and_then(|status| status.conditions.iter_mut().find(|c| c.r#type == "Ready"))
3539+
else {
3540+
return;
3541+
};
3542+
if condition.reason != CONDITION_EXITED {
3543+
return;
3544+
}
3545+
condition.reason = CONDITION_RUNTIME_RESTART.to_string();
3546+
condition.message = format!("Container terminated by signal (exit code {code})");
3547+
}
3548+
34943549
fn container_ready_condition(
34953550
state: ContainerSummaryStateEnum,
34963551
) -> (&'static str, &'static str, &'static str, bool) {
@@ -3514,9 +3569,7 @@ fn container_ready_condition(
35143569
ContainerSummaryStateEnum::PAUSED => {
35153570
("False", "ContainerPaused", "Container is paused", false)
35163571
}
3517-
ContainerSummaryStateEnum::EXITED => {
3518-
("False", "ContainerExited", "Container exited", false)
3519-
}
3572+
ContainerSummaryStateEnum::EXITED => ("False", CONDITION_EXITED, "Container exited", false),
35203573
ContainerSummaryStateEnum::DEAD => ("False", "ContainerDead", "Container is dead", false),
35213574
}
35223575
}

crates/openshell-driver-docker/src/tests.rs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3104,3 +3104,88 @@ fn lifecycle_fence_rejects_polled_exit_from_before_restart() {
31043104
fences.remove("sandbox-1");
31053105
assert!(fences.previous_exit("sandbox-1").is_none());
31063106
}
3107+
3108+
fn exited_sandbox_with_ready_reason(reason: &str) -> DriverSandbox {
3109+
DriverSandbox {
3110+
id: "sbx-exit".to_string(),
3111+
name: "demo".to_string(),
3112+
namespace: String::new(),
3113+
spec: None,
3114+
status: Some(DriverSandboxStatus {
3115+
sandbox_name: "demo".to_string(),
3116+
instance_id: "container-1".to_string(),
3117+
agent_fd: String::new(),
3118+
sandbox_fd: String::new(),
3119+
conditions: vec![DriverCondition {
3120+
r#type: "Ready".to_string(),
3121+
status: "False".to_string(),
3122+
reason: reason.to_string(),
3123+
message: "Container exited".to_string(),
3124+
last_transition_time: String::new(),
3125+
}],
3126+
deleting: false,
3127+
}),
3128+
workspace: String::new(),
3129+
}
3130+
}
3131+
3132+
fn ready_reason(sandbox: &DriverSandbox) -> &str {
3133+
sandbox
3134+
.status
3135+
.as_ref()
3136+
.and_then(|status| status.conditions.iter().find(|c| c.r#type == "Ready"))
3137+
.map(|c| c.reason.as_str())
3138+
.expect("Ready condition present")
3139+
}
3140+
3141+
#[test]
3142+
fn docker_signal_kill_reclassified_as_runtime_restart() {
3143+
// 137 (128+SIGKILL) and 143 (128+SIGTERM) mark an external termination —
3144+
// the signature of a machine/daemon restart — and become recoverable
3145+
// `ContainerRuntimeRestart`.
3146+
for exit_code in [137, 143] {
3147+
let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED);
3148+
let state = ContainerState {
3149+
status: Some(ContainerStateStatusEnum::EXITED),
3150+
oom_killed: Some(false),
3151+
exit_code: Some(exit_code),
3152+
..Default::default()
3153+
};
3154+
apply_docker_exit_classification(&mut sandbox, &state);
3155+
assert_eq!(
3156+
ready_reason(&sandbox),
3157+
CONDITION_RUNTIME_RESTART,
3158+
"exit code {exit_code} should reclassify as runtime restart"
3159+
);
3160+
}
3161+
}
3162+
3163+
#[test]
3164+
fn docker_ordinary_exit_stays_terminal() {
3165+
// An application exit (non-zero error code) stays `ContainerExited` so its
3166+
// failure signal survives instead of being relaunched on startup.
3167+
let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED);
3168+
let state = ContainerState {
3169+
status: Some(ContainerStateStatusEnum::EXITED),
3170+
oom_killed: Some(false),
3171+
exit_code: Some(1),
3172+
..Default::default()
3173+
};
3174+
apply_docker_exit_classification(&mut sandbox, &state);
3175+
assert_eq!(ready_reason(&sandbox), CONDITION_EXITED);
3176+
}
3177+
3178+
#[test]
3179+
fn docker_oom_kill_stays_terminal_despite_137() {
3180+
// An OOM kill reports exit 137 but must NOT be treated as a recoverable
3181+
// restart — it is a genuine failure and stays terminal.
3182+
let mut sandbox = exited_sandbox_with_ready_reason(CONDITION_EXITED);
3183+
let state = ContainerState {
3184+
status: Some(ContainerStateStatusEnum::EXITED),
3185+
oom_killed: Some(true),
3186+
exit_code: Some(137),
3187+
..Default::default()
3188+
};
3189+
apply_docker_exit_classification(&mut sandbox, &state);
3190+
assert_eq!(ready_reason(&sandbox), CONDITION_EXITED);
3191+
}

crates/openshell-driver-podman/src/watcher.rs

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ use tracing::{debug, info, warn};
2626
// Condition reason constants shared across event-building paths.
2727
const CONDITION_RUNNING: &str = "ContainerRunning";
2828
const CONDITION_STARTING: &str = "ContainerStarting";
29-
const CONDITION_EXITED: &str = "ContainerExited";
30-
const CONDITION_STOPPED: &str = "ContainerStopped";
29+
use openshell_core::driver_utils::{
30+
CONDITION_EXITED, CONDITION_RUNTIME_RESTART, CONDITION_STOPPED,
31+
};
3132

3233
pub type WatchStream =
3334
Pin<Box<dyn Stream<Item = Result<WatchSandboxesEvent, ComputeDriverError>> + Send>>;
@@ -445,15 +446,29 @@ fn condition_from_state(state: &ContainerState) -> DriverCondition {
445446
},
446447
"created" => ("False", "ContainerCreated", String::new()),
447448
"exited" | "stopped" => {
448-
let msg = if state.oom_killed {
449-
"Container was killed by the OOM killer".to_string()
450-
} else {
451-
format!("Container exited with code {}", state.exit_code)
452-
};
453-
let reason = if state.oom_killed {
454-
"OOMKilled"
449+
// Exit codes 137 (128+SIGKILL) and 143 (128+SIGTERM) mean the
450+
// container was terminated by an external signal rather than
451+
// exiting on its own — the signature of a machine/daemon restart
452+
// killing running containers. Those are recoverable at gateway
453+
// startup; ordinary application exits (0, non-zero, faults) are not.
454+
let (reason, msg) = if state.oom_killed {
455+
(
456+
"OOMKilled",
457+
"Container was killed by the OOM killer".to_string(),
458+
)
459+
} else if matches!(state.exit_code, 137 | 143) {
460+
(
461+
CONDITION_RUNTIME_RESTART,
462+
format!(
463+
"Container terminated by signal (exit code {})",
464+
state.exit_code
465+
),
466+
)
455467
} else {
456-
CONDITION_EXITED
468+
(
469+
CONDITION_EXITED,
470+
format!("Container exited with code {}", state.exit_code),
471+
)
457472
};
458473
("False", reason, msg)
459474
}
@@ -600,6 +615,32 @@ mod tests {
600615
assert!(cond.message.contains("code 1"));
601616
}
602617

618+
#[test]
619+
fn condition_signal_kill_is_runtime_restart() {
620+
// 137 (128+SIGKILL) and 143 (128+SIGTERM) are external terminations —
621+
// the signature of a machine/daemon restart. They classify as
622+
// recoverable `ContainerRuntimeRestart`, distinct from an ordinary
623+
// application exit.
624+
for exit_code in [137, 143] {
625+
let state = ContainerState {
626+
status: "exited".to_string(),
627+
running: false,
628+
exit_code,
629+
oom_killed: false,
630+
health: None,
631+
started_at: None,
632+
finished_at: Some("2026-04-14T12:30:00Z".to_string()),
633+
};
634+
let cond = condition_from_state(&state);
635+
assert_eq!(cond.status, "False");
636+
assert_eq!(
637+
cond.reason, "ContainerRuntimeRestart",
638+
"exit code {exit_code} should classify as runtime restart"
639+
);
640+
assert!(cond.message.contains(&format!("code {exit_code}")));
641+
}
642+
}
643+
603644
#[test]
604645
fn short_id_truncates() {
605646
assert_eq!(short_id("abc123def456789"), "abc123def456");

0 commit comments

Comments
 (0)