diff --git a/.changeset/fierce-geese-hop.md b/.changeset/fierce-geese-hop.md new file mode 100644 index 000000000..f5336a806 --- /dev/null +++ b/.changeset/fierce-geese-hop.md @@ -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) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ae89532f8..a669c12ac 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -134,26 +134,44 @@ jobs: runs-on: ${{ matrix.os }} # #2952: the `scope: full` lane is sharded three ways (see the matrix # 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 - # 15m20s against a 15-minute cap and was killed mid-run, which is the whole - # of #2952 — the number is unchanged, the work behind it is a third the size. + # #4070: the run 30677442953 figure this comment used to cite (shard 1/3 + # 7m12s) predated the aux-suite growth below and was stale. Shard 1 is the + # 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 - # (see #3057): on PR #3094 it reached 15m05s against this same cap and was - # CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped in - # the install-heavy suites and pushed the single Windows lane over the top. - # Per #869, a timeout bump only moves that cliff; sharding removes it. The - # cap stays at 15 unchanged: each shard now does roughly a third of the - # work, so headroom improves rather than needing a raise. + # (see #3057): on PR #3094 it reached 15m05s against a 15-minute cap and + # was CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped + # in the install-heavy suites and pushed the single Windows lane over the + # top. Per #869, a timeout bump only moves that cliff; sharding removes + # it. That lane does not run any aux suite on its own shard 1 (the + # 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 # headroom factor over its own measured cost. - timeout-minutes: 15 + timeout-minutes: 21 env: GSD_PLUGIN_ROOT: .ci-gsd-plugin-root-disabled # #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 # gave the `test-full` lane, which measures 0.0% spread across 3 bins on # 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 # same fix applied to the same cliff (#869's stated durable follow-up). @@ -320,6 +346,22 @@ jobs: if: matrix.scope == 'full' env: 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 }} # Raw V8 dumps, not rendered reports — coverage-gate merges these. They @@ -334,8 +376,11 @@ jobs: if-no-files-found: error retention-days: 1 - # The aux suites are small (~1m35s combined) and unsharded — running them - # on every shard would triple their cost for no signal. + # The aux suites cost ~216-224s combined (#4070 — see the "Run unit + # 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 if: matrix.scope == 'full' && matrix.shard == '1/3' run: npm run test:integration @@ -357,7 +402,7 @@ jobs: continue-on-error: true env: 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 test-inert: diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 4541f80e7..3a658c5f2 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -221,6 +221,29 @@ function parseShardArg(value) { return { index, total }; } +// Parse RUN_TESTS_SHARD_RESERVE (#4070) — ":", 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 // this is the original round-robin (#1212): // 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 // both layers now share one cost model. Omitting `weightOf` keeps the legacy // 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 (typeof weightOf !== 'function') { return sortedFiles.filter((_, k) => k % total === index - 1); @@ -255,7 +291,15 @@ function selectShard(sortedFiles, { index, total }, weightOf) { const w = weightOf(file); 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 // the caller's sort position, and the lightest-bin scan takes the FIRST // minimum, so the partition is byte-identical across Windows/macOS/Linux — @@ -878,7 +922,32 @@ function main() { if (usingShard) { emptyBeforeShard = selectedNames.length === 0; 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 ":" 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)); @@ -1379,6 +1448,7 @@ module.exports = { ensureBuiltArtifacts, ensureBuiltHooks, parseShardArg, + parseShardReserve, selectShard, positiveNumberEnv, loadTestTimings, diff --git a/tests/ci-full-lane-sharding.test.cjs b/tests/ci-full-lane-sharding.test.cjs index 7b3a425ce..1c8acccb6 100644 --- a/tests/ci-full-lane-sharding.test.cjs +++ b/tests/ci-full-lane-sharding.test.cjs @@ -39,6 +39,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const yaml = require('js-yaml'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); 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) => { const workflow = loadWorkflow('test.yml'); diff --git a/tests/ci-test-job-timeout-budget.test.cjs b/tests/ci-test-job-timeout-budget.test.cjs index e5180c701..bb7692b6d 100644 --- a/tests/ci-test-job-timeout-budget.test.cjs +++ b/tests/ci-test-job-timeout-budget.test.cjs @@ -55,11 +55,25 @@ const HEADROOM_FACTOR = 1.5; const LANE_COSTS = [ { 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 - // whole unit suite. Run 30677442953: shard 1/3 7m12s, 2/3 4m32s, 3/3 3m59s. - // Shard 1 is the long pole because the unsharded aux suites ride on it. - // Before sharding the same lane cost 15m20s and blew a 15-minute cap. + // whole unit suite. Shard 1 is the long pole because the unsharded aux + // suites (integration/security/install/slow) ride on it — #4070 fixed the + // 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 // 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 // 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 - // third of that (~5m), which is already comfortably under the 8m/12m this - // entry requires — so no separate LANE_COSTS entry is added on a number - // that has not actually been measured. Replace this estimate with a real - // measured shard cost once one exists, the same discipline every other - // entry here follows. - evidence: 'run 30677442953 — 7m12s slowest shard', + // third of that (~5m), comfortably under what this entry requires — so no + // separate LANE_COSTS entry is added on a number that has not actually + // been measured. Replace this estimate with a real measured shard cost + // once one exists, the same discipline every other entry here follows. + evidence: 'run 33278340189 — 13m48s completed; run 33285384930 — CANCELLED at ~14m51s (#4070)', }, { 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) => { 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: 'install-smoke.yml', jobKey: 'smoke', envLiteral: '12' }, ]; diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index aeadca169..14a9173ac 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -55,6 +55,24 @@ function runHarness(testDir, args = [], extraEnv = {}) { // doesn't refuse to run with "recursive run() skipping running files". const env = { ...process.env, GSD_TEST_DIR: testDir, ...extraEnv }; 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], { cwd: path.join(__dirname, '..'), 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'); }); + // #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', () => { seed(tmpDir, SHARD_NAMES); // 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)', () => { test('parses i/n into { index, total }', () => { assert.deepStrictEqual(parseShardArg('2/3'), { index: 2, total: 3 }); @@ -1430,6 +1641,31 @@ describe('parseShardArg (#1212)', () => { } }); +// #4070: RUN_TESTS_SHARD_RESERVE env-var grammar — ":", 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 ":" 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)