diff --git a/src/env.d.ts b/src/env.d.ts index 3917c05f95..1abe806d59 100644 --- a/src/env.d.ts +++ b/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; /** 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; }; /** TODO (convergence follow-up): a per-PR LOCK Durable Object (`SubmissionLock` mutex) is a separate, diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 340fa63e08..714bb958a5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3482,10 +3482,15 @@ async function claimTransientLock( key: string, ttlSeconds: number, ): Promise { - 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 { if (!ownerToken) return; const cache = env.SELFHOST_TRANSIENT_CACHE; diff --git a/src/selfhost/redis-cache.ts b/src/selfhost/redis-cache.ts index e607c0cb57..79dc10764b 100644 --- a/src/selfhost/redis-cache.ts +++ b/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; releaseIfValue?(key: string, value: string): Promise }, +): 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; diff --git a/src/server.ts b/src/server.ts index 1b4f4e0b71..f0d47ca724 100644 --- a/src/server.ts +++ b/src/server.ts @@ -478,9 +478,10 @@ async function main(): Promise { 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"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 27da485fe8..40452bd41d 100644 --- a/test/unit/queue.test.ts +++ b/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(); 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 () => { diff --git a/test/unit/selfhost-redis-cache.test.ts b/test/unit/selfhost-redis-cache.test.ts index 34e4f9fb1d..d143447771 100644 --- a/test/unit/selfhost-redis-cache.test.ts +++ b/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(); + }); });