fix: route Paperclip PR and merge-group jobs to dedicated ARC pools (BLO-22428) - #1102
Conversation
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. |
![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: 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.
Sequencing gate correction: this PR's stated blocker (paperclipai#2115) is CLOSED — the live gate is now onprem-k8s#2147Re-measured 2026-08-07T05:55Z. The "Ordering risk" bullet in this PR's description is still correct in substance but now names a dead artifact, and anyone checking the gate before merging would read a misleading answer:
So the hold stands, with the reference updated:
Merging first routes all six heavy paperclipai#2147 status right now: Why the "land within minutes of each other" note matters more than when it was written: Not requesting a review with this comment; paperclipai#2147's gates are running and nothing here needs action until they clear. |
|
Status update on the ordering dependency named in this PR's risk section: Blockcast/onprem-k8s#2180 is now open — it activates Per this PR's own gate: please hold merging #1102 until paperclipai#2180 merges and Tracking issue: BLO-22835. |
…-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>
kkroo
changed the title
d73f380 to
cb6bd74
Compare
|
@ally please review exact head The prior fail-open ternary parser finding is fixed with an exact anchored expression and a chained-forbidden-runner regression. The infrastructure hold is also cleared: onprem-k8s#2180 merged, Argo is Synced/Healthy, and both |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally authoritative review request: exact head |
kkroo
changed the title
cb6bd74 to
d2c97c0
Compare
|
@ally please review exact head |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
d2c97c0 to
82a04c5
Compare
|
@ally please review exact head |
allyblockcast
left a comment
allyblockcast
left a comment
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: 82a04c5
Prior Findings Dispositioned (1)
- prior:d73f380 important 1 — fixed —
scripts/check-github-runner-labels.mjs:27-28—TERNARY_RUNNER_EXPRESSIONis now fully anchored (^\$\{\{\s*github\.event_name\s*==\s*'merge_group'\s*&&\s*'([^']*)'\s*\|\|\s*'([^']*)'\s*\}\}$), with no unrestricted backtracking prefix. A chained expression like${{ cond && 'ubuntu-latest' || github.event_name == 'merge_group' && 'arc-merge-queue' || 'arc-paperclip-general' }}no longer matches the ternary shape at all and falls through to the opaque-string path (valuesFromScalarline 88), which fails closed againstALLOWED_RUNNERS.scripts/check-github-runner-labels.test.js:166-175adds the exact regression test ("rejects a chained expression with an earlier forbidden runner") assertingresult.status === 1for that case, plus companion tests for a disallowed ternary branch and a non-matching opaque expression.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The anchored regex plus per-branch validation (
ALLOWED_RUNNERS.has(entry.value)on bothternaryMatch[1]andternaryMatch[2],scripts/check-github-runner-labels.mjs:86-88,141) closes the fail-open gap with the minimal, non-speculative fix — no broader expression parser was introduced, matching the one ternary shape actually in use. scripts/check-github-runner-labels.test.js:195-202adds a whole-file guard (PR workflow does not route jobs to the shared default pool) that fails if any of the six migrated jobs regress back toruns-on: default; verified against the currentpr.yml— no bareruns-on: defaultremains andpolicy/helm_chart/e2eretain their existing dedicated ARC labels untouched, so the change is scoped correctly.- All CI checks are green on the reviewed head, including the policy guard, the new regression tests, and all serialized/general server shards.
Recommended Action
- None — ready to merge on this head.
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: 82a04c5
Prior Findings Dispositioned (1)
- prior:d73f380 important 1 — fixed —
scripts/check-github-runner-labels.mjs:27-28—TERNARY_RUNNER_EXPRESSIONis now fully anchored (^\$\{\{\s*github\.event_name\s*==\s*'merge_group'\s*&&\s*'([^']*)'\s*\|\|\s*'([^']*)'\s*\}\}$), with no unrestricted backtracking prefix. A chained expression like${{ cond && 'ubuntu-latest' || github.event_name == 'merge_group' && 'arc-merge-queue' || 'arc-paperclip-general' }}no longer matches the ternary shape at all and falls through to the opaque-string path (valuesFromScalarline 88), which fails closed againstALLOWED_RUNNERS.scripts/check-github-runner-labels.test.js:166-175adds the exact regression test ("rejects a chained expression with an earlier forbidden runner") assertingresult.status === 1for that case, plus companion tests for a disallowed ternary branch and a non-matching opaque expression.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The anchored regex plus per-branch validation (
ALLOWED_RUNNERS.has(entry.value)on bothternaryMatch[1]andternaryMatch[2],scripts/check-github-runner-labels.mjs:86-88,141) closes the fail-open gap with the minimal, non-speculative fix — no broader expression parser was introduced, matching the one ternary shape actually in use. scripts/check-github-runner-labels.test.js:195-202adds a whole-file guard (PR workflow does not route jobs to the shared default pool) that fails if any of the six migrated jobs regress back toruns-on: default; verified against the currentpr.yml— no bareruns-on: defaultremains andpolicy/helm_chart/e2eretain their existing dedicated ARC labels untouched, so the change is scoped correctly.- All CI checks are green on the reviewed head, including the policy guard, the new regression tests, and all serialized/general server shards.
Recommended Action
- None — ready to merge on this head.
2 participants
Thinking Path
Linked Issues or Issue Description
No GitHub issue exists for this infrastructure incident. The problem and rollout are tracked in BLO-22428, with the broader queue incident in BLO-21953. The underlying bug is cross-repository runner contention: required Paperclip PR and merge-group lanes shared
default, so sustained PR traffic could strand the sole merge candidate despite available capacity in purpose-built pools.What Changed
pull_requestandmerge_grouptoarc-paperclip-generalfor PRs andarc-merge-queuefor merge groups.default.arc-lightandarc-e2elanes unchanged.scripts/check-github-runner-labels.mjs.github.event_name == merge_grouptwo-branch expression and validate both literal labels. All other expressions remain opaque and fail.Rollout State
The infrastructure-first dependency is satisfied. onprem-k8s#2180 merged at exact reviewed head
cbe38049878059f1e1fe748949e65828bfec4cb6as merge commit72f216b4f22d42b105a3daa736eab8e33d39ab3b. Argo is Synced/Healthy at that revision:arc-default: max 24arc-merge-queue: max 12; listener pod Running and readyarc-paperclip-general: max 16; listener pod Running and readyThere is no remaining infrastructure sequencing hold.
Verification
ruby -ryaml -e ... .github/workflows/pr.yml- YAML 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- 10/10 pass.node --test scripts/__tests__/pr-verify-lane-outcome.test.mjs- 16/16 pass.masterwithout conflicts.Post-merge proof is a
merge_grouprun acquiringarc-merge-queueand a pull-request run acquiringarc-paperclip-general.Risks
default; both dedicated pools then remain idle at minRunners 0.Model Used
OpenAI Codex based on GPT-5 performed the final parser fix validation, current-master rebase, Kubernetes rollout verification, PR metadata correction, and exact-head CI inspection using repository, GitHub CLI, Docker, and Kubernetes tools. Earlier commits were produced through the linked Paperclip platform/SRE workflow.
Related PRs
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateCo-Authored-By: Paperclip noreply@paperclip.ing