ci: cancel a PR's still-running CI when the PR closes - #107
Conversation
Merging or closing a PR does not stop its in-flight runs - GitHub leaves them to finish. Mostly harmless on hosted runners, but this repo's heavy suites run on a small self-hosted pool, where an abandoned run holds an executor a live PR needs. Observed 2026-08-03: five branches' runs queued against five online runners (highcpu-1 offline), and three of those branches had already merged. The per-suite concurrency in pr-highcpu-fast-feedback.yml cannot help - it only supersedes the same suite on the same branch, and nothing about a merge looks like a new push. Covers closed-unmerged too, since an abandoned PR's run is just as wasteful. Two guards worth keeping: it refuses to act when the head ref is the default branch (a merge queues master's own CI under that name, and cancelling the merge commit's build would be actively harmful), and it skips runs whose head repository differs from the PR's, because branch names are not unique across forks. A 409 from a run that finished between listing and cancelling is logged, not failed - the goal is already met. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 3m 48s —— View job Code Review:
|
|
Claude finished @astubbs's task in 2m 4s —— View job Review: PR #107 — cancel a PR's still-running CI when the PR closes
SummarySolid, well-documented addition. The two stated guards (refuse on default branch, skip mismatched head repos) are both implemented, the Finding: fork-repo guard is bypassed when
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
Review finding on #107, and a real bug rather than a nit. The fork-repo guard read: if (headRepo && runRepo && runRepo !== headRepo) continue; GitHub sets pull_request.head.repo to null once the source fork is deleted, which is routine - contributors often delete a fork straight after merge. headRepo is then null, the condition short-circuits to false for EVERY run, and the guard silently disables itself: the job would cancel any run matching the branch name in any repository, including a live PR from another contributor on a branch called 'fix' or 'main'. Precisely the scenario the guard exists to prevent, and it failed open. Now: if (runRepo && (!headRepo || runRepo !== headRepo)) continue; so an unverifiable owner means leave it alone. The log line says which case it hit. The reasoning is recorded next to the check, since 'why is this not the simpler condition' is exactly what invites reverting it. Also documents the reviewer's second, informational finding: fork PRs get a read-only GITHUB_TOKEN whatever the permissions block says, so this workflow cannot cancel a fork PR's runs at all. It does not undercut the goal - the self-hosted heavy jobs are already same-repo only - but it is worth knowing if that ever changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 1m 46s —— View job Code Review:
|
|
Fixed — and this was a real bug, not a nit. Thank you for tracing the null case rather than reading the condition as written. The guard failed open, in exactly its own scenarioif (headRepo && runRepo && runRepo !== headRepo) continue; // beforeYou're right that So the one case the guard exists for is the one case it stopped working. Fixed with your suggested form: if (runRepo && (!headRepo || runRepo !== headRepo)) continue; // after"Cannot verify the owner" now means leave it alone rather than no filter. The log line distinguishes the two cases, and I've recorded the reasoning beside the check — "why isn't this the simpler condition?" is exactly the question that invites reverting it. Second finding recorded rather than dismissedYou're right that On the duplicate reviewsYou reviewed this PR twice, 23 seconds apart, and both passes found this same finding. That's fixed separately in #109: Worth noting the sequence is self-inflicted rather than rare — #104's changelog gate requires the entry to cite the PR number, which doesn't exist until the PR is open, so opening is routinely followed within seconds by a push. 🤖 Generated with Claude Code |
* ci(review): one review per PR at a time, newest wins PR #107 got two bot reviews 23 seconds apart. claude-code-review.yml had no concurrency group at all - unlike maven.yml, pr-checklist.yml, copyright.yml and the highcpu lane - so the run started by opening the PR was never superseded by the run started by the next push. Both completed, both posted. The triggering sequence is not unusual, it is what this repo's own rules produce: #104's changelog gate requires the entry to cite the PR number, and that number does not exist until the PR is opened. So opening a PR is routinely followed within seconds by a push to add the reference - which is exactly the double-trigger. Keyed on the PR number rather than the ref, so it also covers reopened PRs and cannot collide with anything else keyed on a branch name. Deliberately a separate PR rather than folded into #107: touching claude-code-review.yml makes a PR unreviewable by the bot (claude-code-action refuses to run when the workflow differs from the default branch's copy), and #107's fork-repo and default-branch guards are worth having reviewed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * docs(changelog): cite this PR (#109) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * ci(changelog-gate): cite the issue, not this PR Requiring the entry to cite its own PR was a nuisance by construction: the number does not exist until the PR is opened, so every PR needed a second push purely to add the reference. That push is also what double-triggered the review workflow, which is the other half of this PR. An issue number is known before the work starts, and 'which reported problem does this address' is what a changelog reader actually wants - the PR is an implementation detail they can reach from the issue. Two deliberate constraints: - The citation must be an explicit /issues/ link. A bare #NN cannot be told apart from a PR reference without an API call, since GitHub numbers issues and pull requests from one sequence - and 'cite the issue' is the entire point. Fork and upstream issues both count. - Only user-visible sections require one: Breaking, Improvements, Fixes, Examples. Build & CI is exempt, and that is not laziness - of the 12 Build & CI entries predating any of this, 7 cite nothing at all and the rest cite a PR. This project's tooling work is self-directed and has no issue behind it, so requiring one would mean inventing issues or writing changelog-ref: N/A on every CI PR - the same paperwork this change removes, pointing the other way. Existing entries without an issue are left alone. Only newly ADDED entries are checked, so nothing needs backfilling. The opt-out is unchanged for the genuine no-issue case. AGENTS.md's reference convention is updated to match, since it documented the old rule. 34 unit tests, including that a pull link and a bare #NN both correctly fail to count as issue citations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * ci(changelog-gate): read the changelog, and drop the clever half Review on #109 found the section detection was inert, and proved it against this PR's own diff: git has no funcname pattern for asciidoc, so a hunk header for CHANGELOG.adoc reads '@@ ... @@ endif::[]' rather than the heading, and entries are one long line each so three lines of context never reach one either. Section resolved to null for essentially every real entry - and since an unknown section counted as exempt, the gate passed everything silently. It was decoration. Fixed by reading the CHANGELOG itself rather than inferring from the patch. The job already checks the repo out, so the file is right there: find the added line, walk back to the nearest '=== ' heading. Verified end to end against the real changelog - a real Build & CI entry resolves to Build & CI and passes, a real 'fix:' entry with no issue link is flagged. Also took the chance to cut this down, per feedback that it had got too complex for what it is. Gone: Dice-coefficient similarity, edit scoring, content-based pairing of removed against added bullets, EDIT_THRESHOLD and its pinned mispairing limitation. 176 -> 96 lines of logic, 269 -> 147 of tests. What that costs, stated in the code rather than discovered later: editing an old uncited entry now asks for a citation it never had. Telling edits from additions needs the fuzzy matching that was just removed - which was the largest and subtlest part of the file and still mispaired same-template entries. The changelog-ref: N/A opt-out covers the rare case. That is a better trade than machinery nobody can follow, for a check that exists to remind us, not to withstand an adversary. The test suite now models a realistic patch - no heading anywhere in the visible context - which is exactly the case the old tests all avoided by putting '=== Fixes' in the synthetic hunk header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Description
Merging or closing a PR does not stop its in-flight CI. GitHub leaves those runs to finish, and on hosted runners that's mostly harmless — but this repo's heavy suites run on a small self-hosted pool, where an abandoned run holds an executor a live PR needs.
Observed on 2026-08-03: five branches' runs queued against five online runners (
highcpu-1offline), and three of those branches had already merged.The existing per-suite
concurrencyinpr-highcpu-fast-feedback.ymlcan't help here. Its group ishighcpu-${suite}-${ref}, so it only supersedes the same suite on the same branch — and nothing about a merge looks like a new push.What it does
On
pull_request: closed, cancels that PR'squeued/in_progress/waiting/requested/pendingruns for its head branch.Applies to closed-unmerged too — an abandoned PR's run is just as wasteful as a merged one's.
Two guards, both deliberate:
feature/xon a fork must not cancelfeature/xhere.A
409from a run that finished between listing and cancelling is logged rather than failed — the goal (that run no longer holds a runner) is already met.Needs
actions: writeto cancel runs.Notes
actionlint.highcpu-1andmac-laptopare offline, so the pool is running at reduced capacity. That's an infrastructure matter, not a workflow one.Checklist
CHANGELOG.adoc) - Build & CI, citing this PR;README.adocregeneratedconcurrencycannot cover this caseactionlint. It cannot be exercised before merge (it triggers onpull_request: closed), so this PR's own merge is its first real runubuntu-latest, not the self-hosted box. Addsactions: write, scoped by the two guards above so it can only cancel runs belonging to the closing PR's own head repo and never the default branch🤖 Generated with Claude Code