fix(server): release driver-owned sandbox resources on out-of-band removal - #3042
Conversation
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This focused fix is project-valid against linked issue #2352. The initial review found one blocking concurrency regression in the prune path.
Action required: update the prune cleanup so it does not await the driver while holding the gateway-wide synchronization lock, and add the blocked-driver regression test.
Blocking findings:
GATOR-9902b7bb-01: periodic reconciliation holds the global synchronization lock across an unbounded driver RPC.
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Focused implementation for the reproduced resource leak in linked issue #2352; no duplicate candidate found.
- Docs: Architecture documentation updated; no direct Fern UX documentation change required.
- Checks: Current-head branch and Helm gates are pending; pipeline dispatch waits until review feedback is resolved.
- E2E: Required for this sandbox lifecycle change, but not dispatched until the blocker is resolved.
- Head SHA:
9902b7bbd3e57fa88d5712836335472a1f7ee46f - Base SHA:
883a1f01ce3d0ff023163fdb4d989c7c433c7974 - Merge base SHA:
69a05ebb3b154e304a66fe80eed8504e889abc6d - Patch ID:
66c63619807eabd0ac02aad4da65d5e4637cfa8c - Gator payload:
7 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
The prune sweep's second sync_lock acquisition has no inner block scope, so it's held through the rest of prune_missing_sandbox, including the driver cleanup call added in the previous commit. That call is a network RPC to the compute driver; holding the gateway-wide sync_lock across it blocks every other sandbox operation on the gateway for as long as that one driver call takes, not just the sandbox being pruned. Use the same backgrounded spawn_driver_sandbox_cleanup already used by the watch path instead of the inline cleanup_driver_sandbox_resources, which is now unused and removed. Add a regression test that blocks the driver's delete call indefinitely and confirms the prune sweep still completes and removes the store record promptly. Addresses gator finding GATOR-9902b7bb-01 on NVIDIA#3042. Signed-off-by: politerealism <burdcat17@gmail.com>
|
Addressed GATOR-9902b7bb-01: the prune sweep's second |
|
/ok to test 1191803 |
|
Label |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @politerealism. I checked your update that backgrounds the prune-path driver cleanup and the new stuck-driver regression test. The prior lock-held RPC finding is resolved, and the follow-up review found no new blocking issues.
Blocking findings:
- No blocking findings remain
Carried findings:
GATOR-9902b7bb-01: resolved by moving prune cleanup tospawn_driver_sandbox_cleanupand covering a blockedDeleteSandboxcall.
Gator metadata
- Validation: Focused fix for the reproduced resource leak in linked issue #2352.
- Docs: Architecture documentation updated; no direct Fern UX documentation change required.
- Checks: Current-head Branch Checks, Helm Lint, and E2E workflows are dispatched and in progress.
- E2E:
test:e2eapplied;/ok to test 11918038cdf43fa3b58b40b9415be215955bffc1posted; current-head run33422088329is in progress. - Head SHA:
11918038cdf43fa3b58b40b9415be215955bffc1 - Base SHA:
883a1f01ce3d0ff023163fdb4d989c7c433c7974 - Merge base SHA:
69a05ebb3b154e304a66fe80eed8504e889abc6d - Patch ID:
b99f4f0b95633f4c4a9d416c593dccc667385f6e - Gator payload:
7 - Review mode:
follow_up - Previous reviewed SHA:
9902b7bbd3e57fa88d5712836335472a1f7ee46f - Review budget exhausted: no
- Maintainer decision required: no
- Review telemetry: 0 proposed findings, 0 blockers, 0 hypotheses, 0 unchanged-code proposals; 1 finding-bearing round before this clean follow-up.
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
|
@johntmyers this looks more like CI flakiness. Can you rerun the tests? |
|
you might be hitting a race in kube tests i have #3064 to hopefully fix it so may want to wait on that and then rebase |
johntmyers
added
gator:follow-up-needed
…moval Neither the watch path nor the periodic prune sweep call the driver's DeleteSandbox RPC when a sandbox's compute resource is discovered gone out-of-band (container removed without going through OpenShell's own delete path, or the gateway crashing mid-create). Only store-side records were cleaned up, leaving driver-owned secrets and volumes behind indefinitely — a credential leak for the Podman driver's per-sandbox bearer-token and proxy-auth secrets. DeleteSandbox is already idempotent and already reclaims these resources when the underlying container is already gone, so no new RPC is needed: this wires the existing call into the two paths that discover a sandbox is gone without going through an explicit delete request. The watch path defers the actual call to a background task, gated by a non-blocking check of the sandbox's lifecycle gate: watch events are processed sequentially and must never block on a driver call, and skipping the call entirely when a concurrent request-side operation already holds the gate avoids racing that operation's own driver call. The prune sweep calls it inline, since it already makes a blocking GetSandbox call per sandbox as part of its normal operation. Closes NVIDIA#2352 Signed-off-by: politerealism <burdcat17@gmail.com>
The prune sweep's second sync_lock acquisition has no inner block scope, so it's held through the rest of prune_missing_sandbox, including the driver cleanup call added in the previous commit. That call is a network RPC to the compute driver; holding the gateway-wide sync_lock across it blocks every other sandbox operation on the gateway for as long as that one driver call takes, not just the sandbox being pruned. Use the same backgrounded spawn_driver_sandbox_cleanup already used by the watch path instead of the inline cleanup_driver_sandbox_resources, which is now unused and removed. Add a regression test that blocks the driver's delete call indefinitely and confirms the prune sweep still completes and removes the store record promptly. Addresses gator finding GATOR-9902b7bb-01 on NVIDIA#3042. Signed-off-by: politerealism <burdcat17@gmail.com>
1191803 to
31c6fd2
Compare
|
/ok to test 31c6fd2 |
|
@johntmyers rebased onto |
PR Review StatusThe current head is a rebase-equivalent form of the previously reviewed patch, so the resolved concurrency finding remains resolved and no new review round is needed. The maintainer's suggestion to wait for the Kubernetes race fix and rebase has been followed; I verified the effective patch identity and dispatched the required current-head workflows. Blocking findings:
Carried findings:
Gator metadata
|
johntmyers
added
gator:watch-pipeline
johntmyers
added
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: The reviewed fix had no remaining blocking findings, required checks completed successfully, and maintainer approval was present before merge. I removed the active Gator metadata
|
johntmyers
removed
the
gator:approval-needed
2 participants
Summary
Release driver-owned sandbox resources (secrets, volumes) when a sandbox's compute resource is discovered gone out-of-band, instead of only cleaning up the gateway's store-side records. Closes a confirmed credential leak in the Podman driver.
Related Issue
Closes #2352
Changes
spawn_driver_sandbox_cleanup(watch path) andcleanup_driver_sandbox_resources(prune sweep) toComputeRuntime, both calling the existing, already-idempotentDeleteSandboxRPC via a new sharedcall_driver_delete_sandboxhelper.try_lockon the sandbox's lifecycle gate) when a request-side operation is already in flight for that sandbox, since that operation already owns driver-side cleanup and calling again would be redundant or racy.GetSandboxcall per sandbox as part of its normal operation.architecture/compute-runtimes.md's Deletion Lifecycle section to document this behavior.No proto changes. The fix reuses
DeleteSandbox, which is already idempotent and already reclaims Podman's workspace volume, token secret, and proxy-auth secret when the container is already gone — the gap was purely that these two paths never called it.Design Note
An earlier draft added a
CleanupSandboxResources/ReconcileResourcesRPC pair per the design discussed on the issue. Investigation showed neither is needed:DeleteSandboxalready does the reactive-cleanup job, and the gateway already writes a sandbox's store record before calling the driver'sCreateSandbox, so the existing periodic sweep already covers the crash-recovery case (a driver resource with no matching store record never occurs from this code path). AReconcileResources-style RPC would only matter for a store record lost independent ofDeleteSandbox— a different, rarer failure mode than what this issue reports, deliberately left out of scope.This PR closes the ticket for the Podman driver specifically. Docker's own
DeleteSandboxhas a related but separate gap (a branch that doesn't clean up its token file when the container is already gone with no pending record) — filed as #3041, not blocking this PR since it's a pre-existing gap in Docker's own idempotency, not something this change introduces or regresses.Testing
mise run pre-commitpassesapply_deleted_releases_driver_resources_for_out_of_band_removal,apply_deleted_removes_store_record_even_when_driver_cleanup_fails,prune_missing_sandbox_releases_driver_resourcescompute::module suite (169 tests) passes, including the pre-existing lifecycle-gate race tests (waiting_delete_retries_after_leader_failure_recovery,driver_completion_tolerates_watcher_removing_row_in_flight,delete_error_does_not_resurrect_row_removed_by_watcher) and the watch-loop non-blocking test (blocked_delete_does_not_delay_unrelated_deleted_watch_event)Checklist