From 0756be0ce72e54935fb2e8a00a66a25dd5564694 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 26 Aug 2026 03:31:55 +0000 Subject: [PATCH] ci: take the macOS and Windows lanes off pull requests (#10059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: take the macOS and Windows lanes off pull requests Both lanes were costing contributors more than they were telling them. Measured over the 18 hours before this change, on pull requests: - Windows reported 13 failures and 0 successes. The failures are one standing set of Windows-only path and symlink cases — resolved paths into read_many_files, releaseWorktree through an ancestor symlink, the SHA-256 review worktree — repeating across unrelated PRs, so the red X almost never belonged to the diff under it. - macOS queued for a p90 of 42 minutes and up to 159, on a hosted pool this repository does not saturate by itself: 20 of the 63 sampled waits happened with zero macOS jobs of ours running. Neither lane gates a merge — the `main` ruleset carries no required status check — so none of that waiting or noise was buying protection. Leave them on the nightly, the merge queue and dispatch, and drop the pull-request arm plus the classifier job that existed only to feed it. That makes the nightly load-bearing rather than a backstop: it is now the only report of a non-Linux regression, so add a guard for the three ways it could go quiet — the schedule disappearing, a lane no longer accepting it, or a third job joining it and failing the run for reasons that have nothing to do with either platform. The classifier, its script mode and both test files are left in place so restoring the pull-request trigger, once the Windows failures are fixed, is a revert plus two `if` arms. * ci: pin the retired-classifier contract in ci-platform-lanes.test.js The lane change left scripts/tests/ci-platform-lanes.test.js pinning the shape it removed — a sensitive-PR trigger arm and a live classify_platform job — which failed the Test job on this branch. Rewrite the suite to pin the new contract instead: both lanes run on the schedule, the queue and dispatch and on nothing else; the pull-request arm and the classifier are gone whole (a half-restoration — a trigger without its classifier, or the reverse — fails); the classifier's own scripts stay tested so restoring the trigger stays a clean revert; the nightly still reaches exactly the two lanes and its failure still files an issue. That suite already owned the nightly-liveness assertions, so the platform-lane-triggers.test.mjs guard added earlier on this branch duplicated it — dropped, along with its HELPER_TESTS entry. Verified by mutation: deleting the schedule, dropping schedule from one lane, letting a third job onto the nightly, and reintroducing the pull-request arm without its classifier each fail the suite; the branch shape passes 17/17. --- .github/workflows/ci.yml | 151 ++++++++---------------- scripts/tests/ci-platform-lanes.test.js | 135 ++++++++------------- 2 files changed, 98 insertions(+), 188 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2497c9699..0c75cb0408 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,13 +12,15 @@ on: - 'main' - 'release/**' merge_group: - # A daily heartbeat for the macOS and Windows lanes ONLY. Those two are - # gated on `merge_group`, and the merge queue is not enabled on this - # repository — no queue run since 2026-07-02 — so they had stopped running - # entirely: skipped on every pull request, and never reached afterwards. The - # pull-request gate below catches the diffs a path list can recognise; this - # catches everything it cannot, one day later, on `main`. Every other job - # here excludes `schedule` explicitly, so a nightly run is exactly two jobs. + # The macOS and Windows lanes' ONLY remaining trigger, and therefore this + # repository's only signal about a host that is not Linux with a GNU + # userland. Those two are otherwise gated on `merge_group`, and the merge + # queue is not enabled here — no queue run since 2026-07-02 — while their + # pull-request trigger is off until the standing Windows failures are fixed + # (see test_macos). A regression therefore surfaces here, one day later, on + # `main`, and nowhere else: treat a red nightly as a blocker, not as noise. + # Every other job here excludes `schedule` explicitly, so a nightly run is + # exactly two jobs. schedule: - cron: '17 19 * * *' workflow_dispatch: @@ -152,71 +154,6 @@ jobs: echo "ubuntu_runner=${ubuntu_runner}" >> "${GITHUB_OUTPUT}" echo "Selected Linux runner: ${ubuntu_runner}" - # Does this pull request need the macOS and Windows lanes? They are the only - # signal this repository has about a host that is not Linux with a GNU - # userland, and they are expensive, so they run on the diffs whose behaviour - # the HOST decides — shell, CI definitions, the script layer, the runner - # config, the platform-coupled subtrees. The classifier is a net, not a - # proof; the scheduled run on `main` is what covers everything a path list - # cannot see. - # - # Its own job, not a step in classify_pr: that job's outputs pick the Linux - # runner for everything else, and a new failure mode there (this one needs a - # checkout, on a pool whose workspace other jobs can poison) would take the - # whole run's routing with it. Here a failure costs one skipped - # classification, which the gate reads as "run the lanes". - # - # Hosted, and the checkout is of the pull request's BASE commit: this runs - # before any review, so checking out the contributor's head would run their - # classifier with this job's token, and staying off the ECS pool keeps it - # away from the poisoned-workspace class entirely. - classify_platform: - name: 'Classify platform sensitivity' - if: "${{ github.event_name == 'pull_request' }}" - runs-on: 'ubuntu-latest' - timeout-minutes: 5 - continue-on-error: true - permissions: - contents: 'read' - pull-requests: 'read' - outputs: - platform_sensitive: '${{ steps.platform.outputs.platform_sensitive }}' - steps: - - name: 'Check out the classifier from the base branch' - uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 - with: - ref: '${{ github.event.pull_request.base.sha }}' - persist-credentials: false - sparse-checkout: '.github/scripts/ci' - - - name: 'Classify platform sensitivity' - id: 'platform' - env: - GH_TOKEN: '${{ github.token }}' - PR_NUMBER: '${{ github.event.pull_request.number }}' - IS_SAME_REPO_PR: '${{ github.event.pull_request.head.repo.full_name == github.repository }}' - run: |- - set -uo pipefail - # Fail-safe in every direction: only a confident `false` from the - # classifier skips the lanes. A classifier error, an unexpected - # word, a fork PR, or this whole job failing all end as "run" — a - # gate that fails open stops testing without ever saying so. - sensitive=true - if [ "${IS_SAME_REPO_PR}" = 'true' ]; then - set +e - classified="$(.github/scripts/ci/classify-pr-profile.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" platform)" - rc=$? - set -e - case "${rc}:${classified}" in - 0:true|0:false) sensitive="${classified}" ;; - *) echo "::warning::Platform-sensitivity classifier returned rc=${rc} '${classified}'; running the macOS and Windows lanes." ;; - esac - else - echo "Fork PR detected; running the macOS and Windows lanes." - fi - echo "platform_sensitive=${sensitive}" >> "${GITHUB_OUTPUT}" - echo "Platform-sensitive: ${sensitive}" - # # Test: Node # @@ -916,13 +853,35 @@ jobs: # `realpath -m` in a workflow guard, with the suite that pinned it red on # every Mac). # - # Three triggers now, in cost order: a pull request whose diff the - # platform-sensitivity classifier recognises (shell, CI definitions, the - # script layer, the runner config, the platform-coupled subtrees), the merge - # queue if it is ever enabled again, and a nightly run on `main` for - # everything a path list cannot see. The pull-request gate is fail-safe — - # only a confident `false` skips, so a broken classifier costs runner minutes - # rather than coverage. + # The pull-request trigger is OFF, and this is the whole reason the nightly + # above exists. Over the 18 hours measured before it was pulled, the Windows + # lane reported 13 failures and 0 successes on pull requests — one standing + # set of Windows-only path and symlink failures (resolved paths into + # read_many_files, releaseWorktree through an ancestor symlink, the SHA-256 + # review worktree) repeating across unrelated PRs — and macOS queued for a + # p90 of 42 minutes, 159 at worst, on a hosted pool this repository does not + # saturate by itself. Neither lane gates a merge: the `main` ruleset carries + # no required status check. So the cost landed entirely on contributors, who + # read a red X no diff of theirs caused and waited on checks that never had + # to pass. + # + # Two triggers remain, in cost order: the merge queue if it is ever enabled + # again, and a nightly run on `main` — which is now the only thing keeping + # macOS and Windows visible at all, so it is load-bearing rather than a + # backstop. Read a red nightly as a blocker; nothing else will report it. + # + # Restoring the pull-request trigger is the last step of fixing those + # failures, not a separate decision — and it is a REVERT of the commit that + # carried this change, not an edit: the classifier job, its `if` arms on + # both lanes, and the contract pins in scripts/tests/ci-platform-lanes. + # test.js all moved together, and that suite now fails a half-restoration + # (a trigger without its classifier, or the reverse). The classifier script + # `classify-platform-sensitivity.mjs`, the `platform` mode of + # `classify-pr-profile.sh` and both their test files were left untouched + # precisely so the revert stays clean. Worth revisiting at the same time: + # that classifier calls every fork pull request platform-sensitive, which + # is most of the traffic here and most of what the macOS queue was spent + # on. # # Two named jobs, not a matrix: a skipped matrix job reports one collapsed # check name, never the per-OS contexts, so a required-check configuration @@ -932,17 +891,13 @@ jobs: # currently insurance, not a live constraint.) test_macos: name: 'Test (macos-latest, Node 22.x)' - needs: - - 'classify_pr' - - 'classify_platform' + needs: 'classify_pr' if: |- ${{ !cancelled() && ( github.event_name == 'merge_group' || github.event_name == 'schedule' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - needs.classify_platform.outputs.platform_sensitive != 'false') + github.event_name == 'workflow_dispatch' ) }} runs-on: 'macos-latest' @@ -1022,28 +977,24 @@ jobs: # Windows counterpart of test_macos (see that job's note). ECS is the default # with a windows-2022 kill-switch fallback; the check name stays unchanged so - # it matches the required-status-check context. The lane now runs on pull - # requests too, but every pull request runs on hosted windows-2022: a - # pull_request run executes the workflow YAML from the PR's own merge commit, - # so any PR this lane admits could rewrite `runs-on` in the same diff that - # reaches it. A gate the gated tree controls is no gate; the pool is reached - # only by triggers an unreviewed PR cannot open — the post-approval merge - # queue, schedule and dispatch. ECS-only tuning is gated on runner.environment; - # the hosted fallback is the pre-ECS job plus the checkout guard and a - # job-level timeout-minutes. + # it matches the required-status-check context. The `!= 'pull_request'` arm + # of `runs-on` is inert while the pull-request trigger is off, and stays for + # when it returns: a pull_request run executes the workflow YAML from the + # PR's own merge commit, so any PR this lane admits could rewrite `runs-on` + # in the same diff that reaches it. A gate the gated tree controls is no + # gate; the pool is reached only by triggers an unreviewed PR cannot open — + # the post-approval merge queue, schedule and dispatch. ECS-only tuning is + # gated on runner.environment; the hosted fallback is the pre-ECS job plus + # the checkout guard and a job-level timeout-minutes. test_windows: name: 'Test (windows-latest, Node 22.x)' - needs: - - 'classify_pr' - - 'classify_platform' + needs: 'classify_pr' if: |- ${{ !cancelled() && ( github.event_name == 'merge_group' || github.event_name == 'schedule' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - needs.classify_platform.outputs.platform_sensitive != 'false') + github.event_name == 'workflow_dispatch' ) }} runs-on: '${{ vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'' && github.event_name != ''pull_request'' && fromJSON(''["self-hosted", "Windows", "X64", "ecs-win"]'') || fromJSON(''["windows-2022"]'') }}' diff --git a/scripts/tests/ci-platform-lanes.test.js b/scripts/tests/ci-platform-lanes.test.js index 056f43f09c..6be3996d7e 100644 --- a/scripts/tests/ci-platform-lanes.test.js +++ b/scripts/tests/ci-platform-lanes.test.js @@ -12,8 +12,19 @@ // repository's only non-Linux, non-GNU signal was silently off, and a macOS // failure shipped and sat in `main` (#9220). Nothing here can prove a lane // ran; what these tests hold is the wiring that lets it: the triggers, the -// fail-safe direction of the gate, the nightly's blast radius, and the -// alerting that makes a nightly failure visible. +// nightly's blast radius, and the alerting that makes a nightly failure +// visible. +// +// The pull-request trigger and its platform-sensitivity classifier are OFF +// while the standing Windows failures are being fixed (see the note above +// test_macos in ci.yml): on pull requests the Windows lane was reporting +// failures on every PR for defects no PR caused, and neither lane gates a +// merge. That leaves the nightly as the lanes' ONLY live trigger, so the +// assertions here are the wiring that keeps it alive — the schedule exists, +// both lanes accept it, nothing else rides it, and its failure files an +// issue. Restoring the pull-request path means reverting the commit that +// carried this change; the classifier script and its tests were left in +// place so that stays a revert. import { readFileSync } from 'node:fs'; import { describe, expect, it } from 'vitest'; @@ -39,18 +50,25 @@ describe('platform lanes — triggers', () => { }); for (const lane of LANES) { - it(`${lane} runs on the schedule, the queue, a dispatch, and a sensitive PR`, () => { + it(`${lane} runs on the schedule, the queue, and a dispatch`, () => { const cond = condOf(lane); // Presence AND the disjunction between clauses: an `&&` where a `||` // belongs leaves the gate unsatisfiable for a trigger (event_name is // single-valued) while a presence-only check stays green. - expect(cond).toMatch(/event_name == 'schedule'\s*\|\|/); expect(cond).toMatch(/event_name == 'merge_group'\s*\|\|/); - expect(cond).toMatch(/event_name == 'workflow_dispatch'\s*\|\|/); - expect(cond).toContain("github.event_name == 'pull_request'"); - expect(cond).toContain( - 'needs.classify_platform.outputs.platform_sensitive', - ); + expect(cond).toMatch(/event_name == 'schedule'\s*\|\|/); + expect(cond).toContain("github.event_name == 'workflow_dispatch'"); + }); + + it(`${lane} stays off the pull-request path while it is red there`, () => { + // Half a restoration is worse than none: a pull_request arm put back + // without its classifier (or the reverse) either runs the lanes on + // every PR or consults an output no job produces. Restore the trigger + // by reverting the commit that removed it, not by editing one side. + const cond = condOf(lane); + expect(cond).not.toContain("'pull_request'"); + expect(cond).not.toContain('platform_sensitive'); + expect(ci.jobs[lane].needs).not.toContain('classify_platform'); }); it(`${lane}'s triggers are alternatives, not requirements`, () => { @@ -67,11 +85,8 @@ describe('platform lanes — triggers', () => { cond.lastIndexOf(')'), ); expect(group).toContain("github.event_name == 'schedule'"); - expect(group.split('||').length).toBeGreaterThanOrEqual(4); - // The only `&&` allowed inside the group is the one binding the - // pull-request clause to its classifier output. + expect(group.split('||').length).toBeGreaterThanOrEqual(3); for (const clause of group.split('||')) { - if (clause.includes('platform_sensitive')) continue; expect( clause, `event clause is conjoined: ${clause.trim()}`, @@ -79,17 +94,10 @@ describe('platform lanes — triggers', () => { } }); - it(`${lane} skips only on a confident 'false'`, () => { - // The fail-safe direction is the whole design: `== 'true'` would turn - // every classifier error, every skipped classify job and every empty - // output into a silently skipped lane. `!= 'false'` spends runner - // minutes instead of coverage. - const cond = condOf(lane); - expect(cond).toContain("platform_sensitive != 'false'"); - expect(cond).not.toContain("platform_sensitive == 'true'"); - // And the gate must survive a skipped or failed classifier job. - expect(cond).toContain('!cancelled()'); - expect(ci.jobs[lane].needs).toContain('classify_platform'); + it(`${lane} survives an upstream skip`, () => { + // classify_pr is still a `needs` edge; without `!cancelled()` a skip + // or failure there would skip the lane on the nightly too. + expect(condOf(lane)).toContain('!cancelled()'); }); } @@ -166,75 +174,26 @@ describe('platform lanes — triggers', () => { }); }); -describe('platform lanes — the sensitivity classifier job', () => { - const job = ci.jobs.classify_platform; - - it('exists, is cheap, and cannot take the run down with it', () => { - expect(job).toBeDefined(); - expect(job['continue-on-error']).toBe(true); - expect(job['timeout-minutes']).toBeLessThanOrEqual(10); - // Hosted on purpose: it needs a checkout, and the persistent pool's - // workspace is exactly what other jobs have poisoned before. - expect(job['runs-on']).toBe('ubuntu-latest'); - expect(job.outputs.platform_sensitive).toContain( - 'steps.platform.outputs.platform_sensitive', - ); +describe('platform lanes — the retired sensitivity classifier', () => { + it('is gone from the workflow, whole', () => { + // Off with the pull-request trigger it fed: nothing consumes its output, + // so a surviving job would spend a hosted runner per pull request on a + // classification no gate reads — and a surviving reference would consult + // a job that no longer exists. Deleted means deleted everywhere. + expect(ci.jobs.classify_platform).toBeUndefined(); + expect(JSON.stringify(ci)).not.toContain('classify_platform'); }); - it('checks out the base commit, never the pull request head', () => { - // This job runs before any review and executes a script from the tree it - // checks out. The contributor's head would be the contributor's - // classifier, running with this job's token. - const checkout = job.steps.find((s) => - String(s.uses ?? '').includes('actions/checkout'), - ); - expect(checkout).toBeDefined(); - expect(checkout.with.ref).toBe('${{ github.event.pull_request.base.sha }}'); - expect(checkout.with.ref).not.toContain('head'); - expect(checkout.with['persist-credentials']).toBe(false); - }); - - it('answers "run the lanes" for anything it is not sure about', () => { - const run = job.steps.find((s) => s.id === 'platform').run; - // A fork PR is not classified at all — the listing call is the same one - // the profile gate restricts to same-repo PRs. - expect(run).toContain('IS_SAME_REPO_PR'); - expect(run).toContain('sensitive=true'); - // Only the two words the classifier is allowed to say are accepted; a - // non-zero exit or anything else warns and runs the lanes. - expect(run).toContain('0:true|0:false'); - expect(run).toMatch(/::warning::.*running the macOS and Windows lanes/); - // The wrapper call is wrapped in `set +e`/`set -e`: the runner invokes - // `shell: bash` steps with `-e`, so without the guard a non-zero exit - // aborts the step at the assignment and the warn-and-run case above is - // dead code. Same shape as the sibling Classify CI profile step. - expect(run).toContain( - [ - ' set +e', - ' classified="$(.github/scripts/ci/classify-pr-profile.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" platform)"', - ' rc=$?', - ' set -e', - ].join('\n'), - ); - }); - - it('drives the classifier through the shared listing wrapper', () => { - // Not a second listing: the wrapper's comment declares itself the single - // home of that contract, and two call sites listing separately is how the - // same PR ends up classified differently in two places. - const run = job.steps.find((s) => s.id === 'platform').run; - expect(run).toContain( - '.github/scripts/ci/classify-pr-profile.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" platform', - ); - }); - - it('runs the classifier unit tests in CI', () => { - // The helper-test list is the single place both the github_ci_only step - // and the full Test step read; a classifier not named there is untested - // on every profile. + it('keeps its classifier script tested for the restoration', () => { + // The script layer stayed in place precisely so restoring the + // pull-request trigger is a revert. A classifier that rotted untested in + // the meantime would make that revert a regression instead. expect(ci.env.HELPER_TESTS).toContain( '.github/scripts/ci/classify-platform-sensitivity.test.mjs', ); + expect(ci.env.HELPER_TESTS).toContain( + '.github/scripts/ci/classify-pr-profile.test.mjs', + ); }); });