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

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

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