perf(queue): claim the actuation lock before the refresh it exists to prevent (#10181)

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

Author: JSONbored

src/queue/processors.ts

@@ -4397,23 +4397,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,
@@ -4438,6 +4428,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

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);
+  });
+});