Skip to content

fix(server): keep Error-phase sandbox records through the prune sweep - #3498

Open
pkhodade-NV wants to merge 1 commit into
windowsfrom
fix/mxc-sandbox-prune-keeps-error-phase
Open

pkhodade-NV wants to merge 1 commit into
windowsfrom
fix/mxc-sandbox-prune-keeps-error-phase

Conversation

@pkhodade-NV

Copy link
Copy Markdown
Collaborator

Summary

  • The periodic store-vs-backend reconciliation sweep (reconcile_store_with_backend / prune_missing_sandbox in openshell-server) deletes any persisted sandbox record not present in the driver's live backend snapshot, except for Completed and failed-main-process phases.
  • A compute driver whose registry is purely in-process and never rehydrates after a restart (no persistence of its own -- e.g. MXC) reports every previously-known sandbox as "missing" on the very first sweep after gateway startup, including ones already correctly, terminally marked Error by earlier crash detection.
  • Result: the sweep silently deletes those Error-phase records shortly after a gateway restart, racing any client (GetSandbox/ListSandboxes/DeleteSandbox) working with the same sandbox in that window -- some requests see the full record, others see it as already gone, depending purely on timing against the background sweep.

Related Issue

No linked issue -- this is a localized correctness fix to a background reconciliation sweep's phase handling, disproving and superseding an initial hypothesis (that GetSandbox and ListSandboxes read from different backing stores) with a confirmed root cause after tracing both RPC handlers and the sweep itself.

Changes

  • compute/mod.rs: prune_missing_sandbox now treats SandboxPhase::Error the same as the existing Completed exemption -- both are already-settled, informational terminal states with no live compute resource left to reclaim, so the durable record is kept instead of deleted. (Stopping/Stopped/Starting sandboxes are unaffected -- they're still re-marked Error with a ComputeResourceMissing condition, as before.)

Testing

  • Added prune_missing_sandbox_keeps_error_phase_records, mirroring the existing prune_missing_sandbox_releases_driver_resources test harness: puts an Error-phase sandbox in the store, runs a sweep with zero grace period, asserts the record survives with its phase unchanged and no driver delete call was made.
  • Full compute:: module test suite passes: cargo test -p openshell-server --target x86_64-pc-windows-msvc --lib compute::.

Checklist

  • Tests added for the new behavior
  • No unrelated changes bundled in

Originally opened as GitLab MR !116 against our internal mirror; re-opened here against windows for upstream review.

The periodic store-vs-backend reconciliation sweep deleted any
persisted sandbox not present in the driver's live backend snapshot,
except for Completed and failed-main-process phases. A driver whose
registry is in-process-only and never rehydrates after a restart (no
persistence of its own) reports every previously-known sandbox as
missing on the very first sweep after startup -- including ones
already correctly, terminally marked Error by earlier crash detection
-- so the sweep silently deleted them shortly after gateway restart,
racing any client (GetSandbox/ListSandboxes/DeleteSandbox) working
with the same sandbox in that window.

Treat Error the same as the existing Completed exemption: it is
already a settled, informational terminal state with no live compute
resource to reclaim, so keep the durable record instead of deleting
it.

Signed-off-by: Prashant Khodade <pkhodade@nvidia.com>
(cherry picked from commit 721a1659a822a72e76e3c0dffc6847f17129a3fc)
@copy-pr-bot

copy-pr-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

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.

@shailendra-nv shailendra-nv left a comment

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.

Requesting changes because the blanket Error exemption changes established missing-backend cleanup for every compute driver, not only the MXC restart case. Please address the inline finding before merge.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants