From 2431817678ddc6d2ef3c8df71b4098ff8cc4f45b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:18:24 -0700 Subject: [PATCH] fix(locks): early contributor-cap close joins the per-PR actuation lock namespace (#8805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit maybeCloseForContributorCapOnOpen executed its close under only the per-(repo, author) cap lock — outside the "one shared lock namespace covering every mutating PR pass" contract (transient-locks.ts) — so a sweep-fanned agent-regate-pr job holding the PR-actuation lock could plan/execute a different action for the SAME PR concurrently with the cap close. The execution section now also try-claims the per-PR actuation lock, nested author-outer → PR-inner. That is the OPPOSITE nesting of the executor's pre-merge cap re-check (PR-outer → author-inner, #7284) — safe regardless: both locks are non-blocking try-claims, so cross-order contention degrades to both passes deferring cleanly (the end-of-pipeline cap check and the next tick are the backstops), never a blocking-wait deadlock. Documented at the claim site. Contention test mirrors the existing author-lock sibling: the close defers (no PATCH), the normal pipeline falls through. --- src/queue/processors.ts | 13 +++++++++ test/unit/queue-3.test.ts | 57 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d5584ab309..56b99e938f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2800,6 +2800,16 @@ async function maybeCloseForContributorCapOnOpen( pr: { headSha: pr.headSha }, }); if (planned === null || planned.length === 0) return false; + // #8805: ALSO claim the per-PR actuation lock before executing — this early cap-close previously ran + // outside the "one shared lock namespace covering every mutating PR pass" (transient-locks.ts), so a + // sweep-fanned agent-regate-pr job could plan/execute a DIFFERENT action for this same PR concurrently. + // Nesting order here is author-outer → PR-inner, the OPPOSITE of the executor's pre-merge cap re-check + // (PR-outer → author-inner, #7284): safe regardless, because both locks are non-blocking TRY-claims — + // a cross-order contention degrades to both passes deferring cleanly (return false; the end-of-pipeline + // cap check and the next webhook/sweep tick are the backstops), never a blocking-wait deadlock. + const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); + if (!actuationLock.acquired) return false; + try { const installation = await getInstallation(env, installationId); const outcomes = await executeAgentMaintenanceActions( env, @@ -2826,6 +2836,9 @@ async function maybeCloseForContributorCapOnOpen( planned, ); return outcomes.some((outcome) => outcome.actionClass === "close" && outcome.outcome === "completed"); + } finally { + await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken); + } } finally { await releaseContributorCapLock(env, repoFullName, pr.authorLogin, ownerToken); } diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index bf3340e094..b032aa1131 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2946,6 +2946,63 @@ describe("queue processors", () => { expect(aiCalls).toBeGreaterThan(0); }); + it("early cap short-circuit (#8805): a contended PER-PR actuation lock defers the close cleanly — no cross-namespace race with a sibling mutating pass", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "n/a", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 53, title: "Farmer PR one", state: "open", user: { login: "farmer99" }, head: { sha: "f53" }, labels: [], body: "x" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 54, title: "Farmer PR two", state: "open", user: { login: "farmer99" }, head: { sha: "f54" }, labels: [], body: "y" }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", contributorCapLabel: "spam-cap", reviewCheckMode: "required", aiReviewMode: "advisory", contributorOpenPrCap: 2 } }, "repo_file"); + // A sibling mutating pass (e.g. a sweep-fanned agent-regate-pr) already holds THIS PR's actuation lock — + // pre-#8805 the early cap close executed anyway, outside the shared namespace. Now it must defer. + await env.SELFHOST_TRANSIENT_CACHE?.claim?.("pr-actuation-lock:jsonbored/gittensory#56", "sibling-pass-token", 30); + const seen = { closed: false }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/56/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/56/reviews")) return Response.json([]); + if (url.includes("/pulls/56/commits")) return Response.json([]); + if ((url.endsWith("/pulls/53") || url.endsWith("/pulls/54")) && method === "GET") return Response.json({ state: "open" }); + if (url.endsWith("/pulls/56") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 56, state: "closed" }); } + if (url.endsWith("/pulls/56")) return Response.json({ number: 56, state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, mergeable_state: "clean" }); + if (url.includes("/commits/f56/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/f56/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/56/labels")) return Response.json([]); + if (url.includes("/issues/56/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "contributor-cap-pr-lock-contended", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 56, title: "Farmer's 3rd PR", state: "open", user: { login: "farmer99" }, head: { sha: "f56" }, labels: [], body: "x", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + + // The early close DEFERRED (no PATCH close fired from it) and the pipeline fell through, mirroring the + // author-lock contention semantics one namespace over. + expect(seen.closed).toBe(false); + expect(aiCalls).toBeGreaterThan(0); + }); + it("early cap short-circuit (#7284-fix): close autonomy not granted (observe) plans nothing early -- no crash, falls through to the normal pipeline", async () => { let aiCalls = 0; const env = createTestEnv({