perf(ci): unblock the validation lane — occupancy, hosted light jobs, fixture hookTimeout - #2242
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces self-hosted CI lane occupancy and merge-queue ejections by (1) moving “light” jobs to GitHub-hosted runners, (2) gating expensive jobs so they only run where they actually gate merges, and (3) fixing a Vitest hook timeout misconfiguration (and documenting the pitfall) that caused flaky merge-queue failures.
Changes:
- Pin
affected-scope,lint, andtest-packages-resulttoubuntu-latestwith tighter hosted-appropriate timeouts. - Make
coverage-gatefull-mode only, and gatepublish-dry-runto skip PR lane when the merge queue is enabled (while preserving rollback behavior). - Add
hookTimeoutto the MCP conformance fixture Vitest config and document thetestTimeoutvshookTimeouttrap.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
TESTING_STANDARD.md |
Adds a testing convention warning about pairing hookTimeout with testTimeout. |
packages/mcp-conformance-fixture/vitest.config.ts |
Sets hookTimeout to match testTimeout for heavy beforeAll work. |
.github/workflows/test-suite.yml |
Pins selected light jobs to hosted runners; gates coverage; removes video/voice isolation steps. |
.github/workflows/publish-dry-run.yml |
Skips PR-lane packaging validation when merge queue will re-run it; guards summary job accordingly. |
.github/workflows/on-pull-request.yml |
Updates Required CI aggregation logic to require Publish Dry Run only where it runs. |
.github/CI.md |
Updates CI documentation to match runner placement, timeouts, and gating behavior. |
.claude/rules/testing.md |
Adds a concise rule about hookTimeout vs testTimeout for Vitest. |
This was referenced Aug 5, 2026
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)
`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
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
willgriffin
force-pushed
the
perf/issue-2214-ci-throughput-train
branch
from
August 5, 2026 23:43
0712f0d to
9e8558a
Compare
willgriffin
enabled auto-merge
August 5, 2026 23:45
Contributor
|
Removed from the merge queue — reason: MANUAL Someone removed this entry. Removal also restarts validation for every entry that was queued behind it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31059124071 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
willgriffin
added a commit
that referenced
this pull request
Aug 6, 2026
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.
willgriffin
enabled auto-merge
August 6, 2026 00:21
willgriffin
disabled auto-merge
August 6, 2026 00:51
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.
willgriffin
force-pushed
the
perf/issue-2214-ci-throughput-train
branch
from
August 6, 2026 00:53
61b27e0 to
547f6aa
Compare
willgriffin
enabled auto-merge
August 6, 2026 00:58
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 6, 2026
Contributor
|
Removed from the merge queue — reason: CI_FAILURE A required status check went red on the merge-group head. Open the linked run and read the failing leaf job (shard), not the rollup — the rollup only mirrors it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31062353981 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
willgriffin
removed this pull request from the merge queue due to the queue being cleared
Aug 6, 2026
Contributor
|
Removed from the merge queue — reason: QUEUE_CLEARED The whole merge queue was cleared. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31064580822 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
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.
{"schema":"hv-agent-run:v1","runtime":"claude","session":"8a94e288-0358-403f-a3a6-8e81a80e6c5d","issue":"2214","head_sha":"547f6aabf15d4f0380eb2b02dfb5f699aea69e89","policy_revision":"1.0.0","status":"complete"}Why this is one PR
The CI lane is the thing blocking every other PR, and the fixes for it were spread across
three PRs that each needed a full validation pass on the very lane they were trying to unclog
— and two of them edit the same file. Consolidating them means one validation pass and one
merge, at the front of the queue, after which everything else can drain normally.
Supersedes #2217, #2237 and #2239, all closed in favour of this branch. Commits are preserved
by
git cherry-pickwith their original authors.c1261fdfd8d6223a2dd4641bac7348af5e97ed30a39e8558aWhat lands
Stop paying for work the merge group repeats (#2214). Coverage Gate becomes full-mode
only; Publish Dry Run runs only where it actually gates — it skips the pull-request lane but
not when
CI_MERGE_QUEUE_ENABLEDis cleared, since that lever stopsmerge_groupeventsentirely and packaging would otherwise reach
publish.ymlunvalidated. The smrt-video/voicetest isolation steps are dropped. The required aggregator now demands success in the merge
group and tolerates only
skippedon a PR, so re-enabling the job cannot silently un-gate it.Get the light jobs off the metal fleet (#2236).
affected-scope,lintandtest-packages-resultpin toubuntu-latest— a paths-filter, Biome vianpx, and a jobthat only reads
needs.*.result. None touches the workspace install.Stop the class of hook timeout recurring (#2238). The
mcp-conformance-fixtureconfig fixlanded independently on
mainvia #2231 while this PR was open, so that hunk is gone andthis branch takes main's
hookTimeout: 30_000unchanged — the value is main's call, notre-litigated here. What remains is the part still missing from the repo: the rule itself, in
.claude/rules/testing.mdandTESTING_STANDARD.md. Hooks do not inherittestTimeout, andnothing said so, which is why the same bug was fixed three separate times in one afternoon
(#2238, #2239, #2240) after already hitting smrt-svelte's teardown in #1426.
Conflict resolution
#2217 and #2237 both edit
.github/workflows/test-suite.ymland.github/CI.md. The result isthe union of both intents, verified job by job rather than assumed:
CI_HOSTED_FALLBACK_ENABLEDselectorcoverage-gatekeeps perf(ci): stop double-running coverage and packaging gates, drop video/voice isolation #2217's narrowedif: inputs.mode == 'full'. perf(ci): pin the light validation jobs to GitHub-hosted runners #2237 branched beforethat landed and still carried the old
always() && (inputs.mode == 'affected' || …)form.Rebased onto
mainafter #2231 merged; the only conflict was the fixture config describedabove, resolved in main's favour.
Validation
actionlinton all three changed workflows — clean, exit 0js-yaml; the job/if/runs-onmatrix was diffed against bothsource branches to confirm neither PR's intent was dropped in the merge, and re-checked after
the rebase
packages/mcp-conformance-fixture/vitest.config.tsis byte-identical tomain.github/CI.mdhas no duplicated headings and its prosematches the resulting job placement
The workflow changes here are validated by
main's YAML underpull_request_target, so theyfirst execute in the merge queue — expected for this class of change.
Closes #2214
Closes #2236
Closes #2238