fix: delete paused sandboxes transparently

[zyl1121]

Motivation

Deleting a paused sandbox currently fails because the runtime rejects the normal delete operation while the VM is paused. Users must manually resume the sandbox before deleting it.

This PR makes DELETE /sandboxes/{sandboxID} work for a stable paused CubeBox. The sandbox is resumed as part of the delete operation and then removed through the existing destroy workflow. DELETE remains synchronous: success is returned only after cleanup completes.

Design

CubeMaster remains the external DELETE entry point and continues to coordinate both synchronous and asynchronous destroy requests. The atomic resume-before-destroy transition is implemented in Cubelet's Destroy path.

Cubelet owns the authoritative runtime state, containerd task, and the per-sandbox lifecycle lock shared by pause, resume, rollback, and commit. Only paused or pausing Cube runtime deletes enter this bounded gate, so running deletes retain their existing direct path. Keeping the transition in Cubelet keeps state inspection, resume, reconciliation, and normal cleanup together.

This supersedes the API-only paused-delete workaround in #831. That workaround translated a CubeMaster internal error into 409 Conflict after an extra sandbox-status lookup. This PR removes that fallback: DELETE now performs the bounded wake-up itself and returns typed capacity or retryable lifecycle errors only when the actual wake-up cannot proceed.

Behavior

• A stable PAUSED sandbox is resumed internally, then follows the normal destroy workflow.
• A RUNNING sandbox keeps the existing delete path and does not resume.
• If a resume RPC reports an error but runtime reconciliation proves the VM is RUNNING, DELETE continues. Otherwise the sandbox remains retryable.
• The internal wake-up is bounded to 5 seconds, with 20 seconds reserved for cleanup and 5 seconds reserved for the Cubelet RPC response. The existing CubeAPI 30-second request timeout is unchanged.
• DELETE does not emit a caller-visible sandbox.resumed event, reset idle timeout, or create a background retry.

Error Semantics

DELETE for running sandbox and existing 404 behavior are unchanged. For the paused-delete path:

• Capacity or missing resource error returns 409 Conflict.
• PAUSING or another lifecycle operation in progress returns 503 with Retry-After: 2.
• A failed or unproven internal resume, or insufficient remaining Cubelet RPC budget, returns 503 with Retry-After: 5.

Retry-After: 5 follows CubeProxy's existing auto-resume failure backoff, while Retry-After: 2 follows its PAUSING gate. These headers tell the client when to retry; DELETE is not continued or retried in the background.

Scope

This PR does not add a public DELETE parameter or protobuf field, bypass resume admission, change the existing CubeShim delete guard, alter ordinary running DELETE behavior, or add a new metric.

Validation

Focused Linux builder checks passed:

cd Cubelet && go test ./pkg/utils -run TestResourceLock -count=1
cd Cubelet && go test ./services/cubebox -run 'TestResumeTaskLocked|TestDeleteAutoResume|TestDestroy' -count=1
cd CubeMaster && go test ./pkg/service/sandbox -run TestSetSyncDestroyFailure -count=1
cd CubeAPI && cargo test --locked
cd CubeAPI && cargo test --locked delete_paused_sandbox_maps_business_errors_from_cubemaster

The full CubeAPI suite completed with 88 passed; 0 failed. The focused HTTP route test verifies CubeMaster 130409 -> HTTP 409, alongside the 503 and Retry-After cases.

Real-server validation built and atomically deployed the final Cubelet, CubeMaster, and CubeAPI binaries. All three services remained active and both health endpoints returned success.

Live E2E validation created a temporary sandbox, paused it, and deleted it directly without an explicit resume or connect request:

• pause returned 204, and GET reported paused;
• DELETE returned 204 No Content in 629 ms;
• subsequent GET returned 404;
• no matching containerd task or sandbox data path remained.

References

Fixes #219.

Replaces the API-only paused-delete workaround from #831.

[cubesandboxbot]

PR #940 — Transparent deletion of paused sandboxes

Overview

This PR implements automatic resume-before-delete for paused sandboxes across three service layers (Cubelet → CubeMaster → CubeAPI), with appropriate error propagation, Retry-After headers, and comprehensive test coverage. The design is sound and the implementation is production-quality.

---

Key Strengths

