perf(ci): stop double-running coverage and packaging gates, drop video/voice isolation - #2217
Closed
willgriffin wants to merge 5 commits into
Closed
perf(ci): stop double-running coverage and packaging gates, drop video/voice isolation#2217willgriffin wants to merge 5 commits into
willgriffin wants to merge 5 commits into
Conversation
willgriffin
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70cf387b39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces self-hosted (arc-happyvertical) runner slot occupancy by removing duplicate CI work that was being executed on both PR events and again (as the binding gate) in the merge queue, and by removing now-unnecessary special-casing in the sharded package test job. It updates CI documentation to match the new gating behavior.
Changes:
- Gate
Coverage Gateto full-mode runs only (inputs.mode == 'full'), preventing it from running in the affected/PR lane. - Gate
Publish Dry Runtomerge_grouponly, and update theRequired CIaggregator to require it only when it runs (while still failing if it unexpectedly reports a non-skipped/non-successresult on PRs). - Remove the
smrt-video/smrt-voiceisolation pre-steps and run them within the normal sharded package-test batch again; update.github/CI.mdaccordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/test-suite.yml |
Gates coverage to full-mode and removes video/voice isolation so the sharded package tests run uniformly without duplicate/extra steps. |
.github/workflows/on-pull-request.yml |
Makes publish dry-run merge-queue only and updates the required-status aggregator logic to reflect the split. |
.github/CI.md |
Documents that Coverage Gate and Publish Dry Run are merge-group only and updates required-job expectations accordingly. |
willgriffin
force-pushed
the
perf/issue-2214-queue-occupancy
branch
from
August 4, 2026 04:31
70cf387 to
9c34ed1
Compare
willgriffin
added a commit
that referenced
this pull request
Aug 4, 2026
Review finding on #2217: gating the dry run on the event alone breaks the documented rollback. Clearing CI_MERGE_QUEUE_ENABLED stops GitHub producing merge_group events, so the job would never run anywhere — and on-merge-main.yml goes from test and build straight to publish without any pack validation, letting a packaging failure reach the release workflow. Gate on the lever instead of the event: skip only when this is a pull request AND the merge queue is enabled to re-run it. That mirrors the caller's `mode` expression, which already falls back to full validation on the same lever — which is also why Coverage Gate needs no equivalent fix, since `mode` becomes `full` under rollback and it runs on pull requests again by itself. Refs #2214 (item 4)
The isolated smrt-video and smrt-voice steps, their dedicated 4 GB heaps, and their exclusion filters were a workaround for silent `Worker exited unexpectedly` deaths in the Test Packages shards, attributed at the time to container memory pressure. That attribution was wrong. The failing shards recorded cgroup `oom_kill = 0`; the real cause was an AVX2 SIGILL in @happyvertical/pdf on x86-64-v2 metal, fixed by the 0.65.9 guard (#2195, #2196). Every sampled failure predates that bump, and the first full merge-group run after it (30832712023) was green on all three shards. Remove the workaround now that its cause is gone. scripts/test-packages-shard.mjs already round-robins both packages onto the same two runners the isolation steps were pinned to (voice to 1/3, video to 2/3), so shard placement is unchanged; they simply run inside the normal batch again. The VITEST_MAX_WORKERS caps stay: `nproc` reports host CPUs, not the pod's 4-CPU quota, so uncapped Vitest still over-fans. Refs #2214 (item 6)
Coverage Gate had no mode guard, so it ran on every pull request and again in
the merge group. On pull requests that was the expensive copy: the seed `build`
job is full-mode only, so the gate's mandatory full `pnpm run build` ran cold.
Across 14 recent PR runs the job was bimodal at 4.2-4.7 min on a warm remote
cache against 26-45 min cold, and the merge group re-ran it in full regardless.
Gate it to full mode, matching every sibling full-mode job. It keeps its full
meaning there: check-coverage.mjs resolves its diff base as `BASE_REF || 'main'`
plus merge-base, which is correct for the merge_group synthetic head, so the
gate still measures exactly the packages an entry touches.
The trade is that a coverage regression now ejects from the queue instead of
reddening the pull request. Authors keep the local pre-check:
node scripts/check-coverage.mjs --packages <list>
Refs #2214 (item 8)
Publish Dry Run is a full-tree build plus a two-shard pack validation, all on the metal fleet, and it ran on every pull_request_target synchronize while the merge group re-ran it in full regardless. The pull-request copy was duplicate fleet occupancy, never the binding gate. The gate goes inside publish-dry-run.yml rather than on the caller's `if:`. Every reusable or self-hosted job in on-pull-request.yml must carry the canonical trusted-base admission expression byte-for-byte — `hv-agent check-pr` rejects any deviation — so the caller keeps its gate unchanged and the two fleet jobs plus the summary skip themselves. The condition negates pull_request_target rather than testing for merge_group so the workflow_dispatch escape hatch keeps working. The summary job needed the guard explicitly: its `always()` would otherwise leave it running, and failing on missing artifacts, after the jobs it summarizes skip. Required CI learns the split. It requires seven jobs on both events and handles Publish Dry Run separately: success is required in the merge group, `skipped` is accepted on a pull request, and anything else still fails there — so re-enabling the job on pull requests cannot silently leave it un-gated. Fork pull requests are unaffected: they are rejected through the skipped `Validate Changes` result, which is still checked in the unconditional loop. The trade is that a packaging break now ejects from the queue instead of reddening the pull request. Refs #2214 (item 4)
CI.md described Coverage Gate as part of the affected-mode pull-request lane, Publish Dry Run as running for same-repository pull requests, and Required CI as checking the same eight jobs on both events. All three are now stale. Record the split, why it is safe (check-coverage.mjs resolves its base correctly for the merge_group synthetic head), and the trade it accepts. Also drop publish-dry-run from the fork-PR sentence: it is skipped on every pull request now, so it no longer distinguishes a fork run — the skipped validation call does. Refs #2214 (items 4 and 8)
Review finding on #2217: gating the dry run on the event alone breaks the documented rollback. Clearing CI_MERGE_QUEUE_ENABLED stops GitHub producing merge_group events, so the job would never run anywhere — and on-merge-main.yml goes from test and build straight to publish without any pack validation, letting a packaging failure reach the release workflow. Gate on the lever instead of the event: skip only when this is a pull request AND the merge queue is enabled to re-run it. That mirrors the caller's `mode` expression, which already falls back to full validation on the same lever — which is also why Coverage Gate needs no equivalent fix, since `mode` becomes `full` under rollback and it runs on pull requests again by itself. Refs #2214 (item 4)
willgriffin
force-pushed
the
perf/issue-2214-queue-occupancy
branch
from
August 5, 2026 18:47
614deb4 to
3ef5c24
Compare
Contributor
Author
|
Superseded by #2242, which carries this branch's commits cherry-picked verbatim (same authors, same messages) alongside the other two CI-lane fixes. Rationale: these three PRs each needed a full validation pass on the very lane they were unclogging, and #2217/#2237 both edit The merged result is the union of both workflow changes, verified job by job — the three light jobs are hosted per #2236, every heavy job keeps the The branch is preserved. Reopen if #2242 needs to be unwound. |
willgriffin
added a commit
that referenced
this pull request
Aug 5, 2026
Review finding on #2217: gating the dry run on the event alone breaks the documented rollback. Clearing CI_MERGE_QUEUE_ENABLED stops GitHub producing merge_group events, so the job would never run anywhere — and on-merge-main.yml goes from test and build straight to publish without any pack validation, letting a packaging failure reach the release workflow. Gate on the lever instead of the event: skip only when this is a pull request AND the merge queue is enabled to re-run it. That mirrors the caller's `mode` expression, which already falls back to full validation on the same lever — which is also why Coverage Gate needs no equivalent fix, since `mode` becomes `full` under rollback and it runs on pull requests again by itself. Refs #2214 (item 4)
willgriffin
added a commit
that referenced
this pull request
Aug 6, 2026
… fixture hookTimeout (#2242) * perf(ci): drop the smrt-video/smrt-voice test isolation steps The isolated smrt-video and smrt-voice steps, their dedicated 4 GB heaps, and their exclusion filters were a workaround for silent `Worker exited unexpectedly` deaths in the Test Packages shards, attributed at the time to container memory pressure. That attribution was wrong. The failing shards recorded cgroup `oom_kill = 0`; the real cause was an AVX2 SIGILL in @happyvertical/pdf on x86-64-v2 metal, fixed by the 0.65.9 guard (#2195, #2196). Every sampled failure predates that bump, and the first full merge-group run after it (30832712023) was green on all three shards. Remove the workaround now that its cause is gone. scripts/test-packages-shard.mjs already round-robins both packages onto the same two runners the isolation steps were pinned to (voice to 1/3, video to 2/3), so shard placement is unchanged; they simply run inside the normal batch again. The VITEST_MAX_WORKERS caps stay: `nproc` reports host CPUs, not the pod's 4-CPU quota, so uncapped Vitest still over-fans. Refs #2214 (item 6) * perf(ci): run Coverage Gate only in full-mode validation Coverage Gate had no mode guard, so it ran on every pull request and again in the merge group. On pull requests that was the expensive copy: the seed `build` job is full-mode only, so the gate's mandatory full `pnpm run build` ran cold. Across 14 recent PR runs the job was bimodal at 4.2-4.7 min on a warm remote cache against 26-45 min cold, and the merge group re-ran it in full regardless. Gate it to full mode, matching every sibling full-mode job. It keeps its full meaning there: check-coverage.mjs resolves its diff base as `BASE_REF || 'main'` plus merge-base, which is correct for the merge_group synthetic head, so the gate still measures exactly the packages an entry touches. The trade is that a coverage regression now ejects from the queue instead of reddening the pull request. Authors keep the local pre-check: node scripts/check-coverage.mjs --packages <list> Refs #2214 (item 8) * perf(ci): run Publish Dry Run only in the merge queue Publish Dry Run is a full-tree build plus a two-shard pack validation, all on the metal fleet, and it ran on every pull_request_target synchronize while the merge group re-ran it in full regardless. The pull-request copy was duplicate fleet occupancy, never the binding gate. The gate goes inside publish-dry-run.yml rather than on the caller's `if:`. Every reusable or self-hosted job in on-pull-request.yml must carry the canonical trusted-base admission expression byte-for-byte — `hv-agent check-pr` rejects any deviation — so the caller keeps its gate unchanged and the two fleet jobs plus the summary skip themselves. The condition negates pull_request_target rather than testing for merge_group so the workflow_dispatch escape hatch keeps working. The summary job needed the guard explicitly: its `always()` would otherwise leave it running, and failing on missing artifacts, after the jobs it summarizes skip. Required CI learns the split. It requires seven jobs on both events and handles Publish Dry Run separately: success is required in the merge group, `skipped` is accepted on a pull request, and anything else still fails there — so re-enabling the job on pull requests cannot silently leave it un-gated. Fork pull requests are unaffected: they are rejected through the skipped `Validate Changes` result, which is still checked in the unconditional loop. The trade is that a packaging break now ejects from the queue instead of reddening the pull request. Refs #2214 (item 4) * docs(ci): describe the merge-group-only coverage and packaging gates CI.md described Coverage Gate as part of the affected-mode pull-request lane, Publish Dry Run as running for same-repository pull requests, and Required CI as checking the same eight jobs on both events. All three are now stale. Record the split, why it is safe (check-coverage.mjs resolves its base correctly for the merge_group synthetic head), and the trade it accepts. Also drop publish-dry-run from the fork-PR sentence: it is skipped on every pull request now, so it no longer distinguishes a fork run — the skipped validation call does. Refs #2214 (items 4 and 8) * fix(ci): keep Publish Dry Run enabled when the merge queue is disabled Review finding on #2217: gating the dry run on the event alone breaks the documented rollback. Clearing CI_MERGE_QUEUE_ENABLED stops GitHub producing merge_group events, so the job would never run anywhere — and on-merge-main.yml goes from test and build straight to publish without any pack validation, letting a packaging failure reach the release workflow. Gate on the lever instead of the event: skip only when this is a pull request AND the merge queue is enabled to re-run it. That mirrors the caller's `mode` expression, which already falls back to full validation on the same lever — which is also why Coverage Gate needs no equivalent fix, since `mode` becomes `full` under rollback and it runs on pull requests again by itself. Refs #2214 (item 4) * perf(ci): pin the light validation jobs to GitHub-hosted runners `affected-scope`, `lint`, and `test-packages-result` gain nothing from the metal fleet. None runs a Turbo task or `setup-environment`, so none can restore from the internal cache or the hosted shim, and the fleet's memory is irrelevant to all three. On the fleet they could instead wait out a netboot of up to 780s, or queue behind the heavy shards on an 8-slot pool. Hosted minutes are free and unmetered on this public repository, so pinning them to a plain `ubuntu-latest` costs nothing and returns three slots per validation pass to the work that actually needs them. Measured hosted on the full rehearsal (run 30969467329): 6s for the scope filter and 18s for lint. The aggregator does no checkout at all and is structurally identical to the already-hosted `required-ci`. The pinned jobs take a plain label rather than the CI_HOSTED_FALLBACK_ENABLED selector: a lever that can only ever resolve to ubuntu-latest reads as a choice when there is none, and these jobs need no fleet-outage escape hatch because they never depend on the fleet. The lever stays intact on every job that remains on metal, and its shared explanation moves to `build` — the first job in the file that still carries it. Timeouts come down with the jobs, per the existing rule that hosted ceilings may track observed runtime because hosted runners never enter the self-hosted queue: 10 minutes for the two that check out, 5 for the aggregator, matching `required-ci`. This also resolves the aggregation asymmetry CI.md recorded, where `test-packages-result` sat on metal while `required-ci` — the same shape of job — was hosted. Backing a required status argues for starting promptly, not for queueing. Deliberately not moved: test-core x3, test-packages x3, Coverage Gate, Build, and Typecheck, whose hosted duration under merge_group full mode has never been measured and must not move on assumption; and postgres-tests.yml, whose services container needs the dind lane. Refs #2236 Refs happyvertical/iac#1349 * docs(testing): require hookTimeout wherever testTimeout is raised The mcp-conformance-fixture config fix landed independently on main via #2231, so this commit no longer carries it. What it does carry is the part that stops the class recurring: hooks do not inherit testTimeout, and nothing in the repo said so. The failure mode is easy to misread — the file fails with "Hook timed out in 10000ms" while every test reports skipped, which looks nothing like a timeout in the test you were budgeting for. It has now hit smrt-svelte's teardown (#1426) and mcp-conformance-fixture (#2238), the latter ejecting an unrelated PR from the merge queue. Closes #2238 * docs(ci): stop recording which shard voice and video land on The comment claimed the shard script round-robins smrt-voice onto 1/3 and smrt-video onto 2/3, and concluded placement was unchanged by dropping the isolation steps. That mapping is not a property of the script and it is already wrong on this branch: test-packages-shard.mjs sorts every testable package and takes `index % n`, so an assignment moves whenever any package is added or removed anywhere in the workspace. Packages added since the comment was written have shifted both — voice is on 2/3 and video on 3/3 today. The durable statement is that they shard like every other package. The isolation came out because the root cause was fixed (AVX2 SIGILL in @happyvertical/pdf, #2195/#2196), not because placement was preserved. Raised by copilot-pull-request-reviewer on #2242.
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.
Three items off the queue-occupancy patch train. All three cut
arc-happyverticalslot occupancy without reducing what actually gates a merge.
What changed
0fa7e963smrt-video/smrt-voiceisolation steps and their--filter='!...'exclusionsf96c553eCoverage Gatetoinputs.mode == 'full'e4a94de7Publish Dry Runtomerge_group; teachRequired CIthe split9c34ed11.github/CI.mdfor the above614deb47Items 4 and 8 — both jobs ran on every PR and again in the merge group, which
re-ran them in full regardless, so the PR copies were duplicate fleet occupancy
rather than the binding gate. Coverage Gate was the expensive one: the seed
buildjob is full-mode only, so its mandatory fullpnpm run buildran coldon PRs — measured bimodal across 14 recent runs at 4.2–4.7 min warm vs 26–45 min
cold.
Neither loses meaning in the merge group:
check-coverage.mjsresolves its diffbase as
BASE_REF || 'main'plusmerge-base, which is correct for themerge_groupsynthetic head.Item 6 — the isolation steps were a workaround for silent
Worker exited unexpectedlydeaths attributed to memory pressure. Thatattribution was wrong: the failing shards recorded cgroup
oom_kill = 0, and thereal cause was an AVX2 SIGILL in
@happyvertical/pdfon x86-64-v2 metal, fixedby the 0.65.9 guard (#2195, #2196). Every sampled failure predates that bump, and
the first full merge-group run after it (
30832712023) was green on all threeshards.
scripts/test-packages-shard.mjsalready round-robins both packages ontothe same runners the isolation was pinned to (voice → 1/3, video → 2/3), so shard
placement is unchanged and each package still runs exactly once.
Deviation from the issue text
Item 4 as written proposed a paths-filter so packaging-relevant PRs still ran the
dry run. This implements the stricter
merge_group-only form instead: computingthe filter needs its own fleet job, and a job costs a median 16 min of pod queue
wait for ~1 s of work, which is most of what the item was trying to reclaim.
The gate also had to move.
hv-agent check-prrequires every reusable orself-hosted job in
on-pull-request.ymlto carry the canonical trusted-baseadmission expression byte-for-byte, so the caller's
if:cannot be narrowed —the first attempt at this was rejected by the lifecycle preflight. The condition
lives in
publish-dry-run.ymlinstead, negatingpull_request_targetratherthan testing for
merge_groupsoworkflow_dispatchstill works. The summaryjob needed the guard explicitly: its
always()would otherwise leave it running,and failing on missing artifacts, after the jobs it summarizes skip.
Validation
actionlintclean on both touched workflows;yamllintexit 0check-standards,check-agents-chain,check-no-smrt-peers,check-package-dagpassRequired CIaggregator logic unit-tested across 7 result combinations:fork PR (skipped
Validate Changes) still rejected;cancelledstill fails;merge_group+ skipped/failed dry run still fails; PR + skipped passestest-packages-shard.mjsResidual risk
None of the three items is exercised by this PR's own CI. The caller uses
pull_request_target, so GitHub loads workflow YAML from the base branch — thisPR's validation run executes
main's workflows, not the ones in the diff.Confirmed live on head
614deb47a:Coverage Gatequeued andPublish Dry Runexecuted, which is base-branch behaviour, not the gated behaviour here. All
three items therefore first take effect in the merge group.
merge_groupdoes source its YAML from the merge ref — verified, sinceGitHub documents this nowhere: run
30496440494is PR chore(policy): refresh generation 20 kernel #2141's own merge_grouprun, and its
test-packages (2/3)job contains bothRun video package tests in isolationandRun package tests (shard 2/3, excluding core and video)—the step chore(policy): refresh generation 20 kernel #2141 added and the rename it made, neither of which
mainhad at thetime. So the merge queue is the real test here, and a bug in these changes
would break the queue for everyone, not just this PR. I re-verified the
merge_grouppath statically:moderesolves tofullso Coverage Gate runs;the caller's canonical gate admits
merge_groupand the called workflow'sgithub.event_name != 'pull_request_target'is true so Publish Dry Run runs;required-citakes themerge_groupbranch and requires it green.Per the issue, give item 6 a few full-suite transits before declaring it done.
vars.CI_MERGE_QUEUE_ENABLEDis read inside the called workflow. GitHubdocuments that "for reusable workflows, the variables from the caller workflow's
repository are used", and caller and callee are both in this repository, so it
resolves. Worst case if that ever changed: the dry run runs on PRs as it does
today — item 4 would stop saving, nothing would break.
smrt-video/smrt-voicehave never run inside the shared batch under its2048 MB heap cap — that cap landed 2026-08-01, after their 2026-07-29 exclusion.
Both set
fileParallelism: false(one fork each) and video alsoisolate: false,and history shows
oom_kill = 0, so this is watch-and-see rather than a knownproblem.
Accepted trade: a coverage or packaging regression now ejects from the merge
queue instead of reddening the PR. Local pre-check:
node scripts/check-coverage.mjs --packages <list>.The local
codex reviewCLI could not be run this session — it exits silentlyright after its
UserPromptSubmithooks (three attempts, including afterclearing the stale models cache). The GitHub Codex connector did review and
raised one P1, fixed in
614deb47and resolved on the thread: gating the dryrun on the event alone would have disabled it entirely under the documented
CI_MERGE_QUEUE_ENABLEDrollback, letting a packaging failure reachpublish.yml. Copilot reviewed and generated no comments.Closes #2214
Refs #2216
{"schema":"hv-agent-run:v1","runtime":"claude","session":"8a94e288-0358-403f-a3a6-8e81a80e6c5d","issue":"2214","head_sha":"3ef5c24cf2296ea496c8820ecd6d385c60662219","policy_revision":"1.0.0","status":"complete"}