fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer (#4072)

* test(#4070): failing-first regression for shard1 aux-suite budget imbalance

- selectShard has no way to reserve virtual weight on a bin, so the LPT
  unit-test packer cannot account for shard 1's fixed aux-suite cost
  (integration/security/install/slow all pinned to shard 1/3).
- test.yml wires no such reserve into the workflow.
- ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test`
  was stale (7m12s from run 30677442953, predating the aux-suite growth);
  corrected to the real evidence cited in #4070 (13m48s / cancelled at
  ~14m51s), which now honestly fails the file's own 1.5x headroom policy
  against the current 15-minute cap.

All three are expected RED on this commit; see
.gsd/bug/fix-4070-shard1-aux-suite-budget/50-test-matrix.md.

* fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer

selectShard now accepts an optional initialWeights array giving one or more
bins a virtual head start before any file is placed, so LPT converges each
bin's FINAL total (assigned weight + head start) toward equal instead of
raw assigned weight alone. test.yml wires RUN_TESTS_SHARD_RESERVE=1:77 into
the full-scope unit-test step (gated on matrix.scope == 'full', so the
unrelated windows lane is unaffected) -- 77 weight units is the empirical
conversion of the aux suites' ~220s measured fixed cost, derived against
the real tests/test-timings.json (see the diagnosis artifact for the full
computation).

Also corrects two pieces of now-stale bookkeeping this issue exposed:
- ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test`
  carried a 7m12s figure that predated the aux-suite growth; corrected to
  the real pre-fix evidence (13m48s / cancelled at ~14m51s, issue #4070),
  which requires raising timeout-minutes from 15 to 21 (1.5x headroom over
  the real worst-case measurement) to satisfy the file's own policy.
- test.yml's job-header and matrix comments, which still claimed the aux
  suites cost "~1m35s combined" (they now measure ~216-224s).

Closes the gap the previous commit's failing-first tests proved: selectShard
had no reserve-capacity mechanism and test.yml wired none in.

* fix(#4070): correct the reserved-weight property bound; cover main()'s reserve bounds check

Isolated code review found a genuine gap and gsd-test's real run confirmed a real
test bug it exposed:

- The fast-check property "no shard exceeds average(+reserve) + heaviest file" was
  falsified by gsd-test itself (weights=[1,1,1], total=2, reserve=6 on bin 0):
  selectShard is correct, the BOUND was wrong. A reserve large enough that its bin
  never receives a real item stays at exactly that reserve forever -- no amount of
  routing real items elsewhere can dilute a fixed head start below itself -- so the
  true bound is max(reserve, the classic Graham term), not the Graham term alone.
  Verified the corrected bound against the exact counterexample plus 20,000
  additional random trials (zero violations) before re-running gsd-test.

- Isolated review (MAJOR): the shard-total bounds check on RUN_TESTS_SHARD_RESERVE
  and its console.error fallback in main() were untested end-to-end --
  parseShardReserve itself has no concept of the shard total, so only main()
  enforces that guard, and nothing exercised it through the subprocess seam. Added
  an E2E harness test that sets RUN_TESTS_SHARD_RESERVE to an out-of-range index
  via the real CLI, asserts the fallback warning fires, AND asserts the resulting
  file selection is byte-identical to a no-reserve control run against the same
  injected timings table -- proving the reserve was actually ignored, not just
  that a warning printed.

* chore(#4070): backfill changeset PR number

pr:0 -> pr:4072

* fix(#4070): strip leaked RUN_TESTS_SHARD_RESERVE from the harness test's child env

Real GH Actions CI on this PR (run 33288554040, ubuntu shard 2/3) failed 7
tests in the shard-partitioning describe block, all with the same symptom:
`run-tests: no tests in suite "all"` where a real file count was expected.
gsd-test's own dockerized bench run never showed this, and ubuntu shards 1/3
and 3/3 (which run the same test.yml step) passed clean -- the discrepancy is
the tell: only shard 2/3 happened to schedule this specific test FILE for
that run, and the outer CI job's own environment is where the leak lives.

Root cause: test.yml's "Run unit tests" step now sets
RUN_TESTS_SHARD_RESERVE=1:77 (this issue's own reserve mechanism) on the
OUTER job that runs `npm run test:coverage:unit:raw -- --shard N`. The
harness test file's runHarness() helper spawns run-tests.cjs as a CHILD of
that same job via `{...process.env, ...extraEnv}`, so every pre-existing
--shard test in this describe block silently inherited the ambient reserve
-- even though none of them know it exists. A reserve of 77 weight units
utterly dwarfs the ~0.3 total weight of the 9-file synthetic fixtures these
tests use (none are in the real timings table, so all fall back to the same
tiny median weight), so shard index 1 is routed zero files every time --
exactly the observed "no tests" failures, and exactly the skewed 5/4 split
observed on the shard-2 test that expected a plain 3/3/3 round-robin.

Reproduced locally end to end (set RUN_TESTS_SHARD_RESERVE=1:77, spawn the
old runHarness against a synthetic 9-file fixture, --shard 1/3 -- reproduces
the exact "no tests in suite \"all\"" stderr) and confirmed the fix (env
stripped unless a test opts in via extraEnv, as the #4070 E2E bounds-check
test already does) resolves it, before re-running gsd-test.

This is a genuine bug this PR introduced -- a new ambient env var that a
pre-existing subprocess-spawning test helper didn't know to isolate against
-- not a pre-existing flake and not resource contention.

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-30 00:43:02 -04:00
committed by GitHub
parent 23941a871c
commit 86452da7cb
6 changed files with 472 additions and 33 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4072
---
**CI shard 1 no longer runs at 92-99% of its timeout cap.** The full-scope unit-test shard balancer now reserves shard 1's fixed aux-suite cost (integration/security/install/slow) before packing unit-test files onto it, instead of leaving shard 1 to carry that cost on top of an equal unit-test share. (#4070)

View File

@@ -134,26 +134,44 @@ jobs:
runs-on: ${{ matrix.os }} runs-on: ${{ matrix.os }}
# #2952: the `scope: full` lane is sharded three ways (see the matrix # #2952: the `scope: full` lane is sharded three ways (see the matrix
# below), so the budget covers ONE shard, not the whole unit suite. # below), so the budget covers ONE shard, not the whole unit suite.
# Measured on run 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. Shard
# 1 is the long pole because the unsharded aux suites
# (integration/security/install/slow) ride along on it — that is
# deliberate; they total ~1m35s and sharding them would cost more than it
# saves.
# #
# 15 is ~1.9x the slowest measured shard. Before sharding this same lane ran # #4070: the run 30677442953 figure this comment used to cite (shard 1/3
# 15m20s against a 15-minute cap and was killed mid-run, which is the whole # 7m12s) predated the aux-suite growth below and was stale. Shard 1 is the
# of #2952 — the number is unchanged, the work behind it is a third the size. # long pole because the unsharded aux suites
# (integration/security/install/slow) ride along on it — that is
# deliberate, but their combined cost grew well past what was budgeted for
# it (see the matrix comment below), which structurally overloaded shard 1
# relative to shards 2/3: real measured totals hit 13m48s (run
# 33278340189) and CANCELLED at ~14m51s / 99% of the old 15-minute cap
# (run 33285384930). Fixed by RUN_TESTS_SHARD_RESERVE below, which tells
# the LPT unit-test packer (scripts/run-tests.cjs's selectShard) to give
# shard 1 a smaller unit-test share, offsetting its fixed aux-suite cost —
# see tests/run-tests-harness.test.cjs's "selectShard reserved-weight
# partition (#4070)" for the packer-side proof and
# tests/ci-full-lane-sharding.test.cjs for the workflow-wiring guard.
#
# 21 is 1.5x the 14-minute pre-fix worst measurement (ceil(14*1.5)=21) —
# tests/ci-test-job-timeout-budget.test.cjs enforces this arithmetic, not
# just this comment. The rebalancing above only improves shard 1's real
# margin against this cap further; the cap itself is sized to the
# real evidence in hand, not to an unmeasured optimistic estimate. Before
# sharding this same lane ran 15m20s against a 15-minute cap and was
# killed mid-run (#2952) — the number changing here is not a repeat of
# that failure mode, it is re-basing the SAME headroom policy on a number
# that had gone stale.
# #
# The `scope: windows` lane is sharded three ways for the same reason # The `scope: windows` lane is sharded three ways for the same reason
# (see #3057): on PR #3094 it reached 15m05s against this same cap and was # (see #3057): on PR #3094 it reached 15m05s against a 15-minute cap and
# CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped in # was CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped
# the install-heavy suites and pushed the single Windows lane over the top. # in the install-heavy suites and pushed the single Windows lane over the
# Per #869, a timeout bump only moves that cliff; sharding removes it. The # top. Per #869, a timeout bump only moves that cliff; sharding removes
# cap stays at 15 unchanged: each shard now does roughly a third of the # it. That lane does not run any aux suite on its own shard 1 (the
# work, so headroom improves rather than needing a raise. # aux-suite `if:` conditions below are gated on `scope == 'full'`
# specifically), so it needs no reserve and is unaffected by this cap
# change beyond sharing the same job-level `timeout-minutes`.
# tests/ci-test-job-timeout-budget.test.cjs holds every lane here to a # tests/ci-test-job-timeout-budget.test.cjs holds every lane here to a
# headroom factor over its own measured cost. # headroom factor over its own measured cost.
timeout-minutes: 15 timeout-minutes: 21
env: env:
GSD_PLUGIN_ROOT: .ci-gsd-plugin-root-disabled GSD_PLUGIN_ROOT: .ci-gsd-plugin-root-disabled
# #2665: a live-config leak fails the run on Linux/macOS lanes. Windows # #2665: a live-config leak fails the run on Linux/macOS lanes. Windows
@@ -190,7 +208,15 @@ jobs:
# using LPT in scripts/run-tests.cjs — the same cost-aware packer #2472 # using LPT in scripts/run-tests.cjs — the same cost-aware packer #2472
# gave the `test-full` lane, which measures 0.0% spread across 3 bins on # gave the `test-full` lane, which measures 0.0% spread across 3 bins on
# the current table. The aux suites (integration/security/install/slow) # the current table. The aux suites (integration/security/install/slow)
# total ~1m35s and are not worth sharding; they run on shard 1 only. # run on shard 1 only — sharding them too was evaluated and rejected for
# #4070 (see tests/run-tests-harness.test.cjs's "selectShard
# reserved-weight partition (#4070)" comment for the full trade-off):
# they now cost ~216-224s combined (issue #4070's cited evidence — more
# than double the ~1m35s this comment used to claim), and
# `install`/`slow` spawn real subprocesses whose per-invocation startup
# overhead a 3-way split would triple. Instead `RUN_TESTS_SHARD_RESERVE`
# below reserves that fixed cost out of shard 1's LPT unit-test share, so
# shard 1 gets fewer unit-test files rather than more aux-suite runs.
# #
# #3057: the `scope: windows` lane is now sharded three ways too, the # #3057: the `scope: windows` lane is now sharded three ways too, the
# same fix applied to the same cliff (#869's stated durable follow-up). # same fix applied to the same cliff (#869's stated durable follow-up).
@@ -320,6 +346,22 @@ jobs:
if: matrix.scope == 'full' if: matrix.scope == 'full'
env: env:
NODE_OPTIONS: --max-old-space-size=6144 NODE_OPTIONS: --max-old-space-size=6144
# #4070: shard 1 alone also runs the four aux suites below (see their
# comment) — a fixed cost this step's own LPT packer has no other way
# to see. Reserving it out of shard 1's unit-test share (in LPT weight
# units, not milliseconds — see scripts/run-tests.cjs's
# parseShardReserve/selectShard) is what keeps shard 1 from carrying
# both its full unit-test share AND the aux-suite cost on top. Gated on
# `scope == 'full'` specifically, NOT on `matrix.shard == '1/3'` alone,
# because the SAME literal must reach all three full-scope shards'
# invocations identically — each recomputes the whole 3-way partition
# independently and must agree on it (see the `sig` cross-job
# fingerprint diagnostic further down in run-tests.cjs). The
# `scope: windows` lane runs no aux suite on its own shard 1, so this
# must stay empty there. tests/ci-full-lane-sharding.test.cjs pins both
# halves of this contract. Reserve-value derivation:
# .gsd/bug/fix-4070-shard1-aux-suite-budget/10-diagnosis.md.
RUN_TESTS_SHARD_RESERVE: ${{ matrix.scope == 'full' && '1:77' || '' }}
run: npm run test:coverage:unit:raw -- --shard ${{ matrix.shard }} run: npm run test:coverage:unit:raw -- --shard ${{ matrix.shard }}
# Raw V8 dumps, not rendered reports — coverage-gate merges these. They # Raw V8 dumps, not rendered reports — coverage-gate merges these. They
@@ -334,8 +376,11 @@ jobs:
if-no-files-found: error if-no-files-found: error
retention-days: 1 retention-days: 1
# The aux suites are small (~1m35s combined) and unsharded — running them # The aux suites cost ~216-224s combined (#4070 — see the "Run unit
# on every shard would triple their cost for no signal. # tests" step's RUN_TESTS_SHARD_RESERVE env comment) and are unsharded —
# running them on every shard would triple their process-spawn overhead
# for no signal (install/slow spawn real subprocesses per test). That
# fixed cost is reserved out of shard 1's unit-test share above instead.
- name: Run integration tests - name: Run integration tests
if: matrix.scope == 'full' && matrix.shard == '1/3' if: matrix.scope == 'full' && matrix.shard == '1/3'
run: npm run test:integration run: npm run test:integration
@@ -357,7 +402,7 @@ jobs:
continue-on-error: true continue-on-error: true
env: env:
CI_JOB_LABEL: "test (${{ matrix.os }}, ${{ matrix.node-version }}${{ matrix.shard && format(', shard {0}', matrix.shard) || '' }})" CI_JOB_LABEL: "test (${{ matrix.os }}, ${{ matrix.node-version }}${{ matrix.shard && format(', shard {0}', matrix.shard) || '' }})"
CI_JOB_TIMEOUT_MINUTES: '15' CI_JOB_TIMEOUT_MINUTES: '21'
run: node scripts/ci-check-job-near-cap.cjs run: node scripts/ci-check-job-near-cap.cjs
test-inert: test-inert:

View File

@@ -221,6 +221,29 @@ function parseShardArg(value) {
return { index, total }; return { index, total };
} }
// Parse RUN_TESTS_SHARD_RESERVE (#4070) — "<index>:<weight>", e.g. "1:77":
// give shard `index` (1-based) a virtual head start of `weight` LPT weight
// units before any real file is placed. This is an advisory operator knob,
// not a CLI flag with a hard-error contract like --shard: it is set from a
// GitHub Actions matrix expression (test.yml), so a malformed or absent value
// must degrade to "no reserve" rather than take the whole job down — the same
// fail-open precedent as positiveNumberEnv elsewhere in this file. Returns
// `null` for anything malformed; the caller is responsible for checking
// `index` against the shard total it actually has (this function has no
// access to that).
function parseShardReserve(value) {
if (typeof value !== 'string') return null;
const trimmed = value.trim();
if (trimmed === '') return null;
const m = /^(\d+):(-?\d+(?:\.\d+)?)$/.exec(trimmed);
if (!m) return null;
const index = Number(m[1]);
const weight = Number(m[2]);
if (!Number.isInteger(index) || index < 1) return null;
if (!Number.isFinite(weight) || weight < 0) return null;
return { index, weight };
}
// Deterministic partition of an ALREADY-SORTED file list. Without a weigher // Deterministic partition of an ALREADY-SORTED file list. Without a weigher
// this is the original round-robin (#1212): // this is the original round-robin (#1212):
// Shard `index` (1-based) receives every file whose position k in the sorted // Shard `index` (1-based) receives every file whose position k in the sorted
@@ -243,7 +266,20 @@ function parseShardArg(value) {
// This is the same algorithm packChunks uses one level down (#2456/#2463), so // This is the same algorithm packChunks uses one level down (#2456/#2463), so
// both layers now share one cost model. Omitting `weightOf` keeps the legacy // both layers now share one cost model. Omitting `weightOf` keeps the legacy
// round-robin byte-identical — callers with no timing data lose nothing. // round-robin byte-identical — callers with no timing data lose nothing.
function selectShard(sortedFiles, { index, total }, weightOf) { //
// `initialWeights` (#4070, optional): per-bin starting weight, indexed 0..
// total-1 (bin 0 = shard 1). Some shards carry a FIXED cost this function has
// no other way to see — test.yml pins four aux suites to shard 1 only, so
// shard 1's real cost is always "its LPT-balanced unit-test slice PLUS that
// fixed aux cost," while shards 2..N carry only their slice. Giving bin 0 a
// virtual head start before any file is placed makes LPT route FEWER files to
// it, converging every bin's FINAL total (assigned weight + its initial
// weight) toward equal instead of converging raw assigned weight toward
// equal — which is what left shard 1 structurally overloaded. Only meaningful
// on the weighted (LPT) path: the round-robin fallback below has no notion of
// weight to rebalance around, so `initialWeights` is a no-op there — a caller
// with no timing table has no basis to compute a meaningful reserve either.
function selectShard(sortedFiles, { index, total }, weightOf, initialWeights) {
if (total === 1) return sortedFiles; if (total === 1) return sortedFiles;
if (typeof weightOf !== 'function') { if (typeof weightOf !== 'function') {
return sortedFiles.filter((_, k) => k % total === index - 1); return sortedFiles.filter((_, k) => k % total === index - 1);
@@ -255,7 +291,15 @@ function selectShard(sortedFiles, { index, total }, weightOf) {
const w = weightOf(file); const w = weightOf(file);
return Number.isFinite(w) && w >= 0 ? w : 0; return Number.isFinite(w) && w >= 0 ? w : 0;
}; };
const bins = Array.from({ length: total }, () => ({ weight: 0, picks: [] })); // Same clamp applied to a caller-supplied reserve: a hostile value (NaN,
// negative, Infinity — e.g. a malformed RUN_TESTS_SHARD_RESERVE) must
// degrade to "no reserve" rather than poisoning every subsequent
// lightest-bin comparison the same way an unclamped file weight would.
const safeInitial = (i) => {
const w = Array.isArray(initialWeights) ? initialWeights[i] : undefined;
return Number.isFinite(w) && w >= 0 ? w : 0;
};
const bins = Array.from({ length: total }, (_, i) => ({ weight: safeInitial(i), picks: [] }));
// LPT: heaviest first, each into the currently-lightest bin. Ties break on // LPT: heaviest first, each into the currently-lightest bin. Ties break on
// the caller's sort position, and the lightest-bin scan takes the FIRST // the caller's sort position, and the lightest-bin scan takes the FIRST
// minimum, so the partition is byte-identical across Windows/macOS/Linux — // minimum, so the partition is byte-identical across Windows/macOS/Linux —
@@ -878,7 +922,32 @@ function main() {
if (usingShard) { if (usingShard) {
emptyBeforeShard = selectedNames.length === 0; emptyBeforeShard = selectedNames.length === 0;
shardInput = [...selectedNames].sort(); shardInput = [...selectedNames].sort();
selectedNames = selectShard(shardInput, parsed.shard, fileWeightOf()); // #4070: RUN_TESTS_SHARD_RESERVE gives one shard a virtual head start in
// LPT weight units (same units fileWeightOf() already produces — no ms
// conversion needed) BEFORE any file is placed, so the packer routes
// fewer files to a shard that test.yml already knows carries a fixed
// extra cost outside this script's model (the aux suites pinned to shard
// 1 only). Every shard job of a run must build the IDENTICAL
// initialWeights array — the partition is recomputed independently in
// each shard's own process, so a value that differed between them would
// silently desync which files land where (the same hazard the `sig`
// cross-job fingerprint below already guards for the file list itself).
// Malformed/absent/out-of-range input degrades to no reserve — advisory,
// never gating, matching every other knob this script reads from the
// environment.
let initialWeights;
const reserve = parseShardReserve(process.env.RUN_TESTS_SHARD_RESERVE);
if (reserve && reserve.index <= parsed.shard.total) {
initialWeights = Array.from({ length: parsed.shard.total }, () => 0);
initialWeights[reserve.index - 1] = reserve.weight;
} else if (process.env.RUN_TESTS_SHARD_RESERVE) {
console.error(
`run-tests: RUN_TESTS_SHARD_RESERVE="${process.env.RUN_TESTS_SHARD_RESERVE}" is not a `
+ `valid "<index>:<weight>" reserve for --shard total ${parsed.shard.total} — ignoring `
+ '(no reserve applied)',
);
}
selectedNames = selectShard(shardInput, parsed.shard, fileWeightOf(), initialWeights);
} }
const selected = selectedNames.map(f => join(testDir, f)); const selected = selectedNames.map(f => join(testDir, f));
@@ -1379,6 +1448,7 @@ module.exports = {
ensureBuiltArtifacts, ensureBuiltArtifacts,
ensureBuiltHooks, ensureBuiltHooks,
parseShardArg, parseShardArg,
parseShardReserve,
selectShard, selectShard,
positiveNumberEnv, positiveNumberEnv,
loadTestTimings, loadTestTimings,

View File

@@ -39,6 +39,7 @@ const assert = require('node:assert/strict');
const fs = require('node:fs'); const fs = require('node:fs');
const path = require('node:path'); const path = require('node:path');
const yaml = require('js-yaml'); const yaml = require('js-yaml');
const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs');
const WORKFLOWS_DIR = path.join(__dirname, '..', '.github', 'workflows'); const WORKFLOWS_DIR = path.join(__dirname, '..', '.github', 'workflows');
@@ -186,6 +187,75 @@ test('the full test lane is sharded and complete (#2952)', async (t) => {
}); });
}); });
// #4070: the aux suites (integration/security/install/slow) are pinned to
// shard 1 only, but the LPT unit-test packer that balances shards 1/2/3 had
// no visibility into that fixed cost — it balanced the unit-test slice as if
// all three shards carried equal fixed cost, structurally overloading shard
// 1. RUN_TESTS_SHARD_RESERVE tells scripts/run-tests.cjs's selectShard to
// give shard 1 a virtual head start in weight equal to the aux suites'
// measured cost, so the packer routes fewer unit-test files to it. See
// tests/run-tests-harness.test.cjs's "selectShard reserved-weight partition
// (#4070)" describe block for the packer-side math; this file only pins the
// WORKFLOW wiring — that the reserve literal actually reaches the full-scope
// shards and does NOT leak onto the unrelated windows lane.
test('shard 1\'s aux-suite cost is reserved out of the unit-test packer (#4070)', async (t) => {
const workflow = loadWorkflow('test.yml');
// Reserve value derivation (see .gsd/bug/fix-4070-shard1-aux-suite-budget/
// 10-diagnosis.md "Rejected fixes" for the full computation): aux-suite
// fixed cost measured at ~216-224s combined across two real runs (issue
// #4070's cited evidence, runs 33278340189 and 33285384930); shard 1's
// current unit-only real cost (~584-615s, same issue) divided by its
// current unit-only LPT weight (209.46, computed locally against
// tests/test-timings.json) gives an empirical ~2.86s-per-weight-unit
// conversion; 220s / 2.86 ≈ 77 weight units — roughly 77 average-cost test
// files' worth of virtual head start.
const EXPECTED_RESERVE = '1:77';
await t.test('the unit-test step reserves shard 1\'s aux-suite cost for the full-scope lane', () => {
const unitStep = workflow.jobs.test.steps.find(
(s) => typeof s.run === 'string' && s.run.includes('test:coverage:unit:raw'),
);
assert.ok(unitStep, 'no step in the test job runs the raw unit coverage script');
const reserveEnv = unitStep.env && unitStep.env.RUN_TESTS_SHARD_RESERVE;
assert.ok(
reserveEnv,
'the unit-test step declares no RUN_TESTS_SHARD_RESERVE env var — shard 1 would carry '
+ 'the aux-suite cost with no offsetting reduction in its unit-test share (#4070)',
);
assert.match(
String(reserveEnv), /matrix\.scope == 'full'/,
'RUN_TESTS_SHARD_RESERVE must be conditioned on `scope == \'full\'` — the windows lane '
+ 'runs no aux suites on its own shard 1 and must not be penalized for a cost it never pays',
);
assert.match(
String(reserveEnv), new RegExp(escapeRegex(EXPECTED_RESERVE)),
`RUN_TESTS_SHARD_RESERVE does not carry the documented reserve literal ${EXPECTED_RESERVE} — `
+ 'if the aux-suite cost was re-measured, update BOTH this literal and the derivation '
+ 'comment above (and the diagnosis doc) together, the same discipline LANE_COSTS uses',
);
});
await t.test('the windows lane is not given a reserve it does not need', () => {
// The reserve env is a single ternary string shared by every scope, so
// this is really the same assertion as above stated the other way: for
// any matrix entry that is NOT scope:full, the expression must resolve
// falsy. Evaluated structurally (parsing the ternary) rather than by
// simulating GitHub's expression engine.
const unitStep = workflow.jobs.test.steps.find(
(s) => typeof s.run === 'string' && s.run.includes('test:coverage:unit:raw'),
);
const reserveEnv = String((unitStep.env && unitStep.env.RUN_TESTS_SHARD_RESERVE) || '');
const ternary = /\$\{\{\s*matrix\.scope == 'full'\s*&&\s*'[^']*'\s*\|\|\s*'([^']*)'\s*\}\}/.exec(reserveEnv);
assert.ok(ternary, `RUN_TESTS_SHARD_RESERVE is not a scope-gated ternary: ${reserveEnv}`);
assert.equal(
ternary[1], '',
'the non-full-scope branch of the reserve ternary must resolve to an empty string, so '
+ 'the windows and targeted lanes see no reserve at all',
);
});
});
test('the merged coverage gate survives sharding (#2952)', async (t) => { test('the merged coverage gate survives sharding (#2952)', async (t) => {
const workflow = loadWorkflow('test.yml'); const workflow = loadWorkflow('test.yml');

View File

@@ -55,11 +55,25 @@ const HEADROOM_FACTOR = 1.5;
const LANE_COSTS = [ const LANE_COSTS = [
{ {
job: 'test', job: 'test',
measuredMinutes: 8, // #4070: the `run 30677442953 — 7m12s` figure this entry carried before
// was stale — it predated the aux-suite growth this entry now tracks, and
// being stale meant this gate never caught the drift it exists to catch.
// Real measured cost from issue #4070's cited evidence (independently
// re-verified against the workflow's current step order): shard 1/3 hit
// 13m48s on run 33278340189 (successful) and was CANCELLED at ~14m51s (99%
// of the 15-minute cap) on run 33285384930. 14 minutes is the honest
// figure — rounded up from the higher, cancelled-run observation, since a
// cancelled run's own timestamp is still real elapsed time even though the
// job never finished.
measuredMinutes: 14,
// Sharded three ways as of #2952, so this is ONE shard's cost, not the // Sharded three ways as of #2952, so this is ONE shard's cost, not the
// whole unit suite. Run 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. // whole unit suite. Shard 1 is the long pole because the unsharded aux
// Shard 1 is the long pole because the unsharded aux suites ride on it. // suites (integration/security/install/slow) ride on it — #4070 fixed the
// Before sharding the same lane cost 15m20s and blew a 15-minute cap. // LPT unit-test packer to reserve shard 1's aux-suite cost
// (RUN_TESTS_SHARD_RESERVE, see .github/workflows/test.yml and
// tests/ci-full-lane-sharding.test.cjs) so shard 1 gets a smaller
// unit-test share than shards 2/3. Before sharding the same lane cost
// 15m20s and blew a 15-minute cap (#2952).
// //
// This one `timeout-minutes` also covers the `scope: windows` matrix // This one `timeout-minutes` also covers the `scope: windows` matrix
// entries — GitHub applies a single job-level budget across every matrix // entries — GitHub applies a single job-level budget across every matrix
@@ -67,12 +81,11 @@ const LANE_COSTS = [
// (#3057), but no post-sharding per-shard measurement exists yet: its only // (#3057), but no post-sharding per-shard measurement exists yet: its only
// recorded cost is the PRE-sharding whole-suite run that hit 15m05s and was // recorded cost is the PRE-sharding whole-suite run that hit 15m05s and was
// CANCELLED on PR #3094. Each of its three shards should now cost roughly a // CANCELLED on PR #3094. Each of its three shards should now cost roughly a
// third of that (~5m), which is already comfortably under the 8m/12m this // third of that (~5m), comfortably under what this entry requires — so no
// entry requires — so no separate LANE_COSTS entry is added on a number // separate LANE_COSTS entry is added on a number that has not actually
// that has not actually been measured. Replace this estimate with a real // been measured. Replace this estimate with a real measured shard cost
// measured shard cost once one exists, the same discipline every other // once one exists, the same discipline every other entry here follows.
// entry here follows. evidence: 'run 33278340189 — 13m48s completed; run 33285384930 — CANCELLED at ~14m51s (#4070)',
evidence: 'run 30677442953 — 7m12s slowest shard',
}, },
{ {
job: 'test-full', job: 'test-full',
@@ -229,7 +242,7 @@ test('mutation.yml mutate job timeout budgets (#4036)', async (t) => {
test('near-cap check CI_JOB_TIMEOUT_MINUTES literals match each job\'s own timeout-minutes (#4036)', async (t) => { test('near-cap check CI_JOB_TIMEOUT_MINUTES literals match each job\'s own timeout-minutes (#4036)', async (t) => {
const staticLanes = [ const staticLanes = [
{ workflowFile: 'test.yml', jobKey: 'test', envLiteral: '15' }, { workflowFile: 'test.yml', jobKey: 'test', envLiteral: '21' },
{ workflowFile: 'test.yml', jobKey: 'test-full', envLiteral: '45' }, { workflowFile: 'test.yml', jobKey: 'test-full', envLiteral: '45' },
{ workflowFile: 'install-smoke.yml', jobKey: 'smoke', envLiteral: '12' }, { workflowFile: 'install-smoke.yml', jobKey: 'smoke', envLiteral: '12' },
]; ];

View File

@@ -55,6 +55,24 @@ function runHarness(testDir, args = [], extraEnv = {}) {
// doesn't refuse to run with "recursive run() skipping running files". // doesn't refuse to run with "recursive run() skipping running files".
const env = { ...process.env, GSD_TEST_DIR: testDir, ...extraEnv }; const env = { ...process.env, GSD_TEST_DIR: testDir, ...extraEnv };
delete env.NODE_TEST_CONTEXT; delete env.NODE_TEST_CONTEXT;
// #4070: strip RUN_TESTS_SHARD_RESERVE inherited from the OUTER job's own
// environment. test.yml sets it on the "Run unit tests" step for the real
// production shard 1 of the full-scope lane — and since these tests spawn
// run-tests.cjs as a CHILD of that same step, they inherit it via
// `...process.env` above like any other ambient var. Left unstripped, a
// reserve of 77 weight units utterly dwarfs these synthetic 9-file
// fixtures' combined weight (~0.3, since none of them are in the real
// timings table), so shard index 1 gets EVERY file routed away from it —
// a real, reproducible corruption of every test in this describe block,
// not a flake (confirmed live: CI run 33288554040, shard 2/3, 7 of these
// tests failed with exactly this signature). Deleted before `extraEnv` is
// applied above would be too late (spread order), so it is deleted here,
// AFTER composition, then only reinstated if a specific test opted in via
// extraEnv — preserving this file's one legitimate use (the #4070 E2E
// bounds-check test below, which sets it deliberately).
if (!Object.prototype.hasOwnProperty.call(extraEnv, 'RUN_TESTS_SHARD_RESERVE')) {
delete env.RUN_TESTS_SHARD_RESERVE;
}
const r = runNode([HARNESS, ...args], { const r = runNode([HARNESS, ...args], {
cwd: path.join(__dirname, '..'), cwd: path.join(__dirname, '..'),
env, env,
@@ -542,6 +560,56 @@ test('ambient GSD workstream vars are stripped by the runner', () => {
assert.match(r.stderr, /sig=[0-9a-f]+/, 'shard diagnostics must emit an input fingerprint'); assert.match(r.stderr, /sig=[0-9a-f]+/, 'shard diagnostics must emit an input fingerprint');
}); });
// #4070 E2E: main()'s own bounds check on RUN_TESTS_SHARD_RESERVE — an
// index that does not exist for the shard total in play — is invisible to
// every pure in-memory selectShard/parseShardReserve test, because that
// check lives in main() itself (scripts/run-tests.cjs, the
// `reserve.index <= parsed.shard.total` guard and its console.error
// fallback), which only runs through the CLI subprocess seam. Proves both
// halves: the warning fires, AND the selection is provably unaffected
// (byte-identical to a control run with no RUN_TESTS_SHARD_RESERVE at
// all, both against the SAME injected timings table so the comparison
// isn't muddied by table drift).
test('RUN_TESTS_SHARD_RESERVE with an out-of-range index warns and is ignored (#4070)', () => {
seed(tmpDir, SHARD_NAMES);
const timings = {
schema_version: 1, unit: 'ms', timings: Object.fromEntries(
SHARD_NAMES.map((n, i) => [n, i % 3 === 0 ? 30000 : 100]),
),
};
const tablePath = path.join(tmpDir, 'injected-timings-oob.json');
fs.writeFileSync(tablePath, JSON.stringify(timings));
try {
// --shard 1/3 → valid indices are 1..3. "5:999" is out of range.
const withBadReserve = runHarness(
tmpDir, ['--shard', '1/3'],
{ RUN_TESTS_TIMINGS_FILE: tablePath, RUN_TESTS_SHARD_RESERVE: '5:999' },
);
assert.strictEqual(withBadReserve.status, 0, `stderr: ${withBadReserve.stderr}`);
assert.match(
withBadReserve.stderr,
/RUN_TESTS_SHARD_RESERVE="5:999" is not a valid .* for --shard total 3 — ignoring/,
`expected the out-of-range-index fallback warning; got stderr: ${withBadReserve.stderr}`,
);
const control = runHarness(
tmpDir, ['--shard', '1/3'],
{ RUN_TESTS_TIMINGS_FILE: tablePath },
);
assert.strictEqual(control.status, 0, `stderr: ${control.stderr}`);
assert.doesNotMatch(control.stderr, /RUN_TESTS_SHARD_RESERVE/, 'control run must not warn — it sets no reserve at all');
const filesLine = (s) => (s.match(/files=\d+: (.*)$/m) || [])[1] || '';
assert.strictEqual(
filesLine(withBadReserve.stderr), filesLine(control.stderr),
'an out-of-range reserve index must select EXACTLY the same files as no reserve at all — '
+ 'the fallback warning alone is not proof the reserve was actually ignored',
);
} finally {
try { fs.unlinkSync(tablePath); } catch { /* best effort */ }
}
});
test('shard diagnostics report an identical input fingerprint across shards', () => { test('shard diagnostics report an identical input fingerprint across shards', () => {
seed(tmpDir, SHARD_NAMES); seed(tmpDir, SHARD_NAMES);
// The cross-runner divergence guard: every shard of one run computes the // The cross-runner divergence guard: every shard of one run computes the
@@ -1415,6 +1483,149 @@ describe('selectShard weight-aware partition (#2472)', () => {
}); });
}); });
// ─── #4070: reserved-weight shard partition ─────────────────────────────────
//
// `test.yml`'s `scope: full` lane tacks four unsharded aux suites
// (integration/security/install/slow) onto shard 1 only, outside this
// packer's model entirely — it balances the UNIT-TEST slice as if all three
// shards carried equal fixed cost, when shard 1 actually carries a fixed
// aux-suite overhead the other two do not. `initialWeights` lets a caller
// give one (or more) bins a virtual head start before LPT places any real
// file, so the algorithm converges on equalizing FINAL total cost (reserve +
// assigned files) instead of raw assigned-file weight alone — the same
// "greedy into the lightest bin" placement rule, just with non-zero starting
// points.
describe('selectShard reserved-weight partition (#4070)', () => {
const fc = require('fast-check');
const uniform = Array.from({ length: 30 }, (_, i) => `u${String(i).padStart(3, '0')}.test.cjs`);
const uniformWeight = () => 10;
const finalTotals = (files, total, weightOf, initialWeights) => {
const out = [];
for (let i = 1; i <= total; i++) {
const assigned = selectShard(files, { index: i, total }, weightOf, initialWeights)
.reduce((a, f) => a + weightOf(f), 0);
out.push(assigned + ((initialWeights && initialWeights[i - 1]) || 0));
}
return out;
};
// The regression: without reserve support, bin 0 gets an EQUAL share of
// files despite already carrying a head start, so its true final total
// (assigned + reserve) sits well above the other bins' — exactly the shard
// 1 overload this issue reports. With reserve support, LPT starts bin 0
// "already heavier" and hands it fewer files so all three converge.
test('REGRESSION: an initial reserve on one bin rebalances the rest (#4070)', () => {
const reserve = 80; // 8 average-cost files' worth, on a 30-file/300-weight suite
const totals = finalTotals(uniform, 3, uniformWeight, [reserve, 0, 0]);
const spread = Math.max(...totals) - Math.min(...totals);
assert.ok(
spread <= uniformWeight(),
`a reserved bin must converge toward the others' final totals (within one file's `
+ `weight), not just add the reserve on top of an equal share; got totals=${totals} `
+ `(spread=${spread})`,
);
// The reserved bin must have been handed FEWER files than an unreserved bin —
// otherwise "rebalancing" did nothing and the reserve is purely additive.
const reservedBinFiles = selectShard(uniform, { index: 1, total: 3 }, uniformWeight, [reserve, 0, 0]).length;
const unreservedBinFiles = selectShard(uniform, { index: 2, total: 3 }, uniformWeight, [reserve, 0, 0]).length;
assert.ok(
reservedBinFiles < unreservedBinFiles,
`the reserved bin (${reservedBinFiles} files) must receive fewer files than an `
+ `unreserved bin (${unreservedBinFiles}) — otherwise the reserve had no effect on `
+ 'placement',
);
});
test('back-compat: omitting initialWeights reproduces the unreserved partition exactly', () => {
for (let i = 1; i <= 3; i++) {
assert.deepStrictEqual(
selectShard(uniform, { index: i, total: 3 }, uniformWeight),
selectShard(uniform, { index: i, total: 3 }, uniformWeight, undefined),
);
}
});
test('a zero reserve is a no-op', () => {
for (let i = 1; i <= 3; i++) {
assert.deepStrictEqual(
selectShard(uniform, { index: i, total: 3 }, uniformWeight),
selectShard(uniform, { index: i, total: 3 }, uniformWeight, [0, 0, 0]),
);
}
});
test('a reserve applies to any bin index, not only the first', () => {
const reserve = 80;
const totals = finalTotals(uniform, 3, uniformWeight, [0, reserve, 0]);
const spread = Math.max(...totals) - Math.min(...totals);
assert.ok(spread <= uniformWeight(), `expected convergence around index 2; got ${totals}`);
const reservedBinFiles = selectShard(uniform, { index: 2, total: 3 }, uniformWeight, [0, reserve, 0]).length;
const otherBinFiles = selectShard(uniform, { index: 1, total: 3 }, uniformWeight, [0, reserve, 0]).length;
assert.ok(reservedBinFiles < otherBinFiles, `reserved bin 2 should get fewer files; got ${reservedBinFiles} vs ${otherBinFiles}`);
});
test('REGRESSION: a reserve larger than the whole suite still terminates and assigns every file', () => {
const reserve = 1e9;
const shards = [];
for (let i = 1; i <= 3; i++) shards.push(selectShard(uniform, { index: i, total: 3 }, uniformWeight, [reserve, 0, 0]));
const flat = shards.flat();
assert.deepStrictEqual([...flat].sort(), [...uniform].sort(), 'every file must still be placed exactly once');
// The massively-reserved bin should get the fewest (possibly zero) files.
assert.ok(shards[0].length <= shards[1].length && shards[0].length <= shards[2].length);
});
test('REGRESSION: a hostile reserve value clamps to zero instead of poisoning placement', () => {
for (const hostile of [NaN, -5, Infinity]) {
const shards = [];
for (let i = 1; i <= 3; i++) shards.push(selectShard(uniform, { index: i, total: 3 }, uniformWeight, [hostile, 0, 0]));
const sizes = shards.map((s) => s.length);
assert.ok(
Math.max(...sizes) - Math.min(...sizes) <= 1,
`a hostile reserve (${hostile}) must clamp to 0, not collapse/starve a bin; sizes=${sizes}`,
);
}
});
// Generalizes the existing "no shard exceeds average + heaviest file" bound
// (#2472) to include a single reserved bin — but the Graham-style proof
// (the max-load bin was the argmin, hence <= average, at the moment its
// LAST item was placed) only applies to a bin that actually received at
// least one item. A reserve large enough that its bin never receives any
// real item stays at EXACTLY its initial reserve forever — no amount of
// routing real items elsewhere can dilute a fixed head start below itself
// — so the true bound is the LARGER of the classic Graham term and the
// single biggest reserve. (Counterexample that falsified the original,
// reserve-blind-to-domination version of this bound: weights=[1,1,1],
// total=2, reserve=6 on bin 0 — bin 0 receives zero items and stays at 6,
// while (sum+reserve)/total+max = 4.5+1 = 5.5 < 6.)
test('property: no shard exceeds max(reserve, average(+reserve) + heaviest file)', () => {
fc.assert(
fc.property(
fc.array(fc.integer({ min: 1, max: 60000 }), { minLength: 3, maxLength: 60 }),
fc.integer({ min: 2, max: 6 }),
fc.integer({ min: 0, max: 200000 }),
fc.integer({ min: 0, max: 5 }),
(weights, total, reserve, reserveIdxRaw) => {
const files = weights.map((_, i) => `p${String(i).padStart(3, '0')}.test.cjs`);
const w = (f) => weights[Number(f.slice(1, 4))];
const reserveIdx = reserveIdxRaw % total;
const initialWeights = Array.from({ length: total }, (_, i) => (i === reserveIdx ? reserve : 0));
const sums = finalTotals(files, total, w, initialWeights);
const grahamBound = (weights.reduce((a, b) => a + b, 0) + reserve) / total + Math.max(...weights);
const bound = Math.max(reserve, grahamBound);
assert.ok(
Math.max(...sums) <= bound + 1e-9,
`bound violated: max=${Math.max(...sums)} bound=${bound} sums=${sums}`,
);
},
),
{ numRuns: 200, seed: 24724 },
);
});
});
describe('parseShardArg (#1212)', () => { describe('parseShardArg (#1212)', () => {
test('parses i/n into { index, total }', () => { test('parses i/n into { index, total }', () => {
assert.deepStrictEqual(parseShardArg('2/3'), { index: 2, total: 3 }); assert.deepStrictEqual(parseShardArg('2/3'), { index: 2, total: 3 });
@@ -1430,6 +1641,31 @@ describe('parseShardArg (#1212)', () => {
} }
}); });
// #4070: RUN_TESTS_SHARD_RESERVE env-var grammar — "<index>:<weight>", the
// operator knob test.yml uses to tell the full-scope lane's unit-test shard
// selection that shard 1 already carries a fixed aux-suite cost. Fail-open
// on anything malformed (mirrors positiveNumberEnv's precedent elsewhere in
// this file): a typo must degrade to "no reserve", never poison placement or
// throw and take the whole CI job down with it.
describe('parseShardReserve (#4070)', () => {
const { parseShardReserve } = require('../scripts/run-tests.cjs');
test('parses "<index>:<weight>" into { index, weight }', () => {
assert.deepEqual(parseShardReserve('1:77'), { index: 1, weight: 77 });
assert.deepEqual(parseShardReserve('2:0'), { index: 2, weight: 0 });
assert.deepEqual(parseShardReserve('3:12.5'), { index: 3, weight: 12.5 });
});
for (const v of [undefined, null, '', ' ', 'x', '1', '1:', ':77', '0:77', '-1:77', '1:-5', '1.5:77', '1:abc', 'a:b', '1:2:3']) {
test(`rejects malformed value ${JSON.stringify(v)}`, () => {
assert.equal(parseShardReserve(v), null, `expected null for ${JSON.stringify(v)}`);
});
}
test('whitespace around a valid value is tolerated', () => {
assert.deepEqual(parseShardReserve(' 1:77 '), { index: 1, weight: 77 });
});
});
// ──────────────────────────────────────────────────────────────────────── // ────────────────────────────────────────────────────────────────────────
// Folded from tests/bug-969-test-infra-flake-hardening.test.cjs — consolidation epic #1969 (B6 #1975) // Folded from tests/bug-969-test-infra-flake-hardening.test.cjs — consolidation epic #1969 (B6 #1975)