1. Crash-safe sync ordering — The converged RUNNING state is persisted before the delete marker, so a crash between the two syncs leaves a running (not paused, not half-deleted) sandbox.
2. TOCTOU correctness — The lifecycle lock protects against a sandbox transitioning from RUNNING→PAUSED between the initial state read and the destroy workflow. The state is re-read under the lock.
3. Robust deadline budget — The proportional split with clamping prevents degenerate allocations at all remaining-time values.
4. End-to-end error consistency — Each business code flows through isDeleteAutoResumeBusinessCode → map_delete_cubemaster_err with the correct HTTP status and Retry-After header.
5. Thorough test coverage — Every failure mode (pausing rejection, deadline exhaustion, shim timeout, reconciliation, persistence failure, lock contention) is tested, and the running-sandbox path is verified to be unchanged.

---

Actionable Suggestions

1. Extract shared response-reserve helper (delete_resume.go:51 and delete_resume.go:204)

The remaining/6 clamped-to-[1s, 5s] computation is duplicated in newDeleteDeadlineBudget and deleteLifecycleLockDeadline. A small helper would keep them consistent if the formula evolves.

2. Add explanatory comment for error code conflation (service.go:590)

ErrorCode_TaskResumeFailed is used for three distinct scenarios: (a) insufficient RPC response time before even trying the lock, (b) the resume RPC itself failed, and (c) persisted-state sync failed. All three produce the same HTTP 503 with a 5-second retry, so the external contract is identical, but a comment would help operators reading logs understand that case (a) is not a resume attempt.

3. Move syncCtx inside the if opts.persist block (update.go:290)

syncCtx is allocated on every call to resumeLocked but only used when opts.persist is true (i.e., the explicit resume path). For the delete path (persist=false), this is a dead allocation. Minor tidiness.

---

Minor Observations

• The 1/6 and 1/4 proportional split ratios (delete_resume.go:51-52) are opaque without a comment explaining the rationale.
• The handler #[utoipa::path] annotation (handlers/sandboxes.rs:204) describes 503 with the same wording as openapi.yml but omits the explicit Retry-After value mention (2s / 5s) from the description text.
• release_paused_resources_test.go was correctly simplified: the admitResume signature change (dropping the response parameter that was only written by admission errors) removed the need for test code that constructed a dummy UpdateCubeSandboxResponse.

---

No Critical Issues Found

• No security concerns — the lifecycle lock and TOCTOU guards prevent any bypass of the existing state machine.
• No performance regressions — running-sandbox delete retains the existing fast path; the new lock-and-resume overhead only applies to paused sandboxes, and is bounded by the deadline budget.
• No test coverage gaps — all code paths are covered, including the reconciliation case where the shim proves RUNNING despite an RPC error.
• Documentation is accurate and consistent between the English and Chinese lifecycle guides and the OpenAPI spec.

[kinwin-ustc]

Good work. I noticed the automatic resume timeout is 5 seconds. If we support cross-node pause resume in the future, the resume time might be longer (due to retrieving snapshots from remote shared storage). Could these timeouts be made more configurable?

[zyl1121]

Good work. I noticed the automatic resume timeout is 5 seconds. If we support cross-node pause resume in the future, the resume time might be longer (due to retrieving snapshots from remote shared storage). Could these timeouts be made more configurable?

Thanks, that makes sense. The current 5-second resume timeout is actually part of the existing 30-second synchronous DELETE budget, which also has to cover the RPC call and cleanup. Exposing it as an independent setting could therefore create inconsistent deadlines or leave too little time for cleanup.

Making this configuration effective would require revisiting the timeout budget across the full DELETE path. That would involve the API timeout, the CubeMaster-to-Cubelet RPC deadline, the Cubelet cleanup budget, and the corresponding configuration and tests.

Since this PR is focused on fixing deletion of paused sandboxes within the existing synchronous DELETE behavior, I think adding an end-to-end configurable timeout model would broaden its scope considerably. I’d prefer to keep the current conservative budget here and track the timeout configuration work as a follow-up for cross-node pause/resume scenario.

[ls-ggg]

