From fe703c03f551a889943e7e406b46899370b12674 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 03:48:46 -0700 Subject: [PATCH] fix(review): stop the re-gate sweep from reordering PRs by repair status selectRegateCandidates sorted a repair-flagged PR (surfaceRepairPriorityPullNumbers -- missing public surface or current Gate check) ahead of every other candidate, regardless of staleness or creation order. With a mixed backlog of repaired and ordinary PRs, this let a newer PR needing repair cut ahead of older PRs that had merely gone stale -- observed live as PRs dispatching out of their creation/ staleness order. Repair status now only affects eligibility (bypassing the freshness guard, staying in the oldest-first pool despite already having a regate stamp) -- never final order. Every eligible PR, repaired or not, is ordered by the same staleness/ creation-order key plus PR-number tiebreak, so a sweep processes its queue in one deterministic order every time. --- src/settings/agent-sweep.ts | 20 ++++++++++++-------- test/unit/agent-sweep.test.ts | 23 +++++++++++++++++------ test/unit/queue.test.ts | 12 ++++++++++-- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/settings/agent-sweep.ts b/src/settings/agent-sweep.ts index 8979198217..5392088707 100644 --- a/src/settings/agent-sweep.ts +++ b/src/settings/agent-sweep.ts @@ -79,6 +79,17 @@ export type RegateSweepOrderMode = "staleness" | "oldest-first"; * staleness key so continued periodic re-gating keeps converging instead of pinning the oldest PRs forever. * Selection-time only: real-time webhook-driven review is not gated by this sort and can still process any PR * out of order at any moment. + * + * `priorityPullNumbers` (outage repair -- surfaceRepairPriorityPullNumbers, processors.ts) affects ELIGIBILITY + * only, never final order (#selfhost-fifo-ordering): a repair candidate bypasses the freshness guard + * (`priorityBypassesFreshness`) and stays in the oldest-first pool even once it already has a `lastRegatedAt` + * stamp (`hasRepairPriority` in the pool filter below) -- but it is NOT sorted ahead of the rest of the queue. + * An earlier revision additionally sorted repair candidates first, which let a newer PR needing repair (e.g. + * opened during an extended pause, so it has never published anything) jump ahead of older PRs that merely + * went stale -- observed live as PRs dispatching out of their creation/staleness order ("spraying") whenever a + * repo had a mixed backlog of repaired and ordinary candidates. Every eligible PR -- repair or not -- is now + * ordered by the SAME `orderKey` (+ PR-number tiebreak), so a sweep processes its queue in one deterministic + * order every time, regardless of how many candidates happen to need repair. */ export function selectRegateCandidates(input: { pulls: PullRequestRecord[]; @@ -125,8 +136,6 @@ export function selectRegateCandidates(input: { input.priorityPullNumbers instanceof Set ? input.priorityPullNumbers : new Set(input.priorityPullNumbers ?? []); - const repairPriority = (pr: PullRequestRecord): number => - priorityPullNumbers.has(pr.number) ? 0 : 1; const eligible = input.pulls .filter((pr) => pr.state === "open" && !pr.isDraft) .filter((pr) => { @@ -154,12 +163,7 @@ export function selectRegateCandidates(input: { ? creationOrder : regateProgress; return candidates - .sort( - (a, b) => - repairPriority(a) - repairPriority(b) || - orderKey(a) - orderKey(b) || - a.number - b.number, - ) + .sort((a, b) => orderKey(a) - orderKey(b) || a.number - b.number) .slice(0, Math.max(0, max)); } diff --git a/test/unit/agent-sweep.test.ts b/test/unit/agent-sweep.test.ts index 71d8e53e64..974e91102a 100644 --- a/test/unit/agent-sweep.test.ts +++ b/test/unit/agent-sweep.test.ts @@ -142,7 +142,12 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([2, 3]); // stalest re-gate (600m), then 300m; 120m dropped by cap }); - it("REGRESSION (repair priority): missing public surfaces are selected before ordinary stale PRs", () => { + it("#selfhost-fifo-ordering: a repair-flagged PR does NOT jump ahead of staler ordinary PRs — same orderKey for everyone", () => { + // #2 has a missing public surface (surfaceRepairPriorityPullNumbers would flag it) but is also the LEAST + // stale of the three by lastRegatedAt. An earlier revision sorted repair candidates first regardless of + // staleness — this pinned #2 ahead of #1/#3 and was observed live as PRs dispatching out of their + // creation/staleness order ("spraying") whenever a repo had a mixed repair/ordinary backlog. Repair status + // must only affect eligibility (see the freshness-bypass + oldest-first-pool tests below), never order. const pulls = [ pr({ number: 1, lastRegatedAt: minutesAgo(900) }), pr({ number: 2, lastRegatedAt: minutesAgo(10) }), @@ -154,7 +159,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { max: 2, priorityPullNumbers: new Set([2]), }); - expect(picked.map((p) => p.number)).toEqual([2, 1]); + expect(picked.map((p) => p.number)).toEqual([1, 3]); // stalest-by-regate first, same as with no priority set at all }); it("REGRESSION (repair priority): priority repairs can bypass webhook freshness when the current Gate check is missing", () => { @@ -328,7 +333,11 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([2, 3]); // oldest-created (600m), then 300m; 120m dropped by cap }); - it("REGRESSION (repair priority): priority repairs still sort before ordinary oldest-first candidates", () => { + it("#selfhost-fifo-ordering: a repair-flagged PR does NOT jump ahead of older oldest-first candidates", () => { + // #1 is flagged as a repair (e.g. opened during an extended agent pause, so it never published anything) + // but is by far the NEWEST-created of the three. It stays eligible (see the initial-drain test below) but + // must not cut ahead of #2/#3, which were created long before it — creation order is the same for every + // candidate regardless of repair status. const pulls = [ pr({ number: 1, createdAt: minutesAgo(10) }), pr({ number: 2, createdAt: minutesAgo(900) }), @@ -341,12 +350,14 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { max: 2, priorityPullNumbers: new Set([1]), }); - expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (priority) wins despite being newest-created + expect(picked.map((p) => p.number)).toEqual([2, 3]); // oldest-created first, same as with no priority set at all; #1 (newest) dropped by the cap }); it("REGRESSION (repair priority): a priority repair stays eligible during the oldest-first initial drain", () => { // #1 is a priority repair AND has already been regated, while #2 is still in the never-regated initial - // drain. Priority work remains eligible so a repair can preempt the ordinary creation-order backlog. + // drain. Priority work remains ELIGIBLE (not excluded by the initial-drain pool narrowing just because it + // already has a regate stamp) — but, per #selfhost-fifo-ordering, it no longer preempts the ordinary + // creation-order backlog: #2 is older-created than #1, so #2 still sorts first. const pulls = [ pr({ number: 1, @@ -362,7 +373,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { priorityPullNumbers: new Set([1]), priorityBypassesFreshness: true, }); - expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (priority) included despite already having a regate stamp + expect(picked.map((p) => p.number)).toEqual([2, 1]); // #1 (priority) included despite already having a regate stamp, but #2 (older-created) still sorts first }); it("a just-regated PR is excluded while any never-regated PR remains, not re-selected forever by fixed createdAt", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index bb0710f4c1..28c860d7a8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7245,7 +7245,7 @@ describe("queue processors", () => { expect(typeof after?.last_regated_at).toBe("string"); // stamped via a D1 write at dispatch — convergence does not need a GitHub write }); - it("agent re-gate sweep prioritizes PRs missing the current Gate check even when their surface marker is current", async () => { + it("agent re-gate sweep processes strict staleness order even when a PR is missing its current Gate check (#selfhost-fifo-ordering)", async () => { const sent: import("../../src/types").JobMessage[] = []; const env = createTestEnv({ JOBS: { async send(m: import("../../src/types").JobMessage) { sent.push(m); } } as unknown as Queue }); await upsertInstallation(env, { action: "created", installation: { id: 9400, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); @@ -7286,7 +7286,15 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-sweep", requestedBy: "test", repoFullName: "owner/agent-repo" }); const fanned = sent.filter((job) => job.type === "agent-regate-pr"); - expect(fanned.map((job) => (job as Extract).prNumber)).toEqual([2, 1, 3]); + // PR2 is missing its current Gate check (surfaceRepairPriorityPullNumbers would flag it as a repair + // candidate) but is also the LEAST stale by lastRegatedAt (10 min ago vs. 23-25h for the others). An earlier + // revision sorted repair candidates first regardless of staleness, jumping PR2 to the front of this batch -- + // that let a PR needing repair cut ahead of older PRs that merely went stale, observed live as PRs + // dispatching out of order ("spraying") whenever a repo had a mixed repair/ordinary backlog. Repair status + // now only affects ELIGIBILITY (staying in the pool, bypassing the freshness guard), never final order, so + // PR2 takes its rightful (last, since it's the freshest-regated) place and is dropped by the max:3 cap this + // round -- same as it would be with no repair flag at all. + expect(fanned.map((job) => (job as Extract).prNumber)).toEqual([1, 3, 4]); }); it("REGRESSION (#3815): regateSweepOrderMode 'oldest-first' fans out per-PR jobs in creation order with a monotonic delaySeconds stagger", async () => {