From 759fde1123b981c5bd5dacce0d99e00685714fe8 Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Wed, 16 Sep 2026 12:10:33 +0800 Subject: [PATCH] fix(api): stop an in-flight resume from undoing a paused sandbox delete If you DELETE a paused sandbox while a resume for it is still running, the delete returns 204 and drops the snapshot, but the resume then publishes the sandbox back as running. The client is told it is gone while it keeps running until timeout, and the snapshot it would resume from is gone too. The reason is that a paused sandbox only has a snapshot, no running-store record. StartRemoving has nothing to lock or pin, so the kill handler just deletes the snapshot without recording any intent. Resume publishes through storage.Add, which is a plain SET+SADD with no check. Nothing serializes the two. Rather than add a separate tombstone key, I reused the reservation a resume already holds the whole time it runs. Before touching the snapshot the kill handler calls ClaimKill, which looks at the pending set and the storage index in one script: if a resume is in flight (or already finished and back in the index) it bails out and the handler returns 409, so the caller retries the kill against the running sandbox. Otherwise it writes a short-lived claim that reserveScript rejects, so any resume that starts after we commit to the delete loses. The claim only has to outlive the snapshot soft-delete becoming durable - after that a resume fails when it fetches the snapshot - and it is released early if the delete fails. This is the same idea as the ExpectExecutionID pin we already use for running sandboxes: make the write that could bring a removed sandbox back check, atomically, that no kill was accepted first. Returning 409 also matches what resume already does when a sandbox is snapshotting. Fixes #3636 --- .../api/internal/handlers/sandbox_kill.go | 41 ++++++ .../internal/orchestrator/create_instance.go | 10 ++ .../internal/orchestrator/delete_instance.go | 17 +++ packages/api/internal/sandbox/aliases.go | 1 + .../reservations/redis/kill_claim_test.go | 139 ++++++++++++++++++ .../sandbox/reservations/redis/reservation.go | 61 +++++++- .../sandbox/reservations/redis/scripts.go | 68 ++++++++- .../sandbox/reservations/redis/utils.go | 8 + .../internal/sandbox/sandboxtypes/errors.go | 6 + .../internal/sandbox/sandboxtypes/storage.go | 9 ++ packages/api/internal/sandbox/store.go | 11 ++ packages/api/internal/sandbox/store_test.go | 8 + 12 files changed, 377 insertions(+), 2 deletions(-) create mode 100644 packages/api/internal/sandbox/reservations/redis/kill_claim_test.go diff --git a/packages/api/internal/handlers/sandbox_kill.go b/packages/api/internal/handlers/sandbox_kill.go index b30ac6b5a9..a58f1e03f9 100644 --- a/packages/api/internal/handlers/sandbox_kill.go +++ b/packages/api/internal/handlers/sandbox_kill.go @@ -65,9 +65,21 @@ func (a *APIStore) DeleteSandboxesSandboxID( Action: sandbox.StateActionKill, Reason: sandbox.KillReasonRequest, }) + + // runningRemoved records whether RemoveSandbox killed a running record. When + // it did not (the sandbox is paused: only a snapshot exists, no running + // record to lock), deleting the snapshot below races a concurrent resume, + // whose publication (storage.Add) is a lockless SET+SADD with no + // delete-intent check. Without a rendezvous the DELETE can soft-delete the + // snapshot and return 204 while the resume republishes the sandbox as + // running. We fence that window with a kill-claim on the reservation the + // resume holds for its whole lifecycle. + runningRemoved := false + switch { case err == nil: killedOrRemoved = true + runningRemoved = true case errors.Is(err, orchestrator.ErrSandboxNotFound): logger.L().Debug(ctx, "Running sandbox not found", logger.WithSandboxID(sandboxID)) case errors.Is(err, orchestrator.ErrSandboxOperationFailed): @@ -81,12 +93,41 @@ func (a *APIStore) DeleteSandboxesSandboxID( return } + // Paused sandbox: claim the ID against a concurrent resume before touching + // the snapshot. If a resume is in flight (or already finished, so the + // sandbox is running again), refuse with 409 and leave the snapshot intact — + // the client retries the kill against the running sandbox through the locked + // path above. An accepted kill (a claim) is irreversible: reserveScript + // rejects any resume that starts after it. + claimTaken := false + if !runningRemoved { + claimed, claimErr := a.orchestrator.ClaimPausedKill(ctx, teamID, sandboxID) + if claimErr != nil { + telemetry.ReportError(ctx, "error claiming paused sandbox for deletion", claimErr) + a.sendAPIStoreError(c, http.StatusInternalServerError, fmt.Sprintf("Error killing sandbox: %s", claimErr)) + + return + } + if !claimed { + logger.L().Info(ctx, "Refusing to delete paused sandbox: a resume is in flight", logger.WithSandboxID(sandboxID)) + a.sendAPIStoreError(c, http.StatusConflict, fmt.Sprintf("Sandbox %s is resuming; retry the delete once it is running", sandboxID)) + + return + } + claimTaken = true + } + // remove any snapshots when the sandbox is not running deleteSnapshotErr := a.deleteSnapshot(ctx, sandboxID, teamID) switch { case errors.Is(deleteSnapshotErr, db.ErrSnapshotNotFound): // no snapshot found, nothing to do case deleteSnapshotErr != nil: + if claimTaken { + // The snapshot survived, so drop the claim to unblock future resumes + // of this ID rather than making them wait out the claim's TTL. + a.orchestrator.ReleasePausedKillClaim(context.WithoutCancel(ctx), teamID, sandboxID) + } telemetry.ReportError(ctx, "error deleting sandbox", deleteSnapshotErr) a.sendAPIStoreError(c, http.StatusInternalServerError, fmt.Sprintf("Error deleting sandbox: %s", deleteSnapshotErr)) diff --git a/packages/api/internal/orchestrator/create_instance.go b/packages/api/internal/orchestrator/create_instance.go index fdd4ee1726..11774a2b06 100644 --- a/packages/api/internal/orchestrator/create_instance.go +++ b/packages/api/internal/orchestrator/create_instance.go @@ -207,6 +207,16 @@ func (o *Orchestrator) CreateSandbox( "please visit 'https://e2b.dev/docs/billing'", totalConcurrentInstances), Err: fmt.Errorf("team '%s' has reached the maximum number of instances (%d)", team.ID, totalConcurrentInstances), } + case errors.Is(err, sandbox.ErrSandboxKilled): + // A DELETE claimed this sandbox ID for removal while this resume was + // starting. The kill wins: the snapshot is being (or has been) + // deleted, so publishing this sandbox would resurrect a sandbox the + // client was told was gone. Refuse instead. + return sandbox.Sandbox{}, &api.APIError{ + Code: http.StatusNotFound, + ClientMsg: fmt.Sprintf("Sandbox '%s' was deleted", sandboxID), + Err: fmt.Errorf("resume of '%s' refused: %w", sandboxID, err), + } default: logger.L().Error(ctx, "failed to reserve sandbox for team", logger.WithSandboxID(sandboxID), zap.Error(err)) diff --git a/packages/api/internal/orchestrator/delete_instance.go b/packages/api/internal/orchestrator/delete_instance.go index e44537c2eb..70d0fdd981 100644 --- a/packages/api/internal/orchestrator/delete_instance.go +++ b/packages/api/internal/orchestrator/delete_instance.go @@ -29,6 +29,23 @@ const refusalRetryAfter = 10 * time.Second const pauseTimeout = 80 * time.Second +// ClaimPausedKill fences a paused sandbox's ID against a concurrent resume +// before the caller deletes its snapshot. It returns claimed=true when no +// resume is in flight and the caller may proceed; false when a resume is +// pending or the sandbox is already running again, in which case the snapshot +// must be left intact and the kill retried against the running sandbox. +func (o *Orchestrator) ClaimPausedKill(ctx context.Context, teamID uuid.UUID, sandboxID string) (bool, error) { + return o.sandboxStore.ClaimKill(ctx, teamID, sandboxID) +} + +// ReleasePausedKillClaim drops a claim taken by ClaimPausedKill. Best-effort: +// the claim also expires on its own. +func (o *Orchestrator) ReleasePausedKillClaim(ctx context.Context, teamID uuid.UUID, sandboxID string) { + if err := o.sandboxStore.ReleaseKillClaim(ctx, teamID, sandboxID); err != nil { + logger.L().Error(ctx, "failed to release paused-kill claim", zap.Error(err), logger.WithSandboxID(sandboxID)) + } +} + func (o *Orchestrator) RemoveSandbox(ctx context.Context, teamID uuid.UUID, sandboxID string, opts sandbox.RemoveOpts) error { ctx, span := tracer.Start(ctx, "remove-sandbox") defer span.End() diff --git a/packages/api/internal/sandbox/aliases.go b/packages/api/internal/sandbox/aliases.go index a1a5df7743..ac35bb39f6 100644 --- a/packages/api/internal/sandbox/aliases.go +++ b/packages/api/internal/sandbox/aliases.go @@ -59,6 +59,7 @@ var ( ErrRestoreConflict = sandboxtypes.ErrRestoreConflict ErrTransitionRestored = sandboxtypes.ErrTransitionRestored ErrDraining = sandboxtypes.ErrDraining + ErrSandboxKilled = sandboxtypes.ErrSandboxKilled AllowedTransitions = sandboxtypes.AllowedTransitions diff --git a/packages/api/internal/sandbox/reservations/redis/kill_claim_test.go b/packages/api/internal/sandbox/reservations/redis/kill_claim_test.go new file mode 100644 index 0000000000..55d2572fc8 --- /dev/null +++ b/packages/api/internal/sandbox/reservations/redis/kill_claim_test.go @@ -0,0 +1,139 @@ +package redis + +import ( + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/e2b-dev/infra/packages/api/internal/sandbox/sandboxtypes" + storage_redis "github.com/e2b-dev/infra/packages/api/internal/sandbox/storage/redis" +) + +// TestClaimKill_NoResumeInFlight_ClaimsAndBlocksResume covers the common case: +// a paused sandbox with no in-flight resume. ClaimKill succeeds, and any resume +// that starts afterwards is refused so the accepted kill cannot be undone. +func TestClaimKill_NoResumeInFlight_ClaimsAndBlocksResume(t *testing.T) { + t.Parallel() + storage, _ := setupTestReservationStorage(t) + + teamID := uuid.New() + + claimed, err := storage.ClaimKill(t.Context(), teamID, testSandboxID) + require.NoError(t, err) + assert.True(t, claimed, "kill should be claimed when no resume is in flight") + + // A resume that starts after the claim must lose. + _, _, err = storage.Reserve(t.Context(), teamID, testSandboxID, 10) + require.ErrorIs(t, err, sandboxtypes.ErrSandboxKilled) +} + +// TestClaimKill_ResumePending_Refuses covers the race the fix targets: a resume +// is already mid-flight (it holds the reservation) when the DELETE arrives. +// ClaimKill must refuse so the handler returns 409 and leaves the snapshot +// intact, rather than deleting it and letting the resume resurrect the sandbox. +func TestClaimKill_ResumePending_Refuses(t *testing.T) { + t.Parallel() + storage, _ := setupTestReservationStorage(t) + + teamID := uuid.New() + + finishStart, _, err := storage.Reserve(t.Context(), teamID, testSandboxID, 10) + require.NoError(t, err) + require.NotNil(t, finishStart, "first reserve should win the reservation") + + claimed, err := storage.ClaimKill(t.Context(), teamID, testSandboxID) + require.NoError(t, err) + assert.False(t, claimed, "kill must be refused while a resume is pending") +} + +// TestClaimKill_AlreadyRunning_Refuses covers the resume that finished between +// StartRemoving finding no running record and the claim: the sandbox is back in +// the storage index. ClaimKill must refuse so the client retries the kill +// against the running sandbox via the normal locked path. +func TestClaimKill_AlreadyRunning_Refuses(t *testing.T) { + t.Parallel() + storage, client := setupTestReservationStorage(t) + + teamID := uuid.New() + + // Simulate a completed resume: the sandbox is present in the storage index, + // which is what storage.Add does on publication. + indexKey := storage_redis.GetSandboxStorageTeamIndexKey(teamID.String()) + require.NoError(t, client.SAdd(t.Context(), indexKey, testSandboxID).Err()) + + claimed, err := storage.ClaimKill(t.Context(), teamID, testSandboxID) + require.NoError(t, err) + assert.False(t, claimed, "kill must be refused when the sandbox is already running") +} + +// TestReleaseKillClaim_UnblocksResume covers the cleanup path: when the snapshot +// delete fails after a claim was taken, releasing the claim lets future resumes +// of that ID proceed instead of waiting out the claim TTL. +func TestReleaseKillClaim_UnblocksResume(t *testing.T) { + t.Parallel() + storage, _ := setupTestReservationStorage(t) + + teamID := uuid.New() + + claimed, err := storage.ClaimKill(t.Context(), teamID, testSandboxID) + require.NoError(t, err) + require.True(t, claimed) + + // While claimed, a resume is refused. + _, _, err = storage.Reserve(t.Context(), teamID, testSandboxID, 10) + require.ErrorIs(t, err, sandboxtypes.ErrSandboxKilled) + + require.NoError(t, storage.ReleaseKillClaim(t.Context(), teamID, testSandboxID)) + + // After release, the same ID can be reserved again. + finishStart, _, err := storage.Reserve(t.Context(), teamID, testSandboxID, 10) + require.NoError(t, err) + assert.NotNil(t, finishStart) +} + +// TestClaimKill_ConcurrentReserveAndClaim asserts the rendezvous is atomic: +// racing a resume's Reserve against a DELETE's ClaimKill, the two outcomes are +// always consistent — exactly one of "resume reserved" / "kill claimed" wins, +// never both, so a claimed kill is never resurrected and a reserved resume is +// never silently killed. +func TestClaimKill_ConcurrentReserveAndClaim(t *testing.T) { + t.Parallel() + storage, _ := setupTestReservationStorage(t) + + for i := range 50 { + teamID := uuid.New() + sandboxID := "sbx-" + teamID.String() + + var ( + reserved bool + claimed bool + ) + + done := make(chan struct{}, 2) + go func() { + finishStart, _, err := storage.Reserve(t.Context(), teamID, sandboxID, 10) + if err == nil && finishStart != nil { + reserved = true + } + done <- struct{}{} + }() + go func() { + c, err := storage.ClaimKill(t.Context(), teamID, sandboxID) + if err == nil { + claimed = c + } + done <- struct{}{} + }() + <-done + <-done + + // If the kill was claimed, the resume must not have reserved (it either + // lost the race and was refused, or has not started). If the resume + // reserved first, the claim must have been refused. + if claimed && reserved { + t.Fatalf("iteration %d: both resume reserved and kill claimed for the same sandbox", i) + } + } +} diff --git a/packages/api/internal/sandbox/reservations/redis/reservation.go b/packages/api/internal/sandbox/reservations/redis/reservation.go index ee18403d6f..057fb580a8 100644 --- a/packages/api/internal/sandbox/reservations/redis/reservation.go +++ b/packages/api/internal/sandbox/reservations/redis/reservation.go @@ -25,6 +25,14 @@ const ( // and cleaned up. This handles the case where an API instance crashes mid-creation. // 90 seconds is well beyond any realistic sandbox creation time. staleTTL = 90 * time.Second + + // killClaimTTL is how long a DELETE's kill-claim over a sandbox ID blocks a + // resume's reservation. It only has to outlive the snapshot soft-delete + // becoming durable — after that any resume fails when it fetches the + // snapshot — so it is kept short. The claim is normally cleared explicitly + // (ReleaseKillClaim) if the delete fails; this TTL is the backstop for a + // crashed API instance. + killClaimTTL = 30 * time.Second ) var _ sandboxtypes.ReservationStorage = (*ReservationStorage)(nil) @@ -56,8 +64,10 @@ func (s *ReservationStorage) Reserve(ctx context.Context, teamID uuid.UUID, sand now := float64(time.Now().Unix()) staleCutoff := float64(time.Now().Add(-staleTTL).Unix()) + killClaimKey := getKillClaimKey(teamIDStr, sandboxID) + result, err := reserveScript.Run(ctx, s.redisClient, - []string{storageIndexKey, pendingSetKey, resultKeyStr}, + []string{storageIndexKey, pendingSetKey, resultKeyStr, killClaimKey}, sandboxID, limit, now, staleCutoff, ).Int() if err != nil { @@ -77,11 +87,60 @@ func (s *ReservationStorage) Reserve(ctx context.Context, teamID uuid.UUID, sand case reserveResultLimitExceeded: return nil, nil, &sandboxtypes.LimitExceededError{TeamID: teamID} + case reserveResultKilled: + return nil, nil, sandboxtypes.ErrSandboxKilled + default: return nil, nil, fmt.Errorf("unexpected reserve script result: %d", result) } } +// ClaimKill fences off a paused sandbox's ID against a concurrent resume before +// the caller soft-deletes its snapshot. It returns claimed=true when no resume +// is in flight and the caller may proceed with the delete; false when a resume +// is pending or the sandbox is already running again, in which case the caller +// must not delete the snapshot and should surface a retryable conflict. +// +// See claimKillScript for the ordering argument that makes an accepted kill +// irreversible against resume's lockless publication. +func (s *ReservationStorage) ClaimKill(ctx context.Context, teamID uuid.UUID, sandboxID string) (claimed bool, err error) { + teamIDStr := teamID.String() + storageIndexKey := getStorageIndexKey(teamIDStr) + pendingSetKey := getPendingSetKey(teamIDStr) + killClaimKey := getKillClaimKey(teamIDStr, sandboxID) + + result, err := claimKillScript.Run(ctx, s.redisClient, + []string{storageIndexKey, pendingSetKey, killClaimKey}, + sandboxID, int(killClaimTTL.Seconds()), + ).Int() + if err != nil { + return false, fmt.Errorf("failed to run claim-kill script: %w", err) + } + + switch result { + case claimKillResultClaimed: + return true, nil + case claimKillResultInFlight: + return false, nil + default: + return false, fmt.Errorf("unexpected claim-kill script result: %d", result) + } +} + +// ReleaseKillClaim drops a claim taken by ClaimKill. It is best-effort cleanup +// for when the snapshot delete fails after the claim was taken; the claim's TTL +// is the backstop if this never runs. +func (s *ReservationStorage) ReleaseKillClaim(ctx context.Context, teamID uuid.UUID, sandboxID string) error { + killClaimKey := getKillClaimKey(teamID.String(), sandboxID) + + err := releaseKillClaimScript.Run(ctx, s.redisClient, []string{killClaimKey}).Err() + if err != nil { + return fmt.Errorf("failed to run release-kill-claim script: %w", err) + } + + return nil +} + func (s *ReservationStorage) Release(ctx context.Context, teamID uuid.UUID, sandboxID string) error { teamIDStr := teamID.String() pendingSetKey := getPendingSetKey(teamIDStr) diff --git a/packages/api/internal/sandbox/reservations/redis/scripts.go b/packages/api/internal/sandbox/reservations/redis/scripts.go index 29d187ef92..2b7396ae30 100644 --- a/packages/api/internal/sandbox/reservations/redis/scripts.go +++ b/packages/api/internal/sandbox/reservations/redis/scripts.go @@ -12,6 +12,11 @@ const ( reserveResultAlreadyInStorage = 1 reserveResultAlreadyPending = 2 reserveResultLimitExceeded = 3 + reserveResultKilled = 4 + + // ClaimKill result codes + claimKillResultClaimed = 0 + claimKillResultInFlight = 1 ) var ( @@ -22,6 +27,7 @@ var ( // KEYS[1] = storage index key (sandbox:storage:{teamID}:index) // KEYS[2] = pending zset key (sandbox:storage:{teamID}:reservations:pending) // KEYS[3] = result key (sandbox:storage:{teamID}:reservations:sandboxID:result) + // KEYS[4] = kill-claim key (sandbox:storage:{teamID}:reservations:sandboxID:killed) // ARGV[1] = sandboxID // ARGV[2] = limit (-1 means no limit) // ARGV[3] = current Unix timestamp (seconds, float) @@ -32,10 +38,19 @@ var ( // 1 = ALREADY_IN_STORAGE (sandbox exists in storage index) // 2 = ALREADY_PENDING (sandbox already in pending zset) // 3 = LIMIT_EXCEEDED (total count >= limit) + // 4 = KILLED (a DELETE claimed this sandbox ID for removal; see claimKillScript) reserveScript = redis.NewScript(fmt.Sprintf(` -- Clean up stale pending entries (score < cutoff) redis.call('ZREMRANGEBYSCORE', KEYS[2], '-inf', ARGV[4]) + -- Refuse if a concurrent DELETE has claimed this sandbox ID for removal. + -- The claim is written atomically with the paused-delete snapshot removal + -- (claimKillScript); refusing here is what makes an accepted kill + -- irreversible against an in-flight or just-starting resume. + if redis.call('EXISTS', KEYS[4]) == 1 then + return %d + end + -- Check if sandbox already exists in storage index if redis.call('SISMEMBER', KEYS[1], ARGV[1]) == 1 then return %d @@ -61,7 +76,58 @@ var ( -- Reserve: add to pending zset with current timestamp as score redis.call('ZADD', KEYS[2], ARGV[3], ARGV[1]) return %d - `, reserveResultAlreadyInStorage, reserveResultAlreadyPending, reserveResultLimitExceeded, reserveResultReserved)) + `, reserveResultKilled, reserveResultAlreadyInStorage, reserveResultAlreadyPending, reserveResultLimitExceeded, reserveResultReserved)) + + // claimKillScript is the rendezvous point that lets a DELETE of a paused + // sandbox (one with no running-store record, only a snapshot) fence off any + // concurrent resume before it soft-deletes the snapshot. + // + // A paused sandbox has no running record, so StartRemoving finds nothing to + // lock or pin and the kill handler falls straight through to deleting the + // snapshot. Meanwhile a resume's publication (storage.Add) is a lockless + // SET+SADD with no delete-intent check. The two operations share no lock, + // so a DELETE could soft-delete the snapshot and return 204 while an + // in-flight resume republishes the sandbox as running. + // + // This script closes that gap using the reservation the resume already + // holds for its whole lifecycle (Reserve..finishStart): + // - If the sandbox is pending (a resume is mid-flight) or already back in + // the storage index (a resume just finished), it refuses: the caller + // returns 409 and the client retries the kill against the running + // sandbox through the normal locked path. + // - Otherwise it writes a short-lived kill-claim that reserveScript + // rejects, so a resume that only starts after this point loses too. The + // claim just has to outlive the snapshot soft-delete becoming durable; + // once the snapshot is gone, any later resume fails when it fetches it. + // + // KEYS[1] = storage index key + // KEYS[2] = pending zset key + // KEYS[3] = kill-claim key + // ARGV[1] = sandboxID + // ARGV[2] = claim TTL in seconds + // + // Returns: + // 0 = CLAIMED (safe to delete the snapshot) + // 1 = IN_FLIGHT (a resume is pending or already running; refuse the kill) + claimKillScript = redis.NewScript(fmt.Sprintf(` + if redis.call('SISMEMBER', KEYS[1], ARGV[1]) == 1 then + return %d + end + if redis.call('ZSCORE', KEYS[2], ARGV[1]) then + return %d + end + redis.call('SET', KEYS[3], '1', 'EX', tonumber(ARGV[2])) + return %d + `, claimKillResultInFlight, claimKillResultInFlight, claimKillResultClaimed)) + + // releaseKillClaimScript drops a kill-claim written by claimKillScript. It + // is best-effort cleanup for when the snapshot delete fails after the claim + // was taken; the claim's TTL is the backstop if this never runs. + // KEYS[1] = kill-claim key + releaseKillClaimScript = redis.NewScript(` + redis.call('DEL', KEYS[1]) + return 1 + `) // finishStartScript removes a sandbox from the pending zset and sets the result key. // KEYS[1] = pending zset key diff --git a/packages/api/internal/sandbox/reservations/redis/utils.go b/packages/api/internal/sandbox/reservations/redis/utils.go index 043be1f928..72420b45ac 100644 --- a/packages/api/internal/sandbox/reservations/redis/utils.go +++ b/packages/api/internal/sandbox/reservations/redis/utils.go @@ -9,6 +9,7 @@ const ( reservationsKey = "reservations" pendingKey = "pending" resultKey = "result" + killedKey = "killed" notifySuffix = "notify" ) @@ -40,3 +41,10 @@ func getResultKey(teamID, sandboxID string) string { func getReservationRoutingKey(teamID, sandboxID string) string { return redis_utils.CreateKey(getReservationPrefix(teamID), sandboxID, notifySuffix) } + +// getKillClaimKey returns the key holding a DELETE's claim over a sandbox ID, +// written by claimKillScript and checked by reserveScript. +// e.g. sandbox:storage:{teamID}:reservations:sandboxID:killed +func getKillClaimKey(teamID, sandboxID string) string { + return redis_utils.CreateKey(getReservationPrefix(teamID), sandboxID, killedKey) +} diff --git a/packages/api/internal/sandbox/sandboxtypes/errors.go b/packages/api/internal/sandbox/sandboxtypes/errors.go index 1d73f03119..fad3690d2c 100644 --- a/packages/api/internal/sandbox/sandboxtypes/errors.go +++ b/packages/api/internal/sandbox/sandboxtypes/errors.go @@ -65,6 +65,12 @@ func (PauseQueueExhaustedError) Error() string { // RestoreRunning. var ErrExecutionMismatch = errors.New("sandbox execution no longer matches") +// ErrSandboxKilled reports that a reservation was refused because a concurrent +// DELETE claimed the sandbox ID for removal (see ReservationStorage.ClaimKill). +// A resume that hits this must not publish the sandbox: the kill was accepted +// first and is irreversible. +var ErrSandboxKilled = errors.New("sandbox was concurrently killed") + // ErrRestoreConflict reports that the record was rewritten between the // restore's read and its compare-and-set; nothing was written. var ErrRestoreConflict = errors.New("sandbox changed during restoration") diff --git a/packages/api/internal/sandbox/sandboxtypes/storage.go b/packages/api/internal/sandbox/sandboxtypes/storage.go index 072c8afbdb..401f7369b8 100644 --- a/packages/api/internal/sandbox/sandboxtypes/storage.go +++ b/packages/api/internal/sandbox/sandboxtypes/storage.go @@ -45,4 +45,13 @@ type StateTransition struct { type ReservationStorage interface { Reserve(ctx context.Context, teamID uuid.UUID, sandboxID string, limit int) (finishStart func(Sandbox, error), waitForStart func(ctx context.Context) (Sandbox, error), err error) Release(ctx context.Context, teamID uuid.UUID, sandboxID string) error + + // ClaimKill fences a paused sandbox's ID against a concurrent resume before + // its snapshot is deleted. Returns claimed=false when a resume is in flight + // or the sandbox is already running again, so the caller must not delete the + // snapshot and should surface a retryable conflict instead. + ClaimKill(ctx context.Context, teamID uuid.UUID, sandboxID string) (claimed bool, err error) + // ReleaseKillClaim drops a claim taken by ClaimKill (best-effort cleanup on + // a failed delete; the claim also expires on its own). + ReleaseKillClaim(ctx context.Context, teamID uuid.UUID, sandboxID string) error } diff --git a/packages/api/internal/sandbox/store.go b/packages/api/internal/sandbox/store.go index f4cb375f0b..805dd5027d 100644 --- a/packages/api/internal/sandbox/store.go +++ b/packages/api/internal/sandbox/store.go @@ -173,3 +173,14 @@ func (s *Store) Reserve(ctx context.Context, teamID uuid.UUID, sandboxID string, return finishStart, waitForStart, nil } + +// ClaimKill fences a paused sandbox's ID against a concurrent resume before its +// snapshot is deleted. See ReservationStorage.ClaimKill. +func (s *Store) ClaimKill(ctx context.Context, teamID uuid.UUID, sandboxID string) (bool, error) { + return s.reservations.ClaimKill(ctx, teamID, sandboxID) +} + +// ReleaseKillClaim drops a claim taken by ClaimKill. +func (s *Store) ReleaseKillClaim(ctx context.Context, teamID uuid.UUID, sandboxID string) error { + return s.reservations.ReleaseKillClaim(ctx, teamID, sandboxID) +} diff --git a/packages/api/internal/sandbox/store_test.go b/packages/api/internal/sandbox/store_test.go index 1472647639..58530440af 100644 --- a/packages/api/internal/sandbox/store_test.go +++ b/packages/api/internal/sandbox/store_test.go @@ -123,6 +123,14 @@ func (n *NoOpReservationStorage) Release(_ context.Context, _ uuid.UUID, _ strin return nil } +func (n *NoOpReservationStorage) ClaimKill(_ context.Context, _ uuid.UUID, _ string) (bool, error) { + return true, nil +} + +func (n *NoOpReservationStorage) ReleaseKillClaim(_ context.Context, _ uuid.UUID, _ string) error { + return nil +} + // MockStorage wraps real storage and can inject errors type MockStorage struct { Storage