Repository navigation
fix(server): keep Error-phase sandbox records through the prune sweep - #3498
Merged
Merged
Conversation
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 <[email protected]> (cherry picked from commit 721a1659a822a72e76e3c0dffc6847f17129a3fc)
pkhodade-NV
requested review from
a team,
derekwaynecarr,
mrunalp and
sjenning
as code owners
September 20, 2026 15:11
shailendra-nv
requested changes
Sep 21, 2026
shailendra-nv
left a comment
shailendra-nv
left a comment
Collaborator
There was a problem hiding this comment.
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.
The blanket phase == SandboxPhase::Error exemption changed established missing-backend cleanup for every compute driver, not only the MXC restart race the PR intended to fix. It also matched BackendResourceMissing (set by gateway-startup recovery when a previously-known sandbox's backend resource is already gone), StartFailed (startup recovery's driver-error case), and ComputeResourceMissing (this same sweep's own first-pass Error transition for a Stopping/Stopped/Starting sandbox). All three mark exactly the orphaned resources this sweep exists to reclaim across Docker, Podman, VM, Kubernetes, and extension drivers -- exempting them left orphaned names and gateway-owned records in place indefinitely and skipped the idempotent driver cleanup for volumes/secrets until a user explicitly deleted the sandbox. Add is_missing_compute_resource_reason to inspect the sandbox's Ready condition and narrow the exemption to a settled Error record only: one whose reason isn't one of those three. A crashed main process or any other non-resource failure keeps the exemption (no live resource ever expected again); a resource-missing reason keeps flowing through the normal delete-and-cleanup path exactly as before this PR. Adds regression coverage for BackendResourceMissing and ComputeResourceMissing confirming they are still pruned with driver cleanup invoked, and documents the settled-vs-missing-resource retention distinction in architecture/compute-runtimes.md. Signed-off-by: Prashant Khodade <[email protected]>
…une-keeps-error-phase
Collaborator
|
/ok to test |
@shailendra-nv, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
Collaborator
|
/ok to test 777e981 |
shailendra-nv
approved these changes
Sep 22, 2026
shailendra-nv
left a comment
shailendra-nv
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 777e981. The prior Error-phase cleanup finding is addressed by the missing-resource reason filter, regression coverage, and lifecycle documentation. Branch Checks and all applicable required gates pass at this exact head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reconcile_store_with_backend/prune_missing_sandboxinopenshell-server) deletes any persisted sandbox record not present in the driver's live backend snapshot, except forCompletedand failed-main-process phases.Errorby earlier crash detection.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
GetSandboxandListSandboxesread from different backing stores) with a confirmed root cause after tracing both RPC handlers and the sweep itself.Changes
compute/mod.rs:prune_missing_sandboxnow treatsSandboxPhase::Errorthe same as the existingCompletedexemption -- 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/Startingsandboxes are unaffected -- they're still re-markedErrorwith aComputeResourceMissingcondition, as before.)Testing
prune_missing_sandbox_keeps_error_phase_records, mirroring the existingprune_missing_sandbox_releases_driver_resourcestest harness: puts anError-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.compute::module test suite passes:cargo test -p openshell-server --target x86_64-pc-windows-msvc --lib compute::.Checklist
Originally opened as GitLab MR !116 against our internal mirror; re-opened here against
windowsfor upstream review.