fix(compute): recover Error-phase sandboxes on gateway startup

[r3v5]

Summary

• After a Podman/Docker machine restart, sandbox containers exit with SIGTERM (code 143) but remain on disk. The gateway marked them Error and on next startup the resume sweep skipped Error-phase sandboxes entirely — leaving them stuck until the user deleted and recreated them, losing state.
• resume_persisted_sandboxes() now attempts Error-phase sandboxes: if the container still exists it is restarted and the sandbox transitions back to Provisioning → Ready; if the container is gone or the driver fails, the sandbox stays in Error without overwriting the original error reason.
• Adds resume_sandbox() to the Podman driver and wires StartupResume for PodmanComputeDriver so the resume sweep runs on Podman-backed gateways (previously Docker-only).

Related Issue

Closes #2179

Changes

• crates/openshell-driver-podman/src/driver.rs — Added resume_sandbox() method that inspects container state and calls start_container if the container exists but is not running.
• crates/openshell-server/src/compute/mod.rs:
  • Modified phase filter in resume_persisted_sandboxes() to only skip Deleting (not Error)
  • Added clear_sandbox_error() method (mirrors mark_sandbox_error()) to reset phase to Provisioning and set Ready condition to "Resumed"
  • Guarded Ok(false) and Err arms to avoid overwriting existing error state on already-Error sandboxes
  • Implemented StartupResume for PodmanComputeDriver and wired it into new_podman()
  • Added recovered counter to summary log line for observability
  • Removed unused sandbox_phase_should_be_running() function
  • Updated doc comments to reflect new behavior

Testing

Unit tests (3 new + 1 updated)

• resume_persisted_sandboxes_recovers_error_phase_when_container_exists — Ok(true) → phase becomes Provisioning, Ready condition reason = "Resumed"
• resume_persisted_sandboxes_leaves_error_when_container_missing — Ok(false) → phase stays Error, no overwrite
• resume_persisted_sandboxes_leaves_error_when_resume_fails — Err → phase stays Error, no overwrite
• Updated resume_persisted_sandboxes_resumes_running_phases to expect Error-phase sandbox in called_ids

All 1032 tests pass across openshell-server and openshell-driver-podman. Clippy clean.

Manual verification

Full end-to-end reproduction of issue #2179:

1. Created sandbox with Vertex AI provider: openshell sandbox create --name my-podman-sandbox --provider vertex-prod
2. Verified sandbox Ready and container Up (healthy)
3. Stopped Podman machine: podman machine stop
4. Restarted Podman machine: podman machine start
5. Verified container Exited (143) and sandbox stuck in Error (before fix — screenshot 1)
6. Restarted gateway with fix — logs show Resumed sandbox ... phase=Error recovered=true and Sandbox resume sweep complete resumed=1 recovered=1 (screenshot 2)
7. Verified sandbox recovered to Ready and container Up (healthy) (screenshot 2)
8. Verified sandbox usable via openshell term TUI (screenshot 3)

Screenshot 1 — Before fix: sandbox stuck in Error after Podman machine restart

Screenshot 2 — After fix: gateway restart recovers sandbox to Ready

Screenshot 3 — Sandbox usable in OpenShell TUI after recovery

Checklist

• Follows Conventional Commits format
• Signed off (DCO)
• Unit tests added and passing
• Pre-commit checks passing
• Manual end-to-end verification complete
• No proto changes, no new gRPC RPCs, no CLI changes

🤖 Generated with Claude Code

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[r3v5]

Manual Verification Screenshots

Screenshot 1 — Before fix: sandbox stuck in Error after Podman machine restart (exit code 143)

Screenshot 2 — After fix: gateway restart recovers sandbox, logs show recovered=true, sandbox Ready

Screenshot 3 — Sandbox usable in OpenShell TUI after recovery

[r3v5]

I have read the DCO document and I hereby sign the DCO.

[r3v5]

recheck

[r3v5]

PR Review — Claude Code (Opus 4.6)

Overview

After Podman/Docker machine restart, sandbox containers exit with SIGTERM but remain on disk. Gateway previously skipped Error-phase sandboxes during resume sweep, leaving them stuck. This PR makes the resume sweep attempt Error-phase sandboxes — restarting containers that still exist, leaving Error untouched if the container is gone or the driver fails. Also wires StartupResume for Podman (was Docker-only).

