From 2ae0c14689f78bb3be827a4b53789491e67d90b2 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 22:25:37 -0700 Subject: [PATCH] fix(queue): retry policy closes on PR lock contention --- src/queue/processors.ts | 41 +++++++++++++++++++++++++---------------- test/unit/queue.test.ts | 18 ++++++++++-------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 343c32cd9c..7cffd24cb4 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2358,6 +2358,13 @@ export async function releasePrActuationLock( } } +class PrActuationLockContendedError extends Error { + constructor(repoFullName: string, prNumber: number, policy: string) { + super(`pr actuation lock contended for ${repoFullName}#${prNumber} during ${policy}`); + this.name = "PrActuationLockContendedError"; + } +} + /** * True when CI for this PR+headSha has been pending past STUCK_CI_DEFER_MS. Stamps the first-seen time in a * transient cache keyed by repo#pr:headSha — a new push is a new SHA, so the window resets per commit. A missing @@ -3751,8 +3758,8 @@ async function processGitHubWebhook( // closed — closes are one-shot (resubmit, don't reopen). If a non-maintainer reopened a PR whose last close // was by the bot / repo owner / admin, re-close it and skip the re-review. Self-closes (the contributor // closed their own PR) stay reopenable; the bot's own nightly-re-review reopens are exempt. A contended - // actuation lock ALSO skips the re-review (#2135, review round 3) — the winning delivery already owns - // this PR, so this pass must not evaluate/mutate it concurrently under a false "not blocked" reading. + // actuation lock is retryable (#2135/#2447): this pass must not evaluate/mutate the PR concurrently, but + // ordinary maintenance can now hold the same lock and may not enforce this one-shot reopen event. // Deliberately UNCAUGHT here: every step inside maybeRecloseDisallowedReopen already fails safe on its own // (the lock claim/release fail open; recloseDisallowedReopenIfNeeded's own operations all .catch()), so a // swallowing catch at this call site could only ever mask a genuinely unexpected error into a silent @@ -3769,7 +3776,7 @@ async function processGitHubWebhook( payload, ) : "allowed"; - if (reopenOutcome === "reclosed" || reopenOutcome === "lock_contended") { + if (reopenOutcome === "reclosed") { // Stamp the delivery processed like every other owning path — the early return otherwise leaves the // webhook_events row stuck at "queued"/its body hash, mis-reporting the delivery as un-acked (#review-audit). await recordWebhookEvent(env, { @@ -7740,9 +7747,9 @@ async function recordPrPanelRetriggerSkip( /** Draft-dodge guard (#converted-to-draft): a contributor converting an OPEN PR to draft cannot use draft state * to keep a gate-rejected PR alive. When a prior gate failure exists for the PR's current headSha (and the * block has not been maintainer-overridden), close the PR immediately — the gate verdict stands and does not - * reset on draft conversion. Per-PR actuation-locked (#2135): a concurrent delivery for the same PR must not - * evaluate + potentially mutate it at the same time. Lock-contended is a silent no-op for this pass — the - * delivery holding the lock is handling this PR. */ + * reset on draft conversion. Per-PR actuation-locked (#2135/#2447): a concurrent delivery for the same PR must + * not evaluate + potentially mutate it at the same time. Lock contention is retryable because ordinary + * maintenance can hold the shared lock without enforcing this converted_to_draft event. */ async function maybeCloseDraftDodgeAttempt( env: Env, deliveryId: string, @@ -7751,7 +7758,9 @@ async function maybeCloseDraftDodgeAttempt( pr: PullRequestRecord, settings: RepositorySettings, ): Promise { - if (!(await claimPrActuationLock(env, repoFullName, pr.number))) return; + if (!(await claimPrActuationLock(env, repoFullName, pr.number))) { + throw new PrActuationLockContendedError(repoFullName, pr.number, "draft-dodge"); + } try { await closeDraftDodgeAttemptIfBlocked( env, @@ -7937,18 +7946,16 @@ async function closeDraftDodgeAttemptIfBlocked( } } -/** Outcome of {@link maybeRecloseDisallowedReopen}: "reclosed" and "lock_contended" both mean the caller must - * skip the normal re-review pass — a plain boolean can't distinguish "evaluated, not blocked" from "never - * evaluated, another delivery owns this PR", and conflating them let a contended pass fall through to a - * concurrent re-review (#2135, review round 3). */ -type ReopenRecloseOutcome = "reclosed" | "allowed" | "lock_contended"; +/** Outcome of {@link maybeRecloseDisallowedReopen}: "reclosed" means the caller must skip the normal re-review + * pass; a plain boolean can't distinguish "evaluated, not blocked" from "reclosed, stop here". */ +type ReopenRecloseOutcome = "reclosed" | "allowed"; /** Reopen-prevention (#one-shot-reopen): re-close a contributor's reopen of a PR that gittensory / a maintainer * closed (closes are one-shot). Returns "reclosed" when it re-closed (caller skips the re-review). Exempt: the * bot's own re-review reopens, owner/admin reopens, and a contributor reopening a PR they CLOSED THEMSELVES. - * Per-PR actuation-locked (#2135): a concurrent delivery for the same PR (e.g. a check_suite completion racing - * this reopen) must not evaluate + potentially mutate this PR at the same time. Lock-contended returns - * "lock_contended" — the caller skips its own re-review too, since the delivery holding the lock owns this PR. */ + * Per-PR actuation-locked (#2135/#2447): a concurrent delivery for the same PR must not evaluate + potentially + * mutate this PR at the same time. Lock contention is retryable because ordinary maintenance can hold the + * shared lock without enforcing this reopened event. */ async function maybeRecloseDisallowedReopen( env: Env, deliveryId: string, @@ -7957,7 +7964,9 @@ async function maybeRecloseDisallowedReopen( pr: PullRequestRecord, payload: GitHubWebhookPayload, ): Promise { - if (!(await claimPrActuationLock(env, repoFullName, pr.number))) return "lock_contended"; + if (!(await claimPrActuationLock(env, repoFullName, pr.number))) { + throw new PrActuationLockContendedError(repoFullName, pr.number, "reopen-reclose"); + } try { const reclosed = await recloseDisallowedReopenIfNeeded( env, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index a1de5c16f0..3989af800d 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11703,7 +11703,7 @@ describe("one-shot reopen prevention", () => { expect(webhookRow?.status).toBe("processed"); }); - it("skips the reopen-reclose when a concurrent delivery already holds the per-PR actuation lock (#2135)", async () => { + it("retries the reopen-reclose when a concurrent delivery already holds the per-PR actuation lock (#2447)", async () => { const calls: Array<{ url: string; method: string }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -11728,16 +11728,16 @@ describe("one-shot reopen prevention", () => { // happens; the webhook path must stop BEFORE resolveRepositorySettings, the first call the re-review makes. const resolveSettingsSpy = vi.spyOn(repositorySettingsModule, "resolveRepositorySettings"); - await processJob(env, { + await expect(processJob(env, { type: "github-webhook", deliveryId: "reopen-lock-contended", eventName: "pull_request", payload: reopenedPayload("contributor"), - }); + })).rejects.toThrow("pr actuation lock contended"); expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(false); const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ n: number }>(); - expect(audit?.n).toBe(0); // no decision recorded either way — the in-flight delivery owns this pass + expect(audit?.n).toBe(0); // no decision recorded either way — the queue retry owns the deferred decision expect(resolveSettingsSpy).not.toHaveBeenCalled(); // the normal re-review pass never started }); @@ -12245,7 +12245,7 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { expect(audit?.detail).toContain("dry-run: would close"); }); - it("skips the draft-dodge close when a concurrent delivery already holds the per-PR actuation lock (#2135)", async () => { + it("retries the draft-dodge close when a concurrent delivery already holds the per-PR actuation lock (#2447)", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -12263,11 +12263,11 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { // racing this converted_to_draft event) — the lock key it would hold is pre-claimed here. await env.SELFHOST_TRANSIENT_CACHE?.set("pr-actuation-lock:jsonbored/gittensory#42", "1", 60); - await processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-lock-contended", eventName: "pull_request", payload: draftPayload("contributor") }); + await expect(processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-lock-contended", eventName: "pull_request", payload: draftPayload("contributor") })).rejects.toThrow("pr actuation lock contended"); expect(calls.some((c) => c.includes("PATCH") && c.includes("/pulls/42"))).toBe(false); const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ n: number }>(); - expect(audit?.n).toBe(0); // no decision recorded either way — the in-flight delivery owns this pass + expect(audit?.n).toBe(0); // no decision recorded either way — the queue retry owns the deferred decision }); it("REGRESSION: exactly ONE of two genuinely concurrent draft-dodge deliveries for the SAME PR wins the actuation lock (#2135)", async () => { @@ -12290,10 +12290,12 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { await setupRepo(env); await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); - await Promise.all([ + const results = await Promise.allSettled([ processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-race-a", eventName: "pull_request", payload: draftPayload("contributor") }), processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-race-b", eventName: "pull_request", payload: draftPayload("contributor") }), ]); + expect(results.filter((result) => result.status === "fulfilled")).toHaveLength(1); + expect(results.filter((result) => result.status === "rejected")).toHaveLength(1); const patchCalls = calls.filter((c) => c.includes("PATCH") && c.includes("/pulls/42")); expect(patchCalls).toHaveLength(1); // exactly one delivery won the race and closed the PR