ci: guard against semantic merge races breaking main#230
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPR Checks now runs for merge queue events. A new workflow monitors current push-based PR Checks runs on ChangesMain CI alerting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PRChecks
participant MainRedAlert
participant GitHubIssues
PRChecks->>MainRedAlert: completed push run on main
MainRedAlert->>MainRedAlert: compare run head SHA with main tip
MainRedAlert->>GitHubIssues: find open ci-main-red issues
MainRedAlert->>GitHubIssues: comment and close recovered alerts
MainRedAlert->>GitHubIssues: comment on or create failure alert
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/main-red-alert.yml:
- Around line 44-69: Serialize the alert workflow by adding a concurrency
configuration for this workflow/job, using a stable group key for the repository
and workflow and setting the queue behavior to retain pending runs (for example,
queue: max). Apply it to the alert execution containing the getLabel/createLabel
and listForRepo/create operations so simultaneous red-main runs cannot race or
create duplicate issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aab31f50-f3be-45f6-8ab3-3d7deddea64a
📒 Files selected for processing (2)
.github/workflows/main-red-alert.yml.github/workflows/pr-checks.yml
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The merge-race prevention here is sound: merge_group: re-validation is not vacuous (no job in pr-checks.yml carries an if:, so all six run in the queue), concurrency never cancels queue or main runs, permissions are least-privilege, and there's no ${{ }} injection surface in the github-script step. The alerter is the weak half: it reflects CI events, not main's current state, and that produces a false stop-ship. Findings below, highest first. The P2s are worth landing before this is a backstop you can trust; none touches the merge-gate itself.
Findings
-
[P2] Gate the alert on the failed run's commit still being main's tip
.github/workflows/main-red-alert.yml:30
The job fires onconclusion == 'failure' && event == 'push'and never checksrun.head_shaagainst main's current head. A re-run of an older failed run keepsevent == 'push'andhead_branch == main, so re-running a red run after a fixing commit has already gone green (or a late-completing concurrent run) opens a fresh "CI is red on main / stop merging anything" issue while main's tip is green. That false stop-ship is the same release-blocker class this PR exists to surface. Comparerun.head_shatogithub.rest.repos.getBranch({ branch: 'main' }).commit.shaand no-op when they differ. -
[P2] Broaden the trigger beyond
conclusion == 'failure'
.github/workflows/main-red-alert.yml:31
A push-to-main run can also concludetimed_out(thetestjob runs 45 min against a Postgres service) orstartup_failure(malformed workflow). Main is red in both and no issue opens, which is the exact silence this workflow exists to end. Suggestconclusion == 'failure' || conclusion == 'timed_out' || conclusion == 'startup_failure', leavingcancelledout since that's usually a human abort. -
[P2] Add
queue: maxso a pending alert isn't displaced under burst merges
.github/workflows/main-red-alert.yml:23
Every push-to-main completion (green and red) entersgroup: main-red-alert; theif:skip happens at job level, after the run has already taken a slot. Actions keeps at most one pending run per group and a newer arrival cancels the older pending one (cancel-in-progress: falseguards only the in-progress run). So when several PRs land in quick succession, a red alert sitting pending can be cancelled by a later, possibly skipped-green, completion and that red main gets no issue.queue: maxis allowed alongsidecancel-in-progress: falseand closes it; it's the sibling of the duplicate-issue race the current concurrency block already fixed. -
[P3] Close or mark-recovered the issue when a push-to-main run goes green
.github/workflows/main-red-alert.yml:28
The job only runs on failure, so nothing clearsci-main-redafter main is fixed; the "stop merging" issue lingers as a stale event log. A green push-to-mainworkflow_runshould resolve openci-main-redissues (or comment "recovered athead_sha" and close). This plus the freshness gate above are what turn the alert from an event stream into a current-state signal. -
[P3] Exclude PRs from the open-issue lookup
.github/workflows/main-red-alert.yml:64
issues.listForReporeturns pull requests too. If any open PR ever carries theci-main-redlabel,open.data[0]is that PR and the alert comments on it instead of creating the tracking issue. Filter entries with apull_requestkey, or queryis:issue. -
[P3]
github-scriptis pinned to v8 here whilepr-triage.ymlpins v7. Theissues.*surface is identical so behavior is unaffected, but worth converging and confirming the SHA is the real v8.0.0 tag.
One thing I couldn't check from the diff: merge_group: only gates if these check names are marked required for the merge queue in branch protection. Runs either way, but confirm that setting or the re-validation won't block.
Two individually-green PRs can break main in combination when the second merges on checks that ran against a stale base (#113 + #145 compiled fine apart, but #145 added a field to ReceivedRefUpdate that #113's new test initializer never set — E0063 on merged main, fixed by #229). - pr-checks: add merge_group trigger so the suite can gate a GitHub merge queue, which re-validates every PR against the true merged result. - main-red-alert: new workflow that opens/updates a labeled issue whenever a push-to-main PR Checks run fails, so a red main is loud instead of silent (release-please PRs inherit main breakage, as #182 showed).
Two near-simultaneous red-main events could both pass the open-issue lookup and each create a ci-main-red issue (or 422 racing createLabel). A workflow concurrency group with cancel-in-progress: false serializes them; the default single-slot pending queue is enough since a superseded alert's state is re-read by the newest run anyway.
Address review: the alerter reflected CI events rather than main's current state, which could both cry wolf and stay silent. - Freshness gate: a run only opens or closes anything when its commit is still main's tip — re-running an old red run after a fix has landed no longer opens a false stop-ship issue (and a stale green can't close a current one). - Trigger covers timed_out and startup_failure — both are red mains that previously alerted nothing. cancelled stays excluded as a human abort. - Recovery: a green run on the tip comments and closes open ci-main-red issues, turning the alert into a current-state signal. - queue: max — the single-slot pending queue let a later completion displace a pending red alert under burst merges. - Issue lookup filters out pull requests carrying the label. - github-script converged on v8.0.0 (SHA verified against the tag) in pr-triage too.
8a24071 to
4465beb
Compare
2 participants
Summary
Guards against semantic merge races: adds a merge-queue trigger to PR Checks and a workflow that opens an issue whenever CI goes red on main.
Motivation & context
#113 and #145 were each green but merged 39s apart on stale bases; combined they broke test-target compilation on main (E0063, fixed in #229), which silently redded CI and blocked release PR #182. Nothing re-tests a PR against the merged result under current settings.
Kind of change
What changed
.github/workflows/pr-checks.yml: addmerge_group:trigger so the suite can gate a GitHub merge queue..github/workflows/main-red-alert.yml(new): on a failed push-to-main PR Checks run, opens/updates aci-main-redissue.How a reviewer can verify