From 93ec662764d5276eaa7716121320c4055d2e57af Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Thu, 24 Sep 2026 11:11:11 +0000 Subject: [PATCH 1/4] fix(ci): name the failing shard in the merge-queue ejection comment (BLO-28886) BLO-28886 ask 2 asks the ejection signal to name the failing shard. The reporter shipped saying "inspect the merge-group jobs", which hands the lookup back to the reader -- and that lookup is the expensive bit, since a queue-branch failure does not appear in the PR's statusCheckRollup at all. Reads the run's jobs and lists every non-success one. Best-effort: a failed jobs read degrades the comment rather than dropping it, because a lost comment leaves a stuck PR silent, which is what this script exists to stop. No name-filtering of aggregator jobs (`verify`) -- an allowlist rots on the next workflow rename and one extra name costs a reader nothing. Verified against the real payload of run 35871782486, the run that ejected #1962: renders "Failing jobs: \`General tests (workspaces-a)\`, \`verify\`." Each of the three guards mutation-tested individually: dropping the `cancelled` arm, the empty-guard, or the singular/plural each turns the suite red. --- .../scripts/report-merge-queue-ejection.mjs | 32 ++++++++++++++++++- .../report-merge-queue-ejection.test.mjs | 26 +++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index 3c7c77b982c6..9dc7504491ab 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -44,6 +44,22 @@ export function shouldReportCancelledRun({ merged, isInMergeQueue }) { return !merged && !isInMergeQueue; } +// BLO-28886 asks the ejection signal to NAME the failing shard. Without it the +// comment says "inspect the merge-group jobs", i.e. it re-hands the lookup back +// to the reader -- and the whole point of this reporter is that a queue-branch +// failure is invisible from the PR, so that lookup is exactly the expensive bit. +// +// No name-filtering of aggregator jobs (`verify`): a name allowlist rots on the +// next workflow rename, and one extra job name costs a reader nothing. +export function failingJobSummary(jobs) { + const failed = (Array.isArray(jobs) ? jobs : []) + .filter((job) => job?.conclusion === "failure" || job?.conclusion === "cancelled") + .map((job) => job.name) + .filter((name) => typeof name === "string" && name.length > 0); + if (failed.length === 0) return ""; + return ` Failing ${failed.length === 1 ? "job" : "jobs"}: ${failed.map((name) => `\`${name}\``).join(", ")}.`; +} + async function githubRequest(path, options = {}) { const response = await fetch(`https://api.github.com${path}`, { ...options, @@ -118,7 +134,21 @@ export async function reportMergeQueueFailure({ repository, headBranch, conclusi } const outcome = conclusion === "failure" ? "failed" : "was cancelled (a job timeout surfaces this way)"; - const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${outcome} and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`; + // Best-effort: a failure here must NOT lose the ejection report. Losing the + // shard name degrades the comment; losing the comment leaves a stuck PR + // silent, which is the failure this whole script exists to prevent. + // One unpaginated page (default `filter=latest`, so a re-run's older attempts + // are excluded). Ceiling: a workflow past 100 jobs truncates -- 17 today. + let summary = ""; + try { + const { jobs } = await githubRequest( + `/repos/${owner}/${repo}/actions/runs/${runId}/jobs?per_page=100`, + ); + summary = failingJobSummary(jobs); + } catch (error) { + console.warn(`Could not read jobs for run ${runId}: ${error.message}`); + } + const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${outcome} and GitHub may have removed the PR from the queue and dropped auto-merge.${summary} Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`; await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments`, { method: "POST", headers: { "content-type": "application/json" }, diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs index 09b9e559f355..02c7977ada9b 100644 --- a/.github/scripts/tests/report-merge-queue-ejection.test.mjs +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -4,6 +4,7 @@ import { mergeQueuePullRequestNumber, shouldReportMergeQueueFailure, shouldReportCancelledRun, + failingJobSummary, } from "../report-merge-queue-ejection.mjs"; test("extracts PR numbers from merge-group synthetic refs", () => { @@ -37,3 +38,28 @@ test("a cancelled run is reported only when the PR is neither merged nor still q // Still queued for a re-build: not ejected. assert.equal(shouldReportCancelledRun({ merged: false, isInMergeQueue: true }), false); }); + +test("the ejection comment names the failing shards", () => { + // Shape taken from the real run that ejected #1962 (35871782486): the + // aggregator `verify` fails alongside the shard, and is deliberately kept. + assert.equal( + failingJobSummary([ + { name: "General tests (workspaces-a)", conclusion: "failure" }, + { name: "General tests (server 1/4)", conclusion: "success" }, + { name: "verify", conclusion: "failure" }, + ]), + " Failing jobs: `General tests (workspaces-a)`, `verify`.", + ); + // A job killed by `timeout-minutes` surfaces as cancelled, and that is the + // class pr.yml deliberately trades a fast red for -- so it must be named too. + assert.equal( + failingJobSummary([{ name: "General tests (server 4/4)", conclusion: "cancelled" }]), + " Failing job: `General tests (server 4/4)`.", + ); + // Empty, not a dangling "Failing jobs:", when the jobs read gave nothing + // usable -- the comment must still read as a sentence. Covers the + // best-effort catch path, which passes no jobs at all. + assert.equal(failingJobSummary([{ name: "verify", conclusion: "success" }]), ""); + assert.equal(failingJobSummary([]), ""); + assert.equal(failingJobSummary(undefined), ""); +}); From 26536182e740644dab25092095d888206f245c82 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Thu, 24 Sep 2026 14:01:55 +0000 Subject: [PATCH 2/4] fix(ci): don't call a failed merge-group run a timeout (BLO-28886) GitHub marks a run `cancelled` when ANY job is cancelled, and fail-fast cancels the siblings of a job that genuinely failed. So the commonest `cancelled` merge-group run is a real test failure, and reporting it as "a job timeout surfaces this way" sends the reader after infra that isn't there. Live case, run 35993984182, which ejected #1976 at 13:38Z today: run conclusion `cancelled`, but `General tests (workspaces-a)` and `verify` both `failure` -- an after-teardown `ReferenceError: window is not defined` with 3075/3075 tests passing. Adding the job list alone made the comment contradict itself in one sentence: "was cancelled (a job timeout surfaces this way) ... Failing jobs: `General tests (workspaces-a)`, `verify`". Derive the wording from the jobs already fetched. No extra API call. Degrades to the run-level conclusion when the best-effort jobs read returns nothing, so it is never worse than before. Co-Authored-By: Claude --- .../scripts/report-merge-queue-ejection.mjs | 30 ++++++++++++++----- .../report-merge-queue-ejection.test.mjs | 28 +++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index 9dc7504491ab..51fab1e80153 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -60,8 +60,23 @@ export function failingJobSummary(jobs) { return ` Failing ${failed.length === 1 ? "job" : "jobs"}: ${failed.map((name) => `\`${name}\``).join(", ")}.`; } -async function githubRequest(path, options = {}) { - const response = await fetch(`https://api.github.com${path}`, { +// A run whose conclusion is `cancelled` is NOT necessarily a timeout. GitHub +// marks the whole run `cancelled` when ANY job is cancelled, and fail-fast +// cancels the siblings of a job that genuinely failed -- so the commonest +// `cancelled` run is a real test failure wearing a timeout's clothes. Measured +// on run 35993984182 (ejected #1976): run `cancelled`, but `General tests +// (workspaces-a)` and `verify` both `failure`, and the actual cause was an +// after-teardown `ReferenceError: window is not defined` with 3075/3075 tests +// passing. Reading the run-level conclusion alone sends the reader to look for +// an infra timeout that is not there. +export function runOutcomeText(conclusion, jobs) { + const failed = + conclusion === "failure" || + (Array.isArray(jobs) ? jobs : []).some((job) => job?.conclusion === "failure"); + return failed ? "failed" : "was cancelled (a job timeout surfaces this way)"; +} + +async function githubRequest(path, options = {}) { const response = await fetch(`https://api.github.com${path}`, { ...options, headers: { accept: "application/vnd.github+json", @@ -133,22 +148,21 @@ export async function reportMergeQueueFailure({ repository, headBranch, conclusi return { reported: false, reason: "already-reported", number }; } - const outcome = conclusion === "failure" ? "failed" : "was cancelled (a job timeout surfaces this way)"; // Best-effort: a failure here must NOT lose the ejection report. Losing the // shard name degrades the comment; losing the comment leaves a stuck PR // silent, which is the failure this whole script exists to prevent. // One unpaginated page (default `filter=latest`, so a re-run's older attempts // are excluded). Ceiling: a workflow past 100 jobs truncates -- 17 today. - let summary = ""; + let jobs; try { - const { jobs } = await githubRequest( + ({ jobs } = await githubRequest( `/repos/${owner}/${repo}/actions/runs/${runId}/jobs?per_page=100`, - ); - summary = failingJobSummary(jobs); + )); } catch (error) { console.warn(`Could not read jobs for run ${runId}: ${error.message}`); } - const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${outcome} and GitHub may have removed the PR from the queue and dropped auto-merge.${summary} Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`; + const summary = failingJobSummary(jobs); + const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${runOutcomeText(conclusion, jobs)} and GitHub may have removed the PR from the queue and dropped auto-merge.${summary} Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`; await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments`, { method: "POST", headers: { "content-type": "application/json" }, diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs index 02c7977ada9b..bd6ffb9db441 100644 --- a/.github/scripts/tests/report-merge-queue-ejection.test.mjs +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -5,6 +5,7 @@ import { shouldReportMergeQueueFailure, shouldReportCancelledRun, failingJobSummary, + runOutcomeText, } from "../report-merge-queue-ejection.mjs"; test("extracts PR numbers from merge-group synthetic refs", () => { @@ -63,3 +64,30 @@ test("the ejection comment names the failing shards", () => { assert.equal(failingJobSummary([]), ""); assert.equal(failingJobSummary(undefined), ""); }); + +test("a `cancelled` run carrying a failed job is described as failed, not as a timeout", () => { + // Shape taken from run 35993984182, which ejected #1976: run-level conclusion + // `cancelled` (fail-fast cancelled `Build`), but the cause was a genuine + // `failure` in a shard. Reporting that as "a job timeout surfaces this way" + // points the reader at infra instead of at the shard. + assert.equal( + runOutcomeText("cancelled", [ + { name: "Build", conclusion: "cancelled" }, + { name: "General tests (workspaces-a)", conclusion: "failure" }, + { name: "verify", conclusion: "failure" }, + ]), + "failed", + ); + // A genuine timeout: every non-success job is cancelled, nothing failed. + assert.equal( + runOutcomeText("cancelled", [ + { name: "General tests (server 4/4)", conclusion: "cancelled" }, + { name: "verify", conclusion: "success" }, + ]), + "was cancelled (a job timeout surfaces this way)", + ); + // Degrades to the run-level conclusion when the best-effort jobs read gave + // nothing -- i.e. exactly the pre-BLO-28886 behaviour, never worse. + assert.equal(runOutcomeText("cancelled", undefined), "was cancelled (a job timeout surfaces this way)"); + assert.equal(runOutcomeText("failure", undefined), "failed"); +}); From 01649f2423acfc508251f140157b04655ea1feea Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Fri, 25 Sep 2026 03:45:08 +0000 Subject: [PATCH 3/4] fix(ci): don't name fail-fast cancelled jobs as ejection causes (BLO-28886) Ally review on #2019, Important #1. failingJobSummary treated every cancelled job as failing, so fail-fast collateral was reported as a cause: on run 35993984182 (ejected #1976) it named `Build`, which was cancelled BECAUSE `General tests (workspaces-a)` failed. That is the same misdirection runOutcomeText exists to prevent, reproduced one layer down. Filter on conclusion, not name: a cancelled job is signal only when nothing failed (a timeout-minutes kill), so the allowlist-rot objection does not apply. Byte-identical on the #1962 and single-timeout shapes -- both existing assertions still pass unchanged. New test runs BOTH functions over one real payload, which is the split that hid this. Mutation-tested: reverting the arm turns the suite red (6 pass -> 5 pass / 1 fail); restoring it returns 6/6. Also restores the line break in githubRequest's signature (Important #2); no gate catches that, there is no formatter config at the repo root. --- .../scripts/report-merge-queue-ejection.mjs | 17 ++++++++++++++--- .../report-merge-queue-ejection.test.mjs | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index 51fab1e80153..d1d2593a8960 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -51,9 +51,19 @@ export function shouldReportCancelledRun({ merged, isInMergeQueue }) { // // No name-filtering of aggregator jobs (`verify`): a name allowlist rots on the // next workflow rename, and one extra job name costs a reader nothing. +// +// The filter is on CONCLUSION, not name. A `cancelled` job is only signal when +// nothing else failed: fail-fast cancels the siblings of a job that genuinely +// failed, so on run 35993984182 (ejected #1976) `Build` is cancelled collateral +// of `General tests (workspaces-a)`. Naming it sends the reader to a log that +// contains nothing but a cancellation -- the same misdirection runOutcomeText +// exists to prevent, one layer down. When nothing failed, a cancelled job IS the +// cause (a `timeout-minutes` kill surfaces that way) and must still be named. export function failingJobSummary(jobs) { - const failed = (Array.isArray(jobs) ? jobs : []) - .filter((job) => job?.conclusion === "failure" || job?.conclusion === "cancelled") + const list = Array.isArray(jobs) ? jobs : []; + const anyFailure = list.some((job) => job?.conclusion === "failure"); + const failed = list + .filter((job) => job?.conclusion === "failure" || (!anyFailure && job?.conclusion === "cancelled")) .map((job) => job.name) .filter((name) => typeof name === "string" && name.length > 0); if (failed.length === 0) return ""; @@ -76,7 +86,8 @@ export function runOutcomeText(conclusion, jobs) { return failed ? "failed" : "was cancelled (a job timeout surfaces this way)"; } -async function githubRequest(path, options = {}) { const response = await fetch(`https://api.github.com${path}`, { +async function githubRequest(path, options = {}) { + const response = await fetch(`https://api.github.com${path}`, { ...options, headers: { accept: "application/vnd.github+json", diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs index bd6ffb9db441..a1e95e02d362 100644 --- a/.github/scripts/tests/report-merge-queue-ejection.test.mjs +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -91,3 +91,22 @@ test("a `cancelled` run carrying a failed job is described as failed, not as a t assert.equal(runOutcomeText("cancelled", undefined), "was cancelled (a job timeout surfaces this way)"); assert.equal(runOutcomeText("failure", undefined), "failed"); }); + +test("the rendered sentence is coherent on a fail-fast run: no cancelled collateral is named", () => { + // The two functions above are each correct in isolation while the sentence + // they compose is wrong -- that split is what hid the `Build` case. So assert + // on BOTH over ONE real shape: run 35993984182, verbatim from the API. + const jobs = [ + { name: "Build", conclusion: "cancelled" }, + { name: "General tests (workspaces-a)", conclusion: "failure" }, + { name: "General tests (server 1/4)", conclusion: "success" }, + { name: "verify", conclusion: "failure" }, + ]; + assert.equal( + `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`, + "The merge-group run failed. Failing jobs: `General tests (workspaces-a)`, `verify`.", + ); + // `Build` was cancelled BECAUSE workspaces-a failed; its log shows only a + // cancellation. Naming it is the misdirection, not extra detail. + assert.ok(!failingJobSummary(jobs).includes("Build")); +}); From 8d2fe8f60bc553d87ae464eef567d0992c4cc117 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Fri, 25 Sep 2026 04:56:00 +0000 Subject: [PATCH 4/4] fix(ci): name the job that caused the ejection, not the lane that reports it (BLO-28886) `verify` needs: every other lane, so it goes red whenever anything upstream dies. It is a messenger, never a cause -- and because it is a genuine `failure` rather than a `cancelled`, the conclusion filter added in 01649f24 cannot catch it. Measured over the 7 merge-group ejections in the 2026-09-23/25 window: on the 2 rooted in a `policy` timeout (runs 35948766367 and 36015721472) the comment rendered The merge-group run failed. Failing job: `verify`. Both halves wrong. The run was not "failed", it was a 600s cap kill; and `verify`'s log reads only "upstream lane(s) did not run", so the reader is sent nowhere while `policy` is never named. That is the same misdirection this whole script exists to remove, surviving one layer further up. causalJobs() drops a non-success job that started only after EVERY other non-success job finished -- structurally last, therefore downstream of all of them. Detected from timestamps already in the jobs payload, so no extra API call, and no name allowlist to rot on the next workflow rename. Deliberately the narrowest form: "after SOME other job" would drop a genuine second failure that merely started late. Shared by both renderers, because the two used to disagree and a reader only ever sees the sentence they compose. Rendered against all 4 real ejection payloads in the window: 35948766367 -> was cancelled (a job timeout surfaces this way). `policy` 36015721472 -> was cancelled (a job timeout surfaces this way). `policy` 35993984182 -> failed. `General tests (workspaces-a)` 36050446289 -> failed. `General tests (workspaces-a)` Degrades to the previous behaviour when timestamps are absent -- one extra name, never a missing one. 11 pass / 0 fail. Each of the 4 guards mutation-tested individually; two of them had NO failing mutation on the first attempt and their tests were rewritten until they did. --- .../scripts/report-merge-queue-ejection.mjs | 62 ++++++++++++-- .../report-merge-queue-ejection.test.mjs | 80 ++++++++++++++++++- 2 files changed, 135 insertions(+), 7 deletions(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index d1d2593a8960..0cf47c8b6f06 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -49,8 +49,12 @@ export function shouldReportCancelledRun({ merged, isInMergeQueue }) { // to the reader -- and the whole point of this reporter is that a queue-branch // failure is invisible from the PR, so that lookup is exactly the expensive bit. // -// No name-filtering of aggregator jobs (`verify`): a name allowlist rots on the -// next workflow rename, and one extra job name costs a reader nothing. +// No name-filtering of aggregator jobs: a name allowlist rots on the next +// workflow rename. Aggregators are excluded STRUCTURALLY instead -- see +// causalJobs(). An earlier revision of this comment argued "one extra job name +// costs a reader nothing"; measured, that was wrong. On the two `policy` +// timeout ejections of 2026-09-23/25 the extra name was the ONLY name, and it +// was the wrong one. // // The filter is on CONCLUSION, not name. A `cancelled` job is only signal when // nothing else failed: fail-fast cancels the siblings of a job that genuinely @@ -59,8 +63,52 @@ export function shouldReportCancelledRun({ merged, isInMergeQueue }) { // contains nothing but a cancellation -- the same misdirection runOutcomeText // exists to prevent, one layer down. When nothing failed, a cancelled job IS the // cause (a `timeout-minutes` kill surfaces that way) and must still be named. +// +// Both rules below need the same input -- the non-success jobs that could +// actually be a CAUSE -- so they share causalJobs() rather than each deriving +// it. That sharing is the point: the two used to disagree, and a reader only +// ever sees the sentence they compose. +const isNonSuccess = (job) => job?.conclusion === "failure" || job?.conclusion === "cancelled"; + +/** + * Non-success jobs minus the aggregator lanes that merely report upstream death. + * + * `verify` needs: every other lane, so it goes red whenever anything it waits + * on dies -- it is a MESSENGER, never a cause. Naming it is the same + * misdirection as naming fail-fast collateral, and it is the one the + * conclusion filter above cannot catch, because `verify` is a genuine + * `failure` rather than a `cancelled`. Measured on the two `policy` + * timeout ejections in the 2026-09-23/25 window (runs 35948766367 and + * 36015721472): the comment read "The merge-group run failed. Failing job: + * `verify`." -- the reader is sent to a log whose entire content is "upstream + * lane(s) did not run", and `policy`, killed at its 600s cap, is never named. + * + * Detected structurally, not by name: an allowlist of aggregator names rots on + * the next workflow rename. A job that starts only after EVERY other + * non-success job has finished ran last by construction, which is what being + * downstream of all of them means. Deliberately the narrowest form of that + * test -- "after SOME other job" would drop a genuine second failure that + * merely started late. + */ +export function causalJobs(jobs) { + const list = (Array.isArray(jobs) ? jobs : []).filter(isNonSuccess); + const at = (value) => Date.parse(value ?? ""); + const causal = list.filter((job) => { + const started = at(job.started_at); + const others = list.filter((other) => other !== job); + if (!others.length || Number.isNaN(started)) return true; + return !others.every((other) => { + const done = at(other.completed_at); + return !Number.isNaN(done) && started >= done; + }); + }); + // A single non-success job is vacuously "last"; never report nothing when + // something did fail. + return causal.length ? causal : list; +} + export function failingJobSummary(jobs) { - const list = Array.isArray(jobs) ? jobs : []; + const list = causalJobs(jobs); const anyFailure = list.some((job) => job?.conclusion === "failure"); const failed = list .filter((job) => job?.conclusion === "failure" || (!anyFailure && job?.conclusion === "cancelled")) @@ -79,10 +127,14 @@ export function failingJobSummary(jobs) { // after-teardown `ReferenceError: window is not defined` with 3075/3075 tests // passing. Reading the run-level conclusion alone sends the reader to look for // an infra timeout that is not there. +// +// Reads the same causalJobs() set as failingJobSummary: `verify`'s own +// `failure` used to flip this to "failed" on a run whose real cause was +// `policy` hitting its cap, so the sentence asserted a failure and then named +// only the messenger. export function runOutcomeText(conclusion, jobs) { const failed = - conclusion === "failure" || - (Array.isArray(jobs) ? jobs : []).some((job) => job?.conclusion === "failure"); + conclusion === "failure" || causalJobs(jobs).some((job) => job?.conclusion === "failure"); return failed ? "failed" : "was cancelled (a job timeout surfaces this way)"; } diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs index a1e95e02d362..75318ab821a5 100644 --- a/.github/scripts/tests/report-merge-queue-ejection.test.mjs +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -6,6 +6,7 @@ import { shouldReportCancelledRun, failingJobSummary, runOutcomeText, + causalJobs, } from "../report-merge-queue-ejection.mjs"; test("extracts PR numbers from merge-group synthetic refs", () => { @@ -41,8 +42,11 @@ test("a cancelled run is reported only when the PR is neither merged nor still q }); test("the ejection comment names the failing shards", () => { - // Shape taken from the real run that ejected #1962 (35871782486): the - // aggregator `verify` fails alongside the shard, and is deliberately kept. + // Shape taken from the real run that ejected #1962 (35871782486). No + // timestamps here on purpose: causalJobs() can only prove a lane is + // downstream from them, so without them it keeps everything and this + // degrades to naming the aggregator too. Fail-safe direction -- one extra + // name, never a missing one. The timestamped cases below are the real shape. assert.equal( failingJobSummary([ { name: "General tests (workspaces-a)", conclusion: "failure" }, @@ -96,6 +100,8 @@ test("the rendered sentence is coherent on a fail-fast run: no cancelled collate // The two functions above are each correct in isolation while the sentence // they compose is wrong -- that split is what hid the `Build` case. So assert // on BOTH over ONE real shape: run 35993984182, verbatim from the API. + // Untimestamped, so `verify` survives; the timestamped form of this same run + // is asserted below, where it correctly drops to the shard alone. const jobs = [ { name: "Build", conclusion: "cancelled" }, { name: "General tests (workspaces-a)", conclusion: "failure" }, @@ -110,3 +116,73 @@ test("the rendered sentence is coherent on a fail-fast run: no cancelled collate // cancellation. Naming it is the misdirection, not extra detail. assert.ok(!failingJobSummary(jobs).includes("Build")); }); + +test("a `policy` timeout is named as the cause, not the `verify` lane that merely reports it", () => { + // Run 35948766367, verbatim from the API: an ejection whose cause is `policy` + // killed at its 600s cap. `verify` needs: it, so `verify` goes red 14s later + // and is a genuine `failure` -- which the conclusion filter cannot catch. + // Before causalJobs() this rendered "The merge-group run failed. Failing job: + // `verify`.", sending the reader to a log reading only "upstream lane(s) did + // not run". 2 of the 7 merge-group ejections in the 2026-09-23/25 window. + const jobs = [ + { name: "policy", conclusion: "cancelled", started_at: "2026-09-24T02:57:04Z", completed_at: "2026-09-24T03:08:22Z" }, + { name: "verify", conclusion: "failure", started_at: "2026-09-24T03:08:36Z", completed_at: "2026-09-24T03:08:47Z" }, + ]; + assert.equal( + `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`, + "The merge-group run was cancelled (a job timeout surfaces this way). Failing job: `policy`.", + ); +}); + +test("a late-starting second failure is still named: only a job after ALL others is dropped", () => { + // The narrowness guard. `Build` starts before `workspaces-a` finishes, so it + // is not downstream-of-everything and must survive causalJobs() -- it is then + // dropped as fail-fast collateral by the conclusion filter, which is a + // different rule. Widening causalJobs to "after SOME other job" would drop a + // genuine independent failure that merely started late. + const jobs = [ + { name: "Build", conclusion: "cancelled", started_at: "2026-09-24T11:46:54Z", completed_at: "2026-09-24T12:12:33Z" }, + { name: "General tests (workspaces-a)", conclusion: "failure", started_at: "2026-09-24T11:45:42Z", completed_at: "2026-09-24T12:05:41Z" }, + { name: "verify", conclusion: "failure", started_at: "2026-09-24T13:36:47Z", completed_at: "2026-09-24T13:37:41Z" }, + ]; + assert.deepEqual(causalJobs(jobs).map((job) => job.name), ["Build", "General tests (workspaces-a)"]); + assert.equal( + `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`, + "The merge-group run failed. Failing job: `General tests (workspaces-a)`.", + ); +}); + +test("a lone failing job is never dropped for being vacuously last", () => { + const jobs = [{ name: "e2e", conclusion: "failure", started_at: "2026-09-24T11:45:42Z", completed_at: "2026-09-24T12:05:41Z" }]; + assert.equal(failingJobSummary(jobs), " Failing job: `e2e`."); + // Jobs with no timestamps at all (never dispatched) must not vanish either. + assert.equal(failingJobSummary([{ name: "policy", conclusion: "cancelled" }]), " Failing job: `policy`."); +}); + +test("a job that started after SOME but not ALL others is kept (guards the narrowness of causalJobs)", () => { + // The `every` in causalJobs has to be `every`, not `some`. `b` starts after + // `a` finished but while `c` is still running, so it is NOT last and is a + // genuine independent failure. Under a `some` test it would silently vanish + // from the comment. The fail-fast fixture above cannot catch this -- no job + // in it starts after any other one completes -- which is why the widened + // form passed the suite before this test existed. + const jobs = [ + { name: "a", conclusion: "failure", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:10:00Z" }, + { name: "b", conclusion: "failure", started_at: "2026-09-24T10:15:00Z", completed_at: "2026-09-24T10:40:00Z" }, + { name: "c", conclusion: "failure", started_at: "2026-09-24T10:05:00Z", completed_at: "2026-09-24T10:50:00Z" }, + ]; + assert.deepEqual(causalJobs(jobs).map((job) => job.name), ["a", "b", "c"]); +}); + +test("jobs cancelled at the same instant are all reported, not all dropped", () => { + // Reachability of the `causal.length ? causal : list` fallback. Job timestamps + // are second-resolution, so two lanes cancelled together when the run died + // carry identical values -- each then reads as "started after the other + // finished" and both get dropped, leaving the comment naming nothing at all. + // Observed shape: needs:-gated lanes cancelled 0s after creation. + const jobs = [ + { name: "Build", conclusion: "cancelled", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:00:00Z" }, + { name: "e2e", conclusion: "cancelled", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:00:00Z" }, + ]; + assert.equal(failingJobSummary(jobs), " Failing jobs: `Build`, `e2e`."); +});