Key Design Decisions

• Include Error in resume sweep, only skip Deleting (crates/openshell-server/src/compute/mod.rs:787) — Replaces sandbox_phase_should_be_running() filter with simple phase == Deleting check. Error-phase sandboxes now get a recovery attempt instead of being permanently stuck.

• Guard against overwriting existing error state (mod.rs:810-828) — When an already-Error sandbox fails resume (Ok(false) or Err), the original error reason is preserved. Only Ok(true) clears error state. Prevents losing diagnostic info.

• clear_sandbox_error() resets to Provisioning, not Ready (mod.rs:895-930) — Recovered sandboxes go to Provisioning with Ready condition reason "Resumed". Watch loop then drives them to Ready naturally, same path as fresh sandboxes.

• Podman resume_sandbox() uses inspect-then-start (crates/openshell-driver-podman/src/driver.rs:695-712) — Inspects container first; if running returns Ok(true) without restart. NotFound mapped to Ok(false) at both inspect and start points for race safety.

Notable Code

crates/openshell-driver-podman/src/driver.rs:695:

pub async fn resume_sandbox(&self, sandbox_name: &str) -> Result<bool, ComputeDriverError> {
    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)),
    }
}

Potential Concerns

• TOCTOU between inspect and start — Container could be removed between inspect_container and start_container. Handled by catching NotFound on start, so no actual bug, but worth noting the race is accounted for.
• update_message_cas with generation 0 in clear_sandbox_error() (mod.rs:904) — Passing 0 as CAS generation means no optimistic concurrency check. Safe here since this runs at startup before watchers spawn (per doc comment), but fragile if ever called later. Matches existing mark_sandbox_error() pattern though.

Verdict

Solid fix. Tests cover all three outcomes (container exists, gone, driver error). Manual E2E verification thorough. Clean removal of now-unnecessary sandbox_phase_should_be_running(). No blocking concerns.

[maxamillion]

@r3v5 This is great, thank you for looking into this and proposing a fix.

Two points I was hoping to get some clarification on or possibly some changes.

1. As I understand it, the current logic retries every sandbox in Error, which could revive containers that failed for legitimate terminal reasons and replace the original error with Resumed. Would it make sense to restrict recovery to the expected runtime-restart case, such as SIGTERM/exit 143 or an explicitly recoverable stored reason?

2. It looks as though clear_sandbox_error() swallows store/CAS failures, but the caller still logs recovered=true and increments the recovery count. Ideally we could count/log recovery only after the state transition succeeds in order to not mask failures unintentionally.

[r3v5]

@r3v5 This is great, thank you for looking into this and proposing a fix.

Two points I was hoping to get some clarification on or possibly some changes.

1. As I understand it, the current logic retries every sandbox in Error, which could revive containers that failed for legitimate terminal reasons and replace the original error with Resumed. Would it make sense to restrict recovery to the expected runtime-restart case, such as SIGTERM/exit 143 or an explicitly recoverable stored reason?
2. It looks as though clear_sandbox_error() swallows store/CAS failures, but the caller still logs recovered=true and increments the recovery count. Ideally we could count/log recovery only after the state transition succeeds in order to not mask failures unintentionally.

Hi, @maxamillion ! Thanks for the review! I have addressed your two points in the new commit:

1. Recovery is now restricted to ContainerExited and ContainerStopped reasons only — sandboxes that failed for terminal reasons (BackendResourceMissing, ResumeFailed, etc.) are skipped entirely. Added a test for this case.
2. clear_recoverable_error() now returns bool — the caller only logs recovered=true and increments the counter recovered after the CAS write actually succeeds.

[r3v5]

Hey @elezar ! Could you please take a look on my PR? Thanks!

[r3v5]

Rebased PR from main and solved merge conflicts.

[r3v5]

@pimlock @krishicks can I get some eyes?

[r3v5]

Hi @johntmyers ! Can you PTAL?

[johntmyers]

gator-agent

PR Review Status

