fix: route merge_group jobs to the dedicated arc-merge-queue pool (BLO-22428) - #1102
Open
kkroo wants to merge 1 commit into
Open
fix: route merge_group jobs to the dedicated arc-merge-queue pool (BLO-22428)#1102kkroo wants to merge 1 commit into
kkroo wants to merge 1 commit into
Conversation
…-22428)
The six heavy pull_request/merge_group-shared jobs (typecheck_release_registry,
worktree_install, general_tests, build, verify_serialized_server,
canary_dry_run) now resolve runs-on conditionally: merge_group runs land on
the new arc-merge-queue pool, pull_request runs keep using the shared
default label unchanged.
Root cause (BLO-22428 live diagnosis): GitHub's merge-queue check
concurrency is 1, so a deep pull_request backlog on the shared default
label can starve the one queue that actually lands code without itself
needing proportional runner capacity -- a 50-deep merge queue sat 10h47m
with zero merges while arc-default was pinned 30/30 on ordinary PR CI.
check-github-runner-labels.mjs previously treated any `${{ ... }}`
runs-on expression as one opaque, unrecognized runner name. Taught it to
recognize the `<cond> && 'X' || 'Y'` ternary shape and validate both
literal branches individually, so this routing expression doesn't need
a blanket allowance for arbitrary expressions.
Depends on the Blockcast/onprem-k8s PR that creates the arc-merge-queue
scale set (branch platformsre/blo22428-arc-merge-queue-partition) -- do
not merge this before that PR is merged and Argo has synced the new pool
live, or merge_group runs will queue against a label with zero runners.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Author
13 tasks
1 similar comment
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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: d73f380
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit + gstack/review + native-codex]
scripts/check-github-runner-labels.mjs:18— The new regex can accept a chained expression containing an earlier forbidden runner branch. Its unrestricted prefix backtracks to the final&& 'arc-merge-queue' || 'default', so${{ cond && 'ubuntu-latest' || other && 'arc-merge-queue' || 'default' }}is accepted while the checker validates onlyarc-merge-queueanddefault. This makes the ARC-only policy fail open despite the comment's fail-closed guarantee.- Restrict the condition prefix to the one supported expression (or parse and validate every branch), and add a regression test with a forbidden earlier branch in a chained expression.
Suggestions (1)
- [native-codex]
.github/workflows/pr.yml:212— Keep merge blocked untilBlockcast/onprem-k8s#2115has landed and livearc-merge-queuelisteners are verified. The current routing is correct, but merging it first would send all six merge-group lanes to an unavailable label.
Strengths
- The current workflow expression correctly keeps pull-request jobs on
defaultand routes all six shared heavy merge-group jobs to the dedicated pool. - The tests cover the intended two-branch expression, a forbidden selected branch, and unsupported opaque expressions.
- CI is green on the reviewed head, including the policy guard and all serialized server shards.
Recommended Action
- Tighten the runner-expression parser and add the chained-expression regression test before merge.
- Preserve the documented infrastructure-first rollout order.
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.
Replacement for app-authored #1080 so the Ally GitHub App can provide the required independent review/approval.
Exact code head copied from #1080:
d73f38008bcd3e48dd02bd8e341c0f9f98c3c3b1.Thinking Path
The presenting symptom was
Blockcast/paperclip's master merge queue sitting 53 entries deep with a 10h47m gap between merges, whilearc-defaultwas pinned at 30/30 runner pods. The obvious reading — "we are out of CI capacity, add runners" — is wrong, and following it would have cost a cross-team capacity trade for no benefit.The disconfirming measurement is GitHub's merge-queue
max_entries_to_build, which is 1 on ruleset20487141. The queue builds one entry at a time, so its runner demand is flat and small regardless of whether it is 5 deep or 53 deep. Queue depth is not check demand. What starved the queue was not insufficient total capacity but ordering:merge_groupjobs competed for the samedefaultlabel as an unboundedpull_requestbacklog, and lost.That reframes the fix. The queue does not need capacity proportional to its depth; it needs a small pool that
pull_requesttraffic cannot touch. So instead of raisingarc-defaultpast its documented 30-slot ceiling — whicharc/arc-default-values.yamlstates is derived imagefs arithmetic (2625Gi x 1.10 = 2887Gi max; 30 x 38Gi = 2876Gi, 11Gi slack) and whose raise "is a cross-team decision about whose capacity shrinks, not a knob turn" — the companion PR carvesarc-default30 -> 22 and addsarc-merge-queueat 8. Total stays 30, the ledger is untouched, and no team's capacity shrinks. This PR is the other half: routing the traffic onto that new pool.Rejected alternatives: raising the ceiling by shrinking a named idle pool (spends a cross-team trade on the wrong axis, and a single 0-pod sample does not establish sustained idleness); and accepting the ceiling while throttling PR CI demand (treats the symptom, leaves the starvation mechanism intact).
What Changed
.github/workflows/pr.yml— the six heavy jobs shared between thepull_requestandmerge_groupevents (typecheck_release_registry,worktree_install,general_tests,build,verify_serialized_server,canary_dry_run) move fromruns-on: defaultto a conditional:merge_groupruns land on the new dedicated pool;pull_requestruns keep the shareddefaultlabel. Onpull_requestthe ternary evaluates todefault, so PR CI behaviour is unchanged byte-for-byte.pr.ymlis the only workflow in the repo carrying amerge_group:trigger, so no straggler job is left starving ondefault. Theruns-onvalues not touched here are 3xarc-lightand 1xarc-e2e, both already partitioned.scripts/check-github-runner-labels.mjs— addsarc-merge-queueto the allow-list, and teaches the guard to parse exactly the${{ <cond> && 'X' || 'Y' }}shape so it validates both literal branches. Previously it treated any${{ ... }}value as one opaque runner name, which would have flagged the new expression as an unknown label. Any other expression shape still falls through to the opaque-string path and fails closed.scripts/check-github-runner-labels.test.js— three regression cases for the new parsing: a valid ternary with both branches allowed, one with a disallowed branch, and an unrelated opaque expression the parser cannot decompose (must still be rejected).Risks
arc-merge-queuescale set. If this routing goes live first, everymerge_groupjob targets a label with zero live listeners and queues indefinitely — the same starvation this change exists to fix, relocated to a new label. This PR must therefore not merge until feat: PAPERCLIP_APP_NAME — rebrand self-hosted deployment without forking paperclipai/paperclip#2115 is merged andarc-merge-queuepods are confirmed live inarc-runners. That gate is enforced by the sequencing hold on the Paperclip issue and by board approval25fe072c— not by draft status, which has been lifted so the review gates can run.arc-defaultdrops to 22 while master'spr.ymlstill routes all sixmerge_groupjobs todefault— strictly worse than today until this PR lands. Running both pools at full size to avoid the window is not available: 30+8 slots is 3180Gi against a 2887Gi ceiling. The window is structural; the only lever is its duration, so this PR should land within minutes of feat: PAPERCLIP_APP_NAME — rebrand self-hosted deployment without forking paperclipai/paperclip#2115, not via the 53-deep queue.merge_groupconcurrency is ~10 jobs, not 8: the 4verify_serialized_servershards are gated behindgeneral_tests, but typecheck + worktree_install + 6xgeneral_tests+ build + canary_dry_run overlap. AgainstmaxRunners: 8two jobs wait one slot-turn — materially better than starving behind 53 PRs, but the companion values-file comment's word "comfortably" is optimistic.git revertthis commit and all six jobs return toruns-on: defaultimmediately; thearc-merge-queuepool then simply sits idle. No CI breakage. Reverting feat: PAPERCLIP_APP_NAME — rebrand self-hosted deployment without forking paperclipai/paperclip#2115 separately restoresarc-defaultto 30.Model Used
claude-sonnet-5[1m]), which also verified the ledger arithmetic and ran the guard suites locally.claude-opus-5[1m]). The CTO review independently confirmed themax_entries_to_build: 1measurement that motivates partitioning over a capacity raise, checked that the companion ledger change nets to zero, and flagged the ~10-vs-8 peak-concurrency gap above.Verification
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr.yml'))"— YAML still parses.node ./scripts/check-github-runner-labels.mjs—Validated 20 workflows: all runner labels use ARC.node --test scripts/check-github-runner-labels.test.js— 8/8 pass, including the 3 new ternary-parsing cases.node --test scripts/__tests__/pr-verify-lane-outcome.test.mjs— unaffected, 13/13 pass (does not assert onruns-on).Post-merge verifying signal: a
merge_grouprun observed picking up a runner on thearc-merge-queuepool, and merge-queue drain rate measured bymerged_at(not commit dates — under a REBASE queue a commit's committer date is its enqueue time).Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22428
Related: https://paperclip.blockcast.net/BLO/issues/BLO-21953
Related PRs
Searched the open and recent PR list for duplicates before filing this checklist. Nothing duplicates this change; two are adjacent and worth reading together:
ci: load-shed stale merge_group generations superseded by a queue re-stage (BLO-21953). Same incident, different mechanism (sheds superseded generations rather than partitioning capacity). Complementary, not overlapping.fix(ci): run commitperclip-review against the merge queue's landing commit. Touches the review gate, notruns-on:routing.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template — described in-PR above; tracking issues live in Paperclip (BLO-22428 / BLO-21953), not GitHub Issuesruns-on:comments and in the companionarc-merge-queue-values.yamlverify_serialized_servershards are still running and the companion PR's gate is red for an unrelated reason (see BLO-22428)