fix(server): prevent unrelated sandbox deletes from blocking deletion events#2340
fix(server): prevent unrelated sandbox deletes from blocking deletion events#2340pimlock wants to merge 3 commits into
Conversation
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Label |
|
/ok to test 0f18d50 |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 8283eec |
| self.cleanup_sandbox_owned_records(&sandbox).await; | ||
| self.sandbox_index.update_from_sandbox(&transition.deleting); | ||
| self.sandbox_watch_bus.notify(&candidate_id); | ||
| drop(guard); |
There was a problem hiding this comment.
Does returning before we call drop(guard) mean that we run the risk of keeping the lock? (Does Rust have an equivalent to a defer drop(guard)?
There was a problem hiding this comment.
In this case, the guard is dropped explicitly, so it's released before the function returns. Without that, it happens automatically when the value goes out of scope (e.g. function returns). They refer to it as RAII.
Btw using it after drop would fail the compilation, as drop takes the ownership.
I don't know how common this approach of dropping explicitly is, alternatively the part that needs the lock could be a separate function and then the explicit drop wouldn't be needed.
This was first pass the agent took (after iterating on the plan), so it may need some tweaking. One thing I'd like to have is that the sandbox delete lock cannot be acquired if the global lock is acquired, otherwise deadlocks could happen. I think there should be a way of representing it with the type system to make that enforced, but need to spend some more time on this.
| .ok_or_else(|| Status::not_found("sandbox not found"))?; | ||
| let candidate_id = candidate.object_id().to_string(); | ||
| let delete_gate = self.delete_gates.gate_for(&candidate_id); | ||
| let _delete_guard = delete_gate.lock().await; |
There was a problem hiding this comment.
This is dropped when the variable goes out of scope, so right before this function returns.
| let delete_gate = runtime.delete_gates.gate_for(original.object_id()); | ||
| let delete_guard = delete_gate.lock().await; |
There was a problem hiding this comment.
Under which condition is it required to take a lock? Some code (possibly only tests) call gate_for without a subsequent .lock().await.
There was a problem hiding this comment.
The lock ensures only one delete operation is happening at the same time for a given sandbox. Having multiple run at the same time introduces issues related to how the cleanup is performed and could cause inconsistent state in DB.
Before this change, we'd acquire the global lock for the whole deletion process, so the limitation was that only one sandbox operation (deletion or otherwise) could be happening at the same time. That included invoking driver.delete, which may be slow (waiting for the process to respond to sigterm/sigkill).
This applies the global lock more narrowly and uses local lock for the deletion process (the deletions of unrelated sandbox are independent).
This was the simplest solution we found with the agent, it's not bullet proof, of course it works only for a single gateway, if the mechanism relied on DB for leasing the deletion process, that would work if we have multiple gateways, but that seemed too complex for this fix. But it maybe worth exploring other options.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 6a15fd9 |
Summary
Prevent one slow sandbox deletion from blocking unrelated watcher events or deletes. The gateway now serializes duplicate deletes per stable sandbox ID, persists and recovers deletion transitions with version checks, and performs backend calls outside the gateway-wide state guard.
Related Issue
Closes #2326
Changes
SandboxDeleteGuardproving that the ID-scoped gate is held.The delete guard makes the intended lock order visible and harder to violate within the delete workflow, but it does not prevent a future caller from acquiring the global lock before requesting a delete gate. Enforcing that globally would require ranked-lock tracking or a broader refactor that restricts all direct lock acquisition, which is outside this PR's scope.
Deviations from Plan
Settings cleanup intentionally retains its existing best-effort name-based behavior. An earlier atomic persistence extension was removed to keep this PR scoped to the deletion concurrency issue.
Testing
mise run pre-commitpassesTests added or updated:
Checklist