diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 68924a2271..49cf949424 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2218,6 +2218,11 @@ jobs: if: ${{ always() && github.event_name == 'push' && github.ref == 'refs/heads/main' }} runs-on: ubuntu-24.04 timeout-minutes: 5 + # No job-level `concurrency` group on purpose. A concurrency group keeps only ONE pending job and + # replaces it with whichever job became ready last, which follows run-finish order, not commit + # order: a newer red run's routing could be cancelled by an older green run, leaving the issue + # closed while main is red (and cancelling that newer run's whole CI result). Ordering is + # instead enforced by the routed-sha stamp, and re-checked immediately before each write. permissions: contents: read issues: write @@ -2320,12 +2325,97 @@ jobs: } } - const { data: open } = await github.rest.issues.listForRepo({ owner, repo, state: "open", labels: label }); + // A run older than the one that last routed must not undo it. Runs finish out of order: on + // 2026-10-13 the tip's green run closed the issue at 17:27Z and an older commit's run, + // finishing at 17:33Z, reopened it for a failure main no longer had. Every body this job + // writes carries the commit it last routed, and a run whose commit is BEHIND that one + // stands down. Ordering by what was last routed (not by "is this main's tip") means a busy + // merge streak cannot suppress every alert: an older red run still speaks when nothing + // newer has. Fails open: an unreadable ordering routes rather than loses an alert. + const ROUTED_PATTERN = //; + const withRoutedStamp = (text) => { + const base = String(text ?? "").replace(ROUTED_PATTERN, "").trimEnd(); + return headSha ? `${base}\n` : base; + }; + async function isSuperseded(issue) { + const routedSha = ROUTED_PATTERN.exec(String(issue?.body ?? ""))?.[1]; + if (!headSha || !routedSha || routedSha === headSha) return false; + try { + const { data: comparison } = await github.rest.repos.compareCommits({ + owner, + repo, + base: routedSha, + head: headSha, + }); + return comparison?.status === "behind"; + } catch (error) { + core.warning(`Could not order this run against ${routedSha.slice(0, 12)}, routing it anyway: ${error.message}`); + return false; + } + } + const owned = (issue) => String(issue.body ?? "").includes(MARKER) || issue.title === title; - const existing = open.find(owned) ?? null; + + // The most recent issue this job closed, matched by marker only so it never reopens an + // issue a person wrote. It is the ordering reference when nothing is open, and the issue a + // new red spell reopens. + async function latestClosedOwned() { + const { data: closed } = await github.rest.issues.listForRepo({ + owner, + repo, + state: "closed", + labels: label, + sort: "updated", + direction: "desc", + per_page: 20, + }); + return closed.find((issue) => !issue.pull_request && String(issue.body ?? "").includes(MARKER)) ?? null; + } + + // `existing` is our open issue; `previous` is the latest closed one, read only when nothing + // is open. Every write below acts on these two, so the re-check rebuilds both together. + async function readIssueState() { + const { data: open } = await github.rest.issues.listForRepo({ owner, repo, state: "open", labels: label }); + const openOwned = open.find(owned) ?? null; + return { existing: openOwned, previous: openOwned ? null : await latestClosedOwned() }; + } + let { existing, previous } = await readIssueState(); + + // Re-read the issue state right before writing, so a run that routed while this one was + // reading is honoured, and the writes below act on the fresh open/closed issue rather than + // the stale one. This replaces serialisation: the window is a few API calls, not a whole + // job. Fails open like the ordering lookup: an unreadable re-check routes on the state + // already read rather than aborting and losing the alert. + async function supersededNow() { + try { + ({ existing, previous } = await readIssueState()); + } catch (error) { + core.warning(`Could not re-read the pinned issue before writing, routing on the state already read: ${error.message}`); + return false; + } + return isSuperseded(existing ?? previous); + } + + if (await isSuperseded(existing ?? previous)) { + core.info( + `Run for ${headSha.slice(0, 12)} is older than the commit already routed; ` + + `leaving the pinned issue alone (this run was ${red ? "red" : "green"}).`, + ); + return; + } + + if (await supersededNow()) { + core.info(`Run for ${headSha.slice(0, 12)} was superseded while reading; leaving the pinned issue alone.`); + return; + } if (!red) { if (!existing) { + // Record this green run on the closed issue, so a still-older red run that finishes + // after it cannot reopen the issue for a failure main no longer has. + if (previous && withRoutedStamp(previous.body) !== String(previous.body ?? "")) { + await github.rest.issues.update({ owner, repo, issue_number: previous.number, body: withRoutedStamp(previous.body) }); + } core.info("CI on main is green and no failure issue of ours is open."); return; } @@ -2345,7 +2435,14 @@ jobs: MARKER, ].join("\n"), }); - await github.rest.issues.update({ owner, repo, issue_number: existing.number, state: "closed", state_reason: "completed" }); + await github.rest.issues.update({ + owner, + repo, + issue_number: existing.number, + state: "closed", + state_reason: "completed", + body: withRoutedStamp(existing.body), + }); core.info(`Closed #${existing.number} after a green main run.`); return; } @@ -2376,9 +2473,10 @@ jobs: "is a standing defect rather than a regression this push introduced.", MARKER, ].join("\n"); + const stampedBody = withRoutedStamp(body); if (existing) { - await github.rest.issues.update({ owner, repo, issue_number: existing.number, body }); + await github.rest.issues.update({ owner, repo, issue_number: existing.number, body: stampedBody }); await github.rest.issues.createComment({ owner, repo, @@ -2391,18 +2489,8 @@ jobs: // Reopen the most recent issue this job closed rather than opening a new one per red // spell (owner decision, 2026-09-26: main flips red and green often, and 25 issues in a // day was noise). Marker-only match, so it never reopens an issue a person wrote. - const { data: closed } = await github.rest.issues.listForRepo({ - owner, - repo, - state: "closed", - labels: label, - sort: "updated", - direction: "desc", - per_page: 20, - }); - const previous = closed.find((issue) => !issue.pull_request && String(issue.body ?? "").includes(MARKER)) ?? null; if (previous) { - await github.rest.issues.update({ owner, repo, issue_number: previous.number, state: "open", title, body }); + await github.rest.issues.update({ owner, repo, issue_number: previous.number, state: "open", title, body: stampedBody }); await github.rest.issues.createComment({ owner, repo, @@ -2412,7 +2500,7 @@ jobs: await assignAlert(previous.number); core.warning(`CI on main is failing; reopened #${previous.number}.`); } else { - const created = await github.rest.issues.create({ owner, repo, title, labels: [label], body }); + const created = await github.rest.issues.create({ owner, repo, title, labels: [label], body: stampedBody }); await assignAlert(created.data.number); core.warning(`CI on main is failing; opened #${created.data.number}.`); } diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index 99970e2067..2f2a8f9948 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -581,7 +581,9 @@ describe("CI cache safety", () => { * its own case rather than left to review. */ it("never cancels an in-flight run for a base-branch push", () => { - const concurrency = sourceSegment(workflow, "concurrency:", "permissions:", { + // Anchored on the unindented key: job-level `concurrency:` blocks (the main-failure router + // serialises itself) are indented, so this stays the workflow-level contract. + const concurrency = sourceSegment(workflow, "\nconcurrency:\n", "permissions:", { label: "workflow concurrency block", }); diff --git a/tests/ci-main-failure-routing.test.ts b/tests/ci-main-failure-routing.test.ts index ca219f415f..05cf0e6175 100644 --- a/tests/ci-main-failure-routing.test.ts +++ b/tests/ci-main-failure-routing.test.ts @@ -58,7 +58,7 @@ describe("main CI failure routing", () => { it("reopens its most recent closed issue instead of opening a new one", () => { expect(routing).toContain('state: "closed",'); expect(routing).toMatch( - /const previous = closed\.find\(\(issue\) => !issue\.pull_request && String\(issue\.body \?\? ""\)\.includes\(MARKER\)\)/, + /closed\.find\(\(issue\) => !issue\.pull_request && String\(issue\.body \?\? ""\)\.includes\(MARKER\)\)/, ); expect(routing).toContain('state: "open", title, body'); expect(routing.indexOf('state: "open", title, body')).toBeLessThan(routing.indexOf("issues.create(")); @@ -186,14 +186,31 @@ function routingScriptSource() { return body.join("\n"); } +const RUN_SHA = "0123456789abcdef0123456789abcdef01234567"; +const ROUTED_SHA = "fedcba9876543210fedcba9876543210fedcba98"; + async function runRouting(options: { results: { prRequired: string; matrix: string; visual: string }; jobs?: ListedJob[]; listingFails?: boolean; openIssues?: Issue[]; + closedIssues?: Issue[]; + /** Per-call open-issue reads (the last entry repeats), to model the pinned issue changing mid-run. */ + openIssuesByCall?: Issue[][]; + /** Per-call closed-issue reads (the last entry repeats). */ + closedIssuesByCall?: Issue[][]; + /** Make the nth (1-based) open-issue read reject. */ + failOpenReadOnCall?: number; + /** What `compareCommits` reports for the run's commit against the last routed one, or an Error to reject. */ + compare?: "behind" | "ahead" | "identical" | "diverged" | Error; }) { const created: Array<{ title: string; body: string }> = []; const closed: number[] = []; + const updates: Array<{ issue_number: number; state?: string; body?: string }> = []; + const comments: number[] = []; + const compared: Array<{ base: string; head: string }> = []; + let openReads = 0; + let closedReads = 0; const github = { paginate: async () => { if (options.listingFails) throw new Error("Resource not accessible by integration"); @@ -202,19 +219,37 @@ async function runRouting(options: { rest: { actions: { listJobsForWorkflowRun: () => undefined }, issues: { - listForRepo: async ({ state }: { state: string }) => ({ - data: state === "open" ? (options.openIssues ?? []) : [], - }), + listForRepo: async ({ state }: { state: string }) => { + if (state === "open") { + openReads += 1; + if (options.failOpenReadOnCall === openReads) throw new Error("API rate limit exceeded"); + const calls = options.openIssuesByCall; + return { data: calls ? calls[Math.min(openReads, calls.length) - 1]! : (options.openIssues ?? []) }; + } + closedReads += 1; + const calls = options.closedIssuesByCall; + return { data: calls ? calls[Math.min(closedReads, calls.length) - 1]! : (options.closedIssues ?? []) }; + }, create: async (request: { title: string; body: string }) => { created.push(request); return { data: { number: 900 } }; }, - update: async (request: { issue_number: number; state?: string }) => { + update: async (request: { issue_number: number; state?: string; body?: string }) => { + updates.push(request); if (request.state === "closed") closed.push(request.issue_number); }, - createComment: async () => undefined, + createComment: async (request: { issue_number: number }) => { + comments.push(request.issue_number); + }, addAssignees: async () => undefined, }, + repos: { + compareCommits: async (request: { base: string; head: string }) => { + compared.push(request); + if (options.compare instanceof Error) throw options.compare; + return { data: { status: options.compare ?? "ahead" } }; + }, + }, }, }; const context = { repo: { owner: "BigSimmo", repo: "PsychSift" }, runId: 1, serverUrl: "https://github.com" }; @@ -223,7 +258,7 @@ async function runRouting(options: { PR_REQUIRED_RESULT: options.results.prRequired, BROWSER_MATRIX_RESULT: options.results.matrix, VISUAL_BASELINE_RESULT: options.results.visual, - HEAD_SHA: "0123456789abcdef", + HEAD_SHA: RUN_SHA, HEAD_MESSAGE: "Example merge", }; const previous = Object.fromEntries(Object.keys(env).map((key) => [key, process.env[key]])); @@ -236,7 +271,7 @@ async function runRouting(options: { else process.env[key] = value; } } - return { created, closed }; + return { created, closed, updates, comments, compared }; } describe("main CI failure routing, executed", () => { @@ -309,3 +344,192 @@ describe("main CI failure routing, executed", () => { expect(created).toHaveLength(1); }); }); + +/** + * Runs on main finish out of order (2026-10-13: the tip's green run closed #3099, then an older + * commit's run reopened it). Every body the job writes records the commit it last routed, and a run + * whose commit is behind that one stands down. Ordering by what was last routed, not by "is this + * main's tip", means a merge streak cannot suppress every alert. + */ +describe("main CI failure routing ignores superseded runs", () => { + const red = { prRequired: "failure", matrix: "skipped", visual: "skipped" }; + const green = { prRequired: "success", matrix: "success", visual: "success" }; + const failingJobs: ListedJob[] = [{ name: "Build", conclusion: "failure" }]; + const stamp = (sha: string) => ``; + const marked = (sha: string): Issue => ({ + number: 42, + title: "CI on main is failing", + body: `\n${stamp(sha)}`, + }); + + it("does not use a job-level concurrency group that could cancel a newer commit's routing", () => { + const routing = workflow.slice(workflow.indexOf(" main-failure-routing:")); + const jobBlock = routing.slice(0, routing.indexOf(" steps:")); + expect(jobBlock).not.toMatch(/\n {4}concurrency:/); + }); + + it("leaves the issue untouched when a superseded red run finishes after a newer one routed", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(ROUTED_SHA)], + compare: "behind", + }); + expect(result.compared).toMatchObject([{ base: ROUTED_SHA, head: RUN_SHA }]); + expect(result.updates).toHaveLength(0); + expect(result.comments).toHaveLength(0); + expect(result.created).toHaveLength(0); + }); + + it("does not close the issue for a superseded green run", async () => { + const result = await runRouting({ results: green, jobs: [], openIssues: [marked(ROUTED_SHA)], compare: "behind" }); + expect(result.closed).toHaveLength(0); + expect(result.updates).toHaveLength(0); + }); + + it("does not reopen a closed issue for a red run older than the green that closed it", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + closedIssues: [marked(ROUTED_SHA)], + compare: "behind", + }); + expect(result.updates).toHaveLength(0); + expect(result.created).toHaveLength(0); + }); + + it("still alerts when an older commit's run is the newest one to report (merge streak)", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(ROUTED_SHA)], + compare: "ahead", + }); + expect(result.updates).toHaveLength(1); + expect(result.updates[0]!.body).toContain(stamp(RUN_SHA)); + expect(result.updates[0]!.body).not.toContain(stamp(ROUTED_SHA)); + expect(result.comments).toEqual([42]); + }); + + it("opens an issue stamped with the run's commit when a red run finds none", async () => { + const result = await runRouting({ results: red, jobs: failingJobs }); + expect(result.created).toHaveLength(1); + expect(result.created[0]!.body).toContain(stamp(RUN_SHA)); + expect(result.compared).toHaveLength(0); + }); + + it("stamps the closing green run on the issue so a later stale red run cannot reopen it", async () => { + const result = await runRouting({ results: green, jobs: [], openIssues: [marked(ROUTED_SHA)], compare: "ahead" }); + expect(result.closed).toEqual([42]); + expect(result.updates[0]!.body).toContain(stamp(RUN_SHA)); + expect(result.updates[0]!.body).toContain(""); + }); + + it("records a green run on the closed issue when none is open", async () => { + const result = await runRouting({ results: green, jobs: [], closedIssues: [marked(ROUTED_SHA)], compare: "ahead" }); + expect(result.closed).toHaveLength(0); + expect(result.updates).toHaveLength(1); + expect(result.updates[0]!.state).toBeUndefined(); + expect(result.updates[0]!.body).toContain(stamp(RUN_SHA)); + }); + + it("routes a red run that has no recorded commit to order against", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [ownIssueWithoutStamp()], + compare: "behind", + }); + expect(result.compared).toHaveLength(0); + expect(result.updates).toHaveLength(1); + }); + + it("fails open: an ordering lookup that errors still routes the alert", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(ROUTED_SHA)], + compare: new Error("Not Found"), + }); + expect(result.updates).toHaveLength(1); + expect(result.comments).toEqual([42]); + }); + + it("writes to the issue state re-read just before writing, not the state read at the start", async () => { + // A newer green run closed #42 while this older-but-ahead red run was reading: the start sees + // #42 open, the re-check sees it closed. The red run must reopen the closed issue, not update + // the (now closed) one it first saw. + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssuesByCall: [[marked(ROUTED_SHA)], []], + closedIssuesByCall: [[marked(ROUTED_SHA)]], + compare: "ahead", + }); + expect(result.created).toHaveLength(0); + expect(result.updates).toHaveLength(1); + expect(result.updates[0]).toMatchObject({ issue_number: 42, state: "open" }); + expect(result.updates[0]!.body).toContain(stamp(RUN_SHA)); + }); + + it("closes the freshly opened issue on green when a red run reopened it while this run was reading", async () => { + // Start: nothing open, #42 closed. By the re-check an older red run reopened #42. This green + // run is ahead and must close it, not rewrite the closed-issue body and leave it open. + const result = await runRouting({ + results: green, + jobs: [], + openIssuesByCall: [[], [marked(ROUTED_SHA)]], + closedIssuesByCall: [[marked(ROUTED_SHA)]], + compare: "ahead", + }); + expect(result.closed).toEqual([42]); + }); + + it("stands down when the re-check shows a newer run routed while this one was reading", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssuesByCall: [[ownIssueWithoutStamp()], [marked(ROUTED_SHA)]], + compare: "behind", + }); + expect(result.updates).toHaveLength(0); + }); + + it("fails open when the first issue read works but the re-check read fails", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(ROUTED_SHA)], + failOpenReadOnCall: 2, + compare: "ahead", + }); + expect(result.updates).toHaveLength(1); + expect(result.updates[0]!.body).toContain(stamp(RUN_SHA)); + expect(result.comments).toEqual([42]); + }); + + it("treats a diverged history as not superseded", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(ROUTED_SHA)], + compare: "diverged", + }); + expect(result.updates).toHaveLength(1); + }); + + it("does not compare a run with itself when it is the commit last routed", async () => { + const result = await runRouting({ + results: red, + jobs: failingJobs, + openIssues: [marked(RUN_SHA)], + compare: "behind", + }); + expect(result.compared).toHaveLength(0); + expect(result.updates).toHaveLength(1); + }); +}); + +function ownIssueWithoutStamp(): Issue { + return { number: 42, title: "CI on main is failing", body: "" }; +}