diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78e6c8be42..08ad09856c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -539,7 +539,7 @@ jobs: node scripts/partition-test-shards.mjs "$RUNNER_TEMP/turbo-ls.json" \ --shard ${{ matrix.shard }}/6 --exclude @objectstack/dogfood \ > "$RUNNER_TEMP/shard-packages.txt" - echo "Packages on this shard:" + echo 'Items on this shard (a package name, or a package plus a k/n file-level slice):' cat "$RUNNER_TEMP/shard-packages.txt" # --concurrency=4: turbo's default (10) oversubscribes the 4-vCPU @@ -587,11 +587,69 @@ jobs: # vitest's own "use the default" signal. Needs turbo.json's # globalPassThroughEnv entry or turbo strips it — see the script header. export VITEST_MAX_WORKERS="$(node scripts/vitest-worker-cap.mjs)" - FILTERS=$(sed 's/^/--filter=/' "$RUNNER_TEMP/shard-packages.txt" | tr '\n' ' ') mkdir -p "$RUNNER_TEMP/stall-reports" - node scripts/run-with-stall-guard.mjs --log "$RUNNER_TEMP/test-core.log" --stall-minutes 10 \ - --report-dir "$RUNNER_TEMP/stall-reports" -- \ - pnpm turbo run test $FILTERS --concurrency=4 --summarize --log-order=stream + + # Split the shard's ITEMS into the whole packages, which share one + # turbo run as they always have, and the file-level slices, which + # cannot: `--shard=k/n` is passed through to vitest by turbo as a + # RUN-level argument, so it would reach every package in the run — + # and on any package with fewer test files than n that is a hard + # vitest failure (or, with --passWithNoTests, silently no tests at + # all). A slice therefore gets its own invocation, filtered to the one + # package the partitioner sliced. + FILTERS="" + SLICES="" + while read -r PKG SLICE; do + [ -n "$PKG" ] || continue + if [ -n "$SLICE" ]; then + SLICES="$SLICES $PKG=$SLICE" + else + FILTERS="$FILTERS --filter=$PKG" + fi + done < "$RUNNER_TEMP/shard-packages.txt" + + # Each leg tees to its OWN log — run-with-stall-guard opens the log + # with 'w', so a second leg pointed at one path would truncate the + # first leg's output and the completeness guard below would grade half + # a shard. They are concatenated afterwards, and that concatenation + # happens even when a leg failed, because a red suite is exactly when + # the completeness guard earns its keep. + # + # ⛔ A failing leg STOPS the remaining ones, the same way turbo stops + # scheduling on the first failure inside one run. Carrying on would add + # a second full suite to a job that is already red and already inside a + # 30-minute wall — turning an informative red into a killed job with no + # attestation at all, which is the #16173 failure mode itself. + STATUS=0 + LOGS="" + for LEG in __whole__ $SLICES; do + if [ "$LEG" = __whole__ ]; then + [ -n "$FILTERS" ] || continue + LOG="$RUNNER_TEMP/test-core-packages.log" + set -- pnpm turbo run test $FILTERS --concurrency=4 --summarize --log-order=stream + else + PKG="${LEG%%=*}" + SLICE="${LEG#*=}" + LOG="$RUNNER_TEMP/test-core-slice-$(printf '%s' "$PKG" | tr -c 'A-Za-z0-9' '-').log" + set -- pnpm turbo run test "--filter=$PKG" --concurrency=4 --summarize --log-order=stream -- "--shard=$SLICE" + fi + LOGS="$LOGS $LOG" + node scripts/run-with-stall-guard.mjs --log "$LOG" --stall-minutes 10 \ + --report-dir "$RUNNER_TEMP/stall-reports" -- "$@" || { STATUS=$?; break; } + done + + # Only over logs that EXIST, and via an explicit `if` rather than a + # `&&` chain: a leg whose guard never got far enough to open its log + # would otherwise make `cat` non-zero, and under `set -e` that replaces + # the SUITE's exit status with cat's — the step would report the wrong + # reason for its own red. + : > "$RUNNER_TEMP/test-core.log" + for LOG in $LOGS; do + if [ -f "$LOG" ]; then + cat "$LOG" >> "$RUNNER_TEMP/test-core.log" + fi + done + exit $STATUS # --summarize above costs nothing at runtime and writes # `.turbo/runs/.json`: one per-task record with the execution window @@ -635,6 +693,31 @@ jobs: if-no-files-found: ignore retention-days: 1 + # ⛔ THE DRIFT STEP IS DELIBERATELY NOT WIRED HERE YET (#16173). + # + # partition-test-shards.mjs carries a fully-tested `--check-drift` mode — + # it reads the summary uploaded above back and reds when a shard's MEASURED + # test total outruns its PREDICTED one past MAX_MEASURED_OVER_PREDICTED. The + # code, its self-tests and its ablation are all on this branch; only this + # invocation waits, and the wait is a SEQUENCING decision, not an oversight. + # + # Why: scripts/test-shard-timings.json is still stale for @objectstack/cli + # (458.15s recorded, 1231.52s measured), so wiring the step today would red + # the shard carrying a CLI slice on every single PR — a true reading, but one + # that blocks everything until the dataset is refreshed. The file-level split + # above already removes the urgent hazard on its own, taking the worst shard + # from ~1445s (80% of this job's 30-minute wall) to ~1059s (59%) with the + # dataset untouched, because the CLI is halved across two runners instead of + # falling on one. + # + # ⇒ The refresh and this step land TOGETHER in the follow-up, in that order. + # The refresh recipe is in PR #16220's body. When it lands, restore a step + # here that runs `--check-drift` over `.turbo/runs/*.json` with + # `--label "Test Core (${{ matrix.shard }}/6)"`, with NO `if:` and NO + # `continue-on-error` (the point is the red), placed ABOVE the attestation + # pair for the #6082 reason documented on the upload above — so a drift red + # also withholds the attestation, which is the fail-closed direction. + # Runs even when the suite failed — that is when it earns its keep. It # answers TWO questions about a red suite, and needs both to be able to # say anything at all about a green one. diff --git a/scripts/check-test-completeness.mjs b/scripts/check-test-completeness.mjs index 56ca823d72..73306b503b 100644 --- a/scripts/check-test-completeness.mjs +++ b/scripts/check-test-completeness.mjs @@ -144,6 +144,12 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; import process from 'node:process'; import { isEntrypoint } from './invoked-as.mjs'; +// The shard-item grammar, from the script that WRITES the scheduled list. A +// second reader here would be a second grammar, and the two would drift apart +// silently -- an unparsed `@objectstack/cli 1/2` reaches describe() as a +// package name the turbo ls document has never heard of, which this guard +// (correctly, for its own contract) refuses the whole shard over. +import { parseShardItem } from './partition-test-shards.mjs'; const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -298,14 +304,49 @@ export function parseFailedTestPackages(text) { return failed; } +/** + * The scheduled list, folded to PACKAGE names. + * + * `shard-packages.txt` holds shard ITEMS, and since #16173 an item can be a + * file-level slice of a package (`@objectstack/cli 1/2`). Q2 is asked per + * package -- did this package report at all on this shard -- and the reported + * set is keyed by the package name turbo prints, which carries no slice, so an + * unfolded item would be a name the `turbo ls` document does not list and the + * shard would be refused whole. Duplicates collapse for the same reason: a + * package is complete when every slice of it scheduled HERE has reported, and + * partition-test-shards.mjs refuses any split that puts two slices of one + * package on one shard, so "every slice here" is "the one slice here". + */ +export function scheduledPackages(lines) { + const out = []; + const seen = new Set(); + for (const line of lines) { + const { name } = parseShardItem(line); + if (seen.has(name)) continue; + seen.add(name); + out.push(name); + } + return out; +} + // true = every task turbo scheduled succeeded, so nothing was cancelled. // false = it stopped early. null = turbo never printed its roster (it died, or // the log was truncated) -- unknown, and treated as "not completed" by Rule B. +// +// EVERY roster line is folded, not just the last one. A shard's log holds one +// roster per `turbo run` invocation, and since #16173 a shard that carries a +// file-level slice runs TWO (the sliced package needs its own passthrough, which +// turbo applies run-wide). Reading only the last would let a completed second +// invocation vouch for a first one that stopped early -- and Rule B turns that +// into a red on every package the abort left unreached, which is precisely the +// false-red machine this file's header warns about. export function parseRunCompleted(text) { let completed = null; for (const line of text.split('\n')) { const m = line.match(TASKS); - if (m) completed = Number(m[1]) === Number(m[2]); + if (!m) continue; + const thisRun = Number(m[1]) === Number(m[2]); + completed = completed === null ? thisRun : completed && thisRun; } return completed; } @@ -596,7 +637,7 @@ function reportVerdict(verdict) { // not red. A battery BELOW its floor means cases stopped running; the remedy is // to find what stopped registering. const SELF_TEST_BATTERIES = Object.freeze({ - 'check-test-completeness self-test': 67, + 'check-test-completeness self-test': 79, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -956,6 +997,63 @@ function selfTest({ quiet = false } = {}) { 'exit codes: the refusal code collides with a verdict code', ); + // -- File-level slice items in the scheduled list (#16173) ---------------- + // + // `shard-packages.txt` stopped being a list of package names the day one + // package started being sharded below package granularity. Both halves below + // fail in the false-RED direction if they regress, which is the direction + // this file's header spends its length warning about. + eq(scheduledPackages(['@objectstack/spec', '@objectstack/core']), ['@objectstack/spec', '@objectstack/core'], + 'scheduled: plain package names were disturbed'); + eq(scheduledPackages(['@objectstack/cli 1/2']), ['@objectstack/cli'], + 'scheduled: a slice did not fold to its package name'); + eq(scheduledPackages(['@objectstack/cli 1/2', '@objectstack/cli 2/2']), ['@objectstack/cli'], + 'scheduled: two slices of one package did not collapse'); + eq(scheduledPackages(['@objectstack/spec', '@objectstack/cli 2/2', '@objectstack/core']), + ['@objectstack/spec', '@objectstack/cli', '@objectstack/core'], + 'scheduled: folding reordered the list'); + // END-TO-END: an unfolded item reaches describe() as a name the turbo ls + // document cannot resolve, and classifyShard refuses the whole shard over it. + // This asserts the fold is what stops that, not that describe() got lenient. + eq( + classifyShard({ + scheduled: scheduledPackages(['@objectstack/cli 1/2']), + reported: new Set(['@objectstack/cli']), + failed: new Set(), + runCompleted: true, + describe: describe({ '@objectstack/cli': { hasTestScript: true, testFileCount: 268 } }), + }).silent, + [], + 'scheduled: a slice that DID report was still graded silent', + ); + eq( + threw(() => + classifyShard({ + scheduled: ['@objectstack/cli 1/2'], + reported: new Set(['@objectstack/cli']), + failed: new Set(), + runCompleted: true, + describe: describe({ '@objectstack/cli': { hasTestScript: true, testFileCount: 268 } }), + }), + ), + true, + 'scheduled: an UNFOLDED item was accepted, so the fold is not what makes this work', + ); + + // -- One roster per turbo invocation, and a shard can now run two (#16173) -- + const roster = (ok, total) => `Tasks: ${ok} successful, ${total} total`; + eq(parseRunCompleted(roster(4, 4)), true, 'runCompleted: a complete single roster'); + eq(parseRunCompleted(roster(3, 5)), false, 'runCompleted: an incomplete single roster'); + eq(parseRunCompleted('no roster here'), null, 'runCompleted: a log with no roster is unknown, not complete'); + eq(parseRunCompleted(`${roster(4, 4)}\n${roster(2, 2)}`), true, + 'runCompleted: two complete rosters'); + // The load-bearing one: the packages the FIRST invocation never reached would + // otherwise be charged as silent under Rule B. + eq(parseRunCompleted(`${roster(3, 9)}\n${roster(1, 1)}`), false, + 'runCompleted: a completed second invocation vouched for a first that stopped early'); + eq(parseRunCompleted(`${roster(9, 9)}\n${roster(0, 1)}`), false, + 'runCompleted: a second invocation that stopped early was overlooked'); + // ⚠️ The floor is scoped to the LOUD run on purpose. `selfTest({ quiet: true })` // also runs on EVERY production invocation of this gate (see `main()`), and // there nothing claims a self-test verdict — the floor exists to stop a green @@ -1059,10 +1157,12 @@ function main() { if (scheduledPath) { let scheduled; try { - scheduled = readFileSync(scheduledPath, 'utf8') - .split('\n') - .map((l) => l.trim()) - .filter(Boolean); + scheduled = scheduledPackages( + readFileSync(scheduledPath, 'utf8') + .split('\n') + .map((l) => l.trim()) + .filter(Boolean) + ); } catch (err) { console.error(`check-test-completeness: cannot read ${scheduledPath} -- ${err.message}`); process.exit(1); diff --git a/scripts/measure-test-shard-timings.mjs b/scripts/measure-test-shard-timings.mjs index df2fd6424b..2867931ed7 100644 --- a/scripts/measure-test-shard-timings.mjs +++ b/scripts/measure-test-shard-timings.mjs @@ -78,12 +78,51 @@ export function median(values) { return sorted.length % 2 === 1 ? sorted[mid] : (sorted[mid - 1] + sorted[mid]) / 2; } +// Which file-level slice a summary's tasks were run as, or null for a whole +// package (#16173). +// +// turbo records the run's passthrough argv on EVERY task record as +// `cliArguments` -- verified on turbo 2.10.10 against a real executed summary, +// not only a `--dry=json` plan: `pnpm turbo run test --filter= --summarize +// -- --shard=1/2` writes `"cliArguments": ["--shard=1/2"]` and the child really +// receives `pnpm run test --shard=1/2`. It is a RUN-level field replicated per +// task, which is exactly the granularity this needs: ci.yml runs the sliced +// package in its own turbo invocation, so a summary is either a whole-package +// run or one slice's, never a mix. +// +// ⛔ WHY THIS CANNOT BE SKIPPED. The dataset stores each package's WHOLE cost. +// Once the CLI is sharded k/n, a green queue build leaves n summaries each +// holding ~1/n of it, and the median rule -- correct for repeat measurements of +// one package -- would write one slice's duration as the whole package's +// weight. That is the #16173 defect exactly, re-created by the fix for it, in +// the same silent direction: a number that reads right and is n times too +// small. +export function sliceOfCliArguments(args) { + if (!Array.isArray(args)) return null; + for (const arg of args) { + const m = /^--shard[= ](\d+)\/(\d+)$/.exec(String(arg)); + if (!m) continue; + const [index, count] = [Number(m[1]), Number(m[2])]; + if (index < 1 || count < 1 || index > count) { + throw new Error(`a run summary carries an impossible slice spec ${JSON.stringify(arg)}`); + } + return { index, count }; + } + return null; +} + // Pull every genuinely-executed `test` task out of one parsed run summary. // // The shape assertions are loud for the same reason the partitioner's are: // `--summarize` is stable but not contractual, and the failure this script can // cause is a dataset that looks fine and balances nothing. A summary that // carries no `tasks` array is a refusal, never an empty result. +// +// `slices` rides alongside `samples` rather than changing its shape: the drift +// gate in partition-test-shards.mjs reads `samples` as name -> seconds and does +// not need it (a shard runs exactly one slice, so its prediction follows from +// the slice map, not from the summary), while buildDataset below cannot record +// a correct weight without it. export function samplesFromSummary(parsed, label) { const tasks = parsed?.tasks; if (!Array.isArray(tasks)) { @@ -94,6 +133,7 @@ export function samplesFromSummary(parsed, label) { } const samples = new Map(); const skippedCached = []; + const slices = new Map(); for (const task of tasks) { if (task?.task !== 'test') continue; const name = task.package; @@ -113,8 +153,10 @@ export function samplesFromSummary(parsed, label) { const seconds = (endTime - startTime) / 1000; if (!(seconds >= 0)) throw new Error(`${label}: ${name}#test measured ${seconds}s`); samples.set(name, seconds); + const slice = sliceOfCliArguments(task.cliArguments); + if (slice) slices.set(name, slice); } - return { samples, skippedCached }; + return { samples, skippedCached, slices }; } // The weight an unmeasured package gets: its test-file count times this rate. @@ -142,17 +184,59 @@ export function fallbackRate(measured, fileCounts) { export function buildDataset({ perSummary, fileCounts, provenance }) { const bySample = new Map(); const cachedNames = new Set(); - for (const { samples, skippedCached } of perSummary) { + const push = (name, seconds) => { + if (!bySample.has(name)) bySample.set(name, []); + bySample.get(name).push(seconds); + }; + + // A file-sharded package (#16173) arrives as n partial windows across n + // summaries, and the whole-package cost this dataset records is their SUM -- + // not their median, which is the rule for repeat measurements of one package + // and would write 1/n of the truth here. So slices are held back and summed + // per (package, slice count) set; the sum then enters the median pool as ONE + // sample, which keeps the two rules composable when several runs are fed in. + const sliceLedger = new Map(); + for (const { samples, skippedCached, slices } of perSummary) { for (const n of skippedCached) cachedNames.add(n); for (const [name, seconds] of samples) { - if (!bySample.has(name)) bySample.set(name, []); - bySample.get(name).push(seconds); + const slice = slices?.get(name) ?? null; + if (!slice) { + push(name, seconds); + continue; + } + if (!sliceLedger.has(name)) sliceLedger.set(name, new Map()); + const byCount = sliceLedger.get(name); + if (!byCount.has(slice.count)) byCount.set(slice.count, new Map()); + byCount.get(slice.count).set(slice.index, seconds); } } + + // ⛔ An INCOMPLETE slice set is not summed. Summing 1 of 2 slices would record + // half a suite as the whole of it -- a wrong number that reads exactly like a + // right one, which is the hazard this file's cache rule already refuses in the + // other direction. The package instead drops out of `packages` entirely and is + // ESTIMATED from its test-file count like any unmeasured package, and it is + // named in the dataset so a refresh built on a partial artifact set is visible + // in the file rather than inferred from the split going strange later. + const incompleteSlices = []; + for (const [name, byCount] of sliceLedger) { + for (const [count, seen] of byCount) { + if (seen.size === count) { + push(name, [...seen.values()].reduce((a, b) => a + b, 0)); + continue; + } + const missing = []; + for (let i = 1; i <= count; i++) if (!seen.has(i)) missing.push(`${i}/${count}`); + incompleteSlices.push(`${name} (missing ${missing.join(', ')})`); + } + } + incompleteSlices.sort((a, b) => a.localeCompare(b, 'en')); + if (bySample.size === 0) { throw new Error( - 'every `test` task in these summaries was a cache hit or a failure, so there is no ' + - 'measurement here -- re-run with a cold cache (or `--force`) before regenerating.' + 'every `test` task in these summaries was a cache hit, a failure or an incomplete slice ' + + 'set, so there is no measurement here -- re-run with a cold cache (or `--force`) before ' + + 'regenerating.' ); } const measured = new Map( @@ -171,6 +255,7 @@ export function buildDataset({ perSummary, fileCounts, provenance }) { secondsPerTestFileFallback: fallbackRate(measured, fileCounts), packages, skippedAsCached: [...cachedNames].sort((a, b) => a.localeCompare(b, 'en')), + skippedIncompleteSlices: incompleteSlices, }; } @@ -202,7 +287,7 @@ export function buildDataset({ perSummary, fileCounts, provenance }) { // must not red. A battery BELOW its floor means cases stopped running; the // remedy is to find what stopped registering, never to lower the number. const SELF_TEST_BATTERIES = Object.freeze({ - 'measure-test-shard-timings self-test': 22, + 'measure-test-shard-timings self-test': 34, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -368,6 +453,108 @@ function selfTest() { } }); + // File-level slices (#16173): a package the Test Core matrix shards below + // package granularity arrives as n partial windows, and the WHOLE cost this + // dataset records is their sum. Every case below fails in the same direction + // if the reassembly is dropped -- the heaviest package in the workspace + // recorded at 1/n of its real weight, which is #16173 again. + const slicedTask = (pkg, start, end, index, count) => ({ + ...testTask(pkg, start, end), + cliArguments: [`--shard=${index}/${count}`], + }); + + check(() => { + if (sliceOfCliArguments(undefined) !== null || sliceOfCliArguments([]) !== null) { + throw new Error('slice: a run with no passthrough argv was read as a slice'); + } + }); + check(() => { + if (sliceOfCliArguments(['--log-order=stream']) !== null) { + throw new Error('slice: an unrelated passthrough argument was read as a slice'); + } + }); + check(() => { + const s = sliceOfCliArguments(['--shard=2/3']); + if (!s || s.index !== 2 || s.count !== 3) throw new Error(`slice: parsed as ${JSON.stringify(s)}`); + }); + check(() => { + if (!threw(() => sliceOfCliArguments(['--shard=3/2']))) { + throw new Error('slice: an impossible slice spec was accepted'); + } + }); + check(() => { + const { slices } = samplesFromSummary(summary([slicedTask('cli', 0, 400_000, 1, 3)]), 'f'); + const s = slices.get('cli'); + if (!s || s.index !== 1 || s.count !== 3) throw new Error('slice: samplesFromSummary did not record the slice'); + }); + check(() => { + const { slices } = samplesFromSummary(summary([testTask('a', 0, 10_000)]), 'f'); + if (slices.size !== 0) throw new Error('slice: a whole-package task was recorded as a slice'); + }); + + // The load-bearing case: three slices of 400s are ONE 1200s package, not a + // 400s one. Median across the three windows -- the rule for repeat + // measurements -- would answer 400. + const sliced = buildDataset({ + perSummary: [ + samplesFromSummary(summary([slicedTask('cli', 0, 400_000, 1, 3)]), 'f'), + samplesFromSummary(summary([slicedTask('cli', 0, 380_000, 2, 3)]), 'g'), + samplesFromSummary(summary([slicedTask('cli', 0, 420_000, 3, 3), testTask('a', 0, 10_000)]), 'h'), + ], + fileCounts: new Map([['cli', 300], ['a', 5]]), + provenance: {}, + }); + check(() => { + if (sliced.packages.cli !== 1200) { + throw new Error(`slice: three 400s-ish slices summed to ${sliced.packages.cli}, expected 1200`); + } + }); + check(() => { + if (sliced.skippedIncompleteSlices.length !== 0) { + throw new Error(`slice: a complete set was reported incomplete (${sliced.skippedIncompleteSlices.join('; ')})`); + } + }); + check(() => { + if (sliced.packages.a !== 10) throw new Error('slice: an unsliced package in the same set was disturbed'); + }); + + // A partial artifact set -- one shard's summary missing, or its slice a cache + // hit -- must NOT be summed. The package drops out and says so. + const partial = buildDataset({ + perSummary: [ + samplesFromSummary(summary([slicedTask('cli', 0, 400_000, 1, 3)]), 'f'), + samplesFromSummary(summary([slicedTask('cli', 0, 420_000, 3, 3), testTask('a', 0, 10_000)]), 'h'), + ], + fileCounts: new Map([['cli', 300], ['a', 5]]), + provenance: {}, + }); + check(() => { + if (Object.hasOwn(partial.packages, 'cli')) { + throw new Error(`slice: 2 of 3 slices were recorded as a weight (${partial.packages.cli})`); + } + }); + check(() => { + if (!partial.skippedIncompleteSlices.some((s) => s.includes('cli') && s.includes('2/3'))) { + throw new Error(`slice: the incomplete set was not named (${partial.skippedIncompleteSlices.join('; ')})`); + } + }); + + // The two merge rules compose: sum WITHIN a run, median ACROSS runs. + const twoRuns = buildDataset({ + perSummary: [ + samplesFromSummary(summary([slicedTask('cli', 0, 500_000, 1, 2)]), 'r1a'), + samplesFromSummary(summary([slicedTask('cli', 0, 500_000, 2, 2)]), 'r1b'), + samplesFromSummary(summary([testTask('a', 0, 10_000)]), 'r1c'), + ], + fileCounts: new Map([['cli', 300], ['a', 5]]), + provenance: {}, + }); + check(() => { + if (twoRuns.packages.cli !== 1000) { + throw new Error(`slice: a 2-slice set summed to ${twoRuns.packages.cli}, expected 1000`); + } + }); + // Workspace resolution, at the depth that actually caught a defect. A // one-level scan resolves `packages/*` and returns null for the ~60% of the // workspace that lives under `packages/drivers/*`, `packages/services/*` and @@ -549,6 +736,18 @@ function main() { `fallback ${dataset.secondsPerTestFileFallback}s/test-file, ` + `${dataset.skippedAsCached.length} skipped as cached -> ${path.relative(REPO_ROOT, out)}` ); + // Loud, and on stderr beside the summary line: a file-sharded package that + // lost a slice is now ESTIMATED rather than measured, and it is the heaviest + // package in the workspace that this can happen to. Silence here would let a + // refresh built on five of six artifacts read like a complete one. + if (dataset.skippedIncompleteSlices.length > 0) { + console.error( + `measure-test-shard-timings: ⚠ ${dataset.skippedIncompleteSlices.length} package(s) reported an ` + + 'INCOMPLETE set of file-level slices and were left unmeasured (they fall back to the ' + + `test-file-count estimate): ${dataset.skippedIncompleteSlices.join('; ')}. Feed every shard's ` + + 'summary from ONE green run before trusting this dataset.' + ); + } } // Exports bindings, so an import for those exports alone must run nothing (#10667). diff --git a/scripts/partition-test-shards.mjs b/scripts/partition-test-shards.mjs index baa58619b7..0f3f943f3a 100644 --- a/scripts/partition-test-shards.mjs +++ b/scripts/partition-test-shards.mjs @@ -74,8 +74,19 @@ // Usage: // node scripts/partition-test-shards.mjs --shard N/M \ // [--exclude ]... +// node scripts/partition-test-shards.mjs --check-drift ... \ +// [--label ] // node scripts/partition-test-shards.mjs --self-test // +// `--check-drift` is the half that keeps the dataset honest AFTER it is +// written (#16173). Every Test Core shard already passes `--summarize`, so the +// run it just finished has written the measured truth to `.turbo/runs/`; this +// mode reads that back, compares it to what this script PREDICTED for the same +// packages, and reds past MAX_MEASURED_OVER_PREDICTED. Without it the dataset +// rots silently in one direction and the only instrument that notices is a +// shard killed by the job timeout -- which is a shard that produced NO reading +// while the rollup read green. +// // The weight dataset is scripts/test-shard-timings.json, regenerated by // scripts/measure-test-shard-timings.mjs. It is required, not optional: this // script refuses to shard rather than fall back to the old file-count proxy. @@ -94,6 +105,7 @@ import { fileURLToPath } from 'node:url'; import process from 'node:process'; import { isEntrypoint } from './invoked-as.mjs'; +import { samplesFromSummary } from './measure-test-shard-timings.mjs'; const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const TIMINGS_PATH = path.join(REPO_ROOT, 'scripts', 'test-shard-timings.json'); @@ -113,6 +125,220 @@ export const SHARD_COUNT = 6; // numerator up. export const MAX_SHARD_OVER_MEAN = 1.3; +// The factor a shard's MEASURED test total may exceed its PREDICTED total by +// before `--check-drift` reds. The durable half of #16173. +// +// Everything above balances a GENERATED dataset, and a generated file that +// nothing re-measures rots in one direction only: suites get slower, the +// numbers stay put, and the shard that drifted heavy reads as perfectly +// balanced on paper right up until the job timeout kills it. That is not a +// hypothesis. The reading this bound is written against, from run 34009395649 +// attempt 2 (job 101422473016 is the attempt-1 leg that was killed): +// +// @objectstack/cli predicted 458.15s measured 1231.52s = 2.69x +// +// -- one package, 68% of a 30-minute wall, on a shard the previous attempt had +// already lost at 30m05s. Every step was green; the split's own max/mean read +// 1.00x, because a perfectly balanced split of stale numbers is still perfectly +// balanced. Nothing in this repo compared a prediction to an outcome, so the +// only instrument that ever noticed was a killed job. +// +// 1.5 is where the two populations actually separate, measured rather than +// picked. On run 34013842594 -- a GREEN merge_group build, so the full package +// list rather than a pull_request's --affected subset -- the six shards ran +// their `Run this shard's tests` step against the same 672s prediction: +// +// shard 3 462s 0.69x shard 6 776s 1.15x +// shard 5 630s 0.94x shard 4 793s 1.18x +// shard 2 714s 1.06x shard 1 1168s 1.74x <- the one carrying the CLI +// +// Five healthy shards top out at 1.18x and the drifted one sits at 1.74x, on +// the same build, so the gap is not runner noise and one factor separates them +// cleanly. (Those step times include turbo scheduling and any uncached build +// tasks; this gate compares test-task windows only, which is the tighter and +// fairer reading of the same shards.) +// +// 1.5 also satisfies the two ends the bound is answerable to: +// +// - it must fire well below the 2.69x measured above, or the gate would have +// been green straight through the incident it exists to catch; +// - it must sit ABOVE MAX_SHARD_OVER_MEAN, because a dataset accurate to +// within the balance bound cannot be the thing that breaks balance. Gating +// tighter than the split's own tolerance reds on drift the partitioner is +// built to absorb, and a gate that reds on healthy input gets muted. +// +// ⛔ Raising this to absorb a red is the one move that cannot be right: the +// number it would be raised past is a measurement of the dataset being wrong. +export const MAX_MEASURED_OVER_PREDICTED = 1.5; + +// ── SHARDING ONE PACKAGE BELOW PACKAGE GRANULARITY (#16173) ──────────────── +// +// The floor argument at the top of this file is not a caveat, it is a wall: a +// shard can never finish faster than its single heaviest package, so once ONE +// package exceeds MAX_SHARD_OVER_MEAN x the mean, no shard count meets the +// bound and pin 3 says so by name. `@objectstack/cli` crossed that wall. +// Measured on run 34009395649 attempt 2 (job 101427282674): 1231.52s, against a +// 458.15s dataset entry. Substituting the measurement into the committed +// dataset and re-partitioning: +// +// bins 1232/716/714/714/714/714s mean 800.7s max/mean 1.54x (bound 1.30x) +// +// -- so the honest refresh this card asks for reds the partitioner's own +// balancing pins, by their design, and pin 3 names the only remedy: split that +// suite below package granularity. +// +// THIS IS THAT SPLIT, and it is the shape the Dogfood job has run since #4859: +// vitest's own `--shard=k/n` passthrough applied to ONE named package. The +// objection this file records against passthrough is specific and it does not +// reach here -- `--shard` on a package with fewer test files than the shard +// count hard-fails on vitest 4, and `--passWithNoTests` converts that into +// running NOTHING. That is fatal WORKSPACE-WIDE, where three packages own one +// test file each. Applied to one package with 268 of them it cannot arise, and +// `sliceCountFor` below refuses the configuration in which it could. +// +// WHY n = 2, DERIVED RATHER THAN PICKED. n is the smallest integer for which a +// slice fits under the acceptance bound against the mean the refresh produces. +// With the measurement above substituted, the other 70 packages total 3572.66s, +// so the mean is fixed at (3572.66 + 1231.52) / 6 = 800.70s and the bound is +// 1.3 x 800.70 = 1040.91s: +// +// n = 1 1231.52s > 1040.91s RED -- this is today +// n = 2 615.76s <= 1040.91s the derived answer +// +// and the split it produces is bins 801/801/801/801/801/800s, max/mean 1.00x. +// Two is not a floor to sit on quietly either: solving C/2 <= (1.3/6)(3572.66+C) +// for the CLI's whole cost C says n = 2 holds until that suite reaches ~2732s, +// a further 2.2x. Past that, pin 3 reds again naming the floor, and the remedy +// is to raise this number -- never the bound. +// +// ⛔ Slicing is a SCHEDULING fact, not a measurement one: the dataset keeps +// holding each package's WHOLE cost, and the division by n happens here. That +// is what keeps a refresh comparable across a change to this map, and it is why +// measure-test-shard-timings.mjs has to reassemble a package's slices before it +// records one -- see `sliceOfCliArguments` there. +export const FILE_SHARDED_PACKAGES = Object.freeze({ + '@objectstack/cli': 2, +}); + +// The item grammar. A shard item is a package (`@objectstack/cli`) or a SLICE +// of one (`@objectstack/cli 1/2`), and this pair of functions is the only place +// that spelling is written or read -- ci.yml builds the turbo invocation from +// it and check-test-completeness.mjs joins its scheduled list through it, so a +// second reader would be a second grammar. +// +// A space is the separator on purpose: npm package names cannot contain one +// (and `#` and `:` are both turbo task syntax, which `--filter` would try to +// interpret). +const SLICE_SPEC = /^(\S+)\s+([1-9]\d*)\/([1-9]\d*)$/; + +export function formatShardItem(name, slice) { + return slice ? `${name} ${slice.index}/${slice.count}` : name; +} + +export function parseShardItem(line) { + const text = String(line).trim(); + const m = SLICE_SPEC.exec(text); + if (!m) return { name: text, slice: null }; + const [, name, index, count] = m; + if (Number(index) > Number(count)) { + throw new Error(`shard item ${JSON.stringify(text)}: slice index exceeds its count`); + } + return { name, slice: { index: Number(index), count: Number(count) } }; +} + +// How many file-level slices a package is split into, and the ONE place the map +// is consulted. `fileCount` is optional because the two callers know different +// things: weighItems() has the package directory and can enforce the vitest +// floor, while the dataset-level balancing pins have only names and weights. +// +// ⛔ The floor is a REFUSAL, not a clamp. Silently reducing n to the file count +// would hand back a split that balances a quantity CI cannot run, which is the +// #16173 failure shape one level up: a number that reads right and is not. +export function sliceCountFor(name, fileCount = null) { + const n = Object.hasOwn(FILE_SHARDED_PACKAGES, name) ? FILE_SHARDED_PACKAGES[name] : 1; + if (n > 1 && fileCount !== null && fileCount < n) { + throw new Error( + `${name} is configured for ${n} file-level slices but owns ${fileCount} test file(s). ` + + 'vitest --shard hard-fails when the shard count exceeds the file count, and ' + + '--passWithNoTests turns that failure into running NOTHING on every slice. ' + + 'Lower FILE_SHARDED_PACKAGES for this package, or stop slicing it.' + ); + } + return n; +} + +// Expand weighed packages into shard items, splitting a file-sharded package's +// WHOLE weight evenly across its slices. Every downstream consumer -- partition, +// balanceOf, the balancing pins -- sees one flat list of `{name, weight}` whose +// `name` is the item's printed label, so nothing below has to know that some +// items are slices. +export function expandSlices(items) { + const out = []; + for (const it of items) { + const count = it.sliceCount ?? sliceCountFor(it.name); + if (count === 1) { + out.push({ name: it.name, weight: it.weight, pkg: it.name, slice: null }); + continue; + } + for (let index = 1; index <= count; index++) { + const slice = { index, count }; + out.push({ + name: formatShardItem(it.name, slice), + weight: it.weight / count, + pkg: it.name, + slice, + }); + } + } + return out; +} + +// Two slices of the same package must never share a bin, and this asserts it +// rather than assuming it. LPT gives it for free in every arrangement measured +// here -- equal-weight slices are placed consecutively into distinct lightest +// bins -- but "for free" is a property of the weights, not of the algorithm, +// and the day it stops holding the damage is silent in both directions: ci.yml +// would run one turbo invocation per slice against the SAME package on one +// runner (serialising what the split exists to spread), while the completeness +// join, which keys reported packages by name, could not tell the second slice +// from the first. A refusal here costs a red partition step naming the bin. +export function assertSlicesSpread(bins) { + for (const [i, bin] of bins.entries()) { + const seen = new Set(); + for (const label of bin.names) { + const { name, slice } = parseShardItem(label); + if (!slice) continue; + if (seen.has(name)) { + throw new Error( + `bin ${i + 1} holds more than one slice of ${name} (${bin.names.join(', ')}) -- ` + + 'file-level slices of one package must land on different shards or the split ' + + 'spreads nothing. Refusing to shard.' + ); + } + seen.add(name); + } + } + return bins; +} + +// What a run was predicted to spend on a package it just ran. A shard runs +// exactly one slice of a file-sharded package -- the slices are placed in +// distinct bins, asserted in main() -- so the prediction to compare a measured +// window against is the dataset's whole-package entry divided by the slice +// count. Charging a slice the whole package's entry would read as a ~n x +// under-run and, worse, dilute a real overshoot elsewhere on the same shard +// into a ratio that stays under the bound. +// +// `sliceCount` is passed in by `--check-drift` from what the SUMMARY says the +// run actually was, not from FILE_SHARDED_PACKAGES. The two agree on a Test Core +// shard, and only the observed one is right anywhere else: a developer running +// the suite locally runs the CLI whole, and charging that whole run a half-sized +// prediction would report a 2x drift that is purely this function's arithmetic. +// The config remains the default for callers with no run in hand. +export function predictedSecondsFor(name, timings, sliceCount = sliceCountFor(name)) { + return timings.packages[name] / sliceCount; +} + // Where a `packages.items[].path` actually points. // // The document has two writers -- `turbo ls`, which emits repo-relative paths, @@ -221,18 +447,25 @@ export function weighPackage(name, dir, timings) { // while re-opening #10472 exactly. The end-to-end pin in selfTest() below // calls THIS function, which is why it can tell duration from count. export function weighItems(items, excluded, timings, label = 'package list') { - const weighted = []; + const weighed = []; let estimated = 0; for (const it of items) { if (typeof it?.name !== 'string' || typeof it?.path !== 'string') { throw new Error(`${label}: package entry missing name/path: ${JSON.stringify(it)}`); } if (excluded.has(it.name)) continue; - const { seconds, measured } = weighPackage(it.name, packageDir(it.path), timings); + const dir = packageDir(it.path); + const { seconds, measured } = weighPackage(it.name, dir, timings); if (!measured) estimated++; - weighted.push({ name: it.name, weight: seconds }); + // The vitest file-count floor is checked HERE and only here, because this is + // the one weighing path that knows where the package lives. `sliceCountFor` + // throws rather than clamping -- see its header. + const sliceCount = Object.hasOwn(FILE_SHARDED_PACKAGES, it.name) + ? sliceCountFor(it.name, countTestFiles(dir)) + : 1; + weighed.push({ name: it.name, weight: seconds, sliceCount }); } - return { weighted, estimated }; + return { weighted: expandSlices(weighed), estimated, packages: weighed.length }; } // LPT greedy: heaviest package into the currently lightest bin. Deterministic: @@ -267,6 +500,71 @@ export function balanceOf(bins, items = null) { return { totals, sum, mean, max, min: Math.min(...totals), ratio: mean === 0 ? 1 : max / mean, floor }; } +// Compare what a shard was PREDICTED to cost against what it actually cost. +// +// The comparison is over the INTERSECTION of two sets, and both restrictions +// are load-bearing: +// +// - a package measured on this shard but absent from the dataset contributes +// to NEITHER total. Its weight came from the test-file-count estimate in +// weighPackage(), so calling the estimate wrong would red on a brand-new +// package rather than on a rotted dataset entry. It is named in +// `unpredicted` instead, because a shard full of estimates is its own +// (quieter) signal that a refresh is due. +// - a package in the dataset but not in this summary contributes to neither +// either. A shard runs a subset -- of the six bins, and on a pull_request +// of `turbo ls --affected` on top of that -- so charging a shard for +// packages it never ran would make the ratio a function of the diff. +// +// Cache hits and failed suites never reach here: the measurements come from +// samplesFromSummary(), which drops both. That is deliberate reuse rather than +// a second reader -- the generator's ~0s-for-a-replayed-suite hazard is the +// same hazard here, pointing the other way (a cached shard would read as +// enormously FASTER than predicted and quietly vouch for a rotted dataset). +export function driftReport(measured, timings, factor = MAX_MEASURED_OVER_PREDICTED, observedSlices = null) { + const rows = []; + const unpredicted = []; + let predictedTotal = 0; + let measuredTotal = 0; + for (const [name, seconds] of measured) { + if (!Object.hasOwn(timings.packages, name)) { + unpredicted.push(name); + continue; + } + // Through predictedSecondsFor, never the raw dataset entry: a shard that + // ran one SLICE of a file-sharded package was predicted one slice's cost. + // When the caller observed the run's own slice spec, that wins over the + // configured one -- `observedSlices` present but silent about a package + // means the summaries show it running WHOLE, which is a fact about the run. + const predicted = observedSlices + ? predictedSecondsFor(name, timings, observedSlices.get(name)?.count ?? 1) + : predictedSecondsFor(name, timings); + predictedTotal += predicted; + measuredTotal += seconds; + rows.push({ name, predicted, measured: seconds, overshoot: seconds - predicted }); + } + unpredicted.sort((a, b) => a.localeCompare(b, 'en')); + // Sorted by ABSOLUTE overshoot, not by ratio: the reader of a red verdict + // wants the package that cost the shard its minutes, and a 0.1s package that + // came in at 5x its 0.02s entry is noise wearing the biggest ratio. + rows.sort((a, b) => b.overshoot - a.overshoot || a.name.localeCompare(b.name, 'en')); + // `predictedTotal > 0` is the guard against a verdict of Infinity, which is + // what a shard carrying only zero-weight entries would otherwise produce -- + // a red naming no cause. Zero measured packages is the same state and reads + // the same way: NOT MEASURED is not a pass, and it is not a failure either. + const measurable = rows.length > 0 && predictedTotal > 0; + const ratio = measurable ? measuredTotal / predictedTotal : null; + return { + rows, + unpredicted, + predictedTotal, + measuredTotal, + ratio, + measurable, + drifted: measurable && ratio > factor, + }; +} + // Reads the package list out of a `turbo ls --output=json` payload, asserting // two independent properties. They fail for different reasons and both are // loud, because the failure this whole file guards against is the quiet one -- @@ -346,12 +644,18 @@ const SELF_TEST_BATTERIES = Object.freeze({ // testing anything. Flooring at the measured 16 makes that loud, and the // remedy is the one the pin itself names -- pick a new inversion pair from // the dataset -- never lowering this number. - 'the balancing pins (#10472)': 16, + // 5 of these 21 arrived with #16173's file-level slicing, and 3 of those run + // only while @objectstack/cli is still in the dataset and not excluded by + // ci.yml -- the same conditional shape as the inversion pin above, floored at + // the measured count for the same reason. + 'the balancing pins (#10472)': 21, + 'predicted-vs-measured drift (#16173)': 9, + 'file-level slice items (#16173)': 20, }); // DELETING an entry silences that battery's floor exactly as effectively as // zeroing it, so the roster's own size is pinned too. -const SELF_TEST_BATTERY_FLOOR = 7; +const SELF_TEST_BATTERY_FLOOR = 9; // The key an assertion is filed under when no battery is open. It is not a // declared battery, so it reds by the same set difference rather than silently @@ -555,9 +859,15 @@ function selfTest() { if (ciExcludes.size === 0) throw new Error('ci.yml: no --exclude found for the partitioner invocation'); }); const timings = loadTimings(); - const datasetItems = Object.entries(timings.packages) + // Through expandSlices, because that is the item list CI bins. Reading the + // dataset's packages straight into the pin would grade a shape no shard runs: + // after #16173 the CLI reaches the partitioner as file-level slices, and a pin + // that still weighs it whole would red on the refresh that fixes it and go + // green on a revert that removes the slicing. + const datasetPackages = Object.entries(timings.packages) .filter(([name]) => !ciExcludes.has(name)) .map(([name, weight]) => mk(name, weight)); + const datasetItems = expandSlices(datasetPackages); check(() => { if (datasetItems.length < 20) { throw new Error(`dataset: only ${datasetItems.length} package(s) measured -- that is not the workspace`); @@ -593,6 +903,77 @@ function selfTest() { } }); + // 3b. THE SLICING IS LIVE, AND THE SPLIT IT PRODUCES IS SPREAD (#16173). + // Pins 2 and 3 grade whatever item list they are handed, so neither can + // fail because the CLI quietly stopped being sliced: at today's stale + // 458.15s entry an unsliced CLI meets both bounds comfortably. These two + // pin the mechanism rather than the arithmetic it feeds. + check(() => { + if (sliceCountFor('@objectstack/cli') < 2) { + throw new Error( + 'FILE_SHARDED_PACKAGES no longer slices @objectstack/cli. That package measured 1231.52s ' + + 'against a 800.7s post-refresh mean, which no split at any shard count can bin under ' + + `${MAX_SHARD_OVER_MEAN}x -- see pin 3c below for the arithmetic.` + ); + } + }); + check(() => { + assertSlicesSpread(real); + }); + + // 3c. THE DERIVATION OF THE SLICE COUNT, pinned against the measurement it + // was derived from -- and against the counterfactual that makes the pin + // able to fail. + // + // The dataset on disk is still the STALE one (that refresh is the other + // half of #16173), so pin 2 above cannot yet see the arithmetic that + // decided the slice count. Substituting the measurement here is what lets + // the refresh land without this file's own pins going red on it: if the + // slice count is ever lowered, or the CLI grows past what it absorbs, the + // failure arrives HERE with the numbers in it rather than three weeks + // later in a queue build. + const CLI = '@objectstack/cli'; + const CLI_MEASURED = 1231.52; // run 34009395649 attempt 2, job 101427282674 + if (Object.hasOwn(timings.packages, CLI) && !ciExcludes.has(CLI)) { + const refreshed = datasetPackages.map((i) => (i.name === CLI ? mk(CLI, CLI_MEASURED) : i)); + const slicedItems = expandSlices(refreshed); + const sliced = balanceOf(partition(slicedItems, SHARD_COUNT), slicedItems); + check(() => { + if (sliced.ratio > MAX_SHARD_OVER_MEAN) { + throw new Error( + `slice derivation: with ${CLI} at its measured ${CLI_MEASURED}s the sliced split is ` + + `${sliced.ratio.toFixed(2)}x the mean (${sliced.max.toFixed(0)}s vs ${sliced.mean.toFixed(0)}s), ` + + `past the ${MAX_SHARD_OVER_MEAN}x bound. Bins: ${sliced.totals.map((t) => t.toFixed(0)).join('/')}s.` + ); + } + }); + check(() => { + if (sliced.floor > MAX_SHARD_OVER_MEAN * sliced.mean) { + throw new Error( + `slice derivation: one slice of ${CLI} is ${sliced.floor.toFixed(0)}s against a ` + + `${sliced.mean.toFixed(0)}s mean, so even sliced this way NO split at ${SHARD_COUNT} shards ` + + `meets ${MAX_SHARD_OVER_MEAN}x. Raise FILE_SHARDED_PACKAGES['${CLI}'] -- never the bound.` + ); + } + }); + // The counterfactual, and the reason the two cases above are not vacuous: + // UNSLICED, that same refresh must still breach the floor. The day it does + // not, the CLI has come back under the bound on its own and the slicing is + // a candidate for removal -- which is a decision, so this says so loudly + // rather than leaving a pin that passes whatever happens. + const whole = balanceOf(partition(refreshed, SHARD_COUNT), refreshed); + check(() => { + if (whole.floor <= MAX_SHARD_OVER_MEAN * whole.mean) { + throw new Error( + `slice derivation: UNSLICED, ${CLI} at ${CLI_MEASURED}s is ${whole.floor.toFixed(0)}s against a ` + + `${whole.mean.toFixed(0)}s mean and now fits under ${MAX_SHARD_OVER_MEAN}x on its own, so the two ` + + 'cases above no longer prove the slicing is what satisfies the bound. Re-derive the slice ' + + 'count (or retire it) instead of leaving a pin that cannot fail.' + ); + } + }); + } + // 4. The shard count is spelled in ci.yml too, and drift there is silent: // a partitioner cutting six bins for a five-job matrix simply loses a // sixth of the workspace, with every step green. Read it back. @@ -693,6 +1074,287 @@ function selfTest() { }); } + // -- PREDICTED VS MEASURED (#16173) ------------------------------------- + // + // The balancing pins above all read the dataset as GIVEN. None of them can + // fail because a number in it is wrong: a perfectly balanced split of stale + // weights satisfies every one of them, and did, at 1.00x max/mean, on the + // very build whose shard 1 was killed by the 30-minute wall. These pin the + // one comparison that can tell a good dataset from a rotted one. + battery('predicted-vs-measured drift (#16173)'); + const driftTimings = { packages: { slow: 100, fine: 50, zero: 0 }, rate: 2 }; + const measuredMap = (o) => new Map(Object.entries(o)); + + // The card's own reading, to scale: 100s predicted, 269s measured = 2.69x. + const drifted = driftReport(measuredMap({ slow: 269 }), driftTimings); + check(() => { + if (!drifted.drifted) { + throw new Error(`drift: a ${drifted.ratio?.toFixed(2)}x gap was not reported as drift`); + } + }); + check(() => { + if (Math.abs(drifted.ratio - 2.69) > 1e-9) throw new Error(`drift: ratio was ${drifted.ratio}`); + }); + + // A suite that ran a little long is NOT drift -- the bound sits above + // MAX_SHARD_OVER_MEAN precisely so ordinary runner variance stays green. + check(() => { + if (driftReport(measuredMap({ slow: 140 }), driftTimings).drifted) { + throw new Error('drift: a 1.4x reading red under a 1.5x bound'); + } + }); + // The boundary itself: `>` not `>=`, so exactly at the bound is still green. + check(() => { + if (driftReport(measuredMap({ slow: 150 }), driftTimings).drifted) { + throw new Error('drift: a reading exactly AT the bound was called a breach'); + } + }); + + // A package the dataset has never seen is weighed by ESTIMATE in + // weighPackage(), so charging the estimate to the dataset would red on a new + // package instead of on a rotted entry. Excluded from both totals, named. + const withNew = driftReport(measuredMap({ slow: 100, 'brand-new': 900 }), driftTimings); + check(() => { + if (withNew.drifted) throw new Error('drift: an UNMEASURED package was charged to the dataset'); + }); + check(() => { + if (withNew.unpredicted.join() !== 'brand-new') { + throw new Error(`drift: unpredicted was ${withNew.unpredicted.join()}`); + } + }); + + // The mirror restriction: a dataset entry this shard never ran must not + // inflate the predicted side. `fine` and `zero` are in driftTimings and not + // in the summary; if they counted, 100/150 would read as a fast shard and + // vouch for the dataset. + check(() => { + if (driftReport(measuredMap({ slow: 269 }), driftTimings).predictedTotal !== 100) { + throw new Error('drift: a package this shard never ran inflated the predicted total'); + } + }); + + // Infinity is a red naming no cause. A shard carrying only zero-weight + // entries -- and a shard carrying nothing at all -- is NOT MEASURED, which is + // neither a pass nor a failure. + check(() => { + const z = driftReport(measuredMap({ zero: 30 }), driftTimings); + if (z.measurable || z.drifted || z.ratio !== null) { + throw new Error(`drift: a zero predicted total produced ratio ${z.ratio}`); + } + }); + + // THE ONE THAT MATTERS FOR THE READING, and the reason this reuses the + // generator's extractor instead of parsing summaries a second time: a cache + // HIT replays a stored log in milliseconds. Read as a measurement it says the + // suite got ~1000x FASTER than predicted -- a shard that would vouch, loudly + // and in the wrong direction, for whatever the dataset happens to say. Both + // legs go through samplesFromSummary(), which drops replays and failures. + const replayed = samplesFromSummary( + { tasks: [ + { taskId: 'slow#test', task: 'test', package: 'slow', cache: { status: 'HIT' }, + execution: { startTime: 0, endTime: 40, exitCode: 0 } }, + { taskId: 'fine#test', task: 'test', package: 'fine', cache: { status: 'MISS' }, + execution: { startTime: 0, endTime: 200_000, exitCode: 1 } }, + ] }, + 'drift pin' + ); + check(() => { + const r = driftReport(replayed.samples, driftTimings); + if (r.measurable) { + throw new Error('drift: a summary of one replay and one failure was read as a measurement'); + } + }); + + // -- FILE-LEVEL SLICE ITEMS (#16173) ------------------------------------ + // + // The grammar, its refusals, and the two joins that read it. The balancing + // pins above prove the sliced split BALANCES; these prove a slice is a thing + // the rest of the pipeline can carry -- printed, parsed back, charged the + // right prediction, and never doubled onto one shard. + battery('file-level slice items (#16173)'); + + check(() => { + const it = parseShardItem('@objectstack/spec'); + if (it.name !== '@objectstack/spec' || it.slice !== null) { + throw new Error(`item grammar: a bare package name parsed as ${JSON.stringify(it)}`); + } + }); + check(() => { + const it = parseShardItem('@objectstack/cli 2/3'); + if (it.name !== '@objectstack/cli' || it.slice.index !== 2 || it.slice.count !== 3) { + throw new Error(`item grammar: a slice parsed as ${JSON.stringify(it)}`); + } + }); + check(() => { + // Loud, not lenient: `3/2` is a caller bug, and a slice silently clamped or + // read as a package name would schedule vitest to run nothing. + let threw = false; + try { + parseShardItem('@objectstack/cli 3/2'); + } catch { + threw = true; + } + if (!threw) throw new Error('item grammar: an out-of-range slice index was accepted'); + }); + check(() => { + const label = formatShardItem('@objectstack/cli', { index: 1, count: 2 }); + if (label !== '@objectstack/cli 1/2') throw new Error(`item grammar: formatted as ${JSON.stringify(label)}`); + const back = parseShardItem(label); + if (back.name !== '@objectstack/cli' || back.slice.index !== 1 || back.slice.count !== 2) { + throw new Error('item grammar: format -> parse did not round-trip'); + } + }); + check(() => { + if (formatShardItem('@objectstack/spec', null) !== '@objectstack/spec') { + throw new Error('item grammar: an unsliced item did not print as a bare package name'); + } + }); + + check(() => { + if (sliceCountFor('@objectstack/spec') !== 1) { + throw new Error('slice count: a package outside FILE_SHARDED_PACKAGES was sliced'); + } + }); + check(() => { + if (sliceCountFor('@objectstack/cli') !== FILE_SHARDED_PACKAGES['@objectstack/cli']) { + throw new Error('slice count: the configured package did not read its configured count'); + } + }); + check(() => { + // A1: the ONE objection this file records against vitest --shard, made + // unreachable by construction. Slicing below the file count is refused, not + // clamped -- vitest hard-fails there and --passWithNoTests turns that into + // every slice running nothing. + let message = ''; + try { + sliceCountFor('@objectstack/cli', 1); + } catch (err) { + message = err.message; + } + if (!message.includes('owns 1 test file(s)')) { + throw new Error(`slice floor: slicing below the test-file count was not refused (${message || 'no throw'})`); + } + }); + check(() => { + if (sliceCountFor('@objectstack/cli', 500) !== FILE_SHARDED_PACKAGES['@objectstack/cli']) { + throw new Error('slice floor: a package with plenty of test files was refused'); + } + }); + + check(() => { + const [only] = expandSlices([{ name: 'plain', weight: 12 }]); + if (only.name !== 'plain' || only.slice !== null || only.pkg !== 'plain' || only.weight !== 12) { + throw new Error(`expandSlices: an unsliced item came back as ${JSON.stringify(only)}`); + } + }); + check(() => { + const out = expandSlices([{ name: '@objectstack/cli', weight: 1200, sliceCount: 3 }]); + const labels = out.map((i) => i.name).join(', '); + if (labels !== '@objectstack/cli 1/3, @objectstack/cli 2/3, @objectstack/cli 3/3') { + throw new Error(`expandSlices: produced ${labels}`); + } + if (out.some((i) => i.weight !== 400 || i.pkg !== '@objectstack/cli')) { + throw new Error('expandSlices: a slice did not carry an even share of the whole weight, or lost its package'); + } + }); + check(() => { + // The invariant that keeps the mean honest: slicing redistributes weight, + // it never creates or destroys any. A split whose total moved would change + // the bound every other pin is measured against. + const before = [{ name: '@objectstack/cli', weight: 1231.52, sliceCount: 4 }, { name: 'x', weight: 7 }]; + const total = (list) => list.reduce((s, i) => s + i.weight, 0); + if (Math.abs(total(expandSlices(before)) - total(before)) > 1e-9) { + throw new Error('expandSlices: the total weight changed'); + } + }); + + check(() => { + let threw = false; + try { + assertSlicesSpread([{ total: 0, names: ['@objectstack/cli 1/2', '@objectstack/cli 2/2'] }]); + } catch { + threw = true; + } + if (!threw) throw new Error('slice spread: two slices of one package shared a bin and were accepted'); + }); + check(() => { + assertSlicesSpread([ + { total: 0, names: ['@objectstack/cli 1/2', '@objectstack/spec'] }, + { total: 0, names: ['@objectstack/cli 2/2'] }, + ]); + }); + + const sliceTimings = { packages: { '@objectstack/cli': 1200, other: 100 }, rate: 2 }; + check(() => { + const n = FILE_SHARDED_PACKAGES['@objectstack/cli']; + if (predictedSecondsFor('@objectstack/cli', sliceTimings) !== 1200 / n) { + throw new Error('prediction: a sliced package was charged its WHOLE dataset entry'); + } + }); + check(() => { + if (predictedSecondsFor('other', sliceTimings) !== 100) { + throw new Error('prediction: an unsliced package was divided'); + } + }); + check(() => { + // END-TO-END, and the case that says why the two above matter. A shard that + // ran one slice measured 1000s against a 600s slice prediction -- drift. Had + // the slice been charged the whole 1200s entry the same reading would have + // come back 0.83x, i.e. a real overshoot presented as a comfortable + // under-run, and any genuine drift elsewhere on that shard diluted with it. + const measured = measuredMap({ '@objectstack/cli': 1000 }); + const r = driftReport(measured, sliceTimings); + if (!r.drifted) { + throw new Error(`drift: a sliced overshoot read ${r.ratio.toFixed(2)}x and was not reported as drift`); + } + if (Math.abs(r.predictedTotal - 1200 / FILE_SHARDED_PACKAGES['@objectstack/cli']) > 1e-9) { + throw new Error(`drift: the slice was predicted ${r.predictedTotal}s, not its slice share`); + } + }); + + check(() => { + // The run wins over the config. An OBSERVED whole run of a configured-sliced + // package is predicted the WHOLE entry, so the same 1000s reading is a + // comfortable under-run rather than the 1.67x above. Without this, anyone + // running the suite locally (where the CLI runs whole) would get a drift red + // that is purely predictedSecondsFor's arithmetic. + const r = driftReport(measuredMap({ '@objectstack/cli': 1000 }), sliceTimings, undefined, new Map()); + if (r.drifted) { + throw new Error(`drift: an observed WHOLE run was charged a slice-sized prediction (${r.ratio.toFixed(2)}x)`); + } + if (Math.abs(r.predictedTotal - 1200) > 1e-9) { + throw new Error(`drift: an observed whole run was predicted ${r.predictedTotal}s, not the whole 1200s`); + } + }); + check(() => { + // ...and an observed slice count that differs from the configured one is + // honoured, because the summary is the record of what actually ran. + const observed = new Map([['@objectstack/cli', { index: 1, count: 3 }]]); + const r = driftReport(measuredMap({ '@objectstack/cli': 400 }), sliceTimings, undefined, observed); + if (Math.abs(r.predictedTotal - 400) > 1e-9) { + throw new Error(`drift: an observed 1/3 slice was predicted ${r.predictedTotal}s, not 400s`); + } + }); + + check(() => { + // The REAL weighing path, on the REAL package: main() must hand partition() + // slices, not one CLI-shaped lump. Pin 6 above proves weighItems reads + // durations; this proves it splits the one package that has to be split. + const n = FILE_SHARDED_PACKAGES['@objectstack/cli']; + const { weighted, packages } = weighItems( + [{ name: '@objectstack/cli', path: 'packages/cli' }], + new Set(), + loadTimings(), + 'slice pin' + ); + if (packages !== 1 || weighted.length !== n) { + throw new Error(`weighItems: ${packages} package(s) produced ${weighted.length} item(s), expected ${n}`); + } + if (!weighted.every((i) => i.slice && i.pkg === '@objectstack/cli')) { + throw new Error('weighItems: the CLI reached partition() unsliced'); + } + }); + // -- The floor: every declared battery RAN, and ran its cases (#13489) ---- // // Evaluated after every battery has had its chance and BEFORE the verdict, so @@ -741,7 +1403,8 @@ function selfTest() { } console.log( - `partition-test-shards: self-test OK (${datasetItems.length} measured packages, ${SHARD_COUNT} shards, ` + + `partition-test-shards: self-test OK (${datasetPackages.length} measured packages ` + + `-> ${datasetItems.length} shard items, ${SHARD_COUNT} shards, ` + `max/mean ${balance.ratio.toFixed(2)}x <= ${MAX_SHARD_OVER_MEAN}x, floor ${balance.floor.toFixed(0)}s, ` + `bins ${balance.totals.map((t) => t.toFixed(0)).join('/')}s)` ); @@ -749,6 +1412,101 @@ function selfTest() { return SELF_TEST_VERDICT; } +// `--check-drift`: the shard just measured itself, so read that back. +// +// Every verdict this prints is one of exactly three, and NOT MEASURED is a +// first-class one rather than a quiet pass. A shard whose test tasks were all +// cache replays has said nothing about the dataset, and reporting that as OK is +// the #4690 shape -- a check that read nothing reporting as a check that found +// nothing wrong. +function checkDrift(argv) { + const inputs = []; + let label = 'this shard'; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === '--check-drift') continue; + else if (arg === '--label') label = argv[++i]; + else if (arg.startsWith('--')) throw new Error(`unrecognized argument: ${arg}`); + else inputs.push(arg); + } + if (inputs.length === 0) { + console.error( + 'usage: partition-test-shards.mjs --check-drift ... [--label ]' + ); + process.exit(1); + } + // A package normally appears in exactly one summary -- it runs on exactly one + // shard -- so this merge is for the case where it does not (a directory + // holding several runs, or every shard's summary handed over at once). The + // LONGEST window wins, deliberately unlike the generator's median: the + // generator is choosing a weight to balance FUTURE splits with, where one + // unlucky leg must not ratchet the dataset upward forever, while this is + // asking whether a shard fits inside a wall, and the leg that answers that is + // the slow one. + const merged = new Map(); + // What the summaries say each package was RUN as. A shard that carries a + // file-level slice writes two summaries -- one per turbo invocation -- and + // only the slice leg's tasks carry `--shard=k/n`, so this is per package and + // comes from the run rather than from FILE_SHARDED_PACKAGES. A package absent + // here ran whole; that is a reading, not a default. + const observedSlices = new Map(); + for (const input of inputs) { + const { samples, slices } = samplesFromSummary(JSON.parse(readFileSync(input, 'utf8')), input); + for (const [name, seconds] of samples) { + merged.set(name, Math.max(merged.get(name) ?? 0, seconds)); + } + for (const [name, slice] of slices ?? []) observedSlices.set(name, slice); + } + const timings = loadTimings(); + const report = driftReport(merged, timings, MAX_MEASURED_OVER_PREDICTED, observedSlices); + const skipped = + report.unpredicted.length === 0 + ? '' + : ` ${report.unpredicted.length} package(s) carry no dataset entry and were excluded ` + + `(estimated, not predicted): ${report.unpredicted.join(', ')}.`; + + if (!report.measurable) { + console.error( + `shard-timing-drift: NOT MEASURED -- ${label} finished no test task that was both a cache ` + + 'MISS and carried a dataset entry, so this run says nothing about whether ' + + `scripts/test-shard-timings.json is still true.${skipped}` + ); + return; + } + + const head = + `${report.measuredTotal.toFixed(1)}s measured vs ${report.predictedTotal.toFixed(1)}s predicted ` + + `across ${report.rows.length} package(s) = ${report.ratio.toFixed(2)}x ` + + `(bound ${MAX_MEASURED_OVER_PREDICTED}x)`; + const worst = report.rows + .slice(0, 5) + .map( + (r) => + ` ${r.name}: predicted ${r.predicted.toFixed(1)}s, measured ${r.measured.toFixed(1)}s ` + + `(${r.predicted > 0 ? `${(r.measured / r.predicted).toFixed(2)}x, ` : ''}` + + `${r.overshoot >= 0 ? '+' : ''}${r.overshoot.toFixed(1)}s)` + ) + .join('\n'); + + if (!report.drifted) { + console.error(`shard-timing-drift: OK -- ${label}, ${head}.${skipped}`); + return; + } + console.error( + `shard-timing-drift: DRIFT -- ${label}, ${head}.${skipped}\n` + + ' Heaviest overshoots:\n' + + `${worst}\n` + + ' scripts/test-shard-timings.json no longer describes this workspace, so the shard split\n' + + ' is balancing a quantity that is not the runtime. Refresh it -- see\n' + + ' scripts/measure-test-shard-timings.mjs for the two refresh paths -- and ⛔ do NOT\n' + + ' hand-edit the dataset or raise this bound to absorb the gap. Expect the refresh to red\n' + + " this script's own balance pins if a single suite has outgrown the acceptance bound:\n" + + ' that is those pins working, and the remedy they name is splitting that suite below\n' + + ' package granularity, never a different shard count.' + ); + process.exit(1); +} + function main() { const argv = process.argv.slice(2); if (argv.includes('--self-test')) { @@ -762,6 +1520,10 @@ function main() { } return; } + if (argv.includes('--check-drift')) { + checkDrift(argv); + return; + } let listPath = null; let shardSpec = null; @@ -785,8 +1547,8 @@ function main() { const parsed = JSON.parse(readFileSync(listPath, 'utf8')); const items = readPackageItems(parsed, listPath); const timings = loadTimings(); - const { weighted, estimated } = weighItems(items, excluded, timings, listPath); - const bins = partition(weighted, shardCount); + const { weighted, estimated, packages } = weighItems(items, excluded, timings, listPath); + const bins = assertSlicesSpread(partition(weighted, shardCount)); const mine = bins[shardIndex - 1]; const { max, mean, ratio } = balanceOf(bins); // Printed on every shard, not just the imbalanced one, and printed as the @@ -794,10 +1556,11 @@ function main() { // said whether the split was balanced, which is why #10472's imbalance had to // be found by reading six job durations side by side after the fact. console.error( - `shard ${shardSpec}: ${mine.names.length}/${weighted.length} packages, ` + + `shard ${shardSpec}: ${mine.names.length}/${weighted.length} items ` + + `(${weighted.length - packages} of them file-level slices of ${packages} package(s)), ` + `${mine.total.toFixed(1)}s predicted (all bins: ${bins.map((b) => b.total.toFixed(0)).join('/')}s; ` + `max/mean ${ratio.toFixed(2)}x of the ${MAX_SHARD_OVER_MEAN}x bound, max ${max.toFixed(0)}s, mean ${mean.toFixed(0)}s; ` + - `${weighted.length - estimated} measured, ${estimated} estimated from test-file count)` + `${packages - estimated} measured, ${estimated} estimated from test-file count)` ); for (const name of mine.names) console.log(name); }