diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 353d7bd2e..1ddaa823b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4424,23 +4424,13 @@ export async function reReviewStoredPullRequest( scopedLinkedIssueClaimedAt, }); await persistAdvisory(env, advisory); - // #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated - // on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no - // pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit - // stale indefinitely even though this per-PR sweep unit visits every open PR on a bounded cadence. - // Short-circuit the extra read when another reason already forces the refresh. - const otherRefreshReasons = - shouldCollectSlopEvidence(settings) || - settings.manifestPolicyGateMode !== "off" || - (await shouldRefreshFilesForPreMergeChecks(env, repoFullName)); - const reviewsCacheStale = - !otherRefreshReasons && - !isReviewsCacheUpToDate(await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null)); - if (otherRefreshReasons || reviewsCacheStale) { - await refreshPullRequestDetails(env, repoFullName, prNumber).catch( - () => undefined, - ); - } + // #10174: the lock is claimed BEFORE the refresh below, not after. It answers "does another pass + // already own this PR" -- asking that only after paying for the refresh meant every contended pass did + // the work and threw it away. That was the single most frequent audit event on the Orb (1,180 + // github_app.pr_public_surface_lock_contended in under two hours, ~2x the next event), and the refresh + // it wasted is a GitHub read whenever the detail-sync cache misses -- which is exactly what a busy PR + // does, and a busy PR is also what contends. Holding the lock across the refresh is already safe: #9467 + // renews it while work runs, because this unit can span an AI review far longer than a refresh. // #9013: ONE per-PR actuation-lock claim spans the publish pass AND the maintenance pass right after it. // maybePublishPrPublicSurface used to run with no lock at all -- only the LATER maybeRunAgentMaintenance // claimed one -- so two concurrent passes for the SAME PR (this sweep re-review racing a webhook delivery, @@ -4465,6 +4455,23 @@ export async function reReviewStoredPullRequest( }).catch(() => undefined); throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish"); } + // #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated + // on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no + // pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit + // stale indefinitely even though this per-PR sweep unit visits every open PR on a bounded cadence. + // Short-circuit the extra read when another reason already forces the refresh. + const otherRefreshReasons = + shouldCollectSlopEvidence(settings) || + settings.manifestPolicyGateMode !== "off" || + (await shouldRefreshFilesForPreMergeChecks(env, repoFullName)); + const reviewsCacheStale = + !otherRefreshReasons && + !isReviewsCacheUpToDate(await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null)); + if (otherRefreshReasons || reviewsCacheStale) { + await refreshPullRequestDetails(env, repoFullName, prNumber).catch( + () => undefined, + ); + } // #9467: this lock now spans the WHOLE publish -> AI review -> maintain unit (#9013 moved the claim here), // and the AI review alone can outlive the 600s TTL. Renew it while the work runs so a slow-but-healthy pass // cannot have its lock claimed out from under it mid-flight. Compare-and-extend, so if this pass has already diff --git a/test/unit/actuation-lock-ordering.test.ts b/test/unit/actuation-lock-ordering.test.ts new file mode 100644 index 000000000..d55050661 --- /dev/null +++ b/test/unit/actuation-lock-ordering.test.ts @@ -0,0 +1,49 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "vitest"; + +// #10174: the publish-and-maintain pass must claim the actuation lock BEFORE doing the work the lock exists +// to avoid duplicating. +// +// It used to refresh PR details first and ask "does another pass already own this?" second, so every +// contended pass paid for the refresh and threw it away. That was the single most frequent audit event on the +// production Orb -- 1,180 `github_app.pr_public_surface_lock_contended` in under two hours, about twice the +// next event -- and `refreshPullRequestDetails` is a GitHub read whenever the detail-sync cache misses, which +// is exactly what a busy PR does. Busy PRs are also what contend, so the two peak together. It happened in +// the same window the installation exhausted its REST quota. +// +// Asserted structurally, on source order, because there is no behavioural seam here: both orderings produce +// identical results on the happy path and differ only in what a LOSING pass spends before it throws. A unit +// test that exercised the pass could not tell them apart, which is precisely why this drifted unnoticed. + +const SOURCE = readFileSync(join(import.meta.dirname, "..", "..", "src", "queue", "processors.ts"), "utf8"); + +describe("actuation lock ordering (#10174)", () => { + it("sanity: both landmarks still exist, so a rename cannot make this pass vacuously", () => { + expect(SOURCE).toContain("claimPrActuationLock"); + expect(SOURCE).toContain("refreshPullRequestDetails"); + }); + + it("REGRESSION: the publish pass claims the lock before refreshing PR details", () => { + // Scoped to the publish-and-maintain pass by anchoring on its own contention audit event, so the + // assertion cannot be satisfied by some unrelated earlier claim elsewhere in this 16k-line file. + const contention = SOURCE.indexOf('eventType: "github_app.pr_public_surface_lock_contended"'); + expect(contention).toBeGreaterThan(-1); + + const claimBefore = SOURCE.lastIndexOf("claimPrActuationLock", contention); + expect(claimBefore).toBeGreaterThan(-1); + + // The refresh must come AFTER that claim, not before it. + const refreshAfterClaim = SOURCE.indexOf("refreshPullRequestDetails(env, repoFullName, prNumber)", claimBefore); + const refreshBeforeClaim = SOURCE.lastIndexOf("refreshPullRequestDetails(env, repoFullName, prNumber)", claimBefore); + + expect(refreshAfterClaim, "the refresh should follow the lock claim").toBeGreaterThan(claimBefore); + // And there must be no refresh sitting between the advisory persist and the claim. + const persist = SOURCE.lastIndexOf("await persistAdvisory(env, advisory)", claimBefore); + expect( + refreshBeforeClaim < persist, + "refreshPullRequestDetails must not run between persistAdvisory and the lock claim — that is the wasted work this fixes", + ).toBe(true); + }); +});