fix(selfhost): require releaseIfValue when transient cache exposes claim()

src/env.d.ts

@@ -31,13 +31,13 @@ declare global {
       /** Atomic "set only if absent": returns true when this call newly claimed the key, false when it was
        *  already held by someone else. Unlike a get-then-set pair, there is no window where two concurrent
        *  callers can both observe an absent key and both claim it — the store (e.g. Redis SET NX) performs the
-       *  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). */
+       *  check-and-set as one operation. Must be paired with `releaseIfValue` — self-host boot rejects
+       *  `claim()` without it; runtime callers fail open without exclusivity rather than pin locks (#2129). */
       claim?(key: string, value: string, ttlSeconds: number): Promise<boolean>;
       /** Atomic compare-and-delete: deletes `key` only when its current value equals `value`, returning whether
        *  it was removed. Lets a lock holder release its OWN claim without risking a stale post-TTL release
-       *  deleting a different holder's live claim on the same key. Optional; a cache without it skips release
-       *  entirely and relies on the TTL to reclaim the key (#2129). */
+       *  deleting a different holder's live claim on the same key. Required on any adapter that implements
+       *  `claim()` (validated at self-host boot). */
       releaseIfValue?(key: string, value: string): Promise<boolean>;
     };
     /** TODO (convergence follow-up): a per-PR LOCK Durable Object (`SubmissionLock` mutex) is a separate,

src/queue/processors.ts

@@ -3482,10 +3482,15 @@ async function claimTransientLock(
   key: string,
   ttlSeconds: number,
 ): Promise<TransientLockClaim> {
-  if (!env.SELFHOST_TRANSIENT_CACHE?.claim) return { acquired: true, ownerToken: null }; // no atomic primitive — nothing to serialize against.
+  const cache = env.SELFHOST_TRANSIENT_CACHE;
+  if (!cache?.claim) return { acquired: true, ownerToken: null }; // no atomic primitive — nothing to serialize against.
+  // A claim()-only adapter without releaseIfValue would pin locks until TTL after normal work — reject that
+  // shape at self-host boot (assertSelfhostTransientCacheOwnershipRelease). At runtime, fail open without
+  // calling claim() so misconfigured test/custom adapters never acquire an unreleasable lock (#2129/#3153).
+  if (!cache.releaseIfValue) return { acquired: true, ownerToken: null };
   const ownerToken = randomUUID();
   try {
-    const acquired = await env.SELFHOST_TRANSIENT_CACHE.claim(key, ownerToken, ttlSeconds);
+    const acquired = await cache.claim(key, ownerToken, ttlSeconds);
     return { acquired, ownerToken: acquired ? ownerToken : null };
   } catch {
     return { acquired: true, ownerToken: null }; // fail open — see the doc comment above.
@@ -3494,9 +3499,7 @@ async function claimTransientLock(
 
 /** Releases a transient lock ONLY when `ownerToken` still matches the stored value (atomic compare-and-delete),
  *  so a stale holder can never delete a different, live holder's claim on the same key. `ownerToken` is null
- *  on every fail-open claim path (nothing was actually claimed, so nothing to release). A cache with no
- *  releaseIfValue() skips release entirely and relies on the TTL to reclaim the key, rather than falling back
- *  to a blind del() that would reopen the exact race the token scheme exists to close. */
+ *  on every fail-open claim path (nothing was actually claimed, so nothing to release). */
 async function releaseTransientLockIfOwner(env: Env, key: string, ownerToken: string | null): Promise<void> {
   if (!ownerToken) return;
   const cache = env.SELFHOST_TRANSIENT_CACHE;

src/selfhost/redis-cache.ts

@@ -38,4 +38,15 @@ export function createRedisCache(redis: Redis) {
   };
 }
 
+/** Self-host boot guard: `claim()` without ownership-aware release pins actuation locks for minutes. */
+export function assertSelfhostTransientCacheOwnershipRelease(
+  cache: { claim?(key: string, value: string, ttlSeconds: number): Promise<boolean>; releaseIfValue?(key: string, value: string): Promise<boolean> },
+): void {
+  if (cache.claim && !cache.releaseIfValue) {
+    throw new Error(
+      "SELFHOST_TRANSIENT_CACHE.claim requires releaseIfValue for ownership-aware transient locks (#2129)",
+    );
+  }
+}
+
 export type RedisCache = ReturnType<typeof createRedisCache>;

src/server.ts

@@ -478,9 +478,10 @@ async function main(): Promise<void> {
   const { Redis } = await import("ioredis");
   const redisClient = new Redis(redisUrl);
   const { createRedisRateLimiter } = await import("./selfhost/redis-ratelimit");
-  const { createRedisCache } = await import("./selfhost/redis-cache");
+  const { createRedisCache, assertSelfhostTransientCacheOwnershipRelease } = await import("./selfhost/redis-cache");
   const rateLimiter = createRedisRateLimiter(redisClient);
   const webhookCache = createRedisCache(redisClient);
+  assertSelfhostTransientCacheOwnershipRelease(webhookCache);
   // Persist the installation-token cache in Redis so warm GitHub App tokens survive restarts/deploys and are
   // shared across replicas (the in-isolate Map otherwise re-mints — an Orb round-trip — per replica/cold start).
   const { createRedisTokenCache } = await import("./selfhost/redis-token-cache");

test/unit/queue.test.ts

@@ -5458,6 +5458,7 @@ describe("queue processors", () => {
         get: async () => { calls.push("get"); return null; },
         set: async () => { calls.push("set"); },
         claim: async () => { calls.push("claim"); return true; },
+        releaseIfValue: async () => true,
       },
     });
     expect((await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).acquired).toBe(true);
@@ -5555,6 +5556,7 @@ describe("queue processors", () => {
         get: async () => { calls.push("get"); return null; },
         set: async () => { calls.push("set"); },
         claim: async () => { calls.push("claim"); return true; },
+        releaseIfValue: async () => true,
       },
     });
     expect((await claimPrActuationLock(env, "owner/act-repo", 7)).acquired).toBe(true);
@@ -5622,19 +5624,38 @@ describe("queue processors", () => {
     expect(calls).toEqual([]); // a null token means nothing was claimed, so release must never touch the cache
   });
 
-  it("release skips the cache entirely (relies on TTL) when the cache has no releaseIfValue()", async () => {
-    const calls: string[] = [];
+  it("REGRESSION: stale AI-review-lock holder releaseIfValue does not delete a successor's live lock", async () => {
+    const env = createTestEnv({});
+    const staleHolder = await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block");
+    expect(staleHolder.acquired).toBe(true);
+    expect(staleHolder.ownerToken).toBeTruthy();
+    await env.SELFHOST_TRANSIENT_CACHE!.set!("ai-review-lock:owner/agent-repo#7@sha1:block", "successor-token", 1800);
+    await releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block", staleHolder.ownerToken);
+    expect(await env.SELFHOST_TRANSIENT_CACHE!.get!("ai-review-lock:owner/agent-repo#7@sha1:block")).toBe("successor-token");
+    await releaseAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block", "successor-token");
+    expect(await env.SELFHOST_TRANSIENT_CACHE!.get!("ai-review-lock:owner/agent-repo#7@sha1:block")).toBeNull();
+  });
+
+  it("claimPrActuationLock fails open without exclusivity when claim() is present but releaseIfValue is absent (#3153)", async () => {
+    let claimed = false;
+    const store = new Map<string, string>();
     const env = createTestEnv({
       SELFHOST_TRANSIENT_CACHE: {
-        get: async () => null,
-        set: async () => undefined,
-        claim: async (key: string) => { calls.push(`claim:${key}`); return true; },
+        get: async (key: string) => store.get(key) ?? null,
+        set: async (key: string, value: string) => { store.set(key, value); },
+        claim: async (key: string, value: string) => {
+          claimed = true;
+          if (store.has(key)) return false;
+          store.set(key, value);
+          return true;
+        },
       },
     });
-    const claim = await claimPrActuationLock(env, "owner/act-repo", 7);
-    expect(claim.acquired).toBe(true);
-    await releasePrActuationLock(env, "owner/act-repo", 7, claim.ownerToken);
-    expect(calls).toEqual(["claim:pr-actuation-lock:owner/act-repo#7"]); // release never called releaseIfValue — it doesn't exist on this cache
+    const lock = await claimPrActuationLock(env, "owner/act-repo", 7);
+    expect(lock.acquired).toBe(true);
+    expect(lock.ownerToken).toBeNull();
+    expect(claimed).toBe(false);
+    expect(store.size).toBe(0);
   });
 
   it("INVARIANT (#2129 per-PR lock): a maintenance pass defers when another pass already holds the PR's lock", async () => {

test/unit/selfhost-redis-cache.test.ts

@@ -1,6 +1,6 @@
 import type { Redis } from "ioredis";
 import { describe, expect, it } from "vitest";
-import { createRedisCache } from "../../src/selfhost/redis-cache";
+import { assertSelfhostTransientCacheOwnershipRelease, createRedisCache } from "../../src/selfhost/redis-cache";
 
 /** Minimal in-memory stand-in for the ioredis methods the cache uses. Emulates real Redis SET NX
  *  semantics (refuse + return null when NX is requested and the key already exists) so a test
@@ -87,4 +87,13 @@ describe("createRedisCache (#1216 webhook dedup cache)", () => {
     const cache = createRedisCache(brokenRedis);
     await expect(cache.releaseIfValue("lock", "1")).rejects.toThrow("connection refused");
   });
+
+  it("assertSelfhostTransientCacheOwnershipRelease rejects claim() without releaseIfValue at boot (#3153)", () => {
+    expect(() =>
+      assertSelfhostTransientCacheOwnershipRelease({
+        claim: async () => true,
+      }),
+    ).toThrow(/releaseIfValue/);
+    expect(() => assertSelfhostTransientCacheOwnershipRelease(createRedisCache(fakeRedis()))).not.toThrow();
+  });
 });