From bf0d7157338256e6ebdec31c8bce92052bbe1733 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 21 Jul 2026 09:15:59 -0400 Subject: [PATCH] fix(#2472): cost-balanced test sharding and pinned CI base commit (#2480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2472): weight-aware shard partition Windows shard 1/3 hit the 20-minute job cap with no failing assertion. Root cause is the shard layer, not the chunk layer: selectShard partitioned by sorted ARRAY INDEX (k % n, #1212), which balances file COUNTS and ignores file COST. On the real unit suite that produced 12.4m / 19.2m / 15.2m — a 1.23x max/ideal ratio leaving the heaviest shard 5% under the cap. Because assignment keyed off position, inserting one test file re-indexed every file after it and could tip that shard over; deterministic, so a re-run reproduced it exactly. This is NOT the chunk packer (#2456/#2463). That fix works and applies one level down, WITHIN a shard. The across-shard partition predated it and never consumed the cost table. Both layers now share one cost model. selectShard takes an optional weightOf and, when given one, partitions by LPT (longest-processing-time-first) — the same algorithm packChunks uses. Omitting it keeps the legacy round-robin byte-identical, so every existing test above still exercises that path unchanged and callers without timing data lose nothing. A missing timings table yields uniform weight 1, under which LPT degenerates to the equal-count split. Projected on the real suite: 16.4/17.3/13.0 -> 15.6/15.6/15.6 (worst shard 17.3m -> 15.6m). Tests: a skewed-cost regression (round-robin clusters all four heavy files onto one shard at 2.98x ideal; LPT does not), back-compat equivalence, determinism, tie-breaking, order preservation, and two fast-check properties — the partition is exhaustive and disjoint (getting this wrong silently DROPS tests from CI, the worst failure mode for a harness), and no shard exceeds average + heaviest file. Two assertions were corrected during authoring rather than shipped wrong: - an initial "LPT within 4/3 of ideal" bound was false. The 4/3 figure is relative to the OPTIMAL makespan, not the average, and the two differ when item sizes force a pairing. Replaced with Graham's average+max bound, which is what is actually provable. - "weighted is never worse than round-robin" is also false; fast-check falsified it with [19316,10190,1,9128,29353,20227] over 2 shards (rr 48670, lpt 48671). Round-robin can win by luck on a specific input. Dropped, with the counterexample recorded in place so it is not re-asserted later. Closes #2472 Co-Authored-By: Claude Opus 4.8 * fix(#2472): rotate tied bins; restore #1212 test block; lazy cost table Isolated-review findings, all fixed. HIGH — zero weights collapsed the whole partition onto shard 1. The lightest-bin scan compared weight only, and adding a zero-weight file leaves its bin's weight unchanged, so bin 0 stayed tied-minimum forever and every such file landed on it. Verified: all-zero weights gave shard1=[a..f], shard2=[], shard3=[] — two of three CI runners idle while one ran everything. Reachable through safeWeight's own clamp (a NaN/negative/Infinity entry in a corrupted or hand-edited timings table) and through any genuine 0ms measurement, so the clamp reproduced the exact failure its comment claimed to prevent. Ties now break on file COUNT after weight, which rotates. Pinned by two regression tests (all-zero, and clamped NaN/negative/Infinity) plus a property over list size x shard count. The live table has no 0ms entries (min 19ms), so production was not affected — but nothing prevented it. MEDIUM — the new describe block had swallowed #1212's pre-existing property test, which is why a test under a "weight-aware" heading never passed a weigher. That was a bad block boundary in the previous commit, not a bad test: the #2472 describe was opened before #1212's last test instead of after. Moved back where it belongs; #1212 is 762-879 and #2472 is 894-1082. LOW — that relocated property test ran unseeded. Seeded (12120) per the repo's property-test convention so a failure reproduces. Verified passing under the new seed. LOW — hoisting the timings load above the shard block charged a readFileSync + JSON.parse to invocations that exit before needing it (empty selection, --files matching nothing). Now lazily memoized, so neither consumer reads the table unless it is used and it is still read at most once. Real-suite projection unchanged at 15.6m / 15.6m / 15.6m. Co-Authored-By: Claude Opus 4.8 * docs(#2472): correct stale round-robin sharding descriptions The partition is now cost-balanced, so the header block in run-tests.cjs and the two comments in test.yml describing '--shard' as a round-robin over sorted file index were actively wrong. Updated to describe LPT over measured duration, and to state the degenerate case explicitly: with no timing data every file weighs the same and the partition collapses back to k % n, which is why the pre-existing #1212 CLI tests still pass unchanged (their nine synthetic files are absent from the timings table, so all take the identical median weight). Remaining 'round-robin' mentions are correct — they describe the unweighted fallback path. Co-Authored-By: Claude Opus 4.8 * fix(#2472): shard diagnostics, cost-routing E2E test, table validation Second orthogonal review (operational lens) findings, all fixed. HIGH — cross-runner partition divergence. Each of the up-to-12 CI jobs runs its own 'merge base into head' and computes its own partition, so if the inputs differ between jobs (the file list, or the timings table) two jobs can place the same file in different shards or in none. Every job stays internally exhaustive and disjoint, so nothing errors: a test simply never runs and CI stays green. The risk class is pre-existing — round-robin diverges identically when the file set differs between jobs, which is literally this issue's insertion instability — but weighting adds tests/test-timings.json as a second input that must match, so it widens the hole. Properly closing it means pinning the partition inputs per run, a workflow change beyond this fix. What IS closed here is the silence. Each shard now prints an input fingerprint over the FULL pre-partition list and the weight assigned to each file — deliberately not this shard's slice, which would differ by design and be useless for comparison. All shard jobs of one run must print an identical sig; a mismatch is direct proof the runners disagreed about the input. Verified: three independent computations agree, and the sig changes when the input drifts by one file. MEDIUM — nothing proved main() actually threads fileWeightOf() into selectShard. Every pre-existing --shard E2E test uses synthetic filenames absent from the real table, so all collapse to a uniform median weight, under which LPT is mathematically identical to k % n — a typo on that one wiring line would have passed the whole suite. Added an E2E test that injects a table via RUN_TESTS_TIMINGS_FILE with differing costs, placing the heavy files at exactly the indices round-robin hands to shard 1, and asserts shard 1 does NOT receive all three. Plus a test that all three shards emit the same sig. MEDIUM/LOW — no observability. The diagnostic line now reports files, weighed count, aggregate weight, and whether the table loaded, so a table that silently failed to parse shows table=absent/weighed=0 instead of being indistinguishable from a healthy load. (The reviewer confirmed the advisory fallback is already live on next: feat-2296-provider-escalation.test.cjs is missing from the table.) LOW — typeof [] === 'object', so a hand-edit turning the map into a list was accepted as a valid table. Now rejected via Array.isArray, falling back to uniform weight like any other malformed table. LOW — stale round-robin wording in ci-test-scope.test.cjs. Co-Authored-By: Claude Opus 4.8 * fix(#2472): pin every CI job to one base commit Closes the cross-runner divergence at its source instead of only making it visible. Each job of a run executes the rebase-check step independently, minutes apart across a 12-job matrix, and merged the MOVING origin/ ref. If the base advanced mid-run, different jobs merged different trees. That was survivable when jobs only had to agree on pass/fail; it is not once they must 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 — and every job still looks internally consistent, so nothing errors. A test silently never runs and CI stays green. ci-rebase-check.cjs now accepts CI_REBASE_BASE_SHA and pins BOTH the fetch and the merge to that one commit, so the two can never disagree. test.yml passes github.event.pull_request.base.sha on all three rebase-check steps; that value is fixed for the life of a run, so all jobs merge the identical base. This also closes the PRE-EXISTING half of the divergence. Round-robin had the same exposure whenever the test-file set differed between jobs — that is this issue's insertion instability — so the pin fixes the older hole too, not just the timings-table input weighting added. Only a full 40-hex sha is accepted; empty (push/workflow_dispatch), malformed, or injected values fall back to the branch ref rather than handing an arbitrary string to git fetch as a refspec. resolveBaseRefs is extracted pure and exported, and runMain is guarded behind require.main === module, so the pin contract is testable without spawning git. Tests (tests/ci-test-scope.test.cjs): every rebase-check step must carry the pin; a valid sha pins both refs; absence falls back correctly; and five hostile values — short sha, uppercase, --upload-pack= injection, ref expression, empty — are each rejected. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .github/workflows/test.yml | 28 +++- scripts/ci-rebase-check.cjs | 52 ++++++- scripts/run-tests.cjs | 159 +++++++++++++++++-- tests/ci-test-scope.test.cjs | 58 ++++++- tests/run-tests-harness.test.cjs | 255 ++++++++++++++++++++++++++++++- 5 files changed, 531 insertions(+), 21 deletions(-) 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 }, ); }); });