Skip to content

fix(queue): retry policy closes on PR lock contention #2471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 2, 2026
Merged

fix(queue): retry policy closes on PR lock contention #2471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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, {
Expand Down Expand Up @@ -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,
Expand All @@ -7751,7 +7758,9 @@ async function maybeCloseDraftDodgeAttempt(
pr: PullRequestRecord,
settings: RepositorySettings,
): Promise<void> {
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,
Expand Down Expand Up @@ -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,
Expand All @@ -7957,7 +7964,9 @@ async function maybeRecloseDisallowedReopen(
pr: PullRequestRecord,
payload: GitHubWebhookPayload,
): Promise<ReopenRecloseOutcome> {
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,
Expand Down
18 changes: 10 additions & 8 deletions test/unit/queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
});

Expand Down Expand Up @@ -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();
Expand All @@ -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 () => {
Expand All @@ -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
Expand Down
Loading