diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 3d9319a035..7fed01f327 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3706,7 +3706,11 @@ export async function listOtherOpenPullRequestsForAuthor(env: Env, fullName: str .select() .from(pullRequests) .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.state, "open"), not(eq(pullRequests.number, number)), sql`lower(${pullRequests.authorLogin}) = lower(${authorLogin})`)) - .orderBy(asc(pullRequests.number)); + // Keep the per-webhook live-verification and sibling-wake work budget fixed. The cap path only needs the + // lowest-numbered siblings to preserve the "oldest PRs win" rule, and wake coalescing can discover later + // over-cap siblings from their own deliveries without letting one delivery fan out across an unbounded set. + .orderBy(asc(pullRequests.number)) + .limit(100); return rows.map(toPullRequestRecordFromRow); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 138b562334..0d7d49532a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2790,7 +2790,7 @@ async function runAgentMaintenancePlanAndExecute( // way to opt out of the PER-REPO cap specifically, even though `.gittensory.yml`'s own doc comment already // promised this reuse (auto-close-exempt.ts). if (typeof contributorOpenPrCap === "number" && pr.authorLogin && !isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) { - // Complete author-scoped set (not the duplicate-analysis 100-row sample), with every counted sibling + // Fixed-budget author-scoped set (the lowest-numbered sibling sample), with every counted sibling // positively LIVE-confirmed still open before it counts toward an irreversible close decision (#2270 // busy-repo bypass fix). Runs unconditionally now -- not just for isNewAccount -- since a stale-DB-row // false positive is exactly as wrong for an established contributor as for a new one; this supersedes the @@ -2799,12 +2799,12 @@ async function runAgentMaintenancePlanAndExecute( const otherAuthorOpenPullRequests = await listOtherOpenPullRequestsForAuthor(env, repoFullName, pr.number, pr.authorLogin); const confirmedOpen = new Set(); // Bounded concurrency (security review finding): an unbounded Promise.all here scales with the author's - // OWN open-PR count, not a fixed small number -- an author with dozens of open PRs would fire that many + // OWN open-PR sample, not a fixed small number -- an author with dozens of open PRs would fire that many // concurrent GitHub calls from a single webhook, and the delivery-order-guard wake below re-triggers this // same block for every over-cap sibling, compounding into near-quadratic API growth that can exhaust the - // installation's rate-limit budget. Every entry must still be verified (the exact over-cap PR numbers - // below depend on the complete confirmed-open set, not just "is the count over cap"), so this bounds - // concurrency rather than stopping early, mirroring mapWithConcurrency's other callers in this file. + // installation's rate-limit budget. Every sampled entry must still be verified (the exact over-cap PR + // numbers below depend on the confirmed-open sample, not just "is the count over cap"), so this bounds + // concurrency in addition to the repository query's total row cap. await mapWithConcurrency(otherAuthorOpenPullRequests, CONTRIBUTOR_CAP_LIVE_CHECK_CONCURRENCY, async (other) => { const liveState = await fetchLivePullRequestState(env, repoFullName, other.number, token, admissionKey).catch(() => undefined); if (liveState === "open") confirmedOpen.add(other.number); @@ -2820,8 +2820,8 @@ async function runAgentMaintenancePlanAndExecute( } // Webhook-delivery-order guard (#2479 gate finding): delivery order is not guaranteed to match PR creation // order, so a sibling PR's own webhook can process before THIS PR exists in the DB and wrongly conclude the - // author is within the cap. Use the complete author-scoped set (not the duplicate-analysis 100-row sample) - // and only siblings positively confirmed open, matching the issue-cap fail-safe close contract. + // author is within the cap. Use the fixed-budget author-scoped set and only siblings positively confirmed + // open, matching the issue-cap fail-safe close contract. const otherOverCapSiblingNumbers = otherAuthorOpenPullRequests .filter((other) => confirmedOpen.has(other.number) && overCapNumbers.has(other.number)) .map((other) => other.number); @@ -5064,13 +5064,11 @@ async function countLiveOpenWithConcurrencyUntil( // fan-out, same shape as GLOBAL_OPEN_ITEM_LIVE_CHECK_CONCURRENCY above. const NOTIFY_EVALUATE_EVENT_CONCURRENCY = 5; -// The per-repo contributor-cap live-verification (#2270 busy-repo bypass fix) walks the author's COMPLETE -// open-PR set on this repo, not a fixed small number -- an author with dozens of open PRs would otherwise fire -// that many concurrent fetchLivePullRequestState calls from a single webhook, and the delivery-order-guard -// wake below re-triggers this same check for every over-cap sibling, compounding into near-quadratic API -// growth across one busy author's siblings (security review finding). Every entry must still be verified (the -// exact over-cap PR numbers depend on the complete confirmed-open set, not just whether the count is over -// cap), so this bounds concurrency via mapWithConcurrency rather than stopping early. +// The per-repo contributor-cap live-verification (#2270 busy-repo bypass fix) walks a fixed-size author-scoped +// sibling sample. An author with many open PRs would otherwise fire too many concurrent +// fetchLivePullRequestState calls from a single webhook, and the delivery-order-guard wake below can re-trigger +// this same check for over-cap siblings. Every sampled entry must still be verified, so this bounds concurrency +// via mapWithConcurrency in addition to the repository query's total row cap. const CONTRIBUTOR_CAP_LIVE_CHECK_CONCURRENCY = 10; async function mapWithConcurrency(items: T[], concurrency: number, mapper: (item: T) => Promise): Promise { diff --git a/test/unit/duplicate-winner.test.ts b/test/unit/duplicate-winner.test.ts index 452e786d05..fc1b467abf 100644 --- a/test/unit/duplicate-winner.test.ts +++ b/test/unit/duplicate-winner.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from "vitest"; import { isDuplicateClusterWinner, isDuplicateClusterWinnerByClaim, resolveDuplicateClusterWinnerNumber } from "../../src/signals/duplicate-winner"; import { dupWinnerLinkedDuplicateCount, dupWinnerLinkedDuplicateWinnerNumber, linkedIssueDuplicatePullRequestsForGate } from "../../src/queue/processors"; import type { PullRequestRecord } from "../../src/types"; -import { listOtherOpenPullRequests, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { listOtherOpenPullRequests, listOtherOpenPullRequestsForAuthor, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; describe("isDuplicateClusterWinner (#dup-winner)", () => { @@ -303,6 +303,27 @@ describe("listOtherOpenPullRequests ordering (#audit-3.9)", () => { expect(Math.min(...siblingNumbers)).toBe(1); // the true winner #1 is retained despite being inserted last expect(siblingNumbers).not.toContain(102); // the lowest 100 (1..100) are returned, not the first-inserted 100 }); + + it("caps author-scoped contributor-cap siblings at the lowest 100 PRs (resource budget regression)", async () => { + const env = createTestEnv(); + // Insert #1 last so the LIMIT must be applied after numeric ordering, not insertion order. Rows from other + // authors and the subject PR are excluded before the cap, so the fixed live-check budget is all same-author + // siblings and cannot be inflated by unrelated open PRs. + const sameAuthorNumbers = [...Array.from({ length: 101 }, (_, i) => i + 2), 1]; // 2..102, then 1 + for (const n of sameAuthorNumbers) { + await upsertPullRequestFromGitHub(env, "owner/repo", { number: n, title: `Author PR ${n}`, state: "open", user: { login: "Prolific" }, head: { sha: `s${n}` }, labels: [], body: "x" }); + } + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 200, title: "Subject PR", state: "open", user: { login: "prolific" }, head: { sha: "subject" }, labels: [], body: "x" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 201, title: "Other author PR", state: "open", user: { login: "someone-else" }, head: { sha: "other" }, labels: [], body: "x" }); + + const siblings = await listOtherOpenPullRequestsForAuthor(env, "owner/repo", 200, "prolific"); + const siblingNumbers = siblings.map((p) => p.number); + expect(siblings).toHaveLength(100); + expect(siblingNumbers[0]).toBe(1); + expect(siblingNumbers).not.toContain(102); + expect(siblingNumbers).not.toContain(200); + expect(siblingNumbers).not.toContain(201); + }); }); describe("upsertPullRequestFromGitHub createdAt threading (#dup-winner true-creation-time)", () => {