Conversation
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 e2b-dev#3636
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
September 16, 2026 05:51
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
If a client
DELETEs a paused sandbox while a resume for the same sandbox is still in flight, the delete returns204and soft-deletes the snapshot, but the resume then publishes the sandbox back asrunning. The client is told the sandbox is gone while it keeps running until its timeout (and is billable), and the snapshot it would have resumed from has been deleted, so a later pause/resume of that sandbox ends up unrecoverable.Reproduced deterministically on a dev cluster: create + pause, fire an async resume, then
DELETE~20-60 ms later.DELETEreturns204, resume returns201, andGETthen reportsstate=running.Fixes #3636.
Root cause
A paused sandbox has no running-store record — only a snapshot row. So on
DELETE:StartRemovingreads the running store, getsredis.Nil, and returnsErrNotFound. There is nothing to lock and no execution to pin, so the kill handler records no intent and falls straight through to deleting the snapshot.storage.Add, which is a locklessSET+SADDwith no delete-intent check.The two paths share no lock, so they interleave: the delete can remove the snapshot and return
204in the same window the resume is restoring the node and about toAdd.This is the same class of stale-write race that
RemoveOpts.ExpectExecutionIDalready fences for running sandboxes (startTransitionScriptrefuses to overwrite a newer incarnation). The paused case is unprotected precisely because there is no record and no execution ID to pin against.Fix
Rather than introduce a separate tombstone key with its own TTL and GC, this reuses the reservation a resume already holds for its entire lifecycle (
Reserve..finishStart) as the rendezvous point.Before touching the snapshot, the kill handler calls
ClaimKill, a Lua script that checks the pending set and the storage index atomically:409and leaves the snapshot intact; the client retries the kill against the now-running sandbox through the normal locked path.reserveScriptnow rejects any reservation while that claim exists, so a resume that only starts after we commit to the delete loses too, with a clean404.The claim only has to outlive the snapshot soft-delete becoming durable (after that, a resume fails when it fetches the snapshot), so its TTL is short, and it is released eagerly if the delete fails. Net effect: an accepted kill is irreversible — no concurrent resume can publish after it.
Why this shape
reserveScript) and the same stale-entry lifecycle. No new key family, no new GC path.ExpectExecutionIDpin: the write that could resurrect a removed sandbox now checks, atomically, that no kill was accepted first.409when a resume is in flight matches what resume itself already returns for asnapshottingsandbox — a transient conflict the client retries.Testing
packages/api/internal/sandbox/reservations/redis/kill_claim_test.go— new tests against real Redis (testcontainers): claim succeeds and blocks a later resume; claim refused while a resume is pending; claim refused when the sandbox is already running; release unblocks; and a 50-iteration concurrentReservevsClaimKillrace (with-race) asserting the two never both win.go build ./...,go vet, andgolangci-lint(v2, pinned) all clean.409, or204and genuinely gone, never204+running) is ready to run post-deploy.