Repository navigation
Conversation
…finds an orphan
When ExpiredItems finds an orphaned expiration ZSET member (the sandbox key is
already gone, confirmed by a nil MGET) it removed the ZSET member but left the
sandbox ID sitting in the per-team SET index. Those entries accumulate forever:
TeamItems silently skips the nil MGET, the healer only restores missing ZSET
members (it never repairs the team index), and Remove only cleans the team
index on the normal removal path. The leftover comes from paths that delete the
sandbox key without going through Remove (e.g. a pause via finishTransition,
which DELs the key but does not SREM the index).
Prune the team-index entry in the same sweep. Because sandbox IDs are reused
across executions (a resume keeps the ID, changes the execution ID), a plain
SREM would race a concurrent resume that re-creates the key and re-adds the ID
between the MGET and the SREM, and - since the healer does not repair the team
index - that wrong removal would be permanent (the sandbox would vanish from
TeamItems until it is next removed). So the prune is an atomic EXISTS-guarded
SREM (pruneOrphanIndexScript): it removes the ID only if the sandbox key is
still absent at that instant; a resume that re-created the key makes EXISTS != 0
and the SREM is skipped. Both keys share the {teamID} hash tag, so the script is
Redis-Cluster-safe.
The orphan handler reads teamID from the batch it is already iterating, so no
struct change is needed. The ZSET member removal is unchanged.
Rebased onto current main. Scoped to this one fix - the original e2b-dev#3567 also
replaced the add/remove Lua scripts with TxPipelined, an unrelated refactor of
the atomicity primitives in this concurrency-critical path; that is dropped here
(and, notably, this fix needs a Lua script for its atomic check-then-SREM, so
removing Lua elsewhere was not the right direction). Adds a test that the orphan
index entry is pruned, and a test that a live sandbox key is never unindexed by
the sweep.
Closes e2b-dev#3566. Supersedes e2b-dev#3567.
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
October 23, 2026 07:38
This branch has not been deployed
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.
Rebased follow-up to #3567 (closed inactive). The leak is still present on
main, and this fixes a real race the original approach missed.Bug
When
ExpiredItemsfinds an orphaned expiration ZSET member (sandbox key already gone, confirmed by a nil MGET) it removes the ZSET member but leaves the sandbox ID in the per-team SET index. Those entries accumulate forever:TeamItemssilently skips the nil MGET, the healer only restores missing ZSET members (it never repairs the team index), andRemoveonly cleans the team index on the normal path. The leftover comes from paths that delete the sandbox key without going throughRemove— e.g. a pause (finishTransitionDELs the key but does not SREM the index).Fix — and why a plain SREM is unsafe
Sandbox IDs are reused across executions (a resume keeps the ID, changes the execution ID). A plain
SREMwould race a concurrent resume that re-creates the key and re-adds the ID between the MGET and the SREM. Because the healer does not repair the team index, that wrong removal would be permanent — the sandbox would vanish fromTeamItemsuntil it is next removed.So the prune is an atomic EXISTS-guarded SREM (
pruneOrphanIndexScript):It removes the ID only if the sandbox key is still absent at that instant; a resume that re-created the key makes
EXISTS != 0and the SREM is skipped. Both keys share the{teamID}hash tag, so it is Redis-Cluster-safe. The orphan handler readsteamIDfrom the batch it already iterates (no struct change); the ZSET member removal is unchanged.Scope
This fixes only the orphan-index leak. The original #3567 also replaced the add/remove Lua scripts with
TxPipelined— an unrelated refactor of the atomicity primitives in this concurrency-critical path — which is dropped here. (Notably this fix needs a Lua script for its atomic check-then-SREM, so removing Lua elsewhere was not the right direction.)Tests
go build ./...andgo vetpass; the redis tests pass against a real Redis (REDIS_TEST_DATABASE_URL).Closes #3566. Supersedes #3567.