Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion crates/openshell-server/src/compute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3875,9 +3875,24 @@ impl ComputeRuntime {

let sandbox = decode_sandbox_record(&current_record)?;
let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown);
if phase == SandboxPhase::Completed || is_failed_main_process_result(&sandbox) {
if phase == SandboxPhase::Completed
|| phase == SandboxPhase::Error
|| is_failed_main_process_result(&sandbox)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, a gateway-managed driver that returned NotFound during startup left a temporary BackendResourceMissing error for clients to inspect, then the reconciliation sweep pruned it after the orphan grace period. By matching every Error here, the sweep now returns before spawn_driver_sandbox_cleanup and apply_deleted_if_version_locked for BackendResourceMissing, StartFailed, and the ComputeResourceMissing state produced for missing Starting/Stopping/Stopped sandboxes. That affects Docker, Podman, VM, Kubernetes, and extension drivers as well as MXC: orphaned names and gateway-owned records remain indefinitely, and the idempotent driver cleanup for volumes/secrets is skipped until a user explicitly deletes the sandbox.

This is broader than the MXC restart race described by the PR. Please narrow the exemption to the intended settled error categories or MXC-specific case. If blanket retention is intentional, update the startup/reconciliation lifecycle contract and architecture documentation, and add regression coverage for BackendResourceMissing and ComputeResourceMissing so the resource-retention behavior is explicit.

{
// A terminal canonical process may legitimately have removed its
// transient compute object. Keep the durable command result.
//
// Error is already the settled, informational terminal state this
// sweep would otherwise produce for Stopping/Stopped/Starting
// below -- deleting it outright instead of leaving it in place
// silently races a concurrent GetSandbox/ListSandboxes/DeleteSandbox
// caller: a driver whose registry never rehydrates after a
// restart (in-process-only state, e.g. MXC) reports every
// previously-known sandbox as "missing" on the very first sweep
// after startup, even ones already correctly marked Error by
// earlier crash detection, so this is reached far more than the
// "orphaned compute resource" case this pruning otherwise exists
// for.
return Ok(());
}
if matches!(
Expand Down Expand Up @@ -9372,6 +9387,36 @@ mod tests {
);
}

#[tokio::test]
async fn prune_missing_sandbox_keeps_error_phase_records() {
// Regression test: a driver whose registry never rehydrates after a
// restart (in-process-only state, e.g. MXC) reports every
// previously-known sandbox as missing on the first sweep after
// startup -- including ones already correctly, terminally marked
// Error by earlier crash detection. The sweep must not delete those;
// it must treat Error the same as the existing Completed exemption
// and leave the durable record in place.
let driver = ControlledDriver::new();
driver.set_get_outcome(ControlledGetOutcome::Missing);
let runtime = test_runtime(driver.clone()).await;
let sandbox = sandbox_record("sb-1", "sandbox-a", SandboxPhase::Error);
runtime.store.put_message(&sandbox).await.unwrap();

runtime
.reconcile_store_with_backend(Duration::ZERO)
.await
.unwrap();

let retained = runtime
.store
.get_message::<Sandbox>("sb-1")
.await
.unwrap()
.expect("Error-phase sandbox record must survive the prune sweep");
assert_eq!(retained.phase(), SandboxPhase::Error as i32);
assert_eq!(driver.delete_calls(), 0);
}

#[tokio::test]
async fn prune_sweep_does_not_block_on_a_stuck_driver_delete_call() {
// Regression test: the prune sweep's driver cleanup must not be
Expand Down
Loading