Repository navigation
perf(ci): plan the unit shards before the static-analysis gate instead of behind it - #23743
Conversation
…d of behind it A caller's `needs` gate the whole called workflow, so while the plan job lived in unit-tests.yml it could not start until static analysis and typecheck had both finished and passed -- and the shard matrix then waited on it. The two hops were serial when they did not need to be: planning reads the checkout, a git diff against HEAD^1, the import graph and the checked-in timing baseline in config/scripts/ci-shard-timings.json, and consumes nothing that static analysis, typecheck or the native-cache primer produce. Planning moves to its own reusable workflow so pr.yml can run it against code_paths alone, overlapping it with the gate. Measured across 99 runs, the shard matrix is created a median 93s earlier (p25 47s, p90 241s, never later). Planning stays a required predecessor of the shards, so an empty assignment cannot expand the matrix. The gate itself is deliberately left in place. It fires on 22% of runs, and the shard queue wait knees hard above ~9 concurrent ARM jobs -- 4s median below that against 218s at 15-19 -- so admitting 8 doomed shards per failed run would cost more in queue pressure than it returns in latency. Cost is one 37s ubuntu-latest job, which does not touch the ARM pool the shards contend for. A planning failure still fails the PR: the shards are skipped, and verify's check_job requires success whenever the classifier says tests should run, so it reports `test: expected success, got skipped`.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // unit-plan.yml so it can run before the static-analysis gate clears. | ||
| expect(sharedTest.needs).toBeUndefined() | ||
| expect(unitTestWorkflow.jobs.plan).toBeUndefined() | ||
| expect(unitTestWorkflow.on.workflow_call.inputs.shards.required).toBe(true) |
There was a problem hiding this comment.
shards is required and that test depends on unit_plan, but they do not check that the planner exposes the output or that either caller passes it to the test workflow. A broken connection could leave these contract tests passing while the shard matrix fails to expand. Please assert the planner output and both callers’ with.shards expressions.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change moves unit-test shard planning into a reusable workflow. The PR and Node-next workflows pass the planner’s shard assignments to the reusable unit-test workflow. The unit-test workflow uses those assignments in its test matrix. Workflow assertions now cover the planner dependencies and shard input. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Shard planning and artifact handoff remain connected to unit tests; no actionable merge-blocking risk remains. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — hoists unit-shard planning out of the static_analysis/typecheck gate so it overlaps the gate instead of queuing behind it.
- New
unit-plan.yml— theplanjob moved verbatim out ofunit-tests.yml, exposed viaworkflow_callwith aselection_modeinput and ashardsoutput. Still writes/upload theunit-selection-attempt-<attempt>artifact, which the shards resolve run-scoped. unit-tests.yml—planremoved;selection_modereplaced by a requiredshardsinput; the matrix readsfromJSON(inputs.shards)andtesthas noneeds. The required input is the real safety property: no caller can expand the matrix without an assignment.pr.yml— newunit_planjob gated oncode_pathsonly, withtestgaining it inneeds,needs.unit_plan.result == 'success'inif, and passingshardsthrough.verifydeliberately omitsunit_planbecause it sharestest's classifier — and that path still fails closed: a plan failure skipstest, andverify'scheck_job testrequires success whenever the classifier says tests should run.node-next-compat.yml— second caller updated the same way;selection_modestill defaults toshadow, matching prior behavior.- Contract tests — exact assertions for the new shape (
unit_plan.needs, itsif,test.ifcontainingneeds.unit_plan.result == 'success',unit-tests.ymlhaving noplanjob,shards.required === true), rather than relaxing the old ones.
I verified ci-unit-plan.mjs's inputs are independent of the gate (event payload, git diff HEAD^1, import graph, file list, checked-in timing baseline via readTimingBaseline), that the plan job installs with the default native-runtime: none, that selection_mode has no stale caller pointing at unit-tests.yml, and that both — and only — callers of unit-tests.yml are updated. config/scripts/ (275 files / 2506 tests), including the every-workflow census sweep, passes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Before running tests, CI works out how to split them across 8 machines. That planning step was sitting behind the lint-and-typecheck gate, even though it doesn't use anything those checks produce — so the tests waited for lint, then waited for planning, one after the other. Planning now happens at the same time as lint, so the tests start about a minute and a half sooner.
What Changed
The
planjob moves out ofunit-tests.ymlinto a newunit-plan.ymlreusable workflow, called frompr.ymlagainstcode_pathsalone.Why it was serial. A caller job's
needs/ifgate the entire called workflow.pr.yml'stestjob gates onstatic_analysisandtypecheck, soplan— living insideunit-tests.yml— couldn't begin until both finished and passed. The shards then waited onneeds: plan. Two hops in sequence.Why it didn't need to be.
ci-unit-plan.mjs's only inputs areGITHUB_EVENT_PATH, agit diffagainstHEAD^1, the import graph (ci-unit-dependency-graph.mjs), the file list (ci-unit-files.mjs), and a checked-in timing baseline —readTimingBaselinereadsconfig/scripts/ci-shard-timings.jsonoff disk (ci-shard-assignment.mjs:45-49), not from a prior job. It consumes nothing fromstatic_analysis,typecheck, ortest_native_cache.Measured across 99 completed runs (592 shard jobs, 2026-09-28 10:41Z–20:56Z), the shard matrix is created earlier by:
Never later. In 3 of 53 runs the hoisted
planbecame the new binding dependency (its x86 queue spiked to p90 202s), and the saving shrinks there but stays positive.Why the gate itself stays
I went in intending to remove the
static_analysis/typecheckgate ontestentirely — dropping both would save a median 218s — and the data says don't:It fires on 22% of runs, not the ~2% I assumed:
static_analysisfails on 20.2%,typecheckon 4.0%. (Two caveats that cut the other way: 12 of those 20 failures are one branch iterating, and the rate is falling — 34.7% in the older half of the window against 10.0% in the newer half. Even at 10% it is 5× my hypothesis.)The shard pool is capacity-bound. Shard queue wait against concurrent ARM jobs at the moment of queueing:
Flat to ~9 concurrent, then it knees. Measured mean is 6.9 with peak 41 from
pr.ymlalone. Removing the gate would add ~50 doomed runs/day × 8 shards × 402s = +2,680 ARM job-minutes/day (+29%), pushing the mean up ~1.9 into the steep part of that curve.docs/reference/ci-runner-efficiency.md:253is explicit that ARM labels grant no extra concurrency — they share the account limit.So the gate is load-bearing, and this PR captures the latency that was available without touching it. Hoisting
plancosts one 37subuntu-latestjob (max 67s) — zero on the ARM pool the shards contend for, and 1.1% of the 3,216 ARM job-seconds the gate prevents per doomed run.Also worth recording: the shard-start staircase is pure queueing
While measuring I found the 8 "parallel" shards spread their starts over a median 319s. It is not a matrix setting — there is no
max-parallelanywhere inunit-tests.yml(onlyfail-fast: false), and within a run the 8 shards'created_atvalues are identical (spread median 0s, max 1s). They are dispatched simultaneously and then wait for slots. That staircase is nearly as large as the gate's 233s and is entirely account concurrency, so the only levers on it are running fewer jobs or a higher concurrency allowance — not a workflow change. Noting it rather than acting on it.Mechanism, for review
.github/workflows/unit-plan.yml— today'splanjob verbatim, with aworkflow_calloutput forshardsand theselection_modeinput (which onlyplanever used).unit-tests.yml—planremoved;selection_modeinput replaced by a requiredshardsinput; the matrix readsfromJSON(inputs.shards);testno longer hasneeds.pr.yml— newunit_planjob onneeds: [code_paths]with the same classifier gate;testgainsunit_planinneeds,needs.unit_plan.result == 'success'in itsif, and passesshardsthrough.node-next-compat.yml— the second caller, updated the same way.selection_evidenceis unaffected: it downloadsunit-shard-node-*-attempt-N(uploaded by the shards), andplan'sunit-selection-attempt-Nartifact is run-scoped so it still resolves from a sibling called workflow in the same run.Linked Issue
N/A — internal CI performance work, no pre-existing issue.
Visual Proof
N/A — CI configuration only, no user-facing surface.
Testing
pr-code-change-scope,pr-workflow-parallelism,package-electron-runtime-contract,pr-e2e-gate-contract,ci-background-step-barriers,ci-cache-warmup-workflow,pr-test-loc-summary).unit_planmust gate oncode_pathsonly,testmust still requireneeds.unit_plan.result == 'success',unit-tests.ymlmust no longer have aplanjob, and itsshardsinput must berequired: true(so a caller cannot silently expand an empty matrix).shards.Failure behavior, checked explicitly: if planning fails, the shards are skipped, and
verify'scheck_jobrequires success whenever the classifier says tests should run — so it fails withtest: expected success, got skipped. I initially addedunit_plantoverify's ownneedsfor a more direct message, then backed it out: everyverifydependency is required by contract to have a matchingcode_paths.outputs.<job>gate, andunit_plandeliberately sharestest's gate rather than inventing a classifier output. The existing behavior already fails closed, so the indirection is worth more than breaking that convention.AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
shardsinput beingrequired: trueis the safety property — it means neither caller can reach the matrix without a real assignment.ubuntu-latestjob per run and removes none, but takes nothing from the ARM pool, which is the constrained one.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)