From e593de63067733be3a796d3a014481f6cb9342d9 Mon Sep 17 00:00:00 2001 From: AdaAibaby <8872412+adababys@users.noreply.github.com> Date: Fri, 23 Oct 2026 15:36:42 +0800 Subject: [PATCH] fix(api): prune the stale team-index entry when the expiration sweep 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 #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 #3566. Supersedes #3567. --- .../storage/redis/expiration_index_test.go | 45 ++++++++++++++++++- .../internal/sandbox/storage/redis/items.go | 16 +++++-- .../internal/sandbox/storage/redis/scripts.go | 14 ++++++ 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/packages/api/internal/sandbox/storage/redis/expiration_index_test.go b/packages/api/internal/sandbox/storage/redis/expiration_index_test.go index 4ccfb21370..3f5cc029ba 100644 --- a/packages/api/internal/sandbox/storage/redis/expiration_index_test.go +++ b/packages/api/internal/sandbox/storage/redis/expiration_index_test.go @@ -170,17 +170,60 @@ func TestExpiredItems_RemovesOrphanMember(t *testing.T) { storage, client := setupTestStorage(t) - member := expirationMember(uuid.NewString(), "sbx-orphan", uuid.NewString()) + teamID := uuid.New() + const sandboxID = "sbx-orphan" + member := expirationMember(teamID.String(), sandboxID, uuid.NewString()) require.NoError(t, client.ZAdd(t.Context(), globalExpirationSet, redis.Z{ Score: float64(time.Now().Add(-time.Minute).UnixMilli()), Member: member, }).Err()) + // A leftover team-index entry with no sandbox key — the stale state this + // sweep must also prune (the sandbox key is absent, so MGET returns nil). + teamIndexKey := GetSandboxStorageTeamIndexKey(teamID.String()) + require.NoError(t, client.SAdd(t.Context(), teamIndexKey, sandboxID).Err()) items, err := storage.ExpiredItems(t.Context()) require.NoError(t, err) require.Empty(t, items) requireMemberAbsent(t, client, member) + + isMember, err := client.SIsMember(t.Context(), teamIndexKey, sandboxID).Result() + require.NoError(t, err) + require.False(t, isMember, "orphan sandbox ID must be pruned from the team index") +} + +// A sandbox whose key is present must never be unindexed by the orphan sweep, +// even if a stale expired ZSET member names a different (dead) execution of it: +// the EXISTS-guarded prune only fires when the key is truly gone, so a resume +// that re-created the key under the same (reused) sandbox ID keeps its index. +func TestExpiredItems_OrphanPruneSparesLiveKey(t *testing.T) { + t.Parallel() + + storage, client := setupTestStorage(t) + + teamID := uuid.New() + const sandboxID = "sbx-live-again" + + // Live sandbox: real key + team-index entry (as Add writes them). + live := makeIndexedSandbox(teamID, sandboxID, uuid.NewString(), time.Now(), time.Now().Add(time.Hour)) + require.NoError(t, storage.Add(t.Context(), live)) + + // A dead execution of the SAME sandbox ID left an expired orphan-looking member. + deadMember := expirationMember(teamID.String(), sandboxID, uuid.NewString()) + require.NoError(t, client.ZAdd(t.Context(), globalExpirationSet, redis.Z{ + Score: float64(time.Now().Add(-time.Minute).UnixMilli()), + Member: deadMember, + }).Err()) + + _, err := storage.ExpiredItems(t.Context()) + require.NoError(t, err) + + // The dead member is swept, but the live sandbox keeps its team-index entry. + requireMemberAbsent(t, client, deadMember) + isMember, err := client.SIsMember(t.Context(), GetSandboxStorageTeamIndexKey(teamID.String()), sandboxID).Result() + require.NoError(t, err) + require.True(t, isMember, "a live sandbox key must not be unindexed by the orphan sweep") } // TestExpiredItems_SweepsInvalidMember: unparseable members must be removed, not skipped, diff --git a/packages/api/internal/sandbox/storage/redis/items.go b/packages/api/internal/sandbox/storage/redis/items.go index 3acb470386..f76fc39536 100644 --- a/packages/api/internal/sandbox/storage/redis/items.go +++ b/packages/api/internal/sandbox/storage/redis/items.go @@ -3,6 +3,7 @@ package redis import ( "context" "encoding/json" + "errors" "fmt" "time" @@ -107,13 +108,22 @@ func (s *Storage) ExpiredItems(ctx context.Context) ([]sandboxtypes.Sandbox, err for i, raw := range batch.cmd.Val() { ref := batch.refs[i] - // Sandbox key gone but ZSET entry remains — orphaned. Members are - // execution-scoped, so this removal can never unindex a fresh - // execution concurrently re-added under the same sandbox ID. + // Sandbox key gone but ZSET entry remains — orphaned. Remove the + // stale ZSET member, and also prune the leftover team-index entry so + // it does not accumulate indefinitely (the healer never repairs the + // team index). The prune is an atomic EXISTS-guarded SREM so a resume + // that re-creates the key under the same sandbox ID between the MGET + // above and here is never unindexed. if raw == nil { staleMembers = append(staleMembers, ref.member) orphanCount++ + teamIndexKey := GetSandboxStorageTeamIndexKey(batch.teamID) + sandboxKey := getSandboxKey(batch.teamID, ref.sandboxID) + if err := pruneOrphanIndexScript.Run(ctx, s.redisClient, []string{sandboxKey, teamIndexKey}, ref.sandboxID).Err(); err != nil && !errors.Is(err, redis.Nil) { + logger.L().Warn(ctx, "Failed to prune orphan from team index", zap.Error(err), logger.WithSandboxID(ref.sandboxID)) + } + continue } diff --git a/packages/api/internal/sandbox/storage/redis/scripts.go b/packages/api/internal/sandbox/storage/redis/scripts.go index b1ed3f46bb..e1828c72b8 100644 --- a/packages/api/internal/sandbox/storage/redis/scripts.go +++ b/packages/api/internal/sandbox/storage/redis/scripts.go @@ -82,4 +82,18 @@ var ( redis.call('SET', KEYS[3], '', 'EX', ARGV[4]) return 1 `) + + // pruneOrphanIndexScript removes a sandbox ID from the team index ONLY if the + // sandbox key is still absent, atomically. The orphan sweep found the key gone + // via MGET, but between that read and this call a resume can re-create the key + // under the same (reused) sandbox ID and re-add it to the index; the EXISTS + // guard makes sure we never unindex such a live sandbox. The team index is not + // repaired by the healer, so a wrong SREM here would be permanent. + // KEYS[1] = sandbox key, KEYS[2] = team index key, ARGV[1] = sandbox ID. + pruneOrphanIndexScript = redis.NewScript(` + if redis.call('EXISTS', KEYS[1]) == 0 then + return redis.call('SREM', KEYS[2], ARGV[1]) + end + return 0 + `) )