perf(ci): split the 22-min serial frontend job into parallel groups - #1222
Merged
Conversation
`frontend-run` ran `pnpm check:all` end to end — 25 lint gates, then 35k tests
with coverage, then three builds, strictly serial — and was the critical path
of every PR. It is now five jobs (fe-static, fe-test x4 shards, fe-coverage,
fe-servers, fe-build), so the path is the MAX of the groups, not their SUM.
`check:all` is unchanged in what it runs: it now composes check:static,
test:coverage, check:servers and check:build, which expand to the same 31 steps
in the same order (verified by expansion before the split).
Sharding is only safe because coverage survives the merge, which was measured,
not assumed: shards write blob reports and fe-coverage applies the thresholds to
the COMBINED result, reproducing the unsharded numbers (94.28/90.62/93.77/95.13
vs 94.27/90.60/93.74/95.13). Sharding WITHOUT that merge would silently drop the
coverage gate — each shard measuring a fraction of the suite, none representing
the whole — which is worse than being slow.
Splitting check:all into groups opens a drift hole: append `pnpm lint:new-gate`
to check:all directly and it runs locally and in the pre-push hook while NO CI
job runs it — wired-up-looking, and absent from the only thing that blocks a
merge. check-scripts-parity.test.mjs forbids it: check:all must be EXACTLY the
composition of the groups CI runs, and the coverage gate must be applied to the
merged report.
Four gate tests asserted `pkg.scripts["check:all"]).toContain("lint:x")`, which
pinned the SHAPE of the script rather than the property they care about. They
now use invokedScripts() to expand transitively, so they survive regrouping.
Also makes the cjk sub-quadratic RATIO assertion opt-in (PERF=1), matching the
precedent its own sibling set (markdownPipeline/performance.test.ts):
- The function is flatly linear where it matters — 0.23 ms/1k from n=8k to
n=64k — so the failures were never the algorithm.
- A saturated runner inflates a millisecond sample ~14x, while linear and
quadratic differ by only 4x at 4x input. The noise exceeds the signal, so no
bound is both sensitive and stable.
- Widening the input ratio to buy margin does not work: past ~64k the timing
turns over (0.25 -> 1.11 ms/1k at 96k) on V8 string representation, so a
bigger sample measures the engine instead.
- Best-of-N does not rescue it either, and taking the minimum of BOTH sides
actively widens the ratio — the earlier "fix" made this test more fragile.
The absolute ceiling in the same file still runs on every CI run and catches the
quadratic defect this file exists for by orders of magnitude, so nothing is
traded away.
All four fe-test shards failed on this workflow's first run, identically: ERROR: Coverage for lines (49.47%) does not meet global threshold (94.7%) A shard runs a quarter of the suite, so it measures roughly a quarter of the coverage. The global floors are a property of the WHOLE suite and cannot be judged from a fraction — the merge job is the only place they mean anything. The workflow comment already claimed "the shards deliberately do not gate"; the shard command never actually disabled them. Zeroing the four thresholds on the shard run fixes it (verified against a control: the same command WITHOUT the overrides exits 1 with 4 threshold errors, WITH them exits 0 with none). fe-coverage still applies the real floors to the merged report, which is what gates the PR. check-scripts-parity now asserts the shard step zeroes all four thresholds, so a future edit cannot silently restore per-shard gating and leave the matrix permanently red.
fe-test (2) went red and its entire log was: Process completed with exit code 1 No test name, no assertion, no stack — nothing to tell a real regression from a flake without re-running the same partition locally. `--reporter=blob` REPLACES the console reporter, so the diagnostics went into the artifact and never reached the log. Adding `--reporter=default` alongside it restores them; with two reporters the blob path must be addressed per-reporter (`--outputFile.blob=`) or it silently lands elsewhere. Verified locally: blob still written, console output back, exit code unchanged. check-scripts-parity now asserts all three, since a diagnostics-free CI failure is the kind of defect that only announces itself at the worst moment.
…ratchet fe-test shard 2 was a REAL regression, not a flake. With the console reporter restored, it names itself: .github/workflows/ci.yml is listed in BYTE_IDENTICAL but its round trip now differs — a formatting regression on the save path. `ci.yml` sits on the two-way identity ratchet in `ghaWorkflow/save/__tests__/corpusRoundtrip.test.ts`: the file is parsed with the in-house CST parser and re-stringified, and a listed file that stops being byte-identical fails. The `>-` folded scalar I introduced for the shard command does not survive that round trip. The pre-existing `run: |` literal blocks in the same file do, which isolates the folded scalar by elimination. Fixed by writing the command as one long line, NOT as a literal `|` block: `>-` folds to a single command, whereas `|` preserves the newlines, so under `bash -e` each flag line would execute as its own command. Byte-identity is restored and the file stays on the ratchet — removing it from BYTE_IDENTICAL would have recorded a formatting regression as acceptable, which is exactly what a two-way ratchet exists to prevent.
fe-coverage died with `ENOENT: scandir '.vitest-reports'`, but the blobs were written. The shard log shows both, one second apart: 17:09:05 blob report written to .../.vitest-reports/blob-1.json 17:09:06 No files were found with the provided path: .vitest-reports/* vitest writes blobs into `.vitest-reports/`, a DOT-directory, and upload-artifact@v4 defaults to `include-hidden-files: false`, treating everything under one as hidden. So every shard produced its blob and uploaded nothing. `if-no-files-found` defaulting to `warn` is what made it expensive to find: the shards passed, and the failure surfaced two jobs later as an ENOENT naming the CONSUMER of the artifact rather than its producer. Set to `error` so a missing blob fails at the shard that should have made it. check-scripts-parity asserts both, since a silently-skipped upload is invisible by construction — the shard is green and the evidence is a warning nobody reads.
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
1 participant
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.
frontend-runranpnpm check:allend to end — 25 lint gates, then 35k testswith coverage, then three builds, strictly serial — and was the critical path
of every PR. It is now five jobs (fe-static, fe-test x4 shards, fe-coverage,
fe-servers, fe-build), so the path is the MAX of the groups, not their SUM.
check:allis unchanged in what it runs: it now composes check:static,test:coverage, check:servers and check:build, which expand to the same 31 steps
in the same order (verified by expansion before the split).
Sharding is only safe because coverage survives the merge, which was measured,
not assumed: shards write blob reports and fe-coverage applies the thresholds to
the COMBINED result, reproducing the unsharded numbers (94.28/90.62/93.77/95.13
vs 94.27/90.60/93.74/95.13). Sharding WITHOUT that merge would silently drop the
coverage gate — each shard measuring a fraction of the suite, none representing
the whole — which is worse than being slow.
Splitting check:all into groups opens a drift hole: append
pnpm lint:new-gateto check:all directly and it runs locally and in the pre-push hook while NO CI
job runs it — wired-up-looking, and absent from the only thing that blocks a
merge. check-scripts-parity.test.mjs forbids it: check:all must be EXACTLY the
composition of the groups CI runs, and the coverage gate must be applied to the
merged report.
Four gate tests asserted
pkg.scripts["check:all"]).toContain("lint:x"), whichpinned the SHAPE of the script rather than the property they care about. They
now use invokedScripts() to expand transitively, so they survive regrouping.
Also makes the cjk sub-quadratic RATIO assertion opt-in (PERF=1), matching the
precedent its own sibling set (markdownPipeline/performance.test.ts):
n=64k — so the failures were never the algorithm.
quadratic differ by only 4x at 4x input. The noise exceeds the signal, so no
bound is both sensitive and stable.
turns over (0.25 -> 1.11 ms/1k at 96k) on V8 string representation, so a
bigger sample measures the engine instead.
actively widens the ratio — the earlier "fix" made this test more fragile.
The absolute ceiling in the same file still runs on every CI run and catches the
quadratic defect this file exists for by orders of magnitude, so nothing is
traded away.