fix(ci): give a dispatched run its own concurrency group - #5048
Conversation
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.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe CI workflow now isolates manual dispatch runs by ChangesCI concurrency isolation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Manual dispatches receive independent non-cancelling concurrency groups, while push and pull-request runs retain ref-based supersession. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
리뷰 · 우선순위 76 / 80이 PR은 group: 문제는 단순합니다. 이 PR이 바꾸는 규칙은 두 줄입니다. 그룹 키를 테스트 쪽도 방향이 맞습니다. 새 파일
라인 62-64 (.github/workflows/ci.yml, 현재 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a023f5ef24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,109 @@ | |||
| import { expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
Register the new test in both layout inventories
This new test is absent from both scripts/test-layout/layout.json’s explicit map and tests/fixtures/test-layout-expected.json. Its ci- prefix currently lets the regex seed place it, but that is only the temporary fallback; leaving both inventories unchanged means the authoritative map and its independent oracle do not record the test. Add ci-concurrency-groups.test.ts to both files with the ci-workflows domain.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
|
Merging on a recorded macOS exception. Green at head
Platform risk in this diff is as low as it gets: the change is a concurrency key in |
Summary
A dispatched run shared its concurrency group with pushes, so any merge into
devcancelled it.ci.ymlkeyed the group ongithub.refalone withcancel-in-progress: true, and aworkflow_dispatchagainstdevhas the samegithub.refas apushtodev.The lane this made uncompletable is
macos control, the longest job here at roughly fifty minutes. Run 35318264610 was cancelled in the same second its job started, three minutes after being queued — a concurrency cancellation at startup, not a timeout. On a branch receiving merges the probability that none lands inside a fifty-minute window is close to zero, so a maintainer dispatching the lane for release evidence usually got nothing back, and had no signal that they had: a cancelled job reports neither pass nor fail. This is why the lane produced cancellations for months and why they were read as runner capacity; raising its budget from 30 to 75 minutes in #5028 did not change it.After this change:
pushandpull_requestare unchanged. Superseding an older head on the same ref is exactly right for them, and it is what keeps nine Windows shards and two macOS shards from running for an answer nobody will read.workflow_dispatchis keyed ongithub.run_id, which is unique per run, so every dispatch is a group of one. It cancels nothing, nothing cancels it, and two probes of the same ref no longer queue behind each other — they are two questions, not a revision of one.This retires the frozen-ref workaround (
ci/control-probe-2590at56a99d3848), which existed only to get the lane a ref that nothing else pushes to.Closes #5037.
Verification
tests/ci-workflows/ci-concurrency-groups.test.tsis new and evaluates the two expressions rather than matching their text, because a spelling assertion would pass on any rewrite that kept the words. It implements the narrow grammarci.ymluses —${{ ... }}interpolation,github.<field>lookups, single-quoted literals,==/!=, and thea && b || cternary — and refuses anything outside it, so an expression that later grows a function call fails loudly instead of being silently mismodelled. There is a case asserting that refusal, so the evaluator is not vacuous.The four contracts it pins: a dispatch and a push on
refs/heads/devresolve to different groups and the dispatch is not cancellable; two dispatches of the same ref resolve to different groups;pushandpull_requeststill share a group per ref, differ across refs, and stay cancellable; and the workflow declares exactly the three triggers these cases cover, so a fourth cannot inherit the push answer by accident.No existing test pinned
ci.yml's concurrency block. Thecancel-in-progress: trueassertions intests/ci-workflows/ci-workflows.test.tsbelong toservice-lifecycle.ymlandrelease.ymland are untouched.Hosted CI on this branch is the check; no local suite was run.
Checklist
This touches
.github/workflows/, whichMAINTAINERS.mdputs behind security review. The change adds no permission, no secret, no new trigger, and no checkout of head-controlled code: it only alters which runs cancel which. Workflowpermissions: contents: readis unchanged.Summary by CodeRabbit
Chores
Tests