Skip to content

fix(queue): cap contributor PR live checks #3911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 7, 2026
Merged

fix(queue): cap contributor PR live checks #3911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/db/repositories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
26 changes: 12 additions & 14 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -2799,12 +2799,12 @@ async function runAgentMaintenancePlanAndExecute(
const otherAuthorOpenPullRequests = await listOtherOpenPullRequestsForAuthor(env, repoFullName, pr.number, pr.authorLogin);
const confirmedOpen = new Set<number>();
// 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);
Expand All @@ -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);
Expand Down Expand Up @@ -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<T, R>(items: T[], concurrency: number, mapper: (item: T) => Promise<R>): Promise<R[]> {
Expand Down
23 changes: 22 additions & 1 deletion test/unit/duplicate-winner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down Expand Up @@ -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)", () => {
Expand Down