diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1a8e7df918..76c27a1313 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2445,6 +2445,34 @@ async function ciHeadShaResolutionCoalesced( ); } +/** + * Best-effort exclusive claim against the self-host transient cache, shared by every per-PR/per-review advisory + * lock below. Requires the store's native atomic claim() (Redis SET NX) to provide any real exclusivity — it is + * the only way to close the race between two concurrent callers each observing an absent key. A plain + * get-then-set pair CANNOT close that race in general, even with an extra write-then-verify re-read: caller A + * can write its own token, read it straight back, and return true entirely BEFORE caller B's later write/read + * also completes and also returns true — both callers "win" (#confirmed-bug). Rather than pretend to serialize + * via a check that silently fails under exactly the concurrent load this lock exists to guard against, an + * adapter without claim() gets NO exclusivity from this helper: every caller proceeds. This is honest about the + * limitation rather than a false guarantee, and costs nothing in practice — self-host's Redis-backed cache (the + * only cache adapter this codebase ships) always implements claim(), so this is a documented limitation for a + * hypothetical future adapter, not a live gap. A missing cache or a thrown claim() also fails OPEN (returns + * true) — every lock built on this helper is defense-in-depth, never the primary safety gate, and must never + * itself block real work from running. + */ +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. + try { + return await env.SELFHOST_TRANSIENT_CACHE.claim(key, "1", ttlSeconds); + } catch { + return true; // fail open — see the doc comment above. + } +} + // Per-PR advisory lock around maybeRunAgentMaintenance's plan-and-execute critical section (#2129). The TTL is a // crash-safety backstop only — the normal path releases explicitly in a finally block within a few seconds — so // it is sized well above any realistic pass duration (matches CI_COALESCE_WINDOW_SECONDS, an already-vetted @@ -2465,25 +2493,11 @@ export async function claimAgentMaintenanceLock( repoFullName: string, prNumber: number, ): Promise { - const key = agentMaintenanceLockKey(repoFullName, prNumber); - // Atomic claim (#2129): a get-then-set pair has a window between the read and the write where two concurrent - // passes for the SAME PR can both observe an absent key and both claim it, defeating the serializer entirely. - // env.SELFHOST_TRANSIENT_CACHE.claim performs the check-and-set as one operation (Redis SET NX server-side), - // closing that window. Falls back to the non-atomic get/set pair only for a cache adapter that hasn't - // implemented claim yet — strictly no worse than this function's prior behavior. - if (env.SELFHOST_TRANSIENT_CACHE?.claim) { - try { - return await env.SELFHOST_TRANSIENT_CACHE.claim(key, "1", AGENT_MAINTENANCE_LOCK_TTL_SECONDS); - } catch { - return true; // fail open — see the doc comment above. - } - } - // getTransientKey/putTransientKey already fail open internally (a missing cache or a thrown read/write error - // both resolve rather than throw), so this never needs its own try/catch — a cache fault surfaces here as - // "no lock held", which correctly falls through to claiming it. - if (await getTransientKey(env, key)) return false; // another pass is already in-flight for this PR - await putTransientKey(env, key, "1", AGENT_MAINTENANCE_LOCK_TTL_SECONDS); - return true; + return claimTransientLock( + env, + agentMaintenanceLockKey(repoFullName, prNumber), + AGENT_MAINTENANCE_LOCK_TTL_SECONDS, + ); } /** Best-effort release, called from a finally block so the lock frees promptly instead of waiting out the TTL. */ @@ -2501,6 +2515,54 @@ export async function releaseAgentMaintenanceLock( } } +// Per-(repo, PR, head SHA) advisory lock around runAiReviewForAdvisory's expensive grounding/RAG/enrichment/LLM +// section (#confirmed-bug: a webhook pass and an agent-regate-pr sweep pass can independently reach this same +// code for the SAME PR at the SAME head SHA, both miss the cache, and both fire a real LLM call — which can +// return DIFFERENT verdicts). The TTL is a crash-safety backstop only (see AI_REVIEW_LOCK_TTL_SECONDS below), not +// a throughput bound — same philosophy as AGENT_MAINTENANCE_LOCK_TTL_SECONDS (#2129/#2368). +const AI_REVIEW_LOCK_TTL_SECONDS = 1_800; // 30 minutes — see justification below. + +function aiReviewLockKey(repoFullName: string, prNumber: number, headSha: string, mode: string): string { + return `ai-review-lock:${repoFullName.toLowerCase()}#${prNumber}@${headSha.toLowerCase()}:${mode}`; +} + +/** + * Claim the per-(repo, PR, head SHA, mode) advisory lock before the expensive grounding/RAG/enrichment/LLM + * section of runAiReviewForAdvisory. Returns false when another pass already holds it for this exact head (the + * caller must treat this as "another pass is already reviewing this head" and return the inconclusive-hold shape + * below — the next webhook/sweep tick, or the pass that IS running, is the backstop that populates the cache). + * A missing cache or cache hiccup fails OPEN (returns true — the lock is defense-in-depth, never the primary + * safety gate, and must never itself block a real review from running). + */ +export async function claimAiReviewLock( + env: Env, + repoFullName: string, + prNumber: number, + headSha: string, + mode: string, +): Promise { + return claimTransientLock( + env, + aiReviewLockKey(repoFullName, prNumber, headSha, mode), + AI_REVIEW_LOCK_TTL_SECONDS, + ); +} + +/** Best-effort release, called from a finally block so the lock frees promptly instead of waiting out the TTL. */ +export async function releaseAiReviewLock( + env: Env, + repoFullName: string, + prNumber: number, + headSha: string, + mode: string, +): Promise { + try { + await env.SELFHOST_TRANSIENT_CACHE?.del?.(aiReviewLockKey(repoFullName, prNumber, headSha, mode)); + } catch { + // best-effort; the TTL is the backstop if release fails + } +} + /** Read the CI head SHA off a `check_suite`/`check_run` `completed` payload (the event node carries `head_sha`; * `check_run` also nests it under `check_suite.head_sha`). Returns "" when absent. The payload type doesn't model * these events, so we narrow off `Record` the same way the `pull_requests[]` read does. */ @@ -4629,6 +4691,39 @@ export async function runAiReviewForAdvisory( })) ) return undefined; + // Per-(repo, PR, head SHA, mode) advisory lock (#confirmed-bug, mirrors #2129/#2368's claimAgentMaintenanceLock): + // a webhook pass and an agent-regate-pr sweep pass can independently reach this point for the SAME PR at the + // SAME head, both miss the cache (neither has written yet), and both fire a real, wasteful LLM call that can + // 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 findings: AdvisoryFinding[] = [ + { + code: "ai_review_inconclusive", + severity: "warning", + title: "AI review already in progress for this PR head", + detail: "Another Gittensory pass is already running the AI review for this exact PR head. This pass is skipping to avoid a duplicate LLM call.", + action: "The gate is held for a human reviewer rather than passed automatically; it re-evaluates once the in-flight review completes or on the next update.", + }, + ]; + args.advisory.findings.push(...findings); + return { + notes: "AI review is already running for this PR head in another Gittensory pass. Gittensory is holding this PR for manual review until that pass completes.", + reviewerCount: 0, + inlineFindings: [], + findings, + cacheable: false, + }; + } try { // BYOK: decrypt the maintainer's provider key only for confirmed contributors when opted in. Falls back to free Workers AI when // no key is configured or the encryption secret is unavailable (getDecryptedRepositoryAiKey → null). @@ -4920,6 +5015,14 @@ export async function runAiReviewForAdvisory( head_sha: args.advisory.headSha, }); return undefined; + } finally { + await releaseAiReviewLock( + env, + args.repoFullName, + args.pr.number, + args.advisory.headSha, + args.settings.aiReviewMode, + ); } } diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 165274b200..dfc9b96454 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { buildAiReviewDiff, runAiReviewForAdvisory, shouldStartAiReviewForAdvisory } from "../../src/queue/processors"; +import { buildAiReviewDiff, claimAiReviewLock, runAiReviewForAdvisory, shouldStartAiReviewForAdvisory } from "../../src/queue/processors"; import { BEST_REVIEW_MODELS, INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review"; import * as sentryModule from "../../src/selfhost/sentry"; import { upsertRepositoryAiKey } from "../../src/db/repositories"; @@ -524,6 +524,38 @@ describe("runAiReviewForAdvisory", () => { expect(adv.findings.map((f) => f.code)).toEqual(["ai_review_inconclusive"]); }); + it("#confirmed-bug: defers to an already-held AI review lock and never invokes the AI when another pass is in-flight for this exact head", async () => { + const adv = advisory(); + let aiCalls = 0; + const env = aiEnv(async () => { + aiCalls += 1; + return { response: notesOnlyJson() }; + }); + // 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); + + const result = await runAiReviewForAdvisory(env, { + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + + expect(aiCalls).toBe(0); // the AI mock was never invoked — the lock short-circuited before the LLM call + expect(result).toMatchObject({ + reviewerCount: 0, + inlineFindings: [], + cacheable: false, + findings: [expect.objectContaining({ code: "ai_review_inconclusive" })], + }); + expect(result?.notes).toContain("AI review is already running for this PR head in another Gittensory pass"); + expect(adv.findings.map((f) => f.code)).toEqual(["ai_review_inconclusive"]); + }); + it("withholds unstructured AI text while holding the PR for manual review", async () => { const adv = advisory(); const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: "Looks coherent, but please verify the new cache branch before merging." })), { diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index 64e74eee6f..436ca62379 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -131,6 +131,46 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); }); + it("holds the gate NEUTRAL (never a failure-close) when the AI review lock is held by another in-flight pass (#confirmed-bug)", () => { + // Same code, different finding text — the lock-contention finding constructed by runAiReviewForAdvisory's + // new claim-failure branch. advisory.ts only keys on `code`, so this proves the mechanism end-to-end for + // the new finding shape without needing to touch advisory.ts. + const adv: Advisory = { + ...missingIssueAdvisory(), + findings: [ + { + code: "ai_review_inconclusive", + title: "AI review already in progress for this PR head", + severity: "warning", + detail: "Another Gittensory pass is already running the AI review for this exact PR head. This pass is skipping to avoid a duplicate LLM call.", + action: "The gate is held for a human reviewer rather than passed automatically; it re-evaluates once the in-flight review completes or on the next update.", + }, + ], + }; + const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + expect(result.conclusion).toBe("neutral"); + expect(result.blockers).toEqual([]); + }); + + it("a deterministic hard blocker (secret_leak) still FAILS even when the AI review is held by lock contention (#confirmed-bug)", () => { + const adv: Advisory = { + ...missingIssueAdvisory(), + findings: [ + { code: "secret_leak", title: "Possible leaked secret", severity: "critical", detail: "a committed token", action: "remove and rotate it" }, + { + code: "ai_review_inconclusive", + title: "AI review already in progress for this PR head", + severity: "warning", + detail: "Another Gittensory pass is already running the AI review for this exact PR head. This pass is skipping to avoid a duplicate LLM call.", + action: "The gate is held for a human reviewer rather than passed automatically; it re-evaluates once the in-flight review completes or on the next update.", + }, + ], + }; + const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); + }); + it("an enforced pre-merge check (pre_merge_check_required) hard-blocks; the advisory variant never does (#review-pre-merge-checks)", () => { const enforced: Advisory = { ...missingIssueAdvisory(), diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 45afa249e4..0c3bafddbf 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -44,7 +44,7 @@ import { upsertRepositoryFromGitHub, putCachedAiReview, } from "../../src/db/repositories"; -import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAgentMaintenanceLock, contributorEvidenceBatchSize, processJob, releaseAgentMaintenanceLock } from "../../src/queue/processors"; +import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAgentMaintenanceLock, claimAiReviewLock, contributorEvidenceBatchSize, processJob, releaseAgentMaintenanceLock, releaseAiReviewLock } from "../../src/queue/processors"; import { aiReviewCacheInputFingerprint } from "../../src/review/ai-review-cache-input"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; @@ -2686,6 +2686,92 @@ describe("queue processors", () => { expect(audit?.n).toBe(0); }); + it("INVARIANT (#confirmed-bug): a second overlapping pass for the same PR head defers to the AI review lock, holds the gate NEUTRAL, and never calls the AI a second time", async () => { + // Simulates the confirmed TOCTOU race: a webhook pass and an agent-regate-pr sweep pass both reach + // runAiReviewForAdvisory for the SAME PR at the SAME head SHA before either has written the cache. The + // webhook pass (not modeled directly here — job-coalesce keys never match across trigger shapes) is + // simulated by pre-claiming the lock exactly as runAiReviewForAdvisory itself would; the agent-regate-pr + // pass under test must then defer instead of firing its own, potentially-divergent LLM call. + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { + run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; + }, + } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + aiReviewMode: "block", + gatePack: "oss-anti-slop", + }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 49, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a49" }, labels: [], body: "Closes #1" }); + const commentBodies: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/49/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/49")) return Response.json({ number: 49, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a49" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a49/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a49/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/49/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/49/comments") && method === "POST") { + commentBodies.push(String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? "")); + return Response.json({ id: 49 }, { status: 201 }); + } + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + // 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); + + await expect( + processJob(env, { + type: "agent-regate-pr", + deliveryId: "race-ai-review", + repoFullName: "JSONbored/gittensory", + prNumber: 49, + installationId: 123, + }), + ).resolves.toBeUndefined(); + + // The losing pass never called the AI a second time — it deferred to the lock instead of double-spending. + expect(aiCalls).toBe(0); + const finalComment = commentBodies.find((body) => !body.includes("is reviewing")); + expect(finalComment).toContain("Gittensory review needs maintainer review"); + expect(finalComment).toContain("AI review is already running for this PR head in another Gittensory pass"); + // A lock-contention placeholder must never be cached — it would poison the cache for the legitimate attempt. + const cached = await env.DB.prepare("select count(*) as n from ai_review_cache where repo_full_name = ? and pull_number = ?") + .bind("JSONbored/gittensory", 49) + .first<{ n: number }>(); + expect(cached?.n).toBe(0); + }); + it("publishes deterministic surface and reports missing summary when required AI is over quota", async () => { const aiRun = vi.fn(async () => ({ response: "{}" })); const env = createTestEnv({ @@ -3438,10 +3524,14 @@ describe("queue processors", () => { expect(calls).toEqual(["claim"]); // never falls through to the racy get/set pair when claim is available }); - it("claimAgentMaintenanceLock falls back to the get/set pair and still denies a held key when the cache has no claim() (#2368)", async () => { - // A cache adapter that hasn't implemented the atomic claim() primitive yet must still deny a second claim — - // just via the older, non-atomic get-then-set pair (documented as strictly no worse than this function's - // prior behavior), not by skipping the check entirely. + it("claimAgentMaintenanceLock returns true unconditionally when the cache has no claim() — no false exclusivity guarantee (#confirmed-bug, review round 2)", async () => { + // A prior version of this helper fell back to a get-then-set pair (even with an extra write-then-verify + // re-read) when claim() wasn't available. That is NOT a real exclusivity guarantee: caller A can write its + // own token, read it straight back, and return true entirely before caller B's later write/read also + // completes and also returns true -- both callers "win". Rather than pretend to serialize via a check that + // silently fails under exactly the concurrent load this lock exists to guard against, a cache without + // claim() now gets NO exclusivity at all -- every call proceeds, sequential or concurrent, even for a key a + // previous call already "set" via get/set. const values = new Map(); const env = createTestEnv({ SELFHOST_TRANSIENT_CACHE: { @@ -3450,7 +3540,140 @@ describe("queue processors", () => { }, }); expect(await claimAgentMaintenanceLock(env, "owner/agent-repo", 7)).toBe(true); - expect(await claimAgentMaintenanceLock(env, "owner/agent-repo", 7)).toBe(false); + expect(await claimAgentMaintenanceLock(env, "owner/agent-repo", 7)).toBe(true); + }); + + it("REGRESSION (#confirmed-bug, review round 2): claimAgentMaintenanceLock does not falsely deny — and does not falsely claim exclusivity for — two genuinely concurrent callers when the cache has no claim()", async () => { + // Documents the corrected, honest contract under the exact interleaving the gate flagged: with no atomic + // claim() primitive, BOTH concurrent callers proceed (true), because this helper no longer attempts a + // get/set-based serialization that can't actually provide exclusivity. + const values = new Map(); + const yieldThenRun = (fn: () => T): Promise => new Promise((resolve) => queueMicrotask(() => resolve(fn()))); + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: (key: string) => yieldThenRun(() => values.get(key) ?? null), + set: (key: string, value: string) => yieldThenRun(() => { values.set(key, value); }), + }, + }); + const [first, second] = await Promise.all([ + claimAgentMaintenanceLock(env, "owner/agent-repo", 7), + claimAgentMaintenanceLock(env, "owner/agent-repo", 7), + ]); + expect([first, second]).toEqual([true, true]); + }); + + 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); + }); + + it("claimAiReviewLock fails OPEN on a broken transient cache — never itself blocks a real review from running (#confirmed-bug)", async () => { + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => { throw new Error("cache read error"); }, + set: async () => { throw new Error("cache write error"); }, + 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(); + }); + + 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); + }); + + it("claimAiReviewLock fails OPEN when the atomic claim primitive itself throws (#confirmed-bug)", async () => { + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => null, + set: async () => undefined, + claim: async () => { throw new Error("redis unavailable"); }, + }, + }); + expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).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 () => { + // A get-then-set pair has a window between the read and the write where two concurrent callers can both + // observe an absent key and both claim it — exactly what this lock exists to prevent (a webhook pass and a + // sweep pass both missing the cache and both firing a real LLM call). This test races two claims for the + // same tuple via Promise.all (both kick off before either resolves) against the default test cache's + // claim(), which mirrors createRedisCache's atomic SET NX: the check-and-set happens with no `await` + // boundary in between, so it is impossible for both callers to see "unclaimed". + const env = createTestEnv({}); + const [first, second] = await Promise.all([ + claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), + claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), + ]); + expect([first, second].filter(Boolean)).toHaveLength(1); + }); + + it("REGRESSION: claimAiReviewLock calls the atomic claim primitive, not a separate get+set pair, when the cache supports it", async () => { + const calls: string[] = []; + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => { calls.push("get"); return null; }, + set: async () => { calls.push("set"); }, + claim: async () => { calls.push("claim"); return true; }, + }, + }); + expect(await claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block")).toBe(true); + expect(calls).toEqual(["claim"]); // never falls through to the racy get/set pair when claim is available + }); + + it("claimAiReviewLock returns true unconditionally when the cache has no claim() — no false exclusivity guarantee (#confirmed-bug, review round 2)", async () => { + // A prior version of this helper fell back to a get-then-set pair (even with an extra write-then-verify + // re-read) when claim() wasn't available. That is NOT a real exclusivity guarantee: caller A can write its + // own token, read it straight back, and return true entirely before caller B's later write/read also + // completes and also returns true -- both callers "win". Rather than pretend to serialize via a check that + // silently fails under exactly the concurrent load this lock exists to guard against (duplicate LLM calls), + // a cache without claim() now gets NO exclusivity at all -- every call proceeds, sequential or concurrent, + // even for a key a previous call already "set" via get/set. + const values = new Map(); + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async (key: string) => values.get(key) ?? null, + 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); + }); + + 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 () => { + // Documents the corrected, honest contract under the exact interleaving the gate flagged: with no atomic + // claim() primitive, BOTH concurrent callers proceed (true) -- a webhook pass and a sweep pass racing for + // the same PR head both fire their LLM call, same as before this lock existed, rather than one of them + // wrongly believing it has exclusive ownership when it doesn't. + const values = new Map(); + const yieldThenRun = (fn: () => T): Promise => new Promise((resolve) => queueMicrotask(() => resolve(fn()))); + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: (key: string) => yieldThenRun(() => values.get(key) ?? null), + set: (key: string, value: string) => yieldThenRun(() => { values.set(key, value); }), + }, + }); + const [first, second] = await Promise.all([ + claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), + claimAiReviewLock(env, "owner/agent-repo", 7, "sha1", "block"), + ]); + expect([first, second]).toEqual([true, true]); }); it("INVARIANT (#2129 per-PR lock): a maintenance pass defers when another pass already holds the PR's lock", async () => {