From a023f5ef2436cbea7e1ae5a0c4689281712061e0 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 18 Sep 2026 17:33:54 +0900 Subject: [PATCH] fix(ci): give a dispatched run its own concurrency group A `workflow_dispatch` against `dev` landed in the same concurrency group as the `push` runs on `dev`, because the group was keyed on `github.ref` alone and `cancel-in-progress` was unconditionally true. The next merge therefore cancelled the dispatch. Run 35318264610 was cancelled in the same second its job started, three minutes after it was queued. The lane this hurt is `macos control`, the longest job in the workflow at roughly fifty minutes, so on a branch under active development the odds that no merge landed inside its window were close to zero. A maintainer dispatching it for release evidence usually got nothing back and had no reason to notice, because a cancelled job reports neither pass nor fail. Those cancellations were read as runner capacity for months; raising the budget from 30 to 75 minutes in #5028 did not change them. Supersession is still what `push` and `pull_request` want, so they keep it. A dispatch is keyed on `github.run_id` instead, which is unique per run, so each dispatch is a group of one: it cancels nothing and nothing cancels it, including a second dispatch of the same ref. Closes #5037. --- .github/workflows/ci.yml | 23 +++- .../ci-concurrency-groups.test.ts | 109 ++++++++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 tests/ci-workflows/ci-concurrency-groups.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 49654a08ce1..6219b0a4706 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,8 +60,27 @@ permissions: # Retrigger CI after dir-fsync / oauth deadline follow-ups (tip 34a1ac46). concurrency: - group: cross-platform-ci-${{ github.ref }} - cancel-in-progress: true + # `push` and `pull_request` want supersession: a newer head on the same ref + # makes the older one irrelevant, and cancelling it saves an hour of runners + # for an answer nobody will read. + # + # `workflow_dispatch` is the opposite. An operator dispatching a lane has + # asked for evidence about one specific commit, and the next merge into + # `dev` is not a newer answer to that question — it is an unrelated commit + # that happens to share `github.ref`. Keyed on the ref alone, the merge + # cancelled the dispatch, so the `macos control` lane — the longest job in + # this workflow at roughly fifty minutes — could not complete on any branch + # under active development. Run 35318264610 was cancelled in the same second + # its job started, three minutes after it was queued. Cancellations of that + # shape were read as runner capacity for months, and a maintainer dispatching + # the lane for release evidence usually got nothing back without noticing, + # because a cancelled job reports neither pass nor fail (#5037). + # + # `github.run_id` is unique per run, so each dispatch is a group of one: it + # cancels nothing and nothing cancels it, including a second dispatch of the + # same ref. + group: cross-platform-ci-${{ github.event_name == 'workflow_dispatch' && github.run_id || github.ref }} + cancel-in-progress: ${{ github.event_name != 'workflow_dispatch' }} jobs: # Which Windows runner this run is allowed to use. diff --git a/tests/ci-workflows/ci-concurrency-groups.test.ts b/tests/ci-workflows/ci-concurrency-groups.test.ts new file mode 100644 index 00000000000..420e2b2e8bd --- /dev/null +++ b/tests/ci-workflows/ci-concurrency-groups.test.ts @@ -0,0 +1,109 @@ +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { repoPath } from "../helpers/repo-root"; + +/** + * `ci.yml` decides which runs cancel which through two Actions expressions. A + * test that matched their text would pass on any rewrite that kept the words, + * so this file evaluates them instead, against the narrow grammar they use: + * `${{ ... }}` interpolation, `github.` lookups, single-quoted literals, + * `==` and `!=`, and the `a && b || c` ternary idiom. + * + * The evaluator is a model of GitHub, not GitHub, so it refuses anything + * outside that grammar rather than guessing. An expression that grows a + * function call fails here and gets read by a human, which is the only + * honest outcome for a model that would otherwise quietly stop describing + * the thing it models. + */ +type GithubContext = { event_name: string; ref: string; run_id: string }; + +function term(source: string, github: GithubContext): string | boolean { + const text = source.trim(); + const comparison = /^(.+?)\s*(==|!=)\s*(.+)$/.exec(text); + if (comparison) { + const left = term(comparison[1]!, github); + const right = term(comparison[3]!, github); + return comparison[2] === "==" ? left === right : left !== right; + } + const literal = /^'([^']*)'$/.exec(text); + if (literal) return literal[1]!; + const lookup = /^github\.([a-z_]+)$/.exec(text); + if (lookup && lookup[1]! in github) return github[lookup[1]! as keyof GithubContext]; + throw new Error(`unsupported expression term: ${text}`); +} + +/** GitHub treats `false` and the empty string as falsy; nothing else here can be. */ +const falsy = (value: string | boolean): boolean => value === false || value === ""; + +function evaluate(expression: string, github: GithubContext): string | boolean { + let value: string | boolean = false; + for (const alternative of expression.split("||")) { + value = false; + for (const part of alternative.split("&&")) { + value = term(part, github); + if (falsy(value)) break; + } + if (!falsy(value)) return value; + } + return value; +} + +const render = (template: string, github: GithubContext): string => + template.replace(/\$\{\{(.*?)\}\}/g, (_match, expression: string) => String(evaluate(expression, github))); + +const workflow = Bun.YAML.parse(readFileSync(repoPath(".github", "workflows", "ci.yml"), "utf8")) as { + on?: Record; + concurrency?: { group?: string; "cancel-in-progress"?: string | boolean }; +}; + +function concurrency(github: GithubContext): { group: string; cancels: string } { + const block = workflow.concurrency; + expect(typeof block?.group).toBe("string"); + expect(typeof block?.["cancel-in-progress"]).toBe("string"); + return { + group: render(String(block?.group), github), + cancels: render(String(block?.["cancel-in-progress"]), github), + }; +} + +const DEV = "refs/heads/dev"; +const dispatch = (run_id: string): GithubContext => ({ event_name: "workflow_dispatch", ref: DEV, run_id }); +const push = (run_id: string): GithubContext => ({ event_name: "push", ref: DEV, run_id }); + +test("a merge into dev cannot cancel a lane dispatched against dev", () => { + // #5037: the `macos control` lane is the longest job in this workflow at + // roughly fifty minutes, and under one shared group it was cancelled by the + // next merge every time. Run 35318264610 was cancelled in the same second its + // job started. That made the lane uncompletable on any branch under active + // development, and it reported neither pass nor fail while doing it. + expect(concurrency(dispatch("35318264610")).group).not.toBe(concurrency(push("35321034825")).group); + expect(concurrency(dispatch("35318264610")).cancels).toBe("false"); +}); + +test("a second dispatch of the same ref does not queue behind the first", () => { + // Two probes of the same ref are two questions, not a revision of one. + expect(concurrency(dispatch("1")).group).not.toBe(concurrency(dispatch("2")).group); +}); + +test("push and pull_request still supersede an older head on their own ref", () => { + // The saving this buys is real and must survive: a run per superseded head, + // across nine Windows shards and two macOS shards, for an answer nobody reads. + for (const event of ["push", "pull_request"]) { + const older = { event_name: event, ref: DEV, run_id: "1" }; + const newer = { event_name: event, ref: DEV, run_id: "2" }; + const elsewhere = { event_name: event, ref: "refs/heads/preview", run_id: "3" }; + expect(concurrency(older).group).toBe(concurrency(newer).group); + expect(concurrency(older).group).not.toBe(concurrency(elsewhere).group); + expect(concurrency(older).cancels).toBe("true"); + } +}); + +test("every trigger this workflow declares is one the cases above cover", () => { + // A fourth trigger would arrive with no decision recorded about whether it + // supersedes anything, and would inherit the push answer by accident. + expect(Object.keys(workflow.on ?? {}).sort()).toEqual(["pull_request", "push", "workflow_dispatch"]); +}); + +test("the evaluator refuses an expression it does not model", () => { + expect(() => render("${{ startsWith(github.ref, 'refs/tags/') }}", push("1"))).toThrow(/unsupported expression term/); +});