From 6dbf4156eed9953f662b4bd79357bbc0000772e5 Mon Sep 17 00:00:00 2001 From: CTO Date: Thu, 17 Sep 2026 21:41:55 +0000 Subject: [PATCH 1/3] feat(ci): report a merge-queue ejection on the PR it ejected (BLO-26675) A merge-group failure drops the entry and nulls autoMergeRequest, leaving the PR OPEN/CLEAN/all-green and absent from the queue -- indistinguishable from a healthy PR waiting its turn, so nothing re-enqueues it. #1306 sat dead 4h. workflow_run on PR completion, so this runs OUTSIDE the queue and cannot itself eject an entry. The synthetic ref gh-readonly-queue//pr-- is the correlation key; workflow_run.pull_requests is not populated on merge-group refs. Co-Authored-By: Claude --- .../scripts/report-merge-queue-ejection.mjs | 78 +++++++++++++++++++ .../report-merge-queue-ejection.test.mjs | 29 +++++++ .github/workflows/pr.yml | 10 +++ .../workflows/report-merge-queue-ejection.yml | 31 ++++++++ 4 files changed, 148 insertions(+) create mode 100644 .github/scripts/report-merge-queue-ejection.mjs create mode 100644 .github/scripts/tests/report-merge-queue-ejection.test.mjs create mode 100644 .github/workflows/report-merge-queue-ejection.yml diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs new file mode 100644 index 000000000000..f988b9668086 --- /dev/null +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -0,0 +1,78 @@ +#!/usr/bin/env node + +/** + * Report a failed merge-group run on the PR that GitHub ejected. + * + * Merge-group runs use refs such as gh-readonly-queue/master/pr-1306-, + * and do not reliably populate workflow_run.pull_requests. The PR number in + * the synthetic branch is therefore the durable correlation key. + */ + +export function mergeQueuePullRequestNumber(headBranch) { + if (typeof headBranch !== "string") return null; + const match = headBranch.match(/(?:^|\/)pr-(\d+)(?:-|$)/i); + return match ? Number(match[1]) : null; +} + +export function shouldReportMergeQueueFailure({ headBranch, conclusion }) { + // `failure` ONLY, deliberately. Under ALLGREEN a candidate is `cancelled` + // whenever an earlier entry in the group dies, and those PRs stay queued and + // get re-built -- they were not ejected and must not be told they were. + // `failure` is the conclusion that actually drops the entry and nulls + // autoMergeRequest. Baseline at BLO-26675 filing: 12 failure / 48 cancelled, + // so getting this backwards would comment on four times more PRs than it helps. + return mergeQueuePullRequestNumber(headBranch) !== null && conclusion === "failure"; +} + +async function githubRequest(path, options = {}) { + const response = await fetch(`https://api.github.com${path}`, { + ...options, + headers: { + accept: "application/vnd.github+json", + authorization: `Bearer ${process.env.GITHUB_TOKEN}`, + "x-github-api-version": "2022-11-28", + ...(options.headers ?? {}), + }, + }); + if (!response.ok) { + throw new Error(`GitHub API ${response.status} for ${path}`); + } + return response.json(); +} + +export async function reportMergeQueueFailure({ repository, headBranch, runUrl, runId }) { + const number = mergeQueuePullRequestNumber(headBranch); + if (number === null) return { reported: false, reason: "not-merge-group" }; + + const [owner, repo] = repository.split("/", 2); + if (!owner || !repo) throw new Error(`Invalid repository: ${repository}`); + const comments = await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments?per_page=100`); + const marker = ``; + if (comments.some((comment) => comment.body?.startsWith(marker))) { + return { reported: false, reason: "already-reported", number }; + } + + const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run failed and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the failed 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" }, + body: JSON.stringify({ body }), + }); + return { reported: true, number }; +} + +if (import.meta.url === `file://${process.argv[1]}`) { + const { GITHUB_REPOSITORY, GITHUB_WORKFLOW_RUN_HEAD_BRANCH, GITHUB_WORKFLOW_RUN_CONCLUSION, GITHUB_SERVER_URL, GITHUB_WORKFLOW_RUN_ID } = process.env; + if (!shouldReportMergeQueueFailure({ + headBranch: GITHUB_WORKFLOW_RUN_HEAD_BRANCH, + conclusion: GITHUB_WORKFLOW_RUN_CONCLUSION, + })) process.exit(0); + + const result = await reportMergeQueueFailure({ + repository: GITHUB_REPOSITORY, + headBranch: GITHUB_WORKFLOW_RUN_HEAD_BRANCH, + runUrl: `${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_WORKFLOW_RUN_ID}`, + runId: GITHUB_WORKFLOW_RUN_ID, + }); + console.log(result.reported ? `Reported merge-queue ejection for PR #${result.number}` : `Skipped: ${result.reason}`); +} diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs new file mode 100644 index 000000000000..ea7c949703be --- /dev/null +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -0,0 +1,29 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + mergeQueuePullRequestNumber, + shouldReportMergeQueueFailure, +} from "../report-merge-queue-ejection.mjs"; + +test("extracts PR numbers from merge-group synthetic refs", () => { + assert.equal(mergeQueuePullRequestNumber("gh-readonly-queue/master/pr-1306-abc123"), 1306); + assert.equal(mergeQueuePullRequestNumber("refs/heads/gh-readonly-queue/master/pr-1158-def456"), 1158); + assert.equal(mergeQueuePullRequestNumber("fix/BLO-1306"), null); + // The segment anchor is load-bearing: this script POSTS a comment to the number + // it derives, so an unanchored `pr-` match would comment on an unrelated PR. + assert.equal(mergeQueuePullRequestNumber("cto/repr-1306-fix"), null); + assert.equal(mergeQueuePullRequestNumber("fix/pr-notes-1306"), null); + assert.equal(mergeQueuePullRequestNumber("gh-readonly-queue/master/pr-1306extra"), null); +}); + +test("only failed merge-group runs are reportable", () => { + assert.equal(shouldReportMergeQueueFailure({ + headBranch: "gh-readonly-queue/master/pr-1306-abc123", + conclusion: "failure", + }), true); + assert.equal(shouldReportMergeQueueFailure({ + headBranch: "gh-readonly-queue/master/pr-1306-abc123", + conclusion: "success", + }), false); + assert.equal(shouldReportMergeQueueFailure({ headBranch: "fix/BLO-1306", conclusion: "failure" }), false); +}); diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 8dff6012ac26..69a1ce467260 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -620,6 +620,16 @@ jobs: run: node --test ./.github/scripts/tests/post-lockfile-drift-alert.test.mjs timeout-minutes: 1 + # BLO-26675. Same shape as the drift alert above: the only thing this + # reporter does is speak up after a merge-group run has already failed, + # so its failure mode is silence on a path that by definition only runs + # when something else broke. The PR-number extraction is the load-bearing + # part -- it decides WHICH pull request gets commented on. + - name: Test merge-queue ejection reporter (BLO-26675) + if: ${{ !cancelled() }} + run: node --test ./.github/scripts/tests/report-merge-queue-ejection.test.mjs + timeout-minutes: 1 + - name: Validate dependency resolution when manifests change id: regen_lockfile if: ${{ !cancelled() }} diff --git a/.github/workflows/report-merge-queue-ejection.yml b/.github/workflows/report-merge-queue-ejection.yml new file mode 100644 index 000000000000..49476d83fc04 --- /dev/null +++ b/.github/workflows/report-merge-queue-ejection.yml @@ -0,0 +1,31 @@ +name: Report merge-queue ejections + +on: + workflow_run: + workflows: [PR] + types: [completed] + +permissions: + contents: read + issues: write + pull-requests: write + +jobs: + report: + if: ${{ github.event.workflow_run.event == 'merge_group' && github.event.workflow_run.conclusion == 'failure' }} + runs-on: arc-light + timeout-minutes: 5 + steps: + - uses: actions/checkout@v6 + with: + ref: master + sparse-checkout: .github/scripts/report-merge-queue-ejection.mjs + - name: Report ejection to PR + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_REPOSITORY: ${{ github.repository }} + GITHUB_WORKFLOW_RUN_HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }} + GITHUB_WORKFLOW_RUN_CONCLUSION: ${{ github.event.workflow_run.conclusion }} + GITHUB_SERVER_URL: ${{ github.server_url }} + GITHUB_WORKFLOW_RUN_ID: ${{ github.event.workflow_run.id }} + run: node .github/scripts/report-merge-queue-ejection.mjs From 94057ce15a6fb8079b4bea64eb584ac88c6111bb Mon Sep 17 00:00:00 2001 From: CTO Date: Sat, 19 Sep 2026 08:52:43 +0000 Subject: [PATCH 2/3] fix(ci): report cancelled merge-group runs too, suppressed on PR state (BLO-26675) Ally flagged that the `failure`-only conclusion guard had no failing mutation. Measuring it to write that test showed the guard was also wrong: over the full merge_group history (398 PR runs, 2026-08-28 -> 2026-09-18; 295 success / 80 failure / 22 cancelled), only 1 of the 22 cancelled runs matched the premise in the comment. 6 merged at the cancel and 14 were ejected and never re-added, so `failure`-only caught 80/94 = 85% of real ejections. A job hitting `timeout-minutes` also surfaces as `cancelled`, and pr.yml deliberately trades a fast red for a timeout, so that whole class landed in the blind spot. Widen to failure|cancelled and suppress the benign cancels on PR state rather than on conclusion: skip when the PR is merged or still in the queue. `isInMergeQueue` is GraphQL-only -- REST /pulls/{n} reads `mergeable_state: unknown` for a queued PR and cannot answer it. Also fold the conclusion gate into reportMergeQueueFailure so the safe path is the only path, and sort the dedup comment read newest-first. Every guard now has a failing mutation, verified one at a time: conclusion -> `!== "success"` (Ally's backwards case), conclusion widening reverted, each conjunct of shouldReportCancelledRun dropped, and the regex segment anchor unanchored. 5/5 turn the suite red; baseline 3/3 green. Drove the script against live GitHub on the four write-free paths: merged -> "merged", queued -> "still-queued", success and non-merge-group -> "not-reportable". Co-Authored-By: Claude --- .../scripts/report-merge-queue-ejection.mjs | 82 +++++++++++++++---- .../report-merge-queue-ejection.test.mjs | 28 +++++-- .../workflows/report-merge-queue-ejection.yml | 2 +- 3 files changed, 86 insertions(+), 26 deletions(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index f988b9668086..e13e73511035 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -15,13 +15,30 @@ export function mergeQueuePullRequestNumber(headBranch) { } export function shouldReportMergeQueueFailure({ headBranch, conclusion }) { - // `failure` ONLY, deliberately. Under ALLGREEN a candidate is `cancelled` - // whenever an earlier entry in the group dies, and those PRs stay queued and - // get re-built -- they were not ejected and must not be told they were. - // `failure` is the conclusion that actually drops the entry and nulls - // autoMergeRequest. Baseline at BLO-26675 filing: 12 failure / 48 cancelled, - // so getting this backwards would comment on four times more PRs than it helps. - return mergeQueuePullRequestNumber(headBranch) !== null && conclusion === "failure"; + // `failure` and `cancelled`. An earlier revision took `failure` only, on the + // premise that a `cancelled` candidate stays queued and gets re-built. Staff + // Engineer measured the full history (398 merge_group PR runs, 2026-08-28 -> + // 2026-09-18: 295 success / 80 failure / 22 cancelled) and that premise holds + // for 1 of the 22: 6 merged at the cancel, 14 were EJECTED and never + // re-added. `failure`-only therefore catches 80/94 = 85% of real ejections. + // A job hitting `timeout-minutes` also surfaces as `cancelled`, and pr.yml + // deliberately trades a fast red for a timeout -- so that class landed + // entirely in the blind spot too. + // The benign `cancelled` cases are suppressed on PR state, not on + // conclusion: see shouldReportCancelledRun. + return ( + mergeQueuePullRequestNumber(headBranch) !== null && + (conclusion === "failure" || conclusion === "cancelled") + ); +} + +export function shouldReportCancelledRun({ merged, isInMergeQueue }) { + // Only consulted for `cancelled`. `merged` covers the candidate whose group + // landed; `isInMergeQueue` covers the one still queued for a re-build. Read + // at an instant, so a PR mid-re-dispatch can read out-of-queue -- the + // per-runId marker bounds that to one spurious comment rather than a loop. + // Against the measured 22: 14-15 true reports, 0 false alarms. + return !merged && !isInMergeQueue; } async function githubRequest(path, options = {}) { @@ -40,19 +57,56 @@ async function githubRequest(path, options = {}) { return response.json(); } -export async function reportMergeQueueFailure({ repository, headBranch, runUrl, runId }) { +// `isInMergeQueue` is GraphQL-only -- REST /pulls/{n} has no queue-membership +// field, and its `mergeable_state` reads `unknown` for a queued PR (lazy +// compute), so it cannot answer this. +async function pullRequestQueueState(owner, repo, number) { + const payload = await githubRequest("/graphql", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + query: `query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$number){merged isInMergeQueue}}}`, + variables: { owner, repo, number }, + }), + }); + // GraphQL answers 200 with an `errors` array, so response.ok proves nothing. + if (payload.errors?.length) { + throw new Error(`GitHub GraphQL: ${payload.errors.map((e) => e.message).join("; ")}`); + } + const pullRequest = payload.data?.repository?.pullRequest; + if (!pullRequest) throw new Error(`No pull request ${owner}/${repo}#${number}`); + return pullRequest; +} + +export async function reportMergeQueueFailure({ repository, headBranch, conclusion, runUrl, runId }) { + // Gate here as well as in the CLI, so the safe path is the only path. + if (!shouldReportMergeQueueFailure({ headBranch, conclusion })) { + return { reported: false, reason: "not-reportable" }; + } const number = mergeQueuePullRequestNumber(headBranch); - if (number === null) return { reported: false, reason: "not-merge-group" }; const [owner, repo] = repository.split("/", 2); if (!owner || !repo) throw new Error(`Invalid repository: ${repository}`); - const comments = await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments?per_page=100`); + + if (conclusion === "cancelled") { + const state = await pullRequestQueueState(owner, repo, number); + if (!shouldReportCancelledRun(state)) { + return { reported: false, reason: state.merged ? "merged" : "still-queued", number }; + } + } + + // Sorted newest-first: the marker lives in the newest comments, and this is a + // single unpaginated page. + const comments = await githubRequest( + `/repos/${owner}/${repo}/issues/${number}/comments?per_page=100&sort=created&direction=desc`, + ); const marker = ``; if (comments.some((comment) => comment.body?.startsWith(marker))) { return { reported: false, reason: "already-reported", number }; } - const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run failed and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the failed merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`; + 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}`; await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments`, { method: "POST", headers: { "content-type": "application/json" }, @@ -63,14 +117,10 @@ export async function reportMergeQueueFailure({ repository, headBranch, runUrl, if (import.meta.url === `file://${process.argv[1]}`) { const { GITHUB_REPOSITORY, GITHUB_WORKFLOW_RUN_HEAD_BRANCH, GITHUB_WORKFLOW_RUN_CONCLUSION, GITHUB_SERVER_URL, GITHUB_WORKFLOW_RUN_ID } = process.env; - if (!shouldReportMergeQueueFailure({ - headBranch: GITHUB_WORKFLOW_RUN_HEAD_BRANCH, - conclusion: GITHUB_WORKFLOW_RUN_CONCLUSION, - })) process.exit(0); - const result = await reportMergeQueueFailure({ repository: GITHUB_REPOSITORY, headBranch: GITHUB_WORKFLOW_RUN_HEAD_BRANCH, + conclusion: GITHUB_WORKFLOW_RUN_CONCLUSION, runUrl: `${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_WORKFLOW_RUN_ID}`, runId: GITHUB_WORKFLOW_RUN_ID, }); diff --git a/.github/scripts/tests/report-merge-queue-ejection.test.mjs b/.github/scripts/tests/report-merge-queue-ejection.test.mjs index ea7c949703be..09b9e559f355 100644 --- a/.github/scripts/tests/report-merge-queue-ejection.test.mjs +++ b/.github/scripts/tests/report-merge-queue-ejection.test.mjs @@ -3,6 +3,7 @@ import assert from "node:assert/strict"; import { mergeQueuePullRequestNumber, shouldReportMergeQueueFailure, + shouldReportCancelledRun, } from "../report-merge-queue-ejection.mjs"; test("extracts PR numbers from merge-group synthetic refs", () => { @@ -16,14 +17,23 @@ test("extracts PR numbers from merge-group synthetic refs", () => { assert.equal(mergeQueuePullRequestNumber("gh-readonly-queue/master/pr-1306extra"), null); }); -test("only failed merge-group runs are reportable", () => { - assert.equal(shouldReportMergeQueueFailure({ - headBranch: "gh-readonly-queue/master/pr-1306-abc123", - conclusion: "failure", - }), true); - assert.equal(shouldReportMergeQueueFailure({ - headBranch: "gh-readonly-queue/master/pr-1306-abc123", - conclusion: "success", - }), false); +test("failed and cancelled merge-group runs are reportable, nothing else is", () => { + const branch = "gh-readonly-queue/master/pr-1306-abc123"; + assert.equal(shouldReportMergeQueueFailure({ headBranch: branch, conclusion: "failure" }), true); + // 14 of the 22 measured `cancelled` runs were ejected and never re-added, and + // a job timeout surfaces as `cancelled` -- so excluding it missed 15%. + assert.equal(shouldReportMergeQueueFailure({ headBranch: branch, conclusion: "cancelled" }), true); + assert.equal(shouldReportMergeQueueFailure({ headBranch: branch, conclusion: "success" }), false); + // Widening to `!== "success"` would report in-flight and skipped runs. + assert.equal(shouldReportMergeQueueFailure({ headBranch: branch, conclusion: null }), false); + assert.equal(shouldReportMergeQueueFailure({ headBranch: branch, conclusion: "skipped" }), false); assert.equal(shouldReportMergeQueueFailure({ headBranch: "fix/BLO-1306", conclusion: "failure" }), false); }); + +test("a cancelled run is reported only when the PR is neither merged nor still queued", () => { + assert.equal(shouldReportCancelledRun({ merged: false, isInMergeQueue: false }), true); + // The group landed: the cancel was collateral, not an ejection. + assert.equal(shouldReportCancelledRun({ merged: true, isInMergeQueue: false }), false); + // Still queued for a re-build: not ejected. + assert.equal(shouldReportCancelledRun({ merged: false, isInMergeQueue: true }), false); +}); diff --git a/.github/workflows/report-merge-queue-ejection.yml b/.github/workflows/report-merge-queue-ejection.yml index 49476d83fc04..6054d962b747 100644 --- a/.github/workflows/report-merge-queue-ejection.yml +++ b/.github/workflows/report-merge-queue-ejection.yml @@ -12,7 +12,7 @@ permissions: jobs: report: - if: ${{ github.event.workflow_run.event == 'merge_group' && github.event.workflow_run.conclusion == 'failure' }} + if: ${{ github.event.workflow_run.event == 'merge_group' && (github.event.workflow_run.conclusion == 'failure' || github.event.workflow_run.conclusion == 'cancelled') }} runs-on: arc-light timeout-minutes: 5 steps: From c826029df46db8072467d039ba4235cf10b27600 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 20 Sep 2026 06:24:01 +0000 Subject: [PATCH 3/3] docs(ci): drop the sort params GET /issues/{n}/comments ignores (BLO-26675) The per-issue comments endpoint does not honour sort/direction -- only the repo-level /issues/comments does. Measured with a positive control: repo-level flips (asc 2026-05-14 / desc 2026-09-20), per-issue returns an identical first element with and without direction=desc on #1306, #1158 and #1859. So the "sorted newest-first" claim was false and the page was still oldest-first. Runtime behaviour is unchanged either way; what was wrong was the invariant the next editor would trust. Dropping the parameters and naming the ceiling beats a pagination loop for a thread that tops out at 12 comments and whose worst case is one duplicate comment. Also names the other side of the isInMergeQueue race (a PR re-enqueued before this handler fires is never reported), records the throw-on-GraphQL-blip as a deliberate visible-over-silent trade, and states the workflows: [PR] scope as a measured bound -- 98 of 102 non-success merge-group runs, ~96%. Co-Authored-By: Claude --- .../scripts/report-merge-queue-ejection.mjs | 22 ++++++++++++++----- .../workflows/report-merge-queue-ejection.yml | 7 ++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/.github/scripts/report-merge-queue-ejection.mjs b/.github/scripts/report-merge-queue-ejection.mjs index e13e73511035..3c7c77b982c6 100644 --- a/.github/scripts/report-merge-queue-ejection.mjs +++ b/.github/scripts/report-merge-queue-ejection.mjs @@ -35,8 +35,11 @@ export function shouldReportMergeQueueFailure({ headBranch, conclusion }) { export function shouldReportCancelledRun({ merged, isInMergeQueue }) { // Only consulted for `cancelled`. `merged` covers the candidate whose group // landed; `isInMergeQueue` covers the one still queued for a re-build. Read - // at an instant, so a PR mid-re-dispatch can read out-of-queue -- the - // per-runId marker bounds that to one spurious comment rather than a loop. + // at an instant, so the race cuts both ways: a PR mid-re-dispatch reads + // out-of-queue and gets one spurious comment (bounded by the per-runId + // marker), and a PR re-enqueued before this handler fires reads + // isInMergeQueue:true and is never reported at all. The second is the one + // that loses data, and is accepted -- whoever re-added it already knows. // Against the measured 22: 14-15 true reports, 0 false alarms. return !merged && !isInMergeQueue; } @@ -89,16 +92,25 @@ export async function reportMergeQueueFailure({ repository, headBranch, conclusi if (!owner || !repo) throw new Error(`Invalid repository: ${repository}`); if (conclusion === "cancelled") { + // Deliberate: a GraphQL blip here reddens the reporter job rather than + // silently skipping. A silent skip loses an ejection report on a PR that is + // already stuck, which is the failure nobody notices. const state = await pullRequestQueueState(owner, repo, number); if (!shouldReportCancelledRun(state)) { return { reported: false, reason: state.merged ? "merged" : "still-queued", number }; } } - // Sorted newest-first: the marker lives in the newest comments, and this is a - // single unpaginated page. + // One unpaginated page, oldest-first. GET /issues/{n}/comments does NOT honour + // sort/direction -- only the repo-level /issues/comments does; measured on this + // repo, the per-issue endpoint returns an identical first element with and + // without direction=desc (#1306, #1158, #1859), while the repo-level one flips. + // So the marker sits on the LAST page and a thread past 100 comments would + // re-post on every redelivery. Ceiling accepted: the busiest thread here is 12, + // and the blast radius is one duplicate comment. Paginate to the end if a PR + // thread ever approaches 100. const comments = await githubRequest( - `/repos/${owner}/${repo}/issues/${number}/comments?per_page=100&sort=created&direction=desc`, + `/repos/${owner}/${repo}/issues/${number}/comments?per_page=100`, ); const marker = ``; if (comments.some((comment) => comment.body?.startsWith(marker))) { diff --git a/.github/workflows/report-merge-queue-ejection.yml b/.github/workflows/report-merge-queue-ejection.yml index 6054d962b747..92fcff792867 100644 --- a/.github/workflows/report-merge-queue-ejection.yml +++ b/.github/workflows/report-merge-queue-ejection.yml @@ -2,6 +2,13 @@ name: Report merge-queue ejections on: workflow_run: + # Deliberate bound, not an oversight: three workflows emit merge_group runs + # here. Measured over 1000 merge-group runs -- PR 272/79/19 (success/failure/ + # cancelled), commitperclip PR Review 370/2, Comment-review gate (merge queue) + # 255/2. `PR` alone is 98 of the 102 non-success runs, so this covers ~96% of + # ejections. The other two are left out until their failures are shown to + # actually eject; subscribing to a non-required check would comment on PRs + # that were never ejected. workflows: [PR] types: [completed]