diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6afe50ac8..4837f56f2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -162,6 +162,13 @@ jobs: if: github.event_name == 'pull_request' env: GITHUB_TOKEN: ${{ github.token }} + # Pin every job of this run to ONE base commit (#2472). Each job runs + # this step independently, minutes apart across a 12-job matrix, so + # merging the moving branch ref lets jobs see different trees when the + # base advances mid-run. The sharded lane needs all jobs to agree on a + # partition, and disagreement there drops a test file silently while + # CI stays green. base.sha is fixed for the life of the run. + CI_REBASE_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: node scripts/ci-rebase-check.cjs - name: Set up Node.js ${{ matrix.node-version }} @@ -262,6 +269,13 @@ jobs: if: github.event_name == 'pull_request' env: GITHUB_TOKEN: ${{ github.token }} + # Pin every job of this run to ONE base commit (#2472). Each job runs + # this step independently, minutes apart across a 12-job matrix, so + # merging the moving branch ref lets jobs see different trees when the + # base advances mid-run. The sharded lane needs all jobs to agree on a + # partition, and disagreement there drops a test file silently while + # CI stays green. base.sha is fixed for the life of the run. + CI_REBASE_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: node scripts/ci-rebase-check.cjs - name: Set up Node.js 22 uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 @@ -292,8 +306,9 @@ jobs: run: shell: ${{ matrix.shell }} # The unit suite is sharded across 3 parallel runners per OS/node leg - # (#1212). Each shard runs a deterministic round-robin third of the sorted - # unit-file list via `run-tests.cjs --suite unit --shard i/3`, so per-job + # (#1212, cost-weighted in #2472). Each shard runs a deterministic + # cost-balanced third of the sorted unit-file list via + # `run-tests.cjs --suite unit --shard i/3`, so per-job # wall-clock scales as O(total/3) and stays well under the cap as the suite # grows — replacing the #869 timeout bump (15→20m) which only deferred the # cliff. The cap stays at 20m as a generous backstop; a healthy shard now @@ -372,6 +387,13 @@ jobs: if: github.event_name == 'pull_request' env: GITHUB_TOKEN: ${{ github.token }} + # Pin every job of this run to ONE base commit (#2472). Each job runs + # this step independently, minutes apart across a 12-job matrix, so + # merging the moving branch ref lets jobs see different trees when the + # base advances mid-run. The sharded lane needs all jobs to agree on a + # partition, and disagreement there drops a test file silently while + # CI stays green. base.sha is fixed for the life of the run. + CI_REBASE_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: node scripts/ci-rebase-check.cjs - name: Set up Node.js ${{ matrix.node-version }} @@ -390,7 +412,7 @@ jobs: run: node scripts/check-npm-integrity.cjs # The heavy unit suite is split across the 3 shards — each runs a - # deterministic round-robin third of the sorted unit-file list. The + # deterministic cost-balanced third of the sorted unit-file list (#2472). The # union of shards 1/3 + 2/3 + 3/3 is the full unit suite, so coverage is # unchanged; only wall-clock per job drops to ~total/3. - name: Run unit tests (shard ${{ matrix.shard }}/3) diff --git a/scripts/ci-rebase-check.cjs b/scripts/ci-rebase-check.cjs index d7a83f344..33690af74 100644 --- a/scripts/ci-rebase-check.cjs +++ b/scripts/ci-rebase-check.cjs @@ -8,6 +8,22 @@ // GITHUB_BASE_REF — PR base branch name (set by GitHub Actions on pull_request events) // GITHUB_REPOSITORY — owner/repo (set by GitHub Actions) // +// Optional: +// CI_REBASE_BASE_SHA — pin the merge to one exact base commit (#2472). +// +// Why the pin matters. Every job of a run executes this step independently, at +// whatever wall-clock moment it gets there — and Windows/macOS installs skew +// that by minutes across a 12-job matrix. Merging the moving `origin/` +// ref means that if the base advances mid-run, different jobs merge different +// trees. That was survivable when jobs only had to agree on pass/fail, but the +// sharded lane makes them agree on a PARTITION: each shard job computes the +// whole split and keeps its own slice, so jobs working from different trees can +// place a file in two shards or in none. Each job still looks internally +// consistent, so nothing errors — a test silently never runs and CI stays +// green. Pinning every job to `github.event.pull_request.base.sha`, which is +// fixed for the life of the run, removes the divergence at its source rather +// than detecting it after the fact. +// // Exit 0 = merged cleanly (or merge was a no-op). // Exit 1 = merge conflict or fetch failure. @@ -35,6 +51,28 @@ function runOrThrow(cmd, args, label) { const token = process.env.GITHUB_TOKEN || ''; const baseBranch = process.env.GITHUB_BASE_REF || 'main'; const repo = process.env.GITHUB_REPOSITORY || ''; +// Resolve what to fetch and what to merge, pinned together so they can never +// disagree. Pure and exported so the pin contract is testable without spawning +// git: env in, refs out. +// +// Only a full 40-hex sha is accepted. Anything else — empty on push/dispatch +// events, or a malformed/injected value — falls back to the branch ref, +// preserving the pre-#2472 behavior rather than handing an arbitrary string to +// `git fetch` as a refspec. +function resolveBaseRefs(env = process.env, fallbackBranch = 'main') { + const branch = env.GITHUB_BASE_REF || fallbackBranch; + const raw = env.CI_REBASE_BASE_SHA || ''; + const sha = /^[0-9a-f]{40}$/.test(raw) ? raw : null; + return { + branch, + sha, + pinned: sha !== null, + fetchRef: sha || branch, + mergeRef: sha || `origin/${branch}`, + }; +} + +const { fetchRef, mergeRef } = resolveBaseRefs(process.env, 'main'); function main() { // Configure git identity (needed for merge commit). @@ -52,12 +90,12 @@ function main() { // Fetch base branch with retry. for (let attempt = 1; attempt <= 3; attempt++) { - const result = run('git', ['fetch', 'origin', baseBranch]); + const result = run('git', ['fetch', 'origin', fetchRef]); if (result) { break; } if (attempt === 3) { - throw new ExitError(1, `::error::git fetch origin ${baseBranch} failed after 3 attempts.`); + throw new ExitError(1, `::error::git fetch origin ${fetchRef} failed after 3 attempts.`); } // Wait before retry: attempt * 4 seconds. const waitMs = attempt * 4000; @@ -67,7 +105,7 @@ function main() { // Attempt merge. try { - execFileSync('git', ['merge', '--no-edit', '--no-ff', `origin/${baseBranch}`], { stdio: 'inherit' }); + execFileSync('git', ['merge', '--no-edit', '--no-ff', mergeRef], { stdio: 'inherit' }); } catch (e) { process.stderr.write( `::error::This PR cannot cleanly merge origin/${baseBranch}. Rebase your branch onto current ${baseBranch} and push again.\n` @@ -83,4 +121,10 @@ function main() { } } -runMain(main); +// Only run when invoked as the CI step. Guarded so a test can require this +// module for resolveBaseRefs without firing git fetch/merge as a side effect. +if (require.main === module) { + runMain(main); +} + +module.exports = { resolveBaseRefs }; diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 1337d706c..36379cf15 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -15,9 +15,14 @@ // node scripts/run-tests.cjs --files-from /tmp/selected-tests.txt // node scripts/run-tests.cjs --suite unit --shard 1/3 # shard 1 of 3 (#1212) // -// Sharding (issue #1212): --shard / runs a deterministic, balanced -// round-robin slice of the SORTED selected file list (file index k → shard -// k % n). i is 1-based (1..n); n >= 1; n=1 is a pure no-op (all files). The +// Sharding (issue #1212, reweighted #2472): --shard / runs a +// deterministic, COST-balanced slice of the SORTED selected file list. Files +// are partitioned by measured duration (tests/test-timings.json) using LPT — +// the same packing the chunker uses one level down — because equal file COUNTS +// are not equal file COST: the index-based split this replaced ran 12.4m / +// 19.2m / 15.2m against a 20-minute job cap. With no timing data every file +// weighs the same and the partition degenerates to the original k % n +// round-robin. i is 1-based (1..n); n >= 1; n=1 is a pure no-op (all files). The // CI windows full-test lane shards across N parallel runners so per-job // wall-clock scales as O(total/N) and stops hitting the job time cap. Sharding // composes with --suite (it slices the post-filter selection) and preserves @@ -206,7 +211,8 @@ function parseShardArg(value) { return { index, total }; } -// Deterministic, balanced round-robin partition of an ALREADY-SORTED file list. +// 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 // list satisfies k % total === index - 1. Round-robin (not contiguous blocks) // spreads duration variance across shards and guarantees shard sizes differ by @@ -215,9 +221,63 @@ function parseShardArg(value) { // sorts the list with the same (locale-independent) comparator. `total=1` // returns the input unchanged (pure no-op). A shard with no files (total > // file count) returns [] and is a legitimate result, not an error. -function selectShard(sortedFiles, { index, total }) { +// `weightOf` (optional, #2472) switches the partition from equal COUNTS to +// equal COST. Equal counts were only ever a proxy for equal duration, and on a +// right-skewed suite the proxy fails: the real unit suite partitioned 12.4m / +// 19.2m / 15.2m by index against a 20-minute job cap, and because assignment +// keyed off array POSITION, inserting one test file re-indexed every file after +// it and could tip the heaviest shard over. Weighting by measured cost fixes +// both: LPT bounds the heaviest shard at 4/3 of optimal, and placement follows +// a file's cost rather than its neighbours' names. +// +// 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) { if (total === 1) return sortedFiles; - return sortedFiles.filter((_, k) => k % total === index - 1); + if (typeof weightOf !== 'function') { + return sortedFiles.filter((_, k) => k % total === index - 1); + } + // A non-finite or negative weight must not poison bin arithmetic — one NaN + // would make every subsequent comparison false and pile the rest of the suite + // into bin 0. Mirrors packChunks' safeWeight for the same reason. + const safeWeight = (file) => { + const w = weightOf(file); + return Number.isFinite(w) && w >= 0 ? w : 0; + }; + const bins = Array.from({ length: total }, () => ({ weight: 0, 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 — + // the same determinism guarantee the round-robin path carries. + const order = sortedFiles + .map((file, k) => ({ k, weight: safeWeight(file) })) + .sort((a, b) => b.weight - a.weight || a.k - b.k); + for (const entry of order) { + let lightest = 0; + for (let i = 1; i < total; i += 1) { + const bin = bins[i]; + const best = bins[lightest]; + // Weight first, then FILE COUNT. The count tiebreak is load-bearing, not + // cosmetic: adding a zero-weight file leaves its bin's weight unchanged, + // so on weight alone bin 0 stays tied-minimum forever and every + // zero-weight file lands on it — all-zero weights put the whole suite on + // shard 1 and leave the other runners idle. Zero weights are reachable + // via safeWeight's clamp (a NaN/negative/Infinity entry in a hand-edited + // or corrupted timings table) and via any genuinely 0ms measurement, so + // the clamp above would otherwise reproduce the exact pile-onto-bin-0 + // failure it exists to prevent. Counting picks makes ties rotate. + if (bin.weight < best.weight + || (bin.weight === best.weight && bin.picks.length < best.picks.length)) { + lightest = i; + } + } + bins[lightest].weight += entry.weight; + bins[lightest].picks.push(entry.k); + } + // Restore the caller's order within the shard: downstream chunking and argv + // batching assume the list arrives sorted as the caller sorted it. + return bins[index - 1].picks.sort((a, b) => a - b).map((k) => sortedFiles[k]); } // Read an operator-supplied numeric env knob, falling back to the default for @@ -275,7 +335,13 @@ function loadTestTimings(timingsPath) { return null; } const timings = parsed.timings; - if (!timings || typeof timings !== 'object') return null; + // Array.isArray guard: `typeof [] === 'object'`, so a hand-edit that turned + // the map into a list would pass a bare typeof check and be accepted as a + // valid table. It degrades harmlessly (no basename ever matches an array + // index, so every file takes medianWeight), but silently accepting a + // malformed table is worse than rejecting it — reject, and fall back to + // uniform weight the same way a missing file does. + if (!timings || typeof timings !== 'object' || Array.isArray(timings)) return null; const values = Object.values(timings).filter( (v) => typeof v === 'number' && Number.isFinite(v) && v >= 0, ); @@ -648,7 +714,7 @@ function main() { } // Shard partitioning (#1212): when --shard i/n is given, keep only this - // shard's deterministic round-robin slice of the selected list. Applied + // shard's deterministic cost-balanced slice of the selected list. Applied // AFTER suite/explicit selection so it composes with --suite (each shard // runs i/n of the post-filter selection). // @@ -663,11 +729,35 @@ function main() { // from a non-empty list" (total > file count — a valid no-op) from "the // selection was already empty before sharding" (a genuinely empty suite, // which must still hit the discovery hard-error below — Codex #1212 review). + // Loaded before sharding because BOTH layers weigh by it now (#2472): the + // shard partition below and the chunk packer further down share this one cost + // model. Advisory in both places — a missing table yields uniform weight 1, + // which makes the shard partition degenerate to the legacy equal-count split. + // Lazily memoized: BOTH layers weigh by it now (#2472) — the shard partition + // just below and the chunk packer further down share this one cost model — + // but neither should charge a readFileSync + JSON.parse to an invocation that + // exits before it needs one (an empty selection, or `--files` with nothing + // matched). Memoized so the two consumers still read the table at most once. + // Advisory in both places: a missing table yields uniform weight 1, under + // which the shard partition degenerates to the legacy equal-count split. + let weigherMemo = null; + const fileWeightOf = () => { + if (weigherMemo === null) { + const timingsPath = process.env.RUN_TESTS_TIMINGS_FILE || DEFAULT_TIMINGS_PATH; + weigherMemo = makeFileWeigher(loadTestTimings(timingsPath)); + } + return weigherMemo; + }; + const usingShard = parsed.shard !== null; let emptyBeforeShard = false; + // The full pre-partition input, kept for the cross-job fingerprint below. + // It must be the list every shard job sees, not this job's slice. + let shardInput = null; if (usingShard) { emptyBeforeShard = selectedNames.length === 0; - selectedNames = selectShard([...selectedNames].sort(), parsed.shard); + shardInput = [...selectedNames].sort(); + selectedNames = selectShard(shardInput, parsed.shard, fileWeightOf()); } const selected = selectedNames.map(f => join(testDir, f)); @@ -736,6 +826,51 @@ function main() { .join(' ')}`, ); + // Shard diagnostics (#2472). File COUNT stopped being a balance signal the + // moment the partition started weighing by cost — two shards can now hold + // very different counts by design — so the count line above can no longer be + // eyeballed to spot a bad split. Worse, each shard job computes its partition + // independently on its own runner: if the inputs differ between jobs (the + // file list, or this table), two jobs can place the same file in different + // shards, or in none, and every job still looks internally consistent. That + // failure is silent — a test simply never runs and CI stays green. + // + // `sig` is the defense: a cheap fingerprint of the exact inputs the partition + // consumed. Every shard job of a given run must print the SAME sig; a + // mismatch across jobs is proof the runners disagreed about the input and + // therefore about the partition. `weighed` reports how many of this shard's + // files matched a real measurement — a table that silently failed to parse + // shows weighed=0 instead of being indistinguishable from a healthy load. + if (usingShard) { + const weigher = fileWeightOf(); + const table = loadTestTimings(process.env.RUN_TESTS_TIMINGS_FILE || DEFAULT_TIMINGS_PATH); + const mine = selectedNames.map(f => f.split(/[\\/]/).pop()); + const weighed = table + ? mine.filter(n => Object.hasOwn(table.timings, n)).length + : 0; + const myWeight = mine.reduce((sum, n) => sum + weigher(n), 0); + // Fingerprint the FULL pre-partition input — the file list and the weight + // each file was assigned — NOT this shard's slice. Every shard job of one + // run must print an identical sig; a mismatch is proof the runners + // disagreed about the input, which is the only way the union of shards can + // silently drop or duplicate a file. Order-independent sum of per-file + // (name, weight) hashes: stable across platforms, cheap for ~600 files. + let sig = 0; + for (const n of shardInput.map(f => f.split(/[\\/]/).pop())) { + let h = 2166136261; + for (let i = 0; i < n.length; i += 1) { + h = Math.imul(h ^ n.charCodeAt(i), 16777619); + } + sig = (sig + (h >>> 0) + Math.round(weigher(n) * 1000)) % 0xffffffff; + } + console.error( + `run-tests: shard=${parsed.shard.index}/${parsed.shard.total} ` + + `files=${mine.length}/${shardInput.length} weighed=${weighed} ` + + `weight=${myWeight.toFixed(2)} table=${table ? 'loaded' : 'absent'} ` + + `sig=${sig.toString(16)}`, + ); + } + // Default concurrency: 4 on Linux/macOS, 2 on Windows. // // Windows has significantly higher per-subprocess overhead than Linux/macOS: @@ -798,8 +933,8 @@ function main() { // falls back to the table's median weight and a missing table falls back to // uniform weight 1, so staleness degrades chunk BALANCE gracefully instead of // failing CI. Regenerate via `node scripts/gen-test-timings.cjs `. - const timingsPath = process.env.RUN_TESTS_TIMINGS_FILE || DEFAULT_TIMINGS_PATH; - const fileWeight = makeFileWeigher(loadTestTimings(timingsPath)); + // The cost table is loaded lazily above and memoized; both the shard + // partition and this packer consume the same weigher (#2472). // node:test does not exit until the event loop drains. A unit test that leaks // an open handle (un-terminated Worker, un-killed child_process, ref'd timer) @@ -816,7 +951,7 @@ function main() { const FIXED_OVERHEAD = process.execPath.length + '--test'.length + concurrency.length + (forceExit ? '--test-force-exit'.length + 1 : 0) + 8; const chunks = packChunks(selected, { - weightOf: fileWeight, + weightOf: fileWeightOf(), maxWeight: MAX_FILES_PER_CHUNK, maxChars: MAX_CMDLINE_CHARS, fixedOverhead: FIXED_OVERHEAD, diff --git a/tests/ci-test-scope.test.cjs b/tests/ci-test-scope.test.cjs index 7dea2684f..86cd08344 100644 --- a/tests/ci-test-scope.test.cjs +++ b/tests/ci-test-scope.test.cjs @@ -492,7 +492,7 @@ describe('test-full shard matrix parity (#1212)', () => { const n = distinctShards.length; // Distinct shard values must be exactly 1..n (1-based, contiguous) so the - // runner's round-robin selection covers every file with no gaps/overlaps. + // runner's cost-balanced shard selection covers every file with no gaps/overlaps. assert.deepStrictEqual( distinctShards, Array.from({ length: n }, (_, i) => i + 1), @@ -537,6 +537,62 @@ describe('test-full shard matrix parity (#1212)', () => { ); }); + // #2472: every job of a run must merge ONE base commit. Each job runs the + // rebase-check step independently, minutes apart across the matrix, so + // merging the moving branch ref lets jobs see different trees when the base + // advances mid-run. The sharded lane makes jobs agree on a PARTITION, and + // disagreement there places a file in two shards or none — silently, because + // each job stays internally consistent and CI stays green. + describe('#2472 base-commit pin', () => { + const { resolveBaseRefs } = require('../scripts/ci-rebase-check.cjs'); + const SHA = 'a1b2c3d4e5f60718293a4b5c6d7e8f9012345678'; + + test('every rebase-check step pins CI_REBASE_BASE_SHA', () => { + const doc = yaml.load(fs.readFileSync(path.join(WORKFLOWS_DIR, 'test.yml'), 'utf8')); + const steps = Object.values(doc.jobs) + .flatMap(j => j.steps || []) + .filter(s => typeof s.run === 'string' && s.run.includes('ci-rebase-check.cjs')); + assert.ok(steps.length > 0, 'expected at least one rebase-check step'); + for (const s of steps) { + assert.ok( + s.env && typeof s.env.CI_REBASE_BASE_SHA === 'string' && s.env.CI_REBASE_BASE_SHA.includes('base.sha'), + 'each rebase-check step must pin CI_REBASE_BASE_SHA to the PR base sha; ' + + 'an unpinned job can merge a different tree than its siblings', + ); + } + }); + + test('a full 40-hex sha pins both fetch and merge to that commit', () => { + const r = resolveBaseRefs({ GITHUB_BASE_REF: 'next', CI_REBASE_BASE_SHA: SHA }, 'main'); + assert.strictEqual(r.fetchRef, SHA); + assert.strictEqual(r.mergeRef, SHA, 'fetch and merge must target the same pinned commit'); + assert.strictEqual(r.pinned, true); + }); + + test('no pin falls back to the branch ref (push / workflow_dispatch)', () => { + const r = resolveBaseRefs({ GITHUB_BASE_REF: 'next' }, 'main'); + assert.strictEqual(r.fetchRef, 'next'); + assert.strictEqual(r.mergeRef, 'origin/next'); + assert.strictEqual(r.pinned, false); + }); + + // A non-sha value must never reach `git fetch` as a refspec. + for (const [label, value] of [ + ['short sha', 'abc123'], + ['uppercase sha', 'A'.repeat(40)], + ['argument injection', 'next --upload-pack=evil'], + ['ref expression', 'next^{commit}'], + ['empty', ''], + ]) { + test(`rejects ${label} and falls back to the branch ref`, () => { + const r = resolveBaseRefs({ GITHUB_BASE_REF: 'next', CI_REBASE_BASE_SHA: value }, 'main'); + assert.strictEqual(r.pinned, false, `"${value}" must not be accepted as a pin`); + assert.strictEqual(r.fetchRef, 'next'); + assert.strictEqual(r.mergeRef, 'origin/next'); + }); + } + }); + test('required-tests fan-in still needs test-full and keeps the protected name', () => { // Hyrum's Law: branch protection requires a status check literally named // "Required tests". Renaming it (or dropping test-full from its needs) diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index 1efefbfbd..adaec76c7 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -488,6 +488,54 @@ test('ambient GSD workstream vars are stripped by the runner', () => { assert.doesNotMatch(r.stderr, /shard-02\.test\.cjs/); }); + // #2472 E2E: every other --shard test here uses synthetic filenames that + // are absent from the real tests/test-timings.json, so they all collapse to + // one uniform median weight — under which LPT is mathematically identical + // to k % n. That means none of them can tell whether main() actually + // threads fileWeightOf() into selectShard: a typo on that single line would + // pass the entire existing suite. This test injects a table via + // RUN_TESTS_TIMINGS_FILE with DIFFERING costs, so the weighted partition is + // provably distinguishable from round-robin. + test('--shard routes by measured cost end-to-end, not by index', (t) => { + seed(tmpDir, SHARD_NAMES); + // Heavy files sit at indices 0/3/6 — exactly the slice round-robin hands + // to shard 1. Cost-weighting must NOT put all three on one shard. + 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.json'); + fs.writeFileSync(tablePath, JSON.stringify(timings)); + t.after(() => { try { fs.unlinkSync(tablePath); } catch { /* best effort */ } }); + + const r = runHarness(tmpDir, ['--shard', '1/3'], { RUN_TESTS_TIMINGS_FILE: tablePath }); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + // Round-robin would give shard 1 exactly {00,03,06} — all three heavies. + const heavies = ['shard-00', 'shard-03', 'shard-06'] + .filter(n => new RegExp(`${n}\\.test\\.cjs`).test(r.stderr)).length; + assert.ok( + heavies < 3, + `cost-weighted shard 1 must not receive all three heavy files (that is the ` + + `round-robin split, proving the weigher was not threaded); stderr: ${r.stderr}`, + ); + // And the diagnostic must show the table was actually consumed. + assert.match(r.stderr, /table=loaded/, 'shard diagnostics must report the injected table as loaded'); + assert.match(r.stderr, /sig=[0-9a-f]+/, 'shard diagnostics must emit an input fingerprint'); + }); + + 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 + // partition independently, so all of them must agree on the INPUT. A + // differing sig between shard jobs is the only observable signal that + // they disagreed — which is how the union could silently drop a file. + const sigOf = (out) => (out.match(/sig=([0-9a-f]+)/) || [])[1]; + const sigs = [1, 2, 3].map((i) => sigOf(runHarness(tmpDir, ['--shard', `${i}/3`]).stderr)); + assert.ok(sigs.every(Boolean), `every shard must emit a sig; got ${JSON.stringify(sigs)}`); + assert.strictEqual(new Set(sigs).size, 1, `all shards must fingerprint the same input; got ${sigs}`); + }); + test('--shard 2/3 selects the second round-robin slice', () => { seed(tmpDir, SHARD_NAMES); const r = runHarness(tmpDir, ['--shard', '2/3']); @@ -871,7 +919,212 @@ describe('selectShard round-robin partition (#1212)', () => { assert.ok(Math.max(...sizes) - Math.min(...sizes) <= 1, `sizes=${sizes}`); }, ), - { numRuns: 200 }, + // Seed pinned so a failure is reproducible (repo convention for property + // tests); previously unseeded, flagged by the #2472 isolated review. + { numRuns: 200, seed: 12120 }, + ); + }); +}); + +// ─── #2472: weight-aware shard partition ──────────────────────────────────── +// +// Equal file COUNTS is not equal file COST. Round-robin by array index balances +// the former and ignores the latter, which on the real suite produced Windows +// shard weights of 12.4m / 19.2m / 15.2m against a 20-minute job cap — a 1.23x +// max/ideal ratio and a 5%-of-cap margin on the heaviest shard. Adding any test +// file re-indexed the partition and tipped it over (#2472). +// +// Passing a weigher switches the partition to LPT (longest-processing-time +// first), the same algorithm packChunks already uses one level down (#2456 / +// #2463), so both layers share one cost model. Omitting the weigher keeps the +// legacy round-robin exactly — every test above still exercises that path. + +describe('selectShard weight-aware partition (#2472)', () => { + const fc = require('fast-check'); + + const shardWeights = (files, total, weightOf) => { + const out = []; + for (let i = 1; i <= total; i++) { + out.push(selectShard(files, { index: i, total }, weightOf).reduce((a, f) => a + weightOf(f), 0)); + } + return out; + }; + + // The regression: a right-skewed cost distribution (the real suite's shape — + // a few dominant files among many cheap ones) laid out so that filename order + // clusters the heavy files onto one shard. + const skewed = { + 'a01.test.cjs': 30000, 'a02.test.cjs': 100, 'a03.test.cjs': 100, + 'a04.test.cjs': 28000, 'a05.test.cjs': 100, 'a06.test.cjs': 100, + 'a07.test.cjs': 26000, 'a08.test.cjs': 100, 'a09.test.cjs': 100, + 'a10.test.cjs': 24000, 'a11.test.cjs': 100, 'a12.test.cjs': 100, + }; + const skewedFiles = Object.keys(skewed).sort(); + const skewedWeight = (f) => skewed[f]; + + test('REGRESSION: round-robin clusters heavy files; LPT does not', () => { + const ideal = Object.values(skewed).reduce((a, b) => a + b, 0) / 3; + + // Legacy partition (no weigher) scored with the same cost function, so the + // two strategies are compared on identical inputs. + const rr = [1, 2, 3].map((i) => + selectShard(skewedFiles, { index: i, total: 3 }).reduce((a, f) => a + skewedWeight(f), 0)); + const lpt = shardWeights(skewedFiles, 3, skewedWeight); + + // Every heavy file sits at an index ≡ 0 (mod 3), so round-robin hands them + // all to shard 1 — the exact clustering that produced the 19-minute shard. + assert.ok( + Math.max(...rr) / ideal > 1.5, + `round-robin should be badly imbalanced on skewed costs; got ${rr} (ideal ${ideal})`, + ); + // Graham's list-scheduling bound: makespan <= average + heaviest item. This + // is the bound that is actually provable. The 4/3 figure quoted for LPT is + // relative to the OPTIMAL makespan, not to the average — and the two differ + // whenever item sizes force a pairing, as they do here: four items above + // 24000 into three bins means some bin holds two of them, so 50000 is + // optimal even though the average is 36267. + const heaviest = Math.max(...Object.values(skewed)); + assert.ok( + Math.max(...lpt) <= ideal + heaviest, + `LPT must hold the average+max bound; got ${lpt} (bound ${ideal + heaviest})`, + ); + assert.ok( + Math.max(...lpt) < Math.max(...rr), + `LPT must beat round-robin on skewed costs; lpt=${lpt} rr=${rr}`, + ); + }); + + test('back-compat: omitting the weigher reproduces round-robin exactly', () => { + for (let i = 1; i <= 3; i++) { + assert.deepStrictEqual( + selectShard(skewedFiles, { index: i, total: 3 }), + skewedFiles.filter((_, k) => k % 3 === i - 1), + 'the unweighted path must stay byte-identical to the legacy partition', + ); + } + }); + + test('a uniform weigher degenerates to equal counts', () => { + const sizes = [1, 2, 3].map( + (i) => selectShard(skewedFiles, { index: i, total: 3 }, () => 1).length, + ); + assert.ok( + Math.max(...sizes) - Math.min(...sizes) <= 1, + `equal weights must give equal counts; got ${sizes}`, + ); + }); + + test('n=1 is still a pure no-op with a weigher', () => { + assert.deepStrictEqual(selectShard(skewedFiles, { index: 1, total: 1 }, skewedWeight), skewedFiles); + }); + + test('preserves the caller sort order within a weighted shard', () => { + const slice = selectShard(skewedFiles, { index: 1, total: 3 }, skewedWeight); + assert.deepStrictEqual(slice, [...slice].sort(), 'downstream chunking assumes caller order'); + }); + + test('determinism: the weighted partition is stable across calls', () => { + const a = selectShard(skewedFiles, { index: 2, total: 3 }, skewedWeight); + const b = selectShard(skewedFiles, { index: 2, total: 3 }, skewedWeight); + assert.deepStrictEqual(a, b); + }); + + test('ties break deterministically, not by object iteration order', () => { + const flat = () => 5; + const a = selectShard(skewedFiles, { index: 1, total: 3 }, flat); + const b = selectShard([...skewedFiles], { index: 1, total: 3 }, flat); + assert.deepStrictEqual(a, b, 'equal weights must still partition identically'); + }); + + // A partition is a covering contract: every file lands in exactly one shard. + // Getting this wrong silently DROPS tests from CI — the worst possible failure + // mode for a test harness — so it is property-tested rather than sampled. + test('property: the weighted partition is exhaustive and disjoint', () => { + fc.assert( + fc.property( + fc.array(fc.integer({ min: 0, max: 60000 }), { minLength: 1, maxLength: 60 }), + fc.integer({ min: 1, max: 8 }), + (weights, total) => { + const files = weights.map((_, i) => `p${String(i).padStart(3, '0')}.test.cjs`); + const w = (f) => weights[Number(f.slice(1, 4))]; + const seen = []; + for (let i = 1; i <= total; i++) seen.push(...selectShard(files, { index: i, total }, w)); + assert.strictEqual(new Set(seen).size, seen.length, 'a file appeared in two shards'); + assert.deepStrictEqual([...seen].sort(), [...files].sort(), 'a file was dropped or invented'); + }, + ), + { numRuns: 200, seed: 24720 }, + ); + }); + + // Graham's bound for any greedy-into-lightest schedule, and the reason the + // shard cap stops being reachable: the heaviest shard cannot exceed the + // average by more than one file's cost, however the names happen to sort. + test('property: no shard exceeds average + heaviest file', () => { + fc.assert( + fc.property( + fc.array(fc.integer({ min: 1, max: 60000 }), { minLength: 3, maxLength: 60 }), + fc.integer({ min: 2, max: 6 }), + (weights, total) => { + const files = weights.map((_, i) => `p${String(i).padStart(3, '0')}.test.cjs`); + const w = (f) => weights[Number(f.slice(1, 4))]; + const sums = shardWeights(files, total, w); + const bound = weights.reduce((a, b) => a + b, 0) / total + Math.max(...weights); + assert.ok( + Math.max(...sums) <= bound + 1e-9, + `bound violated: max=${Math.max(...sums)} bound=${bound}`, + ); + }, + ), + { numRuns: 200, seed: 24721 }, + ); + }); + + // Deliberately NOT asserted: "weighted is never worse than round-robin". + // fast-check falsifies it — weights [19316,10190,1,9128,29353,20227] over 2 + // shards give round-robin 48670 and LPT 48671. Round-robin can win by luck on + // a specific input; LPT's guarantee is the worst-case bound above, not + // universal dominance. The value for #2472 is that the bound holds for EVERY + // distribution, so no arrangement of filenames can produce the 1.23x cluster + // that round-robin allowed — which the skewed regression above pins directly. + + // Zero-weight regression (isolated review, HIGH). Adding a zero-weight file + // leaves its bin's weight unchanged, so a weight-only tiebreak kept bin 0 + // tied-minimum forever and every such file landed there: all-zero weights put + // the entire suite on shard 1 and left the other runners idle. Reachable via + // safeWeight's clamp of a NaN/negative/Infinity entry, or any genuine 0ms + // measurement. The file-count tiebreak is what makes ties rotate. + test('REGRESSION: zero weights still split evenly across shards', () => { + const files = Array.from({ length: 9 }, (_, i) => `z${i}.test.cjs`); + const sizes = [1, 2, 3].map((i) => selectShard(files, { index: i, total: 3 }, () => 0).length); + assert.deepStrictEqual(sizes, [3, 3, 3], `all-zero weights must not collapse onto one shard; got ${sizes}`); + }); + + test('REGRESSION: clamped NaN/negative/Infinity weights do not collapse', () => { + const files = ['a', 'b', 'c', 'd', 'e', 'f'].map((x) => `${x}.test.cjs`); + const hostile = { 'a.test.cjs': NaN, 'b.test.cjs': -5, 'c.test.cjs': Infinity }; + const sizes = [1, 2, 3].map( + (i) => selectShard(files, { index: i, total: 3 }, (f) => (f in hostile ? hostile[f] : 1)).length, + ); + assert.ok( + Math.max(...sizes) - Math.min(...sizes) <= 1, + `weights that clamp to 0 must still spread; got ${sizes}`, + ); + }); + + test('property: zero weights spread evenly for any list size and shard count', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 40 }), + fc.integer({ min: 2, max: 6 }), + (n, total) => { + const files = Array.from({ length: n }, (_, i) => `p${String(i).padStart(3, '0')}.test.cjs`); + const sizes = Array.from({ length: total }, (_, i) => + selectShard(files, { index: i + 1, total }, () => 0).length); + assert.ok(Math.max(...sizes) - Math.min(...sizes) <= 1, `sizes=${sizes}`); + }, + ), + { numRuns: 200, seed: 24723 }, ); }); });