The 5/20/5-second split ( deleteAutoResumeMaxDuration, deleteDestroyReserve, deleteResponseReserve) in delete_resume.go is hardcoded in absolute seconds, but it implicitly assumes the caller's ctx deadline is ~30s (i.e. CubeMaster's common_timeout_insec, currently defaulting to 30s). These two configs live in different services and can drift independently — if common_timeout_insec is ever tuned down (e.g. to 10s) without updating these constants, latestResumeDeadline will always fail the !latestResumeDeadline.After(now) check, and every delete of a paused sandbox will short-circuit to the "not enough time" error path, not just occasionally but permanently.
Suggest deriving the resume budget
proportionally from the actual remaining time on ctx ( deadline, _ := ctx.Deadline(); remaining := time.Until(deadline)) instead of subtracting fixed absolute reserves. e.g. reserve a percentage of the remaining budget for destroy/response rather than a fixed number of seconds, so this stays correct regardless of what common_timeout_insec / destroy_dead_line are configured to. At minimum this coupling should be documented so operators don't hit a silent, hard-to-diagnose regression when tuning timeouts.

[ls-ggg]

needsPausedPreflight is computed from a status read taken before acquiring sandboxLifecycleLocks. If the sandbox is observed as RUNNING at that point (so needsPausedPreflight == false), the request skips the lock
entirely and falls straight through to engine.Destroy(...) unprotected. If a concurrent Update(pause) wins sandboxLifecycleLocks and pauses the sandbox in the gap between that read and the actual destroy call, this Destroy request ends up doing a plain destroy against what is now a paused sandbox — the exact scenario this PR is meant to guard against — and it does so without holding the lock at all.
This isn't a regression introduced by this PR (the old code read status unlocked too), but since the PR's stated goal is "protect destroy from racing with pause/resume/commit/rollback via the lock," it's worth calling out that the guarantee is currently best-effort rather than fully closed: the decision to take the lock is itself made outside the lock. Would it be worth acquiring sandboxLifecycleLocks
unconditionally on entry (before reading status) and deciding the resume-vs-plain-destroy branch under the lock, rather than deciding whether to lock based on a stale read? If the current scope is intentionally narrower than that, a code comment noting the residual race would help future maintainers avoid assuming this is fully serialized.

[ls-ggg]

Thanks for the PR! Please address the CI failures and the review comments above. Also, could you add more comprehensive e2e test cases (covering concurrency, PAUSING state, and resume timeouts)?

[zyl1121]

Thanks for the PR! Please address the CI failures and the review comments above. Also, could you add more comprehensive e2e test cases (covering concurrency, PAUSING state, and resume timeouts)?

@ls-ggg Thanks for the detailed review. Both inline issues have been addressed in the latest commit:

1. The paused-delete budget is now calculated from the actual Cubelet request deadline. The default 30-second path still allocates 5s for resume, 20s for cleanup, and 5s for the response. For shorter RPC deadlines, DELETE now derives a smaller bounded budget instead of rejecting the request solely because the previous fixed reservations do not fit.
2. Cube runtime DELETE now acquires the per-sandbox lifecycle lock before making its final state decision, then reloads the sandbox while holding that lock. This closes the race where a sandbox could transition from RUNNING to PAUSED between the initial read and destruction.

Additional focused coverage now includes task-loading failures, reconciliation failures, unexpected shim states, and the fallback for empty CubeMaster error messages. The duplicated resume-task test fake has also been consolidated.

For validation, Cubelet was rebuilt and paused DELETE was verified against a real server. For concurrency validation, I ran five real-server trials with Pause and DELETE issued concurrently. In two trials, DELETE reached Cubelet while Pause still held the lifecycle lock and returned 503 Service Unavailable with Retry-After: 2. Retrying after the advertised delay returned 204 No Content. In the other three trials, DELETE did not overlap with the lock-holding window and completed successfully. All five sandboxes were ultimately deleted, with no remaining containerd resources or sandbox data.

The real-server validation covers normal paused deletion and concurrent Pause/DELETE behavior. The PAUSING state and Resume timeout paths are covered by service-level tests that set the relevant lifecycle state or inject failures into the resume and status operations, then verify the resulting DELETE response and retry behavior.

Stable full-stack E2E coverage for those two fault paths (PAUSING state and Resume timeout) cannot currently be added because CubeSandbox does not provide a CI-managed harness spanning CubeAPI -> CubeMaster -> Cubelet, or controlled fault-injection support for holding a sandbox in PAUSING or forcing a shim Resume timeout. Without that infrastructure, a real-server test would be timing-dependent and flaky. Once those capabilities are available, I would be happy to add the corresponding E2E cases.

The previous failed claude-response CI check was unrelated to this PR’s code or tests. It is the known workflow permission issue described in #844: normal review or comment activity can trigger the workflow, which then fails the actor permission check. All 10 PR checks were passing before this comment, as shown below.