fix: split docker-build.yml concurrency group by event_name to eliminate push/pull_request race - #1239
Merged
Merged
Conversation
…ate push/pull_request race
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
… CI comment docker-build.yml's new "See GH #1235 / docs/plans/current_spec.md for the full rationale" comment (added by this PR) pointed to a plan that only ever existed uncommitted, locally stashed on this branch — docs/plans/current_spec.md is shared scratch state and has since been overwritten by an unrelated plan on every branch, so the comment's cross-reference resolved to the wrong document. Recovers the #1235 plan from that stash and commits it permanently under docs/plans/archive/, per this repo's established convention (e.g. b23e449, 6562a64) of preserving finalized plans there once current_spec.md moves on to the next piece of work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-${{ github.event_name }}todocker-build.yml'sconcurrency.groupkey sopushandpull_requestruns for the same branch land in categorically different concurrency groups and can never cancel each other, regardless of registration order.push.branchesto note the group split and cross-reference Redesign docker-build.yml concurrency group to eliminate residual push/pull_request race #1235, so a future reader doesn't assume the concurrency group and the job-levelif:skip (added in fix: skip redundant pull_request docker-build run for main/development-head PRs #1236) are redundant.Closes #1235.
Why (production incident)
#1236 added a job-level
if:skip so apull_requestrun for a main/development-head PR self-resolves to a cleanskippedconclusion instead of a misleadingcancelledred X. But that fix can only clean up the reported status of whichever run loses the concurrency slot — GitHub decides which run to cancel at workflow-run registration time, before any job'sif:is evaluated, so it cannot influence which run wins.This was proven in production within minutes of #1236 merging to
main(merge commit6ed08db3):pushcancelledpull_requestskippedThe
pushrun — the one supposed to do the real build — was cancelled, not thepull_requestrun. Net effect: zero real builds for the commit that landed onmain, and the published image tag went stale until a manualworkflow_dispatchrebuild was run as a stopgap.Research summary (full detail in
docs/plans/current_spec.md§3)Before merging this, every workflow that consumes
docker-build.yml'sworkflow_runcompletion event was re-checked, since splitting the concurrency group means each race commit now produces twoworkflow_runcompletion events instead of (at best) one — one frompush(real build outcome) and one frompull_request(alwaysskipped, structurally guaranteed by #1236's unchanged job-level gate).Verdict: safe to change only this file. All nine
workflow_run: workflows: ["Docker Build, Publish & Test"]consumers were read in full:auto-changelog.ymlconclusion == 'success'auto-versioning.ymlconclusion == 'success'docs-to-issues.ymlconclusion == 'success'docs.yml(build)conclusion == 'success'deployjob has a pre-existing, unrelated gap — not caused/worsened by this PR — tracked separately as #1237)dry-run-history-rewrite.ymlconclusion == 'success'history-rewrite-tests.ymlconclusion == 'success'propagate-changes.ymlconclusion == 'success'security-pr.ymlevent == 'pull_request' && conclusion == 'success'supply-chain-verify.ymlworkflow_run.event == 'pull_request'outrightEvery consumer gates on
conclusion == 'success'(directly or transitively), and thepull_request-sourced race event is structurally guaranteed to concludeskipped— so none of the nine treat it as actionable. No other file requires a change.Also confirmed:
setupjobif:, unchanged by this PR) gates purely onevent_name/head_ref, not on concurrency grouping — so it continues to correctly prevent a redundant real build on thepull_requestside regardless of which group it's in.scan-pr-image'sif:depends only on intra-runneeds:outputs, never on the concurrency group.main/developmentdepends ondocker-build.ymlreporting exactly one conclusion per commit (verified live viagh api repos/Wikid82/Charon/branches/main/protection→ 404, and the one active ruleset has norequired_status_checksrule).Follow-up issues filed
docs.yml'sdeployjob doesn't gate onneeds.build.result == 'success'(pre-existing, unrelated to this fix, low priority).Validation
actionlint .github/workflows/docker-build.yml— zero errorsactionlint .github/workflows/*.yml— zero errors (full-directory check, since actionlint validates cross-fileworkflow_runreferences)lefthook run pre-commit— zero errors (all fast hooks, including actionlint and semgrep)group:line and the one comment block changed, nothing else in the 1340-line filepull_requestrun watched end-to-end on the feature branch head (see PR checks) — confirms the new concurrency group value doesn't break an ordinary feature-branch PR runpush+pull_requestrace againstmain/developmentrequires either unsafely pushing directly to a real production branch, or standing up disproportionate scratch infrastructure — both explicitly rejected. Full confirmation deferred to Post-merge monitoring: verify docker-build.yml push/pull_request race is actually eliminated (GH #1235 follow-up) #1238 (next organic race event, checked viagh run list --workflow=docker-build.yml).Do not merge yet — pending final review.