diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9829d34c61..7f03845843 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2305,6 +2305,52 @@ async function putTransientKey( } } +// Per-PR actuation mutex (#2135). Two DIFFERENT webhook deliveries for the same PR (e.g. a `reopened` event and +// a concurrent `check_suite completed` event) can be dequeued by separate workers at nearly the same time; both +// would read the same stale-but-still-"current" state, both pass their own freshness checks, and both +// independently fire a mutating call. This is a lightweight interim mutex (a full per-PR Durable Object / +// SubmissionLock is a separate, more-involved follow-up — see the TODO in env.d.ts) built on the SAME transient +// cache used for CI-completion coalescing above, claimed ATOMICALLY (see claimTransientLock) so two racing +// deliveries can never both win the claim — a short TTL, best-effort release. A lock-contended caller fails +// OPEN (returns false / skips this pass) rather than blocking — the delivery holding the lock is evaluating +// the SAME PR, and the periodic sweep is the backstop if this specific trigger is dropped. A cache adapter with +// no claim() primitive gets NO exclusivity at all (every call proceeds) rather than a get-then-set pair that +// only *looks* atomic — see claimTransientLock's doc comment for why that fallback was removed. +// +// KNOWN LIMITATION: the lock value is a constant, not a per-holder ownership token, so release does not verify +// it still owns the key — if a holder ran past the TTL, a later claimer's live lock could be deleted by the +// first holder's stale `finally` release, reopening the exact race this mutex exists to close. A per-holder +// token + a conditional (check-then-delete) release would close this properly, but needs a new atomic +// compare-and-delete primitive on the cache adapter — tracked alongside the Durable Object follow-up above. The +// TTL is set generously long specifically so this window is practically unreachable: the guarded operations +// (a handful of sequential GitHub API calls) should never legitimately run anywhere near this long. +const PR_ACTUATION_LOCK_TTL_SECONDS = 600; +function prActuationLockKey(repoFullName: string, prNumber: number): string { + return `pr-actuation-lock:${repoFullName.toLowerCase()}#${prNumber}`; +} +export async function claimPrActuationLock( + env: Env, + repoFullName: string, + prNumber: number, +): Promise { + return claimTransientLock( + env, + prActuationLockKey(repoFullName, prNumber), + PR_ACTUATION_LOCK_TTL_SECONDS, + ); +} +export async function releasePrActuationLock( + env: Env, + repoFullName: string, + prNumber: number, +): Promise { + try { + await env.SELFHOST_TRANSIENT_CACHE?.del?.(prActuationLockKey(repoFullName, prNumber)); + } catch { + // best-effort + } +} + /** * 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 @@ -3736,19 +3782,26 @@ async function processGitHubWebhook( // Reopen-prevention (#one-shot-reopen): a CONTRIBUTOR may not reopen a PR that gittensory or a maintainer // 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. - if ( - payload.action === "reopened" && - installationId && - (await maybeRecloseDisallowedReopen( - env, - deliveryId, - installationId, - repoFullName, - pr, - payload, - ).catch(() => false)) - ) { + // 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. + // 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 + // "allowed" — which would re-permit exactly the disallowed reopen this guard exists to stop. Let it + // propagate and retry instead, same reasoning as the draft-dodge sibling's uncaught getInstallation read. + const reopenOutcome: ReopenRecloseOutcome = + payload.action === "reopened" && installationId + ? await maybeRecloseDisallowedReopen( + env, + deliveryId, + installationId, + repoFullName, + pr, + payload, + ) + : "allowed"; + if (reopenOutcome === "reclosed" || reopenOutcome === "lock_contended") { // 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, { @@ -3806,167 +3859,18 @@ async function processGitHubWebhook( !settings.agentPaused && !isProtectedAutomationAuthor(pr.authorLogin) ) { - const block = await getGateBlockOutcome( + // Deliberately UNCAUGHT here: closeDraftDodgeAttemptIfBlocked catches every operation that should + // fail safely, but leaves the write-permission-readiness getInstallation read (#2134) uncaught on + // purpose so a transient D1 failure propagates and the queue retries instead of misrecording a + // permission denial. + await maybeCloseDraftDodgeAttempt( env, + deliveryId, + installationId, repoFullName, - pr.number, - ).catch(() => undefined); - const repoOwner = repoFullName.includes("/") - ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() - : ""; - const draftDodgeAuthorLogin = (pr.authorLogin ?? "").toLowerCase(); - const authorIsOwner = - draftDodgeAuthorLogin === repoOwner && repoOwner.length > 0; - // Fleet-operator identity (#2133): same ADMIN_GITHUB_LOGINS exemption as the primary close-eligibility - // computation above and hasMaintainerPermission below — an admin login must never be auto-closed here - // either, matching every other actuation path's trusted-operator definition. - const authorIsAdmin = - draftDodgeAuthorLogin.length > 0 && - parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(draftDodgeAuthorLogin); - if ( - block && - block.headSha === pr.headSha && - !block.overridden && - !authorIsOwner && - !authorIsAdmin - ) { - // Respect the agent action mode (#killswitch-gap): the outer guard already excludes a per-repo pause, - // but this close path must also honor the global freeze and dry-run — so a freeze is a COMPLETE stop - // and a dry-run records the would-be close without touching GitHub. - const draftMode = resolveAgentActionMode({ - globalPaused: - isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), - agentPaused: settings.agentPaused, - agentDryRun: settings.agentDryRun, - }); - if (draftMode === "live") { - // Write-permission readiness (#2134): this close bypasses executeAgentMaintenanceActions entirely - // (the whole point is to enforce the gate verdict against the CURRENT headSha even though the PR - // was converted to draft), so it never got the standard pipeline's step-6 PR_WRITE_CLASSES guard. - // Without this, a revoked/never-consented pull_requests:write grant would still attempt the close, - // get a 403 from GitHub, and have it silently swallowed by the .catch() below — with the audit - // event still recorded as "completed" as if the close actually happened. Checked BEFORE the live - // freshness re-check below so a permission-denied installation never pays for a live GitHub fetch. - // Deliberately UNCAUGHT: getInstallation itself never swallows a genuine D1 read failure (it only - // resolves null on a legitimate "row not found" query result), so let a transient storage hiccup - // propagate and fail this whole webhook job -- the queue's own retry re-runs it, and a later attempt - // with a working DB read correctly evaluates readiness. Catching it into `null` here would instead - // permanently misrecord the outcome as "pull_requests: write not granted" (a real GitHub-permission - // problem) when the actual cause was an infra blip, misleading an operator investigating the audit - // trail and burying the fact that no retry ever happens for a caught, definitively-denied outcome. - const draftDodgeInstallation = await getInstallation( - env, - installationId, - ); - /* v8 ignore next -- upsertInstallation already ran unconditionally earlier in this same handler for - * every webhook, so a genuinely-missing row is not reachable through the normal webhook path - * exercised by tests; a synced installation always has a permissions object. */ - const draftDodgeInstallationPermissions = draftDodgeInstallation?.permissions ?? null; - const draftDodgePermissionReadiness = resolveAgentPermissionReadiness({ - autonomy: settings.autonomy, - installationPermissions: draftDodgeInstallationPermissions, - }); - if (draftDodgePermissionReadiness !== "ready") { - /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ - const draftDodgeAuthor = pr.authorLogin ?? "unknown"; - await recordAuditEvent(env, { - eventType: "github_app.draft_dodge_closed", - actor: "gittensory", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "denied", - detail: `denied draft-dodge close for ${draftDodgeAuthor} — pull_requests: write not granted`, - metadata: { - deliveryId, - repoFullName, - headSha: pr.headSha, - blockerCodes: block.blockerCodes, - }, - }).catch( - /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ - () => undefined, - ); - } else { - // Live re-check (#2130): the two async DB reads above (getGateBlockOutcome, resolveAgentActionMode's - // isGlobalAgentFrozen) leave a window where a maintainer could merge/close the PR, or a fresh push - // could clear the gate failure, before this fires. Unlike the main gate-close path — which routes - // every close through executeAgentMaintenanceActions's freshness guard — this handler acted purely - // off the stale webhook-ingestion payload. Re-verify live state immediately before the mutation. - // requireDraft: head/state alone would still read "current" if the author converted the PR BACK - // to ready_for_review in that window -- the draft-dodge close's own justification no longer - // holds, since there is no longer a draft to be "dodging" the gate through. - const freshness = await fetchPullRequestFreshness(env, { - installationId, - repoFullName, - pullNumber: pr.number, - expectedHeadSha: pr.headSha, - requireDraft: true, - }); - if (freshness.status !== "current") { - await recordAuditEvent(env, { - eventType: "github_app.draft_dodge_closed", - actor: "gittensory", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "denied", - detail: `${pullRequestFreshnessDetail(freshness)} — draft-dodge close not executed`, - metadata: { - deliveryId, - repoFullName, - headSha: pr.headSha, - blockerCodes: block.blockerCodes, - }, - }).catch(() => undefined); - } else { - const codes = block.blockerCodes.join(", "); - await createIssueComment( - env, - installationId, - repoFullName, - pr.number, - `Gate verdict stands for this commit — converting to draft does not reset the review. Re-submit a new PR with the issues addressed${codes ? ` (${codes})` : ""}.`, - ).catch(() => undefined); - await closePullRequest( - env, - installationId, - repoFullName, - pr.number, - ).catch(() => undefined); - await recordAuditEvent(env, { - eventType: "github_app.draft_dodge_closed", - actor: "gittensory", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: `closed draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — prior gate failure on headSha ${pr.headSha} stands`, - metadata: { - deliveryId, - repoFullName, - headSha: pr.headSha, - blockerCodes: block.blockerCodes, - }, - }).catch(() => undefined); - } - } - } else if (draftMode === "dry_run") { - /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ - const draftAuthor = pr.authorLogin ?? "unknown"; - await recordAuditEvent(env, { - eventType: "github_app.draft_dodge_closed", - actor: "gittensory", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: `dry-run: would close draft-dodge attempt by ${draftAuthor} — prior gate failure on headSha ${pr.headSha} stands`, - metadata: { - deliveryId, - repoFullName, - headSha: pr.headSha, - blockerCodes: block.blockerCodes, - mode: "dry_run", - }, - }).catch( - /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ - () => undefined, - ); - } - } + pr, + settings, + ); } if ( installationId && @@ -7865,9 +7769,218 @@ 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. */ +async function maybeCloseDraftDodgeAttempt( + env: Env, + deliveryId: string, + installationId: number, + repoFullName: string, + pr: PullRequestRecord, + settings: RepositorySettings, +): Promise { + if (!(await claimPrActuationLock(env, repoFullName, pr.number))) return; + try { + await closeDraftDodgeAttemptIfBlocked( + env, + deliveryId, + installationId, + repoFullName, + pr, + settings, + ); + } finally { + await releasePrActuationLock(env, repoFullName, pr.number); + } +} + +async function closeDraftDodgeAttemptIfBlocked( + env: Env, + deliveryId: string, + installationId: number, + repoFullName: string, + pr: PullRequestRecord, + settings: RepositorySettings, +): Promise { + const block = await getGateBlockOutcome( + env, + repoFullName, + pr.number, + ).catch(() => undefined); + const repoOwner = repoFullName.includes("/") + ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() + : ""; + const draftDodgeAuthorLogin = (pr.authorLogin ?? "").toLowerCase(); + const authorIsOwner = + draftDodgeAuthorLogin === repoOwner && repoOwner.length > 0; + // Fleet-operator identity (#2133): same ADMIN_GITHUB_LOGINS exemption as the primary close-eligibility + // computation elsewhere — an admin login must never be auto-closed here either, matching every other + // actuation path's trusted-operator definition. + const authorIsAdmin = + draftDodgeAuthorLogin.length > 0 && + parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(draftDodgeAuthorLogin); + if ( + block && + block.headSha === pr.headSha && + !block.overridden && + !authorIsOwner && + !authorIsAdmin + ) { + // Respect the agent action mode (#killswitch-gap): the outer guard already excludes a per-repo pause, + // but this close path must also honor the global freeze and dry-run — so a freeze is a COMPLETE stop + // and a dry-run records the would-be close without touching GitHub. + const draftMode = resolveAgentActionMode({ + globalPaused: + isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + agentPaused: settings.agentPaused, + agentDryRun: settings.agentDryRun, + }); + if (draftMode === "live") { + // Write-permission readiness (#2134): this close bypasses executeAgentMaintenanceActions entirely + // (the whole point is to enforce the gate verdict against the CURRENT headSha even though the PR + // was converted to draft), so it never got the standard pipeline's step-6 PR_WRITE_CLASSES guard. + // Without this, a revoked/never-consented pull_requests:write grant would still attempt the close, + // get a 403 from GitHub, and have it silently swallowed by the .catch() below — with the audit + // event still recorded as "completed" as if the close actually happened. Checked BEFORE the live + // freshness re-check below so a permission-denied installation never pays for a live GitHub fetch. + // Deliberately UNCAUGHT: getInstallation itself never swallows a genuine D1 read failure (it only + // resolves null on a legitimate "row not found" query result), so let a transient storage hiccup + // propagate and fail this whole webhook job -- the queue's own retry re-runs it, and a later attempt + // with a working DB read correctly evaluates readiness. Catching it into `null` here would instead + // permanently misrecord the outcome as "pull_requests: write not granted" (a real GitHub-permission + // problem) when the actual cause was an infra blip, misleading an operator investigating the audit + // trail and burying the fact that no retry ever happens for a caught, definitively-denied outcome. + const draftDodgeInstallation = await getInstallation( + env, + installationId, + ); + /* v8 ignore next -- upsertInstallation already ran unconditionally earlier in this same handler for + * every webhook, so a genuinely-missing row is not reachable through the normal webhook path + * exercised by tests; a synced installation always has a permissions object. */ + const draftDodgeInstallationPermissions = draftDodgeInstallation?.permissions ?? null; + const draftDodgePermissionReadiness = resolveAgentPermissionReadiness({ + autonomy: settings.autonomy, + installationPermissions: draftDodgeInstallationPermissions, + }); + if (draftDodgePermissionReadiness !== "ready") { + /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ + const draftDodgeAuthor = pr.authorLogin ?? "unknown"; + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "denied", + detail: `denied draft-dodge close for ${draftDodgeAuthor} — pull_requests: write not granted`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); + return; + } + // Live re-check (#2130): the two async DB reads above (getGateBlockOutcome, resolveAgentActionMode's + // isGlobalAgentFrozen) leave a window where a maintainer could merge/close the PR, or a fresh push + // could clear the gate failure, before this fires. Unlike the main gate-close path — which routes + // every close through executeAgentMaintenanceActions's freshness guard — this handler acted purely + // off the stale webhook-ingestion payload. Re-verify live state immediately before the mutation. + // requireDraft: head/state alone would still read "current" if the author converted the PR BACK + // to ready_for_review in that window -- the draft-dodge close's own justification no longer + // holds, since there is no longer a draft to be "dodging" the gate through. + const freshness = await fetchPullRequestFreshness(env, { + installationId, + repoFullName, + pullNumber: pr.number, + expectedHeadSha: pr.headSha, + requireDraft: true, + }); + if (freshness.status !== "current") { + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "denied", + detail: `${pullRequestFreshnessDetail(freshness)} — draft-dodge close not executed`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + }, + }).catch(() => undefined); + } else { + const codes = block.blockerCodes.join(", "); + await createIssueComment( + env, + installationId, + repoFullName, + pr.number, + `Gate verdict stands for this commit — converting to draft does not reset the review. Re-submit a new PR with the issues addressed${codes ? ` (${codes})` : ""}.`, + ).catch(() => undefined); + await closePullRequest( + env, + installationId, + repoFullName, + pr.number, + ).catch(() => undefined); + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `closed draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — prior gate failure on headSha ${pr.headSha} stands`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + }, + }).catch(() => undefined); + } + } else if (draftMode === "dry_run") { + /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ + const draftAuthor = pr.authorLogin ?? "unknown"; + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `dry-run: would close draft-dodge attempt by ${draftAuthor} — prior gate failure on headSha ${pr.headSha} stands`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + mode: "dry_run", + }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); + } + } +} + +/** 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"; + /** Reopen-prevention (#one-shot-reopen): re-close a contributor's reopen of a PR that gittensory / a maintainer - * closed (closes are one-shot). Returns true 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. */ + * 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. */ async function maybeRecloseDisallowedReopen( env: Env, deliveryId: string, @@ -7875,6 +7988,30 @@ async function maybeRecloseDisallowedReopen( repoFullName: string, pr: PullRequestRecord, payload: GitHubWebhookPayload, +): Promise { + if (!(await claimPrActuationLock(env, repoFullName, pr.number))) return "lock_contended"; + try { + const reclosed = await recloseDisallowedReopenIfNeeded( + env, + deliveryId, + installationId, + repoFullName, + pr, + payload, + ); + return reclosed ? "reclosed" : "allowed"; + } finally { + await releasePrActuationLock(env, repoFullName, pr.number); + } +} + +async function recloseDisallowedReopenIfNeeded( + env: Env, + deliveryId: string, + installationId: number, + repoFullName: string, + pr: PullRequestRecord, + payload: GitHubWebhookPayload, ): Promise { const reopener = (payload.sender?.login ?? "").toLowerCase(); if (!reopener) return false; diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 61d59a5ef9..46dc72421f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -4,6 +4,7 @@ import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { PR_PANEL_COMMENT_MARKER } from "../../src/github/comments"; import * as backfillModule from "../../src/github/backfill"; import * as repositoriesModule from "../../src/db/repositories"; +import * as repositorySettingsModule from "../../src/settings/repository-settings"; import * as sentryModule from "../../src/selfhost/sentry"; import { listCollisionEdges, @@ -44,7 +45,7 @@ import { upsertRepositoryFromGitHub, putCachedAiReview, } from "../../src/db/repositories"; -import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAgentMaintenanceLock, claimAiReviewLock, contributorEvidenceBatchSize, processJob, releaseAgentMaintenanceLock, releaseAiReviewLock } from "../../src/queue/processors"; +import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAgentMaintenanceLock, claimAiReviewLock, claimPrActuationLock, contributorEvidenceBatchSize, processJob, releaseAgentMaintenanceLock, releaseAiReviewLock, releasePrActuationLock } 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"; @@ -3676,6 +3677,93 @@ describe("queue processors", () => { expect([first, second]).toEqual([true, true]); }); + // claimPrActuationLock (#2135) mirrors claimAgentMaintenanceLock's atomic-claim design exactly — same test + // shapes, same reasoning, a different lock namespace. + it("claimPrActuationLock claims when free, denies when held (per-PR), and release frees it again (#2135)", async () => { + const env = createTestEnv({}); + expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(false); + expect(await claimPrActuationLock(env, "owner/act-repo", 8)).toBe(true); + await releasePrActuationLock(env, "owner/act-repo", 7); + expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + }); + + it("claimPrActuationLock fails OPEN on a broken transient cache — never itself blocks actuation (#2135)", 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 claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + await expect(releasePrActuationLock(env, "owner/act-repo", 7)).resolves.toBeUndefined(); + }); + + it("claimPrActuationLock fails OPEN when the atomic claim primitive itself throws (#2135)", async () => { + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => null, + set: async () => undefined, + claim: async () => { throw new Error("redis unavailable"); }, + }, + }); + expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + }); + + it("REGRESSION (#2135): claimPrActuationLock uses an atomic check-and-set, so two genuinely concurrent claims for the SAME PR can never both succeed", async () => { + const env = createTestEnv({}); + const [first, second] = await Promise.all([ + claimPrActuationLock(env, "owner/act-repo", 7), + claimPrActuationLock(env, "owner/act-repo", 7), + ]); + expect([first, second].filter(Boolean)).toHaveLength(1); + }); + + it("REGRESSION (#2135): claimPrActuationLock 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 claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect(calls).toEqual(["claim"]); // never falls through to the racy get/set pair when claim is available + }); + + it("claimPrActuationLock returns true unconditionally when the cache has no claim() — no false exclusivity guarantee (#2135, review round 2)", async () => { + // Mirrors claimAgentMaintenanceLock's #confirmed-bug fix: a get-then-set pair (even with a re-read) is not + // a real exclusivity guarantee under concurrent load, so a cache without claim() now gets NO exclusivity at + // all rather than a fallback that only looks atomic. + 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 claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + expect(await claimPrActuationLock(env, "owner/act-repo", 7)).toBe(true); + }); + + it("REGRESSION (#2135, review round 2): claimPrActuationLock does not falsely claim exclusivity for two genuinely concurrent callers when the cache has no claim()", async () => { + 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([ + claimPrActuationLock(env, "owner/act-repo", 7), + claimPrActuationLock(env, "owner/act-repo", 7), + ]); + 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 () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); @@ -11716,6 +11804,44 @@ 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 () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + // Simulates a DIFFERENT concurrent delivery for the same PR already in flight (e.g. the draft-dodge sibling + // racing this reopen) — the lock key it would hold is pre-claimed here. + await env.SELFHOST_TRANSIENT_CACHE?.set("pr-actuation-lock:jsonbored/gittensory#42", "1", 60); + // REGRESSION (#2135, review round 3): a contended lock previously returned `false`, which the caller's old + // boolean contract read as "not blocked, proceed to normal re-review" -- this spy proves that no longer + // happens; the webhook path must stop BEFORE resolveRepositorySettings, the first call the re-review makes. + const resolveSettingsSpy = vi.spyOn(repositorySettingsModule, "resolveRepositorySettings"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "reopen-lock-contended", + eventName: "pull_request", + payload: reopenedPayload("contributor"), + }); + + 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(resolveSettingsSpy).not.toHaveBeenCalled(); // the normal re-review pass never started + }); + it("does NOT re-close a disallowed reopen on an OBSERVE-only / un-opted-in repo (autonomy floor, #review-audit)", async () => { const calls: Array<{ url: string; method: string }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -12220,6 +12346,62 @@ 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 () => { + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42")) return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await setupRepo(env); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + // Simulates a DIFFERENT concurrent delivery for the same PR already in flight (e.g. a check_suite completion + // 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") }); + + 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 + }); + + it("REGRESSION: exactly ONE of two genuinely concurrent draft-dodge deliveries for the SAME PR wins the actuation lock (#2135)", async () => { + // Unlike the lock-contended test above (which pre-seeds the key before the call even starts), this fires + // two deliveries together via Promise.all with NEITHER pre-claiming anything — exercising the actual + // check-and-set race claimPrActuationLock must arbitrate, not just "the key was already there". A + // get-then-set (non-atomic) implementation lets both deliveries observe an absent key and both proceed, + // which this test would catch as more than one PATCH / more than one completed audit row. + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42")) return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await setupRepo(env); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + await Promise.all([ + 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") }), + ]); + + 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 + const audit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and outcome = 'completed'").bind("github_app.draft_dodge_closed").first<{ n: number }>(); + expect(audit?.n).toBe(1); // exactly one completed close recorded — not two (the race), not zero + }); + it("no-ops when no prior gate failure exists for the PR", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {