From 14df16007ffbb337ac32e156228343b3e5f53a63 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 24 Jul 2026 13:33:16 -0700 Subject: [PATCH] fix(ci): log every pr-ci-sweeper classification and scan by creation time (#113397) --- scripts/github/pr-ci-sweeper.mjs | 84 +++++++++++----- test/scripts/pr-ci-sweeper.test.ts | 150 +++++++++++++++++++++++++++-- 2 files changed, 205 insertions(+), 29 deletions(-) diff --git a/scripts/github/pr-ci-sweeper.mjs b/scripts/github/pr-ci-sweeper.mjs index 32bb6757f75c..44f7691ca78e 100644 --- a/scripts/github/pr-ci-sweeper.mjs +++ b/scripts/github/pr-ci-sweeper.mjs @@ -5,6 +5,12 @@ // attached to the PR head and reruns their PR-event workflows without disturbing // auto-merge. The workflow authenticates with a GitHub App token because // GITHUB_TOKEN-authored events do not trigger new workflow runs. +// +// Observability contract: the re-fire lane logs a decision for every scanned +// PR, including ci-attached skips with the attached run ids. A PR absent from +// the log was outside the scan (created before the lookback), never silently +// classified — silent skips previously made a correctly-skipped PR look like a +// missed dropped-CI PR (2026-07-24, #113355). const CI_WORKFLOW_FILE = "ci.yml"; const LOOKBACK_MS = 24 * 60 * 60 * 1000; @@ -97,6 +103,24 @@ export function classifyRunForRevive({ run, prCreatedAt, prHeadBranch, repoFullN return { action: "revive", reason: "cancelled-pr-event-run" }; } +async function listRecentOpenPrs({ github, owner, repo, now }) { + // Sort by creation time, which is immutable: updated-sort pagination shifts + // items between page fetches whenever bot activity bumps a PR mid-scan, and a + // boundary PR can silently drop out of the listing. Both lanes only act on + // PRs created within the lookback, so stop fetching once a page crosses that + // horizon instead of listing every open PR (~2900 at last count, hourly). + return await github.paginate( + github.rest.pulls.list, + { owner, repo, state: "open", sort: "created", direction: "desc", per_page: 100 }, + (response, done) => { + if (response.data.some((item) => now - Date.parse(item.created_at) > LOOKBACK_MS)) { + done(); + } + return response.data; + }, + ); +} + async function listPullRequestCiRuns({ github, owner, repo, headSha }) { // Manual dispatches or other events against the same SHA neither prove nor // repair the dropped pull_request run; judge only pull_request-event runs, @@ -219,25 +243,13 @@ export async function runPrCiSweeper({ const results = []; let refires = 0; let revives = 0; - const openPrs = await github.paginate(github.rest.pulls.list, { - owner, - repo, - state: "open", - sort: "updated", - direction: "desc", - per_page: 100, - }); + const openPrs = await listRecentOpenPrs({ github, owner, repo, now }); const seenRunIds = new Set(); reviveLane: for (const listed of openPrs) { - if (now - Date.parse(listed.updated_at) > LOOKBACK_MS) { + if (now - Date.parse(listed.created_at) > LOOKBACK_MS) { break; } - if ( - listed.draft || - !listed.auto_merge || - now - Date.parse(listed.created_at) > LOOKBACK_MS || - now - Date.parse(listed.updated_at) < MIN_QUIET_MS - ) { + if (listed.draft || !listed.auto_merge || now - Date.parse(listed.updated_at) < MIN_QUIET_MS) { continue; } const checks = await listLatestChecksForHead({ @@ -377,24 +389,52 @@ export async function runPrCiSweeper({ } } for (const listed of openPrs) { - if (now - Date.parse(listed.updated_at) > LOOKBACK_MS) { - break; - } - if (refires >= MAX_REFIRES_PER_SWEEP) { - core.info(`pr-ci-sweeper: per-sweep re-fire cap (${MAX_REFIRES_PER_SWEEP}) reached`); + if (now - Date.parse(listed.created_at) > LOOKBACK_MS) { break; } if (listed.draft) { + results.push({ + number: listed.number, + sha: listed.head.sha.slice(0, 12), + action: "skip", + reason: "draft", + }); + core.info(`pr-ci-sweeper: skip #${listed.number} (draft)`); continue; } const ciRuns = await listPullRequestCiRuns({ github, owner, repo, headSha: listed.head.sha }); - if (ciRuns.some((run) => run.conclusion !== "startup_failure")) { + // Classify on cheap list data first so most PRs (usually ci-attached) skip + // without the authoritative pulls.get + close-history reads, but still log + // the decision with the attached run ids as evidence: the silent skip here + // previously made "sweeper judged CI attached" indistinguishable from + // "sweeper never saw the PR". + const preliminary = classifyPrForSweep({ pr: listed, ciRuns, botCloseCount: 0, now }); + if (preliminary.action !== "refire") { + const attachedRuns = ciRuns + .filter((run) => run.conclusion !== "startup_failure") + .map((run) => `${run.id ?? "?"}:${run.status ?? "?"}/${run.conclusion ?? "pending"}`); + const detail = preliminary.reason === "ci-attached" ? `: ${attachedRuns.join(", ")}` : ""; + results.push({ number: listed.number, sha: listed.head.sha.slice(0, 12), ...preliminary }); + core.info(`pr-ci-sweeper: skip #${listed.number} (${preliminary.reason}${detail})`); + continue; + } + // Past the cap, keep classifying (so every scanned PR still gets a logged + // decision) but convert would-be re-fires into skips instead of mutating. + if (refires >= MAX_REFIRES_PER_SWEEP) { + results.push({ + number: listed.number, + sha: listed.head.sha.slice(0, 12), + action: "skip", + reason: "refire-cap-reached", + }); + core.info(`pr-ci-sweeper: skip #${listed.number} (refire-cap-reached)`); continue; } // Candidate: fetch authoritative state (mergeable, current head) and the // close history so a racing push or human action wins over the sweep. const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: listed.number }); if (pr.state !== "open" || pr.head.sha !== listed.head.sha) { + core.info(`pr-ci-sweeper: #${listed.number} changed during sweep; leaving it alone`); continue; } const events = await github.paginate(github.rest.issues.listEvents, { @@ -475,7 +515,7 @@ export async function runPrCiSweeper({ await reopenWithRetry({ github, core, owner, repo, pullNumber: pr.number }); } core.info( - `pr-ci-sweeper: checked ${openPrs.length} open PRs, ${results.length} candidates, ${refires} re-fire${refires === 1 ? "" : "s"}, ${revives} revive${revives === 1 ? "" : "s"}${dryRun ? " (dry-run)" : ""}`, + `pr-ci-sweeper: scanned ${openPrs.length} open PRs, ${results.length} classified, ${refires} re-fire${refires === 1 ? "" : "s"}, ${revives} revive${revives === 1 ? "" : "s"}${dryRun ? " (dry-run)" : ""}`, ); return results; } diff --git a/test/scripts/pr-ci-sweeper.test.ts b/test/scripts/pr-ci-sweeper.test.ts index 0660c0cb0d81..e256dc0bd532 100644 --- a/test/scripts/pr-ci-sweeper.test.ts +++ b/test/scripts/pr-ci-sweeper.test.ts @@ -288,29 +288,53 @@ type FakeCheckRun = { function fakeGithub(options: { prs: Array>; - runsBySha: Record>; + runsBySha: Record< + string, + Array<{ conclusion: string | null; event?: string; id?: number; status?: string }> + >; checksByRef?: Record; workflowRunsById?: Record; pullsGetByNumber?: Record>; events?: Array>; + pageSize?: number; }) { const calls: FakeCall[] = []; const record = (method: string, args: Record) => { calls.push({ method, args }); }; const github = { - paginate: (endpoint: { endpointName: string }, args: Record) => { + paginate: ( + endpoint: { endpointName: string }, + args: Record, + mapFn?: (response: { data: unknown[] }, done: () => void) => unknown[], + ) => { record(endpoint.endpointName, args); + // Emulate octokit's paged mapFn contract: the page where done() fires is + // still included in the result, and later pages are never fetched. + const paged = (items: unknown[]) => { + if (!mapFn) { + return Promise.resolve(items); + } + const pageSize = options.pageSize ?? Math.max(items.length, 1); + const collected: unknown[] = []; + let stopped = false; + for (let start = 0; start < items.length && !stopped; start += pageSize) { + record(`${endpoint.endpointName}.page`, { start }); + collected.push( + ...mapFn({ data: items.slice(start, start + pageSize) }, () => { + stopped = true; + }), + ); + } + return Promise.resolve(collected); + }; if (endpoint.endpointName === "pulls.list") { - return Promise.resolve(options.prs); + return paged(options.prs); } if (endpoint.endpointName === "actions.listWorkflowRuns") { return Promise.resolve( (options.runsBySha[args.head_sha as string] ?? []) - .map((run) => ({ - event: run.event ?? "pull_request", - conclusion: run.conclusion, - })) + .map((run) => ({ ...run, event: run.event ?? "pull_request" })) .filter((run) => !args.event || run.event === args.event), ); } @@ -438,10 +462,122 @@ describe("runPrCiSweeper", () => { }); expect(results).toEqual([ { number: 7, sha: "a".repeat(12), action: "refire", reason: "ci-startup-failure" }, + { number: 8, sha: "b".repeat(12), action: "skip", reason: "ci-attached" }, ]); expect(calls.filter((call) => call.method === "pulls.update")).toEqual([]); }); + it("logs a ci-attached skip with the attached run ids", async () => { + const attached = { + ...pr(), + number: 21, + state: "open", + head: { sha: "5".repeat(40) }, + }; + const { github } = fakeGithub({ + prs: [attached], + runsBySha: { + [attached.head.sha]: [{ id: 30105208438, status: "completed", conclusion: "failure" }], + }, + }); + const { core: loggedCore, logs } = recordingCore(); + const results = await runPrCiSweeper({ + github: github as never, + context: context as never, + core: loggedCore as never, + now: NOW, + }); + expect(results).toEqual([ + { number: 21, sha: "5".repeat(12), action: "skip", reason: "ci-attached" }, + ]); + expect(logs).toContain("pr-ci-sweeper: skip #21 (ci-attached: 30105208438:completed/failure)"); + }); + + it("logs draft skips so every scanned PR has a decision", async () => { + const draft = { + ...pr({ draft: true }), + number: 22, + state: "open", + head: { sha: "6".repeat(40) }, + }; + const { github, calls } = fakeGithub({ prs: [draft], runsBySha: {} }); + const { core: loggedCore, logs } = recordingCore(); + const results = await runPrCiSweeper({ + github: github as never, + context: context as never, + core: loggedCore as never, + now: NOW, + }); + expect(results).toEqual([{ number: 22, sha: "6".repeat(12), action: "skip", reason: "draft" }]); + expect(logs).toContain("pr-ci-sweeper: skip #22 (draft)"); + // Drafts skip before the per-head run lookup so they cost no Actions reads. + expect(calls.filter((call) => call.method === "actions.listWorkflowRuns")).toEqual([]); + }); + + it("keeps logging decisions after the per-sweep re-fire cap", async () => { + const dropped = Array.from({ length: 11 }, (_, index) => ({ + ...pr(), + number: 100 + index, + state: "open", + head: { sha: index.toString(16).padStart(2, "0").repeat(20) }, + })); + const { github, calls } = fakeGithub({ prs: dropped, runsBySha: {} }); + const { core: loggedCore, logs } = recordingCore(); + const results = await runPrCiSweeper({ + github: github as never, + context: context as never, + core: loggedCore as never, + dryRun: true, + now: NOW, + }); + expect(results.filter((entry) => entry.action === "refire")).toHaveLength(10); + expect(results.at(-1)).toEqual({ + number: 110, + sha: "0a".repeat(6), + action: "skip", + reason: "refire-cap-reached", + }); + expect(logs).toContain("pr-ci-sweeper: skip #110 (refire-cap-reached)"); + // The capped PR is classified from list data only, never fetched. + expect( + calls.filter((call) => call.method === "pulls.get" && call.args.pull_number === 110), + ).toEqual([]); + }); + + it("stops listing pages once creation dates cross the lookback", async () => { + const recent = { ...pr(), number: 30, state: "open", head: { sha: "7".repeat(40) } }; + const oldA = { + ...pr({ created_at: new Date(NOW - 25 * HOURS).toISOString() }), + number: 31, + state: "open", + head: { sha: "8".repeat(40) }, + }; + const oldB = { + ...pr({ created_at: new Date(NOW - 30 * HOURS).toISOString() }), + number: 32, + state: "open", + head: { sha: "9".repeat(40) }, + }; + const { github, calls } = fakeGithub({ + prs: [recent, oldA, oldB], + runsBySha: {}, + pageSize: 1, + }); + const results = await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + dryRun: true, + now: NOW, + }); + // Page 2 crossed the 24h creation horizon, so page 3 is never fetched and + // the outside-lookback PR on page 2 stops the scan without a decision. + expect(calls.filter((call) => call.method === "pulls.list.page")).toHaveLength(2); + expect(results).toEqual([ + { number: 30, sha: "7".repeat(12), action: "refire", reason: "ci-run-missing" }, + ]); + }); + it("closes and reopens a dropped-CI PR in live mode", async () => { const dropped = { ...pr(),