diff --git a/src/env.d.ts b/src/env.d.ts index 124a4ea4b5..1261d551aa 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -34,6 +34,9 @@ declare global { * check-and-set as one operation. Optional so a cache adapter that hasn't implemented it yet still * type-checks; callers fall back to the non-atomic get/set pair when absent (#2129). */ claim?(key: string, value: string, ttlSeconds: number): Promise; + /** Delete `key` only when its current value equals `value` (compare-and-delete). Returns true when the + * key was removed. Optional; lock release skips when absent and relies on TTL instead of blind `del()`. */ + releaseIfValue?(key: string, value: string): Promise; }; /** TODO (convergence follow-up): a per-PR LOCK Durable Object (`SubmissionLock` mutex) is a separate, * more-involved sub-task — it needs the ported DO class + its own migration tag, not just a binding here. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2812d02a11..44f1aeb1d4 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -315,6 +315,7 @@ import { import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { buildUnifiedReviewDiff } from "../review/review-diff"; import { buildUnifiedCommentBody } from "../review/unified-comment-bridge"; +import { randomUUID } from "node:crypto"; import { isRetryableJobError, RetryableJobError } from "./retryable"; import { screenshotsAllowed } from "../review/visual-wire"; import { isVisualPath } from "../review/visual/paths"; @@ -2059,7 +2060,8 @@ async function maybeRunAgentMaintenance( // critical section (extracted below so the try/finally doesn't force-reindent that whole block); a pass that // loses the race defers cleanly — the next webhook/sweep tick is the backstop. Lightweight stand-in for the // per-PR SubmissionLock Durable Object noted as a longer-term TODO in env.d.ts. - if (!(await claimPrActuationLock(env, repoFullName, pr.number))) return; + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) return; try { await runAgentMaintenancePlanAndExecute(env, { installationId, @@ -2073,7 +2075,7 @@ async function maybeRunAgentMaintenance( liveFacts: args.liveFacts, }); } finally { - await releasePrActuationLock(env, repoFullName, pr.number); + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); } } @@ -3005,23 +3007,24 @@ async function putTransientKey( // than a get-then-set pair that only *looks* atomic — see claimTransientLock's doc comment for why that fallback // was removed. // -// KNOWN LIMITATION: the lock value is a constant, not a per-holder ownership token, so release does not verify -// it still owns the key — if a holder ran past the TTL, a later claimer's live lock could be deleted by the -// first holder's stale `finally` release, reopening the exact race this mutex exists to close. A per-holder -// token + a conditional (check-then-delete) release would close this properly, but needs a new atomic -// compare-and-delete primitive on the cache adapter — tracked alongside the Durable Object follow-up above. The -// TTL is set generously long specifically so this window is practically unreachable: the guarded operations -// (a handful of sequential GitHub API calls, or a maintenance pass's plan-and-execute) should never legitimately -// run anywhere near this long. +// Ownership tokens + releaseIfValue (compare-and-delete) prevent a stale holder's finally block from +// deleting a successor's live lock after TTL expiry (#2129/#2135). const PR_ACTUATION_LOCK_TTL_SECONDS = 600; function prActuationLockKey(repoFullName: string, prNumber: number): string { return `pr-actuation-lock:${repoFullName.toLowerCase()}#${prNumber}`; } + +/** Result of claiming a transient-cache mutex. `ownerToken` is null on fail-open paths (no cache / claim error). */ +export type TransientLockClaim = { + acquired: boolean; + ownerToken: string | null; +}; + export async function claimPrActuationLock( env: Env, repoFullName: string, prNumber: number, -): Promise { +): Promise { return claimTransientLock( env, prActuationLockKey(repoFullName, prNumber), @@ -3032,12 +3035,9 @@ export async function releasePrActuationLock( env: Env, repoFullName: string, prNumber: number, + ownerToken: string | null, ): Promise { - try { - await env.SELFHOST_TRANSIENT_CACHE?.del?.(prActuationLockKey(repoFullName, prNumber)); - } catch { - // best-effort - } + await releaseTransientLockIfOwner(env, prActuationLockKey(repoFullName, prNumber), ownerToken); } // A plain thrown Error still reaches the queue's retry path (this call site is deliberately uncaught, same as @@ -3375,12 +3375,32 @@ async function claimTransientLock( env: Env, key: string, ttlSeconds: number, -): Promise { - if (!env.SELFHOST_TRANSIENT_CACHE?.claim) return true; // no atomic primitive — nothing to serialize against. +): Promise { + if (!env.SELFHOST_TRANSIENT_CACHE?.claim) return { acquired: true, ownerToken: null }; + const ownerToken = randomUUID(); + try { + const acquired = await env.SELFHOST_TRANSIENT_CACHE.claim(key, ownerToken, ttlSeconds); + return { acquired, ownerToken: acquired ? ownerToken : null }; + } catch { + return { acquired: true, ownerToken: null }; + } +} + +async function releaseTransientLockIfOwner( + env: Env, + key: string, + ownerToken: string | null, +): Promise { + if (!ownerToken) return; try { - return await env.SELFHOST_TRANSIENT_CACHE.claim(key, "1", ttlSeconds); + const cache = env.SELFHOST_TRANSIENT_CACHE; + if (cache?.releaseIfValue) { + await cache.releaseIfValue(key, ownerToken); + return; + } + // Without compare-and-delete, skip release and let TTL expire — blind del() would reopen #2129/#2135. } catch { - return true; // fail open — see the doc comment above. + // best-effort; TTL is the backstop if release fails } } @@ -3427,7 +3447,7 @@ export async function claimAiReviewLock( prNumber: number, headSha: string, mode: string, -): Promise { +): Promise { return claimTransientLock( env, aiReviewLockKey(repoFullName, prNumber, headSha, mode), @@ -3442,12 +3462,13 @@ export async function releaseAiReviewLock( prNumber: number, headSha: string, mode: string, + ownerToken: string | null, ): Promise { - try { - await env.SELFHOST_TRANSIENT_CACHE?.del?.(aiReviewLockKey(repoFullName, prNumber, headSha, mode)); - } catch { - // best-effort; the TTL is the backstop if release fails - } + await releaseTransientLockIfOwner( + env, + aiReviewLockKey(repoFullName, prNumber, headSha, mode), + ownerToken, + ); } /** Read the CI head SHA off a `check_suite`/`check_run` `completed` payload (the event node carries `head_sha`; @@ -5892,15 +5913,14 @@ export async function runAiReviewForAdvisory( // return different verdicts. Claim before the expensive section below; a pass that loses the race returns the // same inconclusive-hold shape the "AI produced no usable verdict" path already returns, so the gate is held // (neutral) for a human rather than either pass's independently-decided verdict racing the other's cache write. - if ( - !(await claimAiReviewLock( - env, - args.repoFullName, - args.pr.number, - args.advisory.headSha, - args.settings.aiReviewMode, - )) - ) { + const aiReviewLock = await claimAiReviewLock( + env, + args.repoFullName, + args.pr.number, + args.advisory.headSha, + args.settings.aiReviewMode, + ); + if (!aiReviewLock.acquired) { const findings: AdvisoryFinding[] = [ { code: "ai_review_inconclusive", @@ -6234,6 +6254,7 @@ export async function runAiReviewForAdvisory( args.pr.number, args.advisory.headSha, args.settings.aiReviewMode, + aiReviewLock.ownerToken, ); } } @@ -9462,7 +9483,8 @@ async function maybeCloseDraftDodgeAttempt( pr: PullRequestRecord, settings: RepositorySettings, ): Promise { - if (!(await claimPrActuationLock(env, repoFullName, pr.number))) { + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) { throw new PrActuationLockContendedError(repoFullName, pr.number, "draft-dodge"); } try { @@ -9475,7 +9497,7 @@ async function maybeCloseDraftDodgeAttempt( settings, ); } finally { - await releasePrActuationLock(env, repoFullName, pr.number); + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); } } @@ -9668,7 +9690,8 @@ async function maybeRecloseDisallowedReopen( pr: PullRequestRecord, payload: GitHubWebhookPayload, ): Promise { - if (!(await claimPrActuationLock(env, repoFullName, pr.number))) { + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) { throw new PrActuationLockContendedError(repoFullName, pr.number, "reopen-reclose"); } try { @@ -9682,7 +9705,7 @@ async function maybeRecloseDisallowedReopen( ); return reclosed ? "reclosed" : "allowed"; } finally { - await releasePrActuationLock(env, repoFullName, pr.number); + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); } } diff --git a/src/selfhost/redis-cache.ts b/src/selfhost/redis-cache.ts index 4b55aeeea0..7d5083e71f 100644 --- a/src/selfhost/redis-cache.ts +++ b/src/selfhost/redis-cache.ts @@ -23,6 +23,16 @@ export function createRedisCache(redis: Redis) { const result = await redis.set(key, value, "EX", ttlSeconds, "NX"); return result === "OK"; }, + // Atomic compare-and-delete: only the holder whose token still matches may release the key. + async releaseIfValue(key: string, value: string): Promise { + const result = await redis.eval( + "if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) else return 0 end", + 1, + key, + value, + ); + return result === 1; + }, }; } diff --git a/test/helpers/d1.ts b/test/helpers/d1.ts index 3ca21cd0b2..8284e57042 100644 --- a/test/helpers/d1.ts +++ b/test/helpers/d1.ts @@ -102,7 +102,7 @@ export function createTestEnv(overrides: Partial = {}): Env { async get(key: string) { return transientCache.get(key) ?? null; }, - async set(key: string, value: string) { + async set(key: string, value: string, _ttlSeconds: number) { transientCache.set(key, value); }, async del(key: string) { @@ -117,6 +117,11 @@ export function createTestEnv(overrides: Partial = {}): Env { transientCache.set(key, value); return true; }, + async releaseIfValue(key: string, value: string) { + if (transientCache.get(key) !== value) return false; + transientCache.delete(key); + return true; + }, }, // Per-repo review allowlist: default to the test repos so flag-ON wiring tests activate the // gated review features. Override to "" to assert the dormant (no-repo) default. diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index f772f39e67..bde0996690 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -582,7 +582,7 @@ describe("runAiReviewForAdvisory", () => { // Simulate a webhook pass already in-flight for this exact (repo, PR, head, mode) tuple — the caller under // test (a sweep-shaped pass, say) must defer instead of racing it with a second, independently-decided // LLM call. - expect(await claimAiReviewLock(env, "acme/widgets", 3, "sha3", "block")).toBe(true); + expect((await claimAiReviewLock(env, "acme/widgets", 3, "sha3", "block")).acquired).toBe(true); const result = await runAiReviewForAdvisory(env, { settings: { aiReviewMode: "block" } as RepositorySettings, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 1f5e090f48..5dad606309 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -4180,7 +4180,7 @@ describe("queue processors", () => { // The "first pass" (webhook-shaped) claims the lock for this exact (repo, PR, head, mode) tuple and is still // in-flight when the "second pass" (agent-regate-pr sweep-shaped) below reaches runAiReviewForAdvisory. - expect(await claimAiReviewLock(env, "JSONbored/gittensory", 49, "a49", "block")).toBe(true); + expect((await claimAiReviewLock(env, "JSONbored/gittensory", 49, "a49", "block")).acquired).toBe(true); await expect( processJob(env, { @@ -4960,20 +4960,14 @@ describe("queue processors", () => { it("claimAiReviewLock claims when free, denies when held (per-PR+head+mode, not globally), and release frees it again (#confirmed-bug)", async () => { const env = createTestEnv({}); - // First claim for this exact (repo, PR, head, mode) succeeds — no prior pass in-flight. - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); - // A second, concurrent pass for the SAME PR at the SAME head and mode (regardless of what triggered it — - // webhook or sweep) is denied while the first is still in-flight — exactly the race this lock exists for. - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(false); - // A DIFFERENT head SHA for the same PR is unaffected — a new commit is a genuinely new review, not a dup. - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha2", "block")).toBe(true); - // A DIFFERENT mode for the same PR+head is also unaffected — advisory vs block are independent lock keys. - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "advisory")).toBe(true); - // A DIFFERENT PR in the same repo is unaffected — the lock is per-PR+head+mode, not repo-wide. - expect(await claimAiReviewLock(env, "owner/agent-repo", 8, "sha1", "block")).toBe(true); - // Release (the finally block's job) frees the (PR, head, mode) tuple — a subsequent pass can claim it again. - await releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + const first = await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"); + expect(first.acquired).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(false); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha2", "block")).acquired).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "advisory")).acquired).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 8, "sha1", "block")).acquired).toBe(true); + await releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block", first.ownerToken); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); }); it("claimAiReviewLock fails OPEN on a broken transient cache — never itself blocks a real review from running (#confirmed-bug)", async () => { @@ -4984,14 +4978,14 @@ describe("queue processors", () => { del: async () => { throw new Error("cache delete error"); }, }, }); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); - await expect(releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).resolves.toBeUndefined(); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); + await expect(releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block", null)).resolves.toBeUndefined(); }); it("claimAiReviewLock fails OPEN when no transient cache is configured at all — nothing to serialize against (#confirmed-bug)", async () => { const env = createTestEnv({}); delete env.SELFHOST_TRANSIENT_CACHE; - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); }); it("claimAiReviewLock fails OPEN when the atomic claim primitive itself throws (#confirmed-bug)", async () => { @@ -5002,7 +4996,7 @@ describe("queue processors", () => { claim: async () => { throw new Error("redis unavailable"); }, }, }); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); }); it("REGRESSION: claimAiReviewLock uses an atomic check-and-set, so two genuinely concurrent claims for the SAME (repo, PR, head, mode) can never both succeed", async () => { @@ -5017,7 +5011,7 @@ describe("queue processors", () => { claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), ]); - expect([first, second].filter(Boolean)).toHaveLength(1); + expect([first, second].filter((claim) => claim.acquired)).toHaveLength(1); }); it("REGRESSION: claimAiReviewLock calls the atomic claim primitive, not a separate get+set pair, when the cache supports it", async () => { @@ -5029,7 +5023,7 @@ describe("queue processors", () => { claim: async () => { calls.push("claim"); return true; }, }, }); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); expect(calls).toEqual(["claim"]); // never falls through to the racy get/set pair when claim is available }); @@ -5048,8 +5042,8 @@ describe("queue processors", () => { set: async (key: string, value: string) => { values.set(key, value); }, }, }); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); - expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); + expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true); }); it("REGRESSION (#confirmed-bug, review round 2): claimAiReviewLock does not falsely claim exclusivity for two genuinely concurrent callers when the cache has no claim()", async () => { @@ -5069,7 +5063,7 @@ describe("queue processors", () => { claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), ]); - expect([first, second]).toEqual([true, true]); + expect([first.acquired, second.acquired]).toEqual([true, true]); }); // claimPrActuationLock (#2129/#2135) is the ONE shared per-PR actuation lock: maybeRunAgentMaintenance, @@ -5077,11 +5071,12 @@ describe("queue processors", () => { // three mutating PR paths can race any other (review round 4) — a single namespace, not one lock per path. it("claimPrActuationLock claims when free, denies when held (per-PR), and release frees it again (#2135)", async () => { const env = createTestEnv({}); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(false); - expect(await claimPrActuationLock(env, "owner/act-repo", 8)).toBe(true); - await releasePrActuationLock(env, "owner/act-repo", 7); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + const first = await claimPrActuationLock(env, "owner/act-repo", 7); + expect(first.acquired).toBe(true); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(false); + expect((await claimPrActuationLock(env, "owner/act-repo", 8)).acquired).toBe(true); + await releasePrActuationLock(env, "owner/act-repo", 7, first.ownerToken); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); }); it("claimPrActuationLock fails OPEN on a broken transient cache — never itself blocks actuation (#2135)", async () => { @@ -5092,8 +5087,8 @@ describe("queue processors", () => { del: async () => { throw new Error("cache delete error"); }, }, }); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); - await expect(releasePrActuationLock(env, "owner/act-repo", 7)).resolves.toBeUndefined(); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); + await expect(releasePrActuationLock(env, "owner/act-repo", 7, null)).resolves.toBeUndefined(); }); it("claimPrActuationLock fails OPEN when the atomic claim primitive itself throws (#2135)", async () => { @@ -5104,7 +5099,7 @@ describe("queue processors", () => { claim: async () => { throw new Error("redis unavailable"); }, }, }); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); }); it("REGRESSION (#2135): claimPrActuationLock uses an atomic check-and-set, so two genuinely concurrent claims for the SAME PR can never both succeed", async () => { @@ -5113,7 +5108,7 @@ describe("queue processors", () => { claimPrActuationLock(env, "owner/act-repo", 7), claimPrActuationLock(env, "owner/act-repo", 7), ]); - expect([first, second].filter(Boolean)).toHaveLength(1); + expect([first, second].filter((claim) => claim.acquired)).toHaveLength(1); }); it("REGRESSION (#2135): claimPrActuationLock calls the atomic claim primitive, not a separate get+set pair, when the cache supports it", async () => { @@ -5125,7 +5120,7 @@ describe("queue processors", () => { claim: async () => { calls.push("claim"); return true; }, }, }); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); expect(calls).toEqual(["claim"]); // never falls through to the racy get/set pair when claim is available }); @@ -5139,8 +5134,8 @@ describe("queue processors", () => { set: async (key: string, value: string) => { values.set(key, value); }, }, }); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); - expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); + expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true); }); it("REGRESSION (#2135, review round 2): claimPrActuationLock does not falsely claim exclusivity for two genuinely concurrent callers when the cache has no claim()", async () => { @@ -5156,7 +5151,19 @@ describe("queue processors", () => { claimPrActuationLock(env, "owner/act-repo", 7), claimPrActuationLock(env, "owner/act-repo", 7), ]); - expect([first, second]).toEqual([true, true]); + expect([first.acquired, second.acquired]).toEqual([true, true]); + }); + + it("REGRESSION: stale actuation-lock holder releaseIfValue does not delete a successor's live lock", async () => { + const env = createTestEnv({}); + const staleHolder = await claimPrActuationLock(env, "owner/act-repo", 7); + expect(staleHolder.acquired).toBe(true); + expect(staleHolder.ownerToken).toBeTruthy(); + await env.SELFHOST_TRANSIENT_CACHE!.set!("pr-actuation-lock:owner/act-repo#7", "successor-token", 600); + await releasePrActuationLock(env, "owner/act-repo", 7, staleHolder.ownerToken); + expect(await env.SELFHOST_TRANSIENT_CACHE!.get!("pr-actuation-lock:owner/act-repo#7")).toBe("successor-token"); + await releasePrActuationLock(env, "owner/act-repo", 7, "successor-token"); + expect(await env.SELFHOST_TRANSIENT_CACHE!.get!("pr-actuation-lock:owner/act-repo#7")).toBeNull(); }); it("INVARIANT (#2129 per-PR lock): a maintenance pass defers when another pass already holds the PR's lock", async () => { diff --git a/test/unit/selfhost-redis-cache.test.ts b/test/unit/selfhost-redis-cache.test.ts index f12499dcf7..83e9b2f7c5 100644 --- a/test/unit/selfhost-redis-cache.test.ts +++ b/test/unit/selfhost-redis-cache.test.ts @@ -21,6 +21,13 @@ function fakeRedis(): Redis & { _store: Map } { _store.delete(k); return 1; }, + async eval(_script: string, _numkeys: number, key: string, expected: string) { + if (_store.get(key) === expected) { + _store.delete(key); + return 1; + } + return 0; + }, } as unknown as Redis & { _store: Map }; } @@ -63,4 +70,14 @@ describe("createRedisCache (#1216 webhook dedup cache)", () => { const cache = createRedisCache(brokenRedis); await expect(cache.claim("lock", "1", 60)).rejects.toThrow("connection refused"); }); + + it("releaseIfValue deletes only when the stored value matches (#2129 ownership release)", async () => { + const r = fakeRedis(); + const cache = createRedisCache(r); + await cache.set("lock", "holder-a", 60); + expect(await cache.releaseIfValue("lock", "holder-b")).toBe(false); + expect(await cache.get("lock")).toBe("holder-a"); + expect(await cache.releaseIfValue("lock", "holder-a")).toBe(true); + expect(await cache.get("lock")).toBeNull(); + }); });