Thanks @r3v5. I checked your new runtime-restart reason, the recovery allowlist, and the regression coverage. GATOR-428b8592-01 is resolved for ordinary Podman exits, and I resolved its review thread. One related current-head blocker remains: Docker still records every exited container as ContainerExited, so the tightened allowlist can no longer recover Docker machine-restart victims.

Action required: classify Docker exit 137/143 records distinctly from ordinary exits and cover both recovery and terminal behavior.

Blocking findings:

• GATOR-b58ecee3-01: Docker runtime-restart recovery is unreachable under the new reason gate.

Carried findings:

• None

Gator metadata

• Validation: Project-valid concentrated fix for linked issue #2179.
• Docs: No Fern update needed because this remains an internal startup-recovery correction without a CLI, API, configuration, or documented-workflow change.
• Checks: Current-head required checks are pending and test dispatch is deferred while review feedback remains.
• E2E: test:e2e remains applied; current-head E2E has not yet been confirmed queued, running, or complete.
• Head SHA: b58ecee398d26a49816edb74852a74c90cddb8f0
• Base SHA: de4c1fecf564cd16854447ec9c08659e7b64061b
• Merge base SHA: de4c1fecf564cd16854447ec9c08659e7b64061b
• Patch ID: a0286570c067ddc08898e7be0fd7a431c24653e7
• Gator payload: 7
• Review mode: follow_up
• Previous reviewed SHA: 428b85924b0cb2aad3dba4e3055b55a59e6ddfc3
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:in-review

[r3v5]

Addressed GATOR-b58ecee3-01 (Docker restart recovery unreachable) in b778f01.

The Docker driver now mirrors the Podman classification. current_snapshots inspects EXITED containers to recover the exit code and OOM flag (the list-summary path has no exit code), then apply_docker_exit_classification reclassifies non-OOM signal kills (137/143) to the ContainerRuntimeRestart ready reason. Ordinary exits and OOM kills stay terminal (ContainerExited).

Verify: an exited container with code 143 and oom_killed=false persists ContainerRuntimeRestart and is recovered at startup; code 1 (or OOM+137) stays ContainerExited and terminal.

New unit tests: docker_signal_kill_reclassified_as_runtime_restart, docker_ordinary_exit_stays_terminal, docker_oom_kill_stays_terminal_despite_137. Full suite green (docker 116, server 405, core 1412; pre-commit clean).

[johntmyers]

/ok to test b778f01

[johntmyers]

gator-agent

PR Review Status

Thanks @r3v5. I checked the Docker exit inspection and classification you added at b778f01f, including the ordinary-exit and OOM safeguards and their regression coverage. GATOR-b58ecee3-01 is resolved, its review thread is closed, and no blocking findings remain.

Blocking findings:

• No blocking findings remain

Carried findings:

• None

Gator metadata

• Validation: Project-valid concentrated fix for linked issue #2179.
• Docs: No Fern update needed because this remains an internal startup-recovery correction without a CLI, API, configuration, or documented-workflow change.
• Checks: Current-head mirror setup is pending; required Branch Checks, Helm Lint, and E2E have not yet been confirmed queued, running, or complete.
• E2E: test:e2e is applied and /ok to test b778f01f568f747923c8c1b4cf1c252e6d4d80e0 was posted; mirror setup is pending before E2E dispatch can be confirmed.
• Head SHA: b778f01f568f747923c8c1b4cf1c252e6d4d80e0
• Base SHA: de4c1fecf564cd16854447ec9c08659e7b64061b
• Merge base SHA: de4c1fecf564cd16854447ec9c08659e7b64061b
• Patch ID: 23efc9c3b51e15bd31581e2e1173f3749ba6b9ed
• Gator payload: 7
• Review mode: follow_up
• Previous reviewed SHA: b58ecee398d26a49816edb74852a74c90cddb8f0
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:in-review

[johntmyers]

gator-agent

Merge Decision Nudge

This PR has been in gator:merge-ready for more than 48 business hours with maintainer approval present and no merge or close decision.

@NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr, can someone merge this PR or close/request changes if it should not proceed?

[johntmyers]

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: The PR reached gator:merge-ready with maintainer approval and all required current-head checks green, including E2E.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.