ci: test master's head so a direct-merge regression is visible (BLO-27871) - #1391
Conversation
…7871) pr.yml triggers on pull_request and merge_group only, so master's head is never tested when a commit lands outside the merge queue. On 2026-08-16 a 12-PR direct-merge batch put master red at 04:12Z; nothing reported it and the only signal was four PRs ejecting from the queue one at a time, each burning a full ~30-minute merge_group build to rediscover the identical failure. Master stayed red ~5.5h and the queue was blocked ~7h (BLO-27861). Adds master-health.yml: on push to master, a gate job checks for a successful pr.yml merge_group run at the same SHA and skips the suite when one exists, so commits that landed through the queue cost nothing. Commits that landed outside it run the same general_tests matrix pr.yml uses. Verified against the real incident: the gate returns skip for the queue-landed chain (a9b48d9 -> 6646531 -> d4fb292) and build for 12ce928, the direct-merged commit that actually broke master. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d42c649
Well-reasoned workflow with unusually good empirical backing. Two gaps between the stated fail-closed invariant and what the YAML actually does.
Critical Issues (0)
Important Issues (2)
-
[gstack/review]
.github/workflows/master-health.yml:109— Theif:has noalways(), so a gate job failure fails open: the suite is skipped entirely and the master head goes untested. The fail-closed handling at lines 90-93 covers only the case wheregh apiexits non-zero inside a running step. It does not cover the job itself failing — ARC pod unavailable, thetimeout-minutes: 5at line 55 firing on a network stall, or any runner-level fault. In GitHub Actions a job whoseif:contains no status-check function still carries an implicitsuccess()on itsneeds, sogeneral_testsis skipped rather than run. The run does go red, so it is not silent — but it is red for an infra reason, the commit is never actually tested, and nothing retries it: the next push supersedes this run viacancel-in-progress. That is precisely the "silently skipped suite" the header comment says must never happen.if: always() && (needs.gate.outputs.pretested != '1' || github.event_name == 'workflow_dispatch'). With the gate failed,needs.gate.outputs.pretestedis empty,'' != '1'is true, and the suite runs — matching the intent already documented at line 90.
-
[native-codex]
.github/workflows/master-health.yml:105— The gate's skip condition and the fallback's coverage are asymmetric. The gate skips when a successfulpr.ymlmerge_group run exists — that run is all 12 jobs (policy,helm_chart,typecheck_release_registry,worktree_install,opencode_responses_replay,opencode_k8s_seed_cold_start,general_tests,vendor_claude_k8s,verify,build,canary_dry_run,e2e). The non-skip path runs onlygeneral_tests. So a direct-merged commit that breaks typecheck, the build, the Helm chart, or e2e is still invisible, which is the same class of gap the workflow is named for. The 2026-08-16 incident happened to land ingeneral-server; the next one need not.- Either narrow the promise — rename to something like "Master general tests" and say in the header that only
general_testsis replicated — or add the cheap static jobs (typecheck_release_registry,build) to close most of the remaining surface for little cost.
- Either narrow the promise — rename to something like "Master general tests" and say in the header that only
Suggestions (3)
- [pr-review-toolkit]
.github/workflows/master-health.yml:40— The concurrency group keys only ongithub.ref, which isrefs/heads/masterfor bothpushandworkflow_dispatch. An operator dispatch whose documented purpose (lines 29-31) is "confirm a red master has recovered" can be cancelled by an unrelated incoming push, or cancel one. Adding-${{ github.event_name }}to the group keeps the bulk-drain collapsing behaviour while letting a manual verification run to completion. - [gstack/review]
.github/workflows/master-health.yml:83— No in-repo precedent for theghCLI onarc-light; the only otherGH_TOKENuser,ally-review-consistency.yml, runs ondefault. If the image lacksgh,if !correctly catches exit 127 and fails closed — but the gate then permanently degrades to always-run, the cost optimisation is dead, and the only evidence is one log line. Worth confirming the image shipsgh, or distinguishing "query failed" from "gh missing" in the message. - [native-codex]
.github/workflows/master-health.yml:25— Detection lands as a red check on a push-triggered run, which by default notifies the pushing actor only. Given the failure mode being closed is 7h of company-wide queue blockage, consider whether this needs to reach a channel rather than one person's inbox.
Strengths
- The path filter on
.github/workflows/pr.yml(lines 76-99) is the subtle detail most implementations get wrong, and the reasoning is correct:commitperclip-review.ymldoes fire onmerge_group(verified at head), so counting any green merge_group run would let a passing review mask a failing suite — failing open in exactly the case the guard exists for. - The matrix is genuinely identical to
pr.yml:521-546, and the PR-only "Restore regenerated PR lockfile" step is correctly omitted rather than copied along with it. permissions:is explicit and minimal (contents: read,actions: read) on a repo whosepr.ymldeclares none.- Verification against the actual incident commits — including
12ce928d(the direct-merged breaker) yieldingpretested=0and the queued commits yieldingpretested=1— is the right evidence to bring, and the simulated-API-failure row is a real fail-closed test rather than an assertion about one. - Comments explain why rather than what, including the deliberate non-use of
arc-merge-queueand the scope boundary against the prevention-half ruleset change.
Recommended Action
- No Critical issues — nothing blocks on correctness of the gate logic itself.
- Address both Important issues this cycle: the
always()guard at line 109 is a one-line fix and directly restores the stated fail-closed invariant; the coverage asymmetry needs either a scope decision or two more jobs. - Consider Suggestions opportunistically.
…onest scope Important #1 (real bug): the general_tests `if:` carried an implicit success() on needs, so a gate JOB failure (ARC pod, 5-min timeout, runner fault) skipped the suite instead of running it — the exact silently-skipped suite the header forbids. The in-step fallback only covered `gh api` exiting non-zero inside a running step. Adds always(). Important #2 (scope): the gate skips on a full 12-job pr.yml merge_group run but the non-skip path replicated only general_tests. Resolved by narrowing the promise rather than adding jobs, because measurement says widening is expensive: 36.5% of landings bypass the queue (66 of 181 over 7 days), so this already adds ~9.4 full suite runs/day to a pool with known eviction pressure (BLO-20369). Renamed to 'Master general tests' and documented the coverage boundary explicitly. Suggestions: concurrency now keys on event_name so an operator dispatch and a push cannot cancel each other; recorded the in-repo precedent for gh on arc-light (commitperclip-review.yml). Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: de9df20
Both prior Important findings are genuinely fixed, and the accompanying comments are better than the code they explain. One new issue: the fix for the first prior finding reached for always(), which restores fail-closed detection but takes out the cancellation behaviour the cost model depends on.
Critical Issues (0)
Important Issues (1)
- [native-codex]
.github/workflows/master-health.yml:138—always()defeatscancel-in-progress, and this workflow's cost budget is built on that cancellation.always()is documented to return true even when the run is cancelled, which is why GitHub's own guidance on that function says to "instead consider!cancelled()". Job-levelif:is evaluated when the job is dispatched, so the exposure is precisely a job that has not started yet — which here isgeneral_testsduring the window the gate is still running. That window is the bulk-drain scenario the concurrency block at lines 45-50 was written for: a commit every ~15s (12 in ~3 minutes on 2026-08-16) against a gate that takes seconds, so nearly every run in the drain is cancelled whilegateis still in flight. Each of those cancelled runs can then dispatchgeneral_testsanyway — 6 matrix legs attimeout-minutes: 90apiece, up to ~72 pods for one drain instead of the 6 the design promises, onto the pool with documented eviction pressure that lines 29-34 explicitly budget against (BLO-20369). Note the second-order effect: the incident being closed here was itself company-wide queue starvation, so the failure mode is self-inflicted contention during exactly the event the workflow is watching.!cancelled()is strictly better and preserves everything the lines 127-136 comment promises: on a gate failure it is still true (it only excludes cancellation),needs.gate.outputs.pretestedis still empty,'' != '1'still holds, and the suite still runs fail-closed. It just stops resurrecting superseded runs.- Mind the YAML: a scalar beginning with
!is a tag indicator and will fail to parse, so this specific rewrite needs the explicit expression form rather than a bare token swap —
if: ${{ !cancelled() && (needs.gate.outputs.pretested != '1' || github.event_name == 'workflow_dispatch') }} - Worth updating the comment's first line too, since "
always()is load-bearing" is the reasoning that will otherwise pull the next reader back to it.
Prior Findings Dispositioned (2)
- prior:d42c649 important 1 — fixed —
.github/workflows/master-health.yml:138— theif:now carries a status-check function, so the implicitsuccess()onneedsis gone and a gate job failure no longer skips the suite. The mechanism is also documented at lines 127-136, including the'' != '1'reasoning and the "nothing retries a skipped run" consequence. (The choice ofalways()over!cancelled()introduces a separate cost regression, filed above as a new finding rather than a reopening of this one — the fail-closed property this finding asked for is present.) - prior:d42c649 important 2 — fixed —
.github/workflows/master-health.yml:25— resolved via the "narrow the promise" option rather than by adding jobs. The workflow is renamed toMaster general tests(line 1), and the newSCOPEblock at lines 25-34 states plainly that onlygeneral_testsis replicated, names the classes still uncaught (typecheck, build, Helm chart, e2e), and justifies the boundary with the 36.5%-bypass / ~9.4-suites-per-day arithmetic plus a revisit trigger. The asymmetry is now a documented decision instead of an implied promise, which is what the finding asked for.
Suggestions (2)
- [gstack/review]
.github/workflows/master-health.yml:105— The gate skips only onconclusion == "success", which is read at push time. A merge queue merges once its required checks pass; if anypr.ymljob is not required, the run can still be in progress at the moment the push event fires, the gate sees no successful run, and the suite runs redundantly. That direction is fail-closed and therefore safe — but the gate exists purely to buy back the ~63.5% of landings that arrive via the queue, so if this races routinely the optimisation quietly delivers nothing while still costing a pod per push. Worth confirming against branch protection whether everypr.ymljob is required; if not, this is measurable from a handful of recent master pushes before committing to the current shape. - [pr-review-toolkit]
.github/workflows/master-health.yml:64—gatehas noif:, so onworkflow_dispatchit still runs a pod and can hold up to 5 minutes beforegeneral_testsstarts, even though line 138 discards its output for that event.if: github.event_name != 'workflow_dispatch'would skip it — and since thepretestedoutput would then be empty, the existing'' != '1'condition already yields the same run-anyway result with no change to line 138. Minor, but the dispatch path is the one an operator uses while master is red and waiting on it.
Strengths
- The
always()comment at lines 127-136 is the right kind of comment: it explains the invisible failure mode (implicitsuccess()onneeds), enumerates the concrete triggers, and states the consequence of getting it wrong. The finding above is a refinement of the mechanism, not a disagreement with the reasoning — and the reasoning is what makes the refinement cheap to apply. - The
SCOPEblock is an unusually honest header. Most workflows named after a guarantee quietly over-promise; this one states what it does not cover, prices the alternative, and gives a revisit trigger. - The concurrency fix keys on
github.event_name(line 55) with the rationale inline, correctly preserving push-drain collapsing while isolating operator dispatches. ghonarc-lightis now backed by a named in-repo precedent (lines 70-72) rather than an assumption.github.shaandgithub.repositoryreach the script throughenv:(lines 80-83) instead of${{ }}interpolation into the shell body — the correct pattern, and one this repo does not apply uniformly elsewhere.- The workflow-path filter at lines 104-106 remains the subtle detail most implementations miss, and the empirical head_sha chain at lines 87-92 is real verification rather than an assertion.
Recommended Action
- No Critical issues.
- Address the
always()→!cancelled()change this cycle — one line plus a comment touch-up, and it restores the cancellation semantics the cost model at lines 29-34 assumes. - Consider the two Suggestions opportunistically; the gate-race one is worth a quick measurement since it determines whether the gate pays for itself.
Ally review of de9df20. always() is true even when the run is CANCELLED, and job-level if: is evaluated at dispatch time, so during a bulk drain (12 commits in ~3 min against a gate that takes seconds) every superseded run would still dispatch the 6-leg matrix at timeout-minutes: 90 each -- up to ~72 pods for one drain instead of 6, onto the pool the SCOPE block explicitly budgets against, during the exact event this workflow watches. !cancelled() keeps the fail-closed property the previous commit added (it excludes only cancellation, not failure) without resurrecting superseded runs. Wrapped in ${{ }} because a YAML scalar starting with ! is a tag indicator and will not parse bare. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both review rounds addressed at Important —
|
| PR | run completed | merged | lead |
|---|---|---|---|
| #1387 | 11:13:50 | 11:14:06 | +16s |
| #1385 | 12:56:22 | 12:56:41 | +19s |
| #1381 | 11:53:15 | 11:53:28 | +13s |
| #1380 | 02:26:18 | 02:26:31 | +13s |
| #1379 | 00:29:19 | 00:29:32 | +13s |
| #1377 | 03:43:53 | 03:44:16 | +23s |
| #1371 | 14:14:28 | 14:14:49 | +21s |
7/7 complete before the merge, consistently 13–23s ahead — which makes sense mechanically: the merge is caused by the merge_group checks passing, so completion necessarily precedes the push. The gate sees conclusion: success at push time and the ~63.5% saving is real, not theoretical. (#1382 in the same window has no queue run at all — a direct merge, i.e. the case this workflow is for.)
Suggestion 2 (skip gate on workflow_dispatch) — declining, deliberately
Your reasoning is correct, including that the empty pretested would flow through the existing condition unchanged. I am leaving it: the gain is ~30s on a hand-triggered path, and the cost is that the workflow's control flow then depends on skip-propagation through needs interacting with a status-check function — a subtlety that is easy to misread later, on the one path an operator uses while master is red. Happy to take it if you feel strongly; I would rather the dispatch path stay boring.
Suggestion 3 (notification routing) — agreed, out of scope here
A red push-run notifying only the pushing actor is a genuine gap given the failure mode is company-wide queue blockage. That is alert routing rather than detection, and it wants the same treatment as the rest of our CI alerting; noted on BLO-27871 rather than bolted on here.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3ba636f
Looks good. The one prior blocker is fixed, and fixed precisely — the change is the single if: line plus the reasoning for it, with no incidental drift. I re-derived the truth table rather than taking the comment's word for it, and it holds in all four states: gate failure → !cancelled() true, pretested empty, '' != '1' true → suite runs (fail-closed preserved); run cancelled → false → suite skipped (the cost regression closed); gate success → gate output decides; dispatch → runs regardless.
Critical Issues (0)
Important Issues (0)
Prior Findings Dispositioned (1)
- prior:de9df20 important 1 — fixed —
.github/workflows/master-health.yml:152—always()is replaced by${{ !cancelled() && (...) }}, exactly the rewrite the finding asked for, including the${{ }}wrapper. I verified the parse hazard the finding warned about is genuinely avoided rather than merely commented: the raw scalar at line 152 begins${{, not!, so it is a plain scalar and no YAML tag indicator is in play (pr.yml:662is in-repo precedent for the same form). Cancellation no longer resurrects superseded runs, and the fail-closed path the previous fix established is untouched.
Suggestions (1)
- [pr-review-toolkit] The two suggestions from the previous round are unaddressed and still stand as written — the gate-race question at
.github/workflows/master-health.yml:106(conclusion == "success"read at push time, which decides whether the gate pays for itself) and the redundant gate pod onworkflow_dispatchat.github/workflows/master-health.yml:64. Deliberately not re-expanded here; they are on record in thede9df208review and neither blocks.
Strengths
- The comment at lines 137-149 does the thing that makes this class of bug non-recurring: it states the distinction is "a real distinction here rather than style," names the exact mechanism (
always()is true even when cancelled; job-levelif:evaluates at dispatch time), prices the consequence in concrete units (6 legs ×timeout-minutes: 90per cancelled run), and — unusually — documents the YAML tag-indicator trap that makes the naive one-token fix fail. A reader who wants to "simplify" this back toalways()has to argue with the reasoning first. - The edit is minimal in the right way. Comparing
de9df208...3ba636ff, the only functional change is that one line; the matrix, gate script, permissions, and concurrency block are untouched. A fix that also tidies unrelated lines is a fix that is harder to review and harder to revert. - Re-verified at this head: the matrix is still byte-identical to
pr.yml:522-547, and the PR-only "Restore regenerated PR lockfile" step (pr.yml:558-564, gated onneeds.policy.outputs.lockfile_regenerated) remains correctly omitted rather than copied along with it. - The gate's ordering property is stronger than the comment claims credit for: if a direct merge lands while a PR is mid-queue, GitHub rebuilds the merge group on the new base, so either the landed SHA matches a green
pr.ymlmerge_group run that already covered both commits, or it does not match and the suite runs. Both branches are safe — the optimisation has no window where an untested commit is masked by a later queued one.
Recommended Action
- No Critical issues.
- No Important issues — the prior blocker is resolved and nothing new was introduced.
- Consider the two carried-forward suggestions opportunistically; the gate-race measurement is the one with real information value, since it determines whether the gate earns its pod.
Thinking Path
Linked Issues or Issue Description
What happened on 2026-08-16
A 12-PR direct-merge batch landed between 04:09:52Z and 04:12:53Z. #1353 and #1366 were each green against their own base but semantically conflicting together, so master went red at 04:12Z. Nothing reported it. Four PRs were ejected from the queue one at a time — #1344, #1169, #1378, #1381 — each burning a full ~30-minute
merge_groupbuild againstmaximumEntriesToBuild: 1to rediscover the identical assertion failure. Master stayed red ~5.5h; the queue was blocked ~7h and drained tototalCount=0.What Changed
.github/workflows/master-health.yml(new, only file) — workflow nameMaster general tests;on: push: [master]plusworkflow_dispatch.gatejob (arc-light, 5 min) — queries for a successfulpr.ymlrun atevent=merge_group&head_sha=<this commit>. If one exists, the suite is skipped.general_testsjob (arc-paperclip-general) — runs only when the gate says the commit did not come through the queue. Matrix is byte-identical topr.yml'sgeneral_tests(asserted below), so coverage matches a queue build.concurrency: master-health-${{ github.ref }},cancel-in-progress: true— a bulk drain (12 commits in ~3 min) collapses to a single build of the final head.Two details that are load-bearing, and easy to get wrong
path, not merely on a greenmerge_grouprun.commitperclip-review.ymlalso fires onmerge_group. Counting any green run there would let a passing review mask a failing test suite — the guard would fail open in exactly the case it exists to catch.pretested=0and the suite runs. A redundant suite is a cheap mistake; a silently skipped one is the bug this workflow exists to prevent.Verification
The enabling fact, verified empirically: a
merge_groupbuild'shead_shais the commit that lands on master. The 2026-08-16 queue chain, where each queue branch's base is the previous entry's landed commit:head_shapr-1381-9a2143479a214347a9b48d9ea9b48d9e(#1381 merge commit)pr-1385-a9b48d9ea9b48d9e6646531466465314pr-1123-6646531466465314d4fb292fd4fb292f= current master HEADThe gate's
run:block was extracted verbatim from the workflow and executed against live API data — not a reimplementation of its logic:d4fb292fpretested=1→ skipa9b48d9epretested=1→ skip3c5fb33dpretested=1→ skip12ce928dpretested=0→ build64e322c3pretested=0→ buildd4fb292fw/ unreachable repopretested=0→ build (fail-closed)So against the real incident it would have fired on the breaking commit at 04:12Z, with zero wasted builds on the legitimately-queued ones.
Static checks:
node ./scripts/check-github-runner-labels.mjs→Validated 24 workflows: all runner labels use ARC.(24 includes the new file)pr.yml→IDENTICAL: Trueruns-onisarc-light/arc-paperclip-general.arc-merge-queueis never used — it appears only in a comment explaining whyPost-merge: this workflow fires on its own merge commit. That commit lands via the queue, so the expected first observation is the gate skipping — which demonstrates the free path live.
workflow_dispatchthen exercises the build path on demand.Risks
Low, and bounded by construction.
pr.ymlis untouched, so this cannot affect PR or merge-queue behaviour — deliberate, given the file gates every merge in the repo and was the subject of the incident.Cost — MEASURED, and I had this wrong in the first revision of this PR. I originally wrote "~3 non-queue landings/day"; that was an unverified guess and it is low by ~3×. Measured over the 7 days to 2026-08-16, per landing event (a push fires once per push tip, not once per commit — a rebase-merged N-commit PR lands N commits but only the tip carries the
merge_groupSHA, so counting commits overstates this):arc-lightgeneral_testsmatrixSo this adds ~9.4 suite runs/day (6 pods each) to
arc-paperclip-general. That is not free on a pool with documented eviction pressure (BLO-20369), and it is the reason Important fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2 was resolved by narrowing scope rather than by adding jobs.The 36.5% is itself the more important finding and is reported on BLO-27871: bypassing the queue is routine here, not exceptional, which is what makes the detection gap expensive — and it strengthens the case for the prevention half (requiring the queue), which is a human/repo-admin decision this PR does not assume.
Nothing is added to
arc-merge-queue, so merge-queue latency is unaffected.False "skip". Requires a successful
pr.ymlmerge_grouprun at that exact SHA — i.e. that tree genuinely passed the full suite. Correct even if a human then lands the identical SHA by hand.False "build". Costs one redundant suite. This is the deliberate direction of the fail-closed choice.
Not covered: this is detection. Prevention — requiring the queue so nothing lands outside it — is a repo-admin ruleset change and is intentionally not in this PR: it is human-only, and it would remove a manual drain valve that is a legitimate response to a wedged queue. The two are complementary; the operator's decision on that half has not been assumed.
Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use (GitHub REST API, Prometheus, shell).Checklist
gh pr list --limit 400; note--limit 100silently truncates) and grepped formaster.health|master.red|push.trigger|untested|workflow. No duplicate; ci(policy): fail CI when workflow content escapes its block scalar (BLO-23128) #1183 and feat(metrics): add DB-backed workflow_run cancellation gauge (BLO-21078) #989 touch workflows but are unrelatedarc-merge-queueis avoided