From 954036f3dd9d6febe8701eb376ab24981f8c10fa Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:32:04 -0700 Subject: [PATCH] fix(regate-sweep): prevent oldest-first starvation --- src/settings/agent-sweep.ts | 85 ++++++----- test/unit/agent-sweep.test.ts | 280 +++++++++++++++++++++++++++------- 2 files changed, 265 insertions(+), 100 deletions(-) diff --git a/src/settings/agent-sweep.ts b/src/settings/agent-sweep.ts index 1b4772c09a..e71ffede59 100644 --- a/src/settings/agent-sweep.ts +++ b/src/settings/agent-sweep.ts @@ -54,15 +54,13 @@ export type RegateSweepOrderMode = "staleness" | "oldest-first"; * `oldest-first`'s sort key alone cannot advance past an already-dispatched PR — without something else, the * same oldest `max` PRs would recur every sweep forever. * - * `oldest-first` instead excludes whichever PR(s) hold the CURRENT candidate pool's single most-recent - * `lastRegatedAt` value — i.e. whichever PR(s) this repo's immediately preceding sweep dispatched (every - * candidate in one dispatch is stamped with the exact same `lastRegatedAt`, see markPullRequestsRegated), - * deferring them until an even-newer dispatch supersedes them. This is a RELATIVE comparison within the pool - * on every call, never an absolute time window against `now` — so, unlike a fixed freshness window, it holds - * regardless of how far apart consecutive sweeps actually run (a delayed/backpressured sweep, or dry-run/pause - * suppressing GitHub's `updatedAt` entirely), giving `oldest-first` the same timing-independent, full-coverage- - * in-ceil(open/max)-sweeps guarantee `staleness` has. 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. + * `oldest-first` therefore drains never-regated PRs before cycling already-regated ones: while any eligible + * non-priority PR lacks `lastRegatedAt`, the candidate pool is narrowed to those never-regated PRs (plus any + * priority repairs). That preserves deterministic creation-order backlog recovery and covers a large initial + * backlog in ceil(open/max) sweeps. Once every eligible PR has been swept at least once, it falls back to the + * 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. */ export function selectRegateCandidates(input: { pulls: PullRequestRecord[]; @@ -77,7 +75,9 @@ export function selectRegateCandidates(input: { const max = input.max ?? SWEEP_MAX_PRS; const orderMode = input.orderMode ?? "staleness"; const nowMs = Date.parse(input.now); - const freshCutoff = Number.isFinite(nowMs) ? nowMs - freshnessWindowMs : Number.NaN; + const freshCutoff = Number.isFinite(nowMs) + ? nowMs - freshnessWindowMs + : Number.NaN; // Don't-race-webhook guard: a PR whose GitHub `updatedAt` is within the window was almost certainly just gated // by its webhook. A missing/unparseable timestamp = not recently touched = eligible (epoch). const webhookFreshness = (pr: PullRequestRecord): number => { @@ -88,7 +88,9 @@ export function selectRegateCandidates(input: { // PR sorts maximally stale and is picked first; ties broken by PR number. This is the convergence key — it // advances on every sweep regardless of whether GitHub writes are suppressed. const regateProgress = (pr: PullRequestRecord): number => { - const regated = pr.lastRegatedAt ? Date.parse(pr.lastRegatedAt) : Number.NaN; + const regated = pr.lastRegatedAt + ? Date.parse(pr.lastRegatedAt) + : Number.NaN; if (Number.isFinite(regated)) return regated; const created = pr.createdAt ? Date.parse(pr.createdAt) : Number.NaN; return Number.isFinite(created) ? created : 0; @@ -101,7 +103,6 @@ export function selectRegateCandidates(input: { const created = pr.createdAt ? Date.parse(pr.createdAt) : Number.NaN; return Number.isFinite(created) ? created : 0; }; - const orderKey = orderMode === "oldest-first" ? creationOrder : regateProgress; const priorityPullNumbers = input.priorityPullNumbers instanceof Set ? input.priorityPullNumbers @@ -111,40 +112,36 @@ export function selectRegateCandidates(input: { const eligible = input.pulls .filter((pr) => pr.state === "open" && !pr.isDraft) .filter((pr) => { - if ( - input.priorityBypassesFreshness && - priorityPullNumbers.has(pr.number) - ) + if (input.priorityBypassesFreshness && priorityPullNumbers.has(pr.number)) return true; if (!Number.isFinite(freshCutoff)) return true; return webhookFreshness(pr) <= freshCutoff; }); - // Most-recent-dispatch exclusion (#3815, "oldest-first" mode only — see the doc comment above): find the - // single latest lastRegatedAt value across the currently-eligible pool, then defer whichever PR(s) hold it. - // A pool with no lastRegatedAt at all (nothing ever dispatched) has no most-recent value, so nothing defers. - let mostRecentRegatedMs = Number.NEGATIVE_INFINITY; - if (orderMode === "oldest-first") { - for (const pr of eligible) { - const regated = pr.lastRegatedAt ? Date.parse(pr.lastRegatedAt) : Number.NaN; - if (Number.isFinite(regated) && regated > mostRecentRegatedMs) mostRecentRegatedMs = regated; - } - } - // Only called (via deferredCount/the final filter below) when orderMode is already "oldest-first" — the - // caller gates on that, so this never needs its own mode check. - const isMostRecentlyDispatched = (pr: PullRequestRecord): boolean => { - if (input.priorityBypassesFreshness && priorityPullNumbers.has(pr.number)) return false; - const regated = pr.lastRegatedAt ? Date.parse(pr.lastRegatedAt) : Number.NaN; - return Number.isFinite(regated) && regated === mostRecentRegatedMs; + const hasBeenRegated = (pr: PullRequestRecord): boolean => { + const regated = pr.lastRegatedAt + ? Date.parse(pr.lastRegatedAt) + : Number.NaN; + return Number.isFinite(regated); }; - const deferredCount = orderMode === "oldest-first" ? eligible.filter(isMostRecentlyDispatched).length : 0; - // Starvation guard: only actually defer when doing so still leaves at least one candidate. If EVERY eligible - // PR ties on the same lastRegatedAt (e.g. a small backlog whose entire open-PR count fits in one sweep, so - // every PR was dispatched together last time), deferring all of them would starve the sweep forever with - // nothing better to fall back to — proceed with the full pool instead. - const shouldDefer = deferredCount > 0 && deferredCount < eligible.length; - return eligible - .filter((pr) => !shouldDefer || !isMostRecentlyDispatched(pr)) - .sort((a, b) => repairPriority(a) - repairPriority(b) || orderKey(a) - orderKey(b) || a.number - b.number) + const hasRepairPriority = (pr: PullRequestRecord): boolean => + priorityPullNumbers.has(pr.number); + const oldestFirstInitialDrain = + orderMode === "oldest-first" && + eligible.some((pr) => !hasBeenRegated(pr) && !hasRepairPriority(pr)); + const candidates = oldestFirstInitialDrain + ? eligible.filter((pr) => !hasBeenRegated(pr) || hasRepairPriority(pr)) + : eligible; + const orderKey = + orderMode === "oldest-first" && oldestFirstInitialDrain + ? creationOrder + : regateProgress; + return candidates + .sort( + (a, b) => + repairPriority(a) - repairPriority(b) || + orderKey(a) - orderKey(b) || + a.number - b.number, + ) .slice(0, Math.max(0, max)); } @@ -156,7 +153,11 @@ export function selectRegateCandidates(input: { * drains, so without this guard a second full sweep would pile duplicate per-PR jobs onto the unfinished one. A * missing/never-regated/unparseable timestamp means no sweep is in flight (proceed). Pure + deterministic. */ -export function isRegateSweepDraining(latestRegatedAt: string | null | undefined, now: string, windowMs: number = SWEEP_FRESHNESS_MS): boolean { +export function isRegateSweepDraining( + latestRegatedAt: string | null | undefined, + now: string, + windowMs: number = SWEEP_FRESHNESS_MS, +): boolean { if (!latestRegatedAt) return false; const stampedMs = Date.parse(latestRegatedAt); const nowMs = Date.parse(now); diff --git a/test/unit/agent-sweep.test.ts b/test/unit/agent-sweep.test.ts index 05e88cffd6..71d8e53e64 100644 --- a/test/unit/agent-sweep.test.ts +++ b/test/unit/agent-sweep.test.ts @@ -1,12 +1,20 @@ import { describe, expect, it } from "vitest"; -import { SWEEP_FRESHNESS_MS, SWEEP_MAX_PRS, isRegateSweepDraining, selectRegateCandidates } from "../../src/settings/agent-sweep"; +import { + SWEEP_FRESHNESS_MS, + SWEEP_MAX_PRS, + isRegateSweepDraining, + selectRegateCandidates, +} from "../../src/settings/agent-sweep"; import type { PullRequestRecord } from "../../src/types"; const NOW = "2026-06-17T12:00:00.000Z"; const nowMs = Date.parse(NOW); -const minutesAgo = (m: number): string => new Date(nowMs - m * 60 * 1000).toISOString(); +const minutesAgo = (m: number): string => + new Date(nowMs - m * 60 * 1000).toISOString(); -function pr(overrides: Partial & { number: number }): PullRequestRecord { +function pr( + overrides: Partial & { number: number }, +): PullRequestRecord { return { repoFullName: "owner/repo", title: `PR ${overrides.number}`, @@ -20,32 +28,58 @@ function pr(overrides: Partial & { number: number }): PullReq describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { describe("don't-race-webhook freshness guard (GitHub updatedAt)", () => { it("drops PRs whose GitHub updatedAt is within the freshness window (a webhook is gating them)", () => { - const pulls = [pr({ number: 1, updatedAt: minutesAgo(1) }), pr({ number: 2, updatedAt: minutesAgo(120) })]; + const pulls = [ + pr({ number: 1, updatedAt: minutesAgo(1) }), + pr({ number: 2, updatedAt: minutesAgo(120) }), + ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([2]); // #1 updated 1m ago is inside the 2-min window }); it("treats a missing updatedAt as NOT recently touched (eligible, never starved by the freshness guard)", () => { - const pulls = [pr({ number: 1, updatedAt: minutesAgo(1) }), pr({ number: 2 })]; + const pulls = [ + pr({ number: 1, updatedAt: minutesAgo(1) }), + pr({ number: 2 }), + ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([2]); // #1 fresh → dropped; #2 has no updatedAt → eligible }); it("keeps a PR whose lastRegatedAt is old but whose updatedAt is fresh OUT (the guard wins over the sort key)", () => { - const pulls = [pr({ number: 1, updatedAt: minutesAgo(1), lastRegatedAt: minutesAgo(999) })]; + const pulls = [ + pr({ + number: 1, + updatedAt: minutesAgo(1), + lastRegatedAt: minutesAgo(999), + }), + ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([]); // stalest by re-gate, but a webhook just touched it → skip }); it("live case: when updatedAt and lastRegatedAt move together, the PR is eligible once outside the window (not double-excluded)", () => { - const pulls = [pr({ number: 1, updatedAt: minutesAgo(120), lastRegatedAt: minutesAgo(120) })]; + const pulls = [ + pr({ + number: 1, + updatedAt: minutesAgo(120), + lastRegatedAt: minutesAgo(120), + }), + ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([1]); // both old → freshness allows it, re-gate orders it }); it("keeps every open non-draft PR when `now` is unparseable (no freshness cutoff possible)", () => { - const pulls = [pr({ number: 1, createdAt: minutesAgo(5) }), pr({ number: 2, createdAt: minutesAgo(600) }), pr({ number: 3, isDraft: true })]; - const picked = selectRegateCandidates({ pulls, now: "not-a-date", freshnessWindowMs: 30 * 60 * 1000 }); + const pulls = [ + pr({ number: 1, createdAt: minutesAgo(5) }), + pr({ number: 2, createdAt: minutesAgo(600) }), + pr({ number: 3, isDraft: true }), + ]; + const picked = selectRegateCandidates({ + pulls, + now: "not-a-date", + freshnessWindowMs: 30 * 60 * 1000, + }); expect(picked.map((p) => p.number)).toEqual([2, 1]); // drafts still excluded; both non-draft kept, stalest-created first }); }); @@ -55,15 +89,26 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { // #1 was re-gated recently but created long ago; #2 was re-gated long ago but created recently. The re-gate // marker — not createdAt — drives the order, so #2 (stalest re-gate) comes first. const pulls = [ - pr({ number: 1, lastRegatedAt: minutesAgo(10), createdAt: minutesAgo(1000) }), - pr({ number: 2, lastRegatedAt: minutesAgo(100), createdAt: minutesAgo(1) }), + pr({ + number: 1, + lastRegatedAt: minutesAgo(10), + createdAt: minutesAgo(1000), + }), + pr({ + number: 2, + lastRegatedAt: minutesAgo(100), + createdAt: minutesAgo(1), + }), ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([2, 1]); }); it("INVARIANT arm (ii): falls back to createdAt when lastRegatedAt is absent — oldest-created sorts first", () => { - const pulls = [pr({ number: 1, createdAt: minutesAgo(10) }), pr({ number: 2, createdAt: minutesAgo(600) })]; + const pulls = [ + pr({ number: 1, createdAt: minutesAgo(10) }), + pr({ number: 2, createdAt: minutesAgo(600) }), + ]; const picked = selectRegateCandidates({ pulls, now: NOW }); expect(picked.map((p) => p.number)).toEqual([2, 1]); // no lastRegatedAt on either → createdAt orders them }); @@ -76,7 +121,11 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { it("a never-regated PR (lastRegatedAt absent) outranks a just-regated one — the property that makes the sweep converge", () => { const pulls = [ - pr({ number: 1, lastRegatedAt: minutesAgo(1), createdAt: minutesAgo(1000) }), // just re-gated → freshest + pr({ + number: 1, + lastRegatedAt: minutesAgo(1), + createdAt: minutesAgo(1000), + }), // just re-gated → freshest pr({ number: 2, createdAt: minutesAgo(50) }), // never re-gated → its createdAt (50m) is staler than #1's re-gate (1m) ]; const picked = selectRegateCandidates({ pulls, now: NOW }); @@ -110,8 +159,16 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { it("REGRESSION (repair priority): priority repairs can bypass webhook freshness when the current Gate check is missing", () => { const pulls = [ - pr({ number: 1, updatedAt: minutesAgo(1), lastRegatedAt: minutesAgo(900) }), - pr({ number: 2, updatedAt: minutesAgo(120), lastRegatedAt: minutesAgo(800) }), + pr({ + number: 1, + updatedAt: minutesAgo(1), + lastRegatedAt: minutesAgo(900), + }), + pr({ + number: 2, + updatedAt: minutesAgo(120), + lastRegatedAt: minutesAgo(800), + }), ]; const picked = selectRegateCandidates({ pulls, @@ -141,14 +198,23 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { // the live cap so it stays correct as SWEEP_MAX_PRS is tuned for the REST budget (#audit-rate-headroom). const open = SWEEP_MAX_PRS * 2; // exactly two full sweeps' worth of stale open PRs const sweepsNeeded = Math.ceil(open / SWEEP_MAX_PRS); - const pulls = Array.from({ length: open }, (_, i) => pr({ number: i + 1, createdAt: minutesAgo(1000 - i), updatedAt: minutesAgo(1000) })); + const pulls = Array.from({ length: open }, (_, i) => + pr({ + number: i + 1, + createdAt: minutesAgo(1000 - i), + updatedAt: minutesAgo(1000), + }), + ); const stampedAt = new Map(); const covered = new Set(); let sweepNow = nowMs; for (let sweep = 0; sweep < sweepsNeeded; sweep++) { sweepNow += 5 * 60 * 1000; // each sweep runs ~5 min later (outside the freshness window) const now = new Date(sweepNow).toISOString(); - const view = pulls.map((p) => ({ ...p, lastRegatedAt: stampedAt.get(p.number) ?? p.lastRegatedAt })); + const view = pulls.map((p) => ({ + ...p, + lastRegatedAt: stampedAt.get(p.number) ?? p.lastRegatedAt, + })); const picked = selectRegateCandidates({ pulls: view, now }); expect(picked.length).toBe(SWEEP_MAX_PRS); // each sweep fills the cap until the queue is drained for (const p of picked) { @@ -163,46 +229,87 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { it("defaults: freshness window is two minutes and the cap is bounded for the shared REST budget (#audit-rate-headroom)", () => { expect(SWEEP_FRESHNESS_MS).toBe(2 * 60 * 1000); expect(SWEEP_MAX_PRS).toBe(3); // 3 × 3 repos × 9 GETs × 30 ticks/hr ≈ 2.4k/hr, leaving headroom for webhooks - const pulls = Array.from({ length: 40 }, (_, i) => pr({ number: i + 1, createdAt: minutesAgo(120 + i) })); - expect(selectRegateCandidates({ pulls, now: NOW })).toHaveLength(SWEEP_MAX_PRS); + const pulls = Array.from({ length: 40 }, (_, i) => + pr({ number: i + 1, createdAt: minutesAgo(120 + i) }), + ); + expect(selectRegateCandidates({ pulls, now: NOW })).toHaveLength( + SWEEP_MAX_PRS, + ); }); describe("orderMode: oldest-first (#3815)", () => { it("orders by createdAt ascending when neither PR has ever been regated", () => { - const pulls = [pr({ number: 1, createdAt: minutesAgo(1000) }), pr({ number: 2, createdAt: minutesAgo(1) })]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first" }); + const pulls = [ + pr({ number: 1, createdAt: minutesAgo(1000) }), + pr({ number: 2, createdAt: minutesAgo(1) }), + ]; + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (oldest-created) first, unlike staleness (which has no history here either, so both modes agree in this case) }); - it("defers whichever PR holds the pool's single most-recent lastRegatedAt, regardless of its own createdAt age", () => { + it("orders by re-gate staleness after the oldest-first initial drain is complete", () => { // #1 was re-gated most recently (10m ago) despite being the OLDEST-created PR by far; #2 was re-gated - // longer ago (100m) despite being the NEWEST-created. Naively sorting by pure createdAt would put #1 - // first every time even though it was just dispatched — oldest-first instead defers whichever PR holds - // the pool's freshest lastRegatedAt (here, #1), leaving #2 as the only eligible candidate this tick. + // longer ago (100m) despite being the NEWEST-created. Once every eligible PR has a lastRegatedAt stamp, + // oldest-first uses re-gate staleness so ongoing sweeps keep converging instead of pinning old PRs. const pulls = [ - pr({ number: 1, lastRegatedAt: minutesAgo(10), createdAt: minutesAgo(1000) }), - pr({ number: 2, lastRegatedAt: minutesAgo(100), createdAt: minutesAgo(1) }), + pr({ + number: 1, + lastRegatedAt: minutesAgo(10), + createdAt: minutesAgo(1000), + }), + pr({ + number: 2, + lastRegatedAt: minutesAgo(100), + createdAt: minutesAgo(1), + }), ]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first" }); - expect(picked.map((p) => p.number)).toEqual([2]); + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); + expect(picked.map((p) => p.number)).toEqual([2, 1]); }); it("does not starve the sweep when EVERY eligible PR ties on the same lastRegatedAt (a fully-covered small backlog)", () => { // Both PRs were dispatched together in the exact same prior sweep (identical lastRegatedAt stamp — see - // markPullRequestsRegated, which stamps every candidate in one UPDATE). Deferring "whichever holds the - // most recent value" would defer BOTH here, returning nothing — the starvation guard falls back to the - // full pool instead, since there is nothing better to wait for. + // markPullRequestsRegated, which stamps every candidate in one UPDATE). Since the initial drain is complete, + // oldest-first falls back to the full staleness pool rather than returning nothing. const pulls = [ - pr({ number: 1, createdAt: minutesAgo(1000), lastRegatedAt: minutesAgo(10) }), - pr({ number: 2, createdAt: minutesAgo(500), lastRegatedAt: minutesAgo(10) }), + pr({ + number: 1, + createdAt: minutesAgo(1000), + lastRegatedAt: minutesAgo(10), + }), + pr({ + number: 2, + createdAt: minutesAgo(500), + lastRegatedAt: minutesAgo(10), + }), ]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first" }); + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); expect(picked.map((p) => p.number)).toEqual([1, 2]); // both tie → guard proceeds with the full pool, oldest first }); it("falls back to the epoch (sorts as oldest) when createdAt is absent, tie broken by PR number", () => { - const pulls = [pr({ number: 9, createdAt: minutesAgo(5) }), pr({ number: 4 }), pr({ number: 7 })]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first" }); + const pulls = [ + pr({ number: 9, createdAt: minutesAgo(5) }), + pr({ number: 4 }), + pr({ number: 7 }), + ]; + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); expect(picked.map((p) => p.number)).toEqual([4, 7, 9]); // #4 and #7 (no createdAt) tie at epoch, then #9 }); @@ -212,7 +319,12 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { pr({ number: 2, createdAt: minutesAgo(600) }), pr({ number: 3, createdAt: minutesAgo(300) }), ]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first", max: 2 }); + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + max: 2, + }); expect(picked.map((p) => p.number)).toEqual([2, 3]); // oldest-created (600m), then 300m; 120m dropped by cap }); @@ -232,12 +344,15 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (priority) wins despite being newest-created }); - it("REGRESSION (repair priority): a priority repair bypasses the most-recent-dispatch deferral too", () => { - // #1 is a priority repair AND happens to hold the pool's single most-recent lastRegatedAt (it was just - // dispatched). Without priorityBypassesFreshness it would be deferred like any other PR; with it, the - // repair still gets included this tick. + 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. const pulls = [ - pr({ number: 1, createdAt: minutesAgo(10), lastRegatedAt: minutesAgo(1) }), + pr({ + number: 1, + createdAt: minutesAgo(10), + lastRegatedAt: minutesAgo(1), + }), pr({ number: 2, createdAt: minutesAgo(900) }), ]; const picked = selectRegateCandidates({ @@ -247,36 +362,59 @@ 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 being the most-recently-dispatched + expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (priority) included despite already having a regate stamp }); - it("a just-regated PR is excluded by the most-recent-dispatch check, not re-selected forever by its fixed createdAt", () => { - // createdAt never changes, so without the most-recent-dispatch exclusion #1 (oldest-created) would recur - // every sweep even after being dispatched. #1 holds the pool's only lastRegatedAt value → it is deferred; - // #2 (never regated) becomes the sole eligible candidate this tick. - const pulls = [pr({ number: 1, createdAt: minutesAgo(1000), lastRegatedAt: minutesAgo(1) }), pr({ number: 2, createdAt: minutesAgo(500) })]; - const picked = selectRegateCandidates({ pulls, now: NOW, orderMode: "oldest-first" }); - expect(picked.map((p) => p.number)).toEqual([2]); // #1 just regated → deferred; #2 is the next-oldest eligible + it("a just-regated PR is excluded while any never-regated PR remains, not re-selected forever by fixed createdAt", () => { + // createdAt never changes, so without the initial-drain narrowing #1 (oldest-created) would recur every + // sweep even after being dispatched. #2 has never been regated, so it becomes the sole ordinary candidate. + const pulls = [ + pr({ + number: 1, + createdAt: minutesAgo(1000), + lastRegatedAt: minutesAgo(1), + }), + pr({ number: 2, createdAt: minutesAgo(500) }), + ]; + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); + expect(picked.map((p) => p.number)).toEqual([2]); // #1 already regated → skipped; #2 is the next-oldest never-regated PR }); it("REGRESSION (convergence): ceil(open/cap) sweeps with all GitHub writes suppressed cover ALL open PRs under oldest-first too", () => { // Mirrors the staleness-mode convergence test above: dry-run/paused world where GitHub updatedAt never // moves, AND sweeps are spaced 5 minutes apart — well past any fixed freshness window, proving this // does NOT rely on wall-clock timing. createdAt is fixed too (by construction), so convergence for - // oldest-first depends entirely on the most-recent-dispatch exclusion advancing each round: after a - // sweep, its dispatched PRs share the freshest lastRegatedAt and are deferred, letting the next-oldest - // batch surface — without it this would loop forever. - const open = SWEEP_MAX_PRS * 2; + // oldest-first depends on never-regated PRs staying ahead of already-regated PRs during the initial drain: + // after each sweep, its dispatched PRs are stamped, so the next never-regated creation-order batch surfaces + // — without that, backlogs larger than two batches loop over the oldest stamped batches forever. + const open = SWEEP_MAX_PRS * 3; const sweepsNeeded = Math.ceil(open / SWEEP_MAX_PRS); - const pulls = Array.from({ length: open }, (_, i) => pr({ number: i + 1, createdAt: minutesAgo(1000 - i), updatedAt: minutesAgo(1000) })); + const pulls = Array.from({ length: open }, (_, i) => + pr({ + number: i + 1, + createdAt: minutesAgo(1000 - i), + updatedAt: minutesAgo(1000), + }), + ); const stampedAt = new Map(); const covered = new Set(); let sweepNow = nowMs; for (let sweep = 0; sweep < sweepsNeeded; sweep++) { sweepNow += 5 * 60 * 1000; const now = new Date(sweepNow).toISOString(); - const view = pulls.map((p) => ({ ...p, lastRegatedAt: stampedAt.get(p.number) ?? p.lastRegatedAt })); - const picked = selectRegateCandidates({ pulls: view, now, orderMode: "oldest-first" }); + const view = pulls.map((p) => ({ + ...p, + lastRegatedAt: stampedAt.get(p.number) ?? p.lastRegatedAt, + })); + const picked = selectRegateCandidates({ + pulls: view, + now, + orderMode: "oldest-first", + }); expect(picked.length).toBe(SWEEP_MAX_PRS); for (const p of picked) { expect(covered.has(p.number)).toBe(false); @@ -286,6 +424,32 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { } expect(covered.size).toBe(open); }); + + it("falls back to re-gate staleness once every eligible PR has been swept once", () => { + const pulls = [ + pr({ + number: 1, + createdAt: minutesAgo(1000), + lastRegatedAt: minutesAgo(5), + }), + pr({ + number: 2, + createdAt: minutesAgo(900), + lastRegatedAt: minutesAgo(500), + }), + pr({ + number: 3, + createdAt: minutesAgo(800), + lastRegatedAt: minutesAgo(50), + }), + ]; + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); + expect(picked.map((p) => p.number)).toEqual([2, 3, 1]); + }); }); });