From c0e00f26a75bc2729e3769b040668f36baf9b96b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:15:11 -0700 Subject: [PATCH] perf(queue): claim the actuation lock before the refresh it exists to prevent The publish-and-maintain pass refreshed PR details and THEN asked "does another pass already own this PR", so every contended pass did the work and threw it away. github_app.pr_public_surface_lock_contended is the single most frequent audit event on the production Orb: 1,180 occurrences between 09:00 and 10:53 today, roughly 10 per minute and about twice the next event. Each one is a discarded pass. The cost is real but worth stating precisely. refreshPullRequestDetails is itself cached -- it consults the detail-sync state and reuses stored pull_request_files rows when the last sync covered the current head SHA -- so a contention does not always cost a GitHub call. It always costs the sync-state reads, and on a cache miss it costs a token fetch plus the files/reviews fetch. That miss is what a busy PR produces, and a busy PR is also what contends, so the two peak together. This happened in the same window the installation exhausted its REST quota and 66 queue jobs stalled behind deferred_by: rate_limit. Claiming first changes no semantics: the lock's stated purpose (#9013) is to make "does another pass already own this PR" one question with one answer for the whole publish-then-maintain unit, and asking it before the expensive part is strictly better. Holding it across the refresh is already safe -- #9467 renews the lock while work runs precisely because this unit can span an AI review far longer than a refresh. The second contention site is deliberately untouched: it does not refresh beforehand, so it does not have this defect. Guarded by a test asserting SOURCE ORDER, which is unusual and deliberate. Both orderings behave identically on the happy path and differ only in what a LOSING pass spends before it throws, so no behavioural test can distinguish them -- which is why this drifted unnoticed. The test anchors on the publish pass's own contention audit event so an unrelated claim elsewhere in this 16k-line file cannot satisfy it, and asserts both landmarks still exist so a rename cannot make it pass vacuously. Closes #10174 --- src/queue/processors.ts | 41 +++++++++++-------- test/unit/actuation-lock-ordering.test.ts | 49 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 17 deletions(-) create mode 100644 test/unit/actuation-lock-ordering.test.ts 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); + }); +});