fix(orchestrator): treat persistence cleanup failures as operation failures - #82
fix(orchestrator): treat persistence cleanup failures as operation failures#82Ayush7614 wants to merge 2 commits into
Conversation
…ilures Delete no longer reports success when paused artifacts remain on disk, which could resurrect sandboxes after restart. Resume no longer leaves a Resuming marker after success; a failed record delete rolls the sandbox back to Paused.
|
🔍 OpenCodeReview found 7 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if let Err(err) = self | ||
| .persister | ||
| .delete_record_and_artifacts(&sandbox_id) | ||
| .await | ||
| { | ||
| warn!(error = ?err, "failed to delete persisted sandbox state"); | ||
| if let Err(restore_err) = self | ||
| .store | ||
| .update_state_if_state(&sandbox_id, previous_state, &[SandboxState::Killing]) | ||
| .await |
There was a problem hiding this comment.
[bug · high]
For a running sandbox, the runtime has already been stopped and its handle and proxy route detached before this fallible call. Restoring only the metadata state to Running therefore leaves a sandbox that is reported as running but is unreachable and has no live handle; resume_sandbox will also treat it as already running. Perform persistence deletion before detaching/stopping the runtime, or implement a rollback that genuinely restores/restarts the handle and proxy route. Please add a failure-path test for deleting a running sandbox, since the new test covers only the paused case.
| .await?; | ||
| orchestrator.pause_sandbox(created.id).await?; | ||
| persister.clear_calls(); | ||
| persister.fail_next(RecordingCall::DeleteRecordAndArtifacts); |
There was a problem hiding this comment.
[test · high]
This failure injection is atomic, but the real file-backed implementation first removes the record and then removes the artifact directory. If artifact removal fails, this test still expects metadata to return to Paused even though the durable record is already gone; a restart would then lose the sandbox. Add a persister test double (or file-backed failure test) that fails after record removal, and adjust the production operation to preserve recoverability or make cleanup transactional.
| // Retry succeeds once persistence cleanup works again. | ||
| orchestrator.delete_sandbox(created.id).await?; | ||
| assert!(orchestrator.get_sandbox(&created.id).await?.is_none()); |
There was a problem hiding this comment.
[test · high]
This only tests failure before metadata removal. The new production ordering deletes durable paused state before calling the fallible store.remove; if that removal fails, metadata remains (typically in Killing) while its recovery record/artifacts are already gone. Add a metadata-store removal failure case and ensure the implementation either restores durable state or otherwise makes the two deletions atomic/recoverable.
| .await?; | ||
| orchestrator.pause_sandbox(created.id).await?; | ||
| persister.clear_calls(); | ||
| persister.fail_next(RecordingCall::DeleteRecord); |
There was a problem hiding this comment.
[test · medium]
The rollback introduced for this failure is itself fallible, but this test only covers successful RollbackResuming. If rollback fails, production currently logs and ignores it after changing metadata back to Paused, while the durable record remains Resuming and is discarded on restart. Add a rollback-failure case and define/verify a consistent recoverable outcome rather than masking this secondary failure.
Address OpenCodeReview: delete durable state before detaching a running runtime, remove overlay artifacts before the paused record, retry metadata removal after durable cleanup, and surface resume rollback failures instead of leaving a discardable Resuming marker.
| Self::remove_artifact_root(&self.sandbox_artifact_root(sandbox_id)).await?; | ||
| self.remove_record(sandbox_id).await?; |
There was a problem hiding this comment.
[bug · high]
If artifact removal succeeds but remove_record fails, this returns an error while leaving the durable record (and, in the updated caller, restoring the sandbox to its previous state) even though all backing artifacts are already gone. A paused sandbox can then neither resume nor recover after restart. The stated guarantee also does not hold when remove_dir_all partially removes a directory before failing. Delete the record first so a later artifact-cleanup failure leaves only orphaned artifacts, which are already handled by startup cleanup, rather than a loadable record pointing at missing files.
Suggestion:
| Self::remove_artifact_root(&self.sandbox_artifact_root(sandbox_id)).await?; | |
| self.remove_record(sandbox_id).await?; | |
| self.remove_record(sandbox_id).await?; | |
| Self::remove_artifact_root(&self.sandbox_artifact_root(sandbox_id)).await?; |
| if let Err(err) = self | ||
| .persister | ||
| .delete_record_and_artifacts(&sandbox_id) | ||
| .await | ||
| { |
There was a problem hiding this comment.
[bug · high]
Durable paused state is irreversibly removed before several fallible/cancellation-sensitive operations. If sandbox.stop() fails, the code below restores previous_state even though the record/artifacts are already gone; if store.remove() fails twice or this future is cancelled, metadata can remain in Killing with the runtime detached/stopped and no restart recovery state. This is especially dangerous because delete_record_and_artifacts can also fail after deleting artifacts but before deleting the record. Use a reversible/tombstoned persistence transaction, or defer final durable deletion until runtime/store cleanup has succeeded while retaining enough state to roll back every intermediate failure.
Suggestion:
| if let Err(err) = self | |
| .persister | |
| .delete_record_and_artifacts(&sandbox_id) | |
| .await | |
| { | |
| // Stage durable deletion here, but only commit it after runtime and | |
| // metadata cleanup succeed; abort the staged deletion on any failure. |
| // cleanup_failed_launch already attempted rollback_resuming once. | ||
| // Retry so a transient failure cannot leave a Resuming marker that | ||
| // load_all would discard on the next process start. | ||
| if let Err(rollback_err) = self.persister.rollback_resuming(&sandbox_id).await { |
There was a problem hiding this comment.
[bug · medium]
This second rollback is unconditional because cleanup_failed_launch swallows the first result. If the first rollback succeeded, this rewrites an already-paused record; that extra write can fail and replace the original delete_record error even though durable rollback was successful. Have cleanup return the first rollback result/status and retry only when that attempt failed, preserving the original error once recovery succeeds.
| if let Err(err) = self.persister.rollback_resuming(&plan.sandbox_id()).await { | ||
| warn!(error = %format_args!("{err:#}"), "failed to restore persisted sandbox record lifecycle during launch rollback"); | ||
| } | ||
| if let Err(err) = self |
There was a problem hiding this comment.
[bug · high]
When rollback_resuming fails, execution still changes the in-memory metadata to Paused. Persistence remains Resuming, and load_all may discard that record after restart, creating the exact inconsistency the new ordering intends to prevent. This affects every earlier resume failure path that calls cleanup_failed_launch, not just the delete_record failure path with its extra retry. Only transition metadata to Paused after durable rollback succeeds; otherwise retain a non-stable state and propagate/record the cleanup failure so it can be retried.
Suggestion:
| if let Err(err) = self.persister.rollback_resuming(&plan.sandbox_id()).await { | |
| warn!(error = %format_args!("{err:#}"), "failed to restore persisted sandbox record lifecycle during launch rollback"); | |
| } | |
| if let Err(err) = self | |
| if let Err(err) = self.persister.rollback_resuming(&plan.sandbox_id()).await { | |
| warn!(error = %format_args!("{err:#}"), "failed to restore persisted sandbox record lifecycle during launch rollback"); | |
| return; | |
| } | |
| if let Err(err) = self |
| // First metadata remove fails after durable cleanup; retry delegates. | ||
| control.push_remove_action(StoreAction::Fail(StoreError::Backend { | ||
| source: anyhow::anyhow!("injected metadata remove failure after durable cleanup"), | ||
| })); |
There was a problem hiding this comment.
[test · high]
This only exercises the successful retry. The newly added retry branch has a critical untested exhaustion path: if both remove calls fail, durable state is already deleted and the runtime has already been stopped/detached, while metadata remains Killing. A later delete can then wait indefinitely for that transition. Add a case that queues two remove failures (especially for a running sandbox), verifies the error, and requires the implementation to leave a recoverable state so a subsequent delete can complete.
| vec![ | ||
| RecordingCall::MarkResuming, | ||
| RecordingCall::DeleteRecord, | ||
| RecordingCall::RollbackResuming, | ||
| RecordingCall::RollbackResuming | ||
| ] |
There was a problem hiding this comment.
[test · medium]
This expectation codifies an unconditional second durable rollback even when the first RollbackResuming succeeded. The retry should occur only after a failed rollback; otherwise every delete-record failure performs redundant I/O and a failure on the second write can obscure the already-successful recovery. Have the cleanup path return its rollback result and expect only one RollbackResuming in this success case.
| assert!( | ||
| matches!( | ||
| metadata.state, | ||
| SandboxState::Paused | SandboxState::Running | SandboxState::Resuming | ||
| ), | ||
| "unexpected state after rollback failure: {:?}", | ||
| metadata.state | ||
| ); |
There was a problem hiding this comment.
[test · medium]
This assertion accepts every lifecycle state relevant to the operation, so it cannot detect an orphaned running runtime/route or metadata left permanently transitional. In this setup the metadata update itself is not failed, so cleanup should deterministically stop/detach the resumed runtime and restore metadata to Paused, even if durable rollback fails. Assert SandboxState::Paused and assert_proxy_paused (plus the expected metrics) to verify that invariant.
|
Thanks for the contribution. After reviewing this, I don’t think we should merge this change. |
2 participants
Summary
Resumingmarker failed, so a crash could discard the paused record.Pausedif record cleanup fails.Test plan
delete_sandbox_fails_when_persisted_state_cannot_be_removedresume_delete_record_failure_rolls_back_to_pausedcargo test -p agentenv --lib resume_marks_resumingcargo test -p agentenv --lib delete_sandbox_fails_when_persisted