From 2688087e62f20ee1a5e96d75b4e92e272664aea5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:02:17 +0000 Subject: [PATCH 01/10] wip(devx): partition-test-shards gains --check-drift (predicted vs measured) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- scripts/partition-test-shards.mjs | 284 +++++++++++++++++++++++++++++- 1 file changed, 283 insertions(+), 1 deletion(-) diff --git a/scripts/partition-test-shards.mjs b/scripts/partition-test-shards.mjs index baa58619b7..67b9b432c1 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,34 @@ 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 the smallest round factor satisfying both ends: +// +// - 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. +export const MAX_MEASURED_OVER_PREDICTED = 1.5; + // Where a `packages.items[].path` actually points. // // The document has two writers -- `turbo ls`, which emits repo-relative paths, @@ -267,6 +307,64 @@ 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) { + 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; + } + const predicted = timings.packages[name]; + 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 -- @@ -347,11 +445,12 @@ const SELF_TEST_BATTERIES = Object.freeze({ // 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, + 'predicted-vs-measured drift (#16173)': 9, }); // 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 = 8; // 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 @@ -693,6 +792,97 @@ 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'); + } + }); + // -- The floor: every declared battery RAN, and ran its cases (#13489) ---- // // Evaluated after every battery has had its chance and BEFORE the verdict, so @@ -749,6 +939,94 @@ 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(); + for (const input of inputs) { + const { samples } = samplesFromSummary(JSON.parse(readFileSync(input, 'utf8')), input); + for (const [name, seconds] of samples) { + merged.set(name, Math.max(merged.get(name) ?? 0, seconds)); + } + } + const timings = loadTimings(); + const report = driftReport(merged, timings); + 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 +1040,10 @@ function main() { } return; } + if (argv.includes('--check-drift')) { + checkDrift(argv); + return; + } let listPath = null; let shardSpec = null; From 90709014aa525993d7102bce5f07fc899fdcc4ba Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:03:08 +0000 Subject: [PATCH 02/10] wip(devx): wire --check-drift into the Test Core shard job Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78e6c8be42..1394c4f31c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -635,6 +635,38 @@ jobs: if-no-files-found: ignore retention-days: 1 + # The durable half of #16173. The summary uploaded above is not only the + # input to the NEXT refresh of scripts/test-shard-timings.json — it is + # this shard measuring itself, right now, against the prediction the + # partition step printed minutes ago. Nothing compared those two numbers, + # and that is the whole defect: the dataset is generated, it rots in one + # direction only (suites get slower, the file stays put), and a shard that + # has drifted heavy reads as perfectly balanced until the job timeout + # kills it. Measured: @objectstack/cli was predicted 458.15s and ran + # 1231.52s — 2.69× — while this job's own max/mean banner read 1.00×. + # + # ⛔ The failure mode being closed is NOT a slow shard. It is that a + # killed shard produces no reading at all while the rollup reads green + # (#16157 remains open on that half), so the cost of letting this rot is a + # PR that lands with a whole shard unmeasured. A comparison that only + # warned would inherit exactly that: something true, printed, and unread. + # + # No `if:` — a suite that already failed must not also be charged with + # drift, and the step is skipped for free when the job is already red. No + # `continue-on-error` either: the point is the red. It sits ABOVE the + # attestation pair for the #6082 reason documented on the upload above — + # anything below that pair can fail a job whose credential already counts + # as a pass — so a drift red also withholds the attestation, which is the + # fail-closed direction. + - name: Check this shard's predicted-vs-measured timing drift + run: | + if ! ls .turbo/runs/*.json > /dev/null 2>&1; then + echo "No turbo run summary — nothing was measured, so there is nothing to compare." + exit 0 + fi + node scripts/partition-test-shards.mjs --check-drift .turbo/runs/*.json \ + --label "Test Core (${{ matrix.shard }}/6)" + # 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. From 2ddbab85c9f0c6377b575eb60cd37da358181904 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:19:29 +0000 Subject: [PATCH 03/10] feat(devx): red the Test Core shard when its predicted time stops matching the measured one scripts/test-shard-timings.json is generated and nothing re-measured it, so it rots in one direction only: suites get slower, the file stays put, and the shard that drifted heavy reads as perfectly balanced right up until the 30-minute wall kills it. Measured on run 34009395649: @objectstack/cli predicted 458.15s, ran 1231.52s (2.69x) while the split's own banner read max/mean 1.00x. partition-test-shards.mjs gains --check-drift, and every Test Core shard now runs it over the summary --summarize has just written. It reuses the generator's samplesFromSummary so a cache replay cannot be read as a fast suite, compares only the intersection of measured-and-predicted packages, and reports NOT MEASURED as its own verdict rather than as a pass. The bound is 1.5x, where the populations separate on green merge_group build 34013842594: five healthy shards at 0.69-1.18x, the drifted one at 1.74x. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- scripts/partition-test-shards.mjs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/partition-test-shards.mjs b/scripts/partition-test-shards.mjs index 67b9b432c1..8505e54ab2 100644 --- a/scripts/partition-test-shards.mjs +++ b/scripts/partition-test-shards.mjs @@ -143,7 +143,22 @@ export const MAX_SHARD_OVER_MEAN = 1.3; // 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 the smallest round factor satisfying both ends: +// 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; @@ -151,6 +166,9 @@ export const MAX_SHARD_OVER_MEAN = 1.3; // 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; // Where a `packages.items[].path` actually points. From 13ec15f41ff1b81f4dc42eaaf8acac81bf3f9e15 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:11:00 +0000 Subject: [PATCH 04/10] feat(devx): shard @objectstack/cli below package granularity as file-level slice items The partitioner's own pin 3 names this remedy: no split at any shard count can bin a package that exceeds 1.3x the mean, and @objectstack/cli measured 1231.52s against a 800.7s post-refresh mean. A shard item is now a package OR a k/n slice of one; the slice count is derived from that measurement rather than picked, and the balancing pins bin the sliced items so the pending dataset refresh lands instead of reding them. The generator gains the other half: turbo records a run's passthrough argv per task as `cliArguments`, so a package's slices are reassembled (summed) into one whole-package weight before the median rule sees it. Without that, the next refresh would record the heaviest suite at 1/n of its real cost. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- scripts/check-test-completeness.mjs | 112 ++++++- scripts/measure-test-shard-timings.mjs | 213 +++++++++++- scripts/partition-test-shards.mjs | 446 ++++++++++++++++++++++++- 3 files changed, 745 insertions(+), 26 deletions(-) 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 8505e54ab2..38ccd58ad5 100644 --- a/scripts/partition-test-shards.mjs +++ b/scripts/partition-test-shards.mjs @@ -171,6 +171,167 @@ export const MAX_SHARD_OVER_MEAN = 1.3; // 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 THIS shard 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. +export function predictedSecondsFor(name, timings) { + return timings.packages[name] / sliceCountFor(name); +} + // Where a `packages.items[].path` actually points. // // The document has two writers -- `turbo ls`, which emits repo-relative paths, @@ -279,18 +440,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: @@ -356,7 +524,9 @@ export function driftReport(measured, timings, factor = MAX_MEASURED_OVER_PREDIC unpredicted.push(name); continue; } - const predicted = timings.packages[name]; + // Through predictedSecondsFor, never the raw dataset entry: a shard that + // ran one SLICE of a file-sharded package was predicted one slice's cost. + const predicted = predictedSecondsFor(name, timings); predictedTotal += predicted; measuredTotal += seconds; rows.push({ name, predicted, measured: seconds, overshoot: seconds - predicted }); @@ -462,13 +632,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)': 18, }); // 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 = 8; +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 @@ -672,9 +847,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`); @@ -710,6 +891,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. @@ -901,6 +1153,172 @@ function selfTest() { } }); + // -- 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 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 @@ -949,7 +1367,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)` ); @@ -1085,8 +1504,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 @@ -1094,10 +1513,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); } From b5c2e07ccee315c25ef30ee89f4d37b3e987a285 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:19:31 +0000 Subject: [PATCH 05/10] feat(devx): ci.yml and the completeness join understand a file-level shard item A shard item is now a package or a package plus a k/n slice. turbo applies a passthrough run-wide, so a slice cannot share an invocation with packages that own fewer test files than n -- the whole packages keep one turbo run and each slice gets its own, filtered to the one package it slices. Each leg tees to its own log because run-with-stall-guard truncates, and the legs are concatenated even when one failed, which is when the completeness guard earns its keep. The scheduled join folds items to package names (an unfolded one reaches describe() as a package the turbo ls document never listed, and the guard refuses the whole shard over it), and parseRunCompleted folds every roster line rather than the last, so a completed second invocation cannot vouch for a first that stopped early. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 60 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1394c4f31c..b1a75ee3ea 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,61 @@ 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 + + if [ -n "$LOGS" ]; then + cat $LOGS > "$RUNNER_TEMP/test-core.log" + fi + exit $STATUS # --summarize above costs nothing at runtime and writes # `.turbo/runs/.json`: one per-task record with the execution window From 4e2869e38c00ab97ebca7e680a3b460465b1610f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:28:36 +0000 Subject: [PATCH 06/10] fix(devx): assembling the shard log must not replace the suite's exit status A leg whose stall guard never got far enough to open its log would make `cat` non-zero, and under `set -e` that becomes the step's exit code -- the step would report the wrong reason for its own red, on exactly the failure path this job exists to describe. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b1a75ee3ea..51019571c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -638,9 +638,16 @@ jobs: --report-dir "$RUNNER_TEMP/stall-reports" -- "$@" || { STATUS=$?; break; } done - if [ -n "$LOGS" ]; then - cat $LOGS > "$RUNNER_TEMP/test-core.log" - fi + # `|| true`, and only over logs that exist: 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 From a039f1e33aa7b22aabb52ac176e2015f17254192 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:31:43 +0000 Subject: [PATCH 07/10] fix(devx): --check-drift predicts from the slice the SUMMARY records, not the config A package's slice count for the drift comparison now comes from the run's own `cliArguments`, so a package the summaries show running WHOLE is charged the whole dataset entry. FILE_SHARDED_PACKAGES stays the default for callers with no run in hand. The two agree on a Test Core shard; only the observed one is right anywhere else -- a developer running the CLI suite locally runs it whole, and charging that a half-sized prediction reported a ~2x drift that was purely this function's arithmetic. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- scripts/partition-test-shards.mjs | 59 ++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/scripts/partition-test-shards.mjs b/scripts/partition-test-shards.mjs index 38ccd58ad5..0f3f943f3a 100644 --- a/scripts/partition-test-shards.mjs +++ b/scripts/partition-test-shards.mjs @@ -321,15 +321,22 @@ export function assertSlicesSpread(bins) { return bins; } -// What THIS shard was predicted to spend on a package it just ran. A shard runs +// 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. -export function predictedSecondsFor(name, timings) { - return timings.packages[name] / sliceCountFor(name); +// +// `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. @@ -514,7 +521,7 @@ export function balanceOf(bins, items = null) { // 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) { +export function driftReport(measured, timings, factor = MAX_MEASURED_OVER_PREDICTED, observedSlices = null) { const rows = []; const unpredicted = []; let predictedTotal = 0; @@ -526,7 +533,12 @@ export function driftReport(measured, timings, factor = MAX_MEASURED_OVER_PREDIC } // Through predictedSecondsFor, never the raw dataset entry: a shard that // ran one SLICE of a file-sharded package was predicted one slice's cost. - const predicted = predictedSecondsFor(name, timings); + // 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 }); @@ -638,7 +650,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ // the measured count for the same reason. 'the balancing pins (#10472)': 21, 'predicted-vs-measured drift (#16173)': 9, - 'file-level slice items (#16173)': 18, + 'file-level slice items (#16173)': 20, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -1300,6 +1312,30 @@ function selfTest() { } }); + 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 @@ -1408,14 +1444,21 @@ function checkDrift(argv) { // 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 } = samplesFromSummary(JSON.parse(readFileSync(input, 'utf8')), input); + 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); + const report = driftReport(merged, timings, MAX_MEASURED_OVER_PREDICTED, observedSlices); const skipped = report.unpredicted.length === 0 ? '' From 3593acda12b095d2b6bbbd173bfa8a9f81c23c66 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:33:50 +0000 Subject: [PATCH 08/10] docs(devx): the log-assembly comment names the construct that is actually there Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 51019571c5..decd38bd29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -638,10 +638,11 @@ jobs: --report-dir "$RUNNER_TEMP/stall-reports" -- "$@" || { STATUS=$?; break; } done - # `|| true`, and only over logs that exist: 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. + # 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 From d911e6b552d40963c485f1b51b00a42a70a1d037 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:47:18 +0000 Subject: [PATCH 09/10] ci(devx): hold the drift step's CI wiring until the dataset refresh lands with it The --check-drift mode, its self-tests and its ablation stay on the branch; only this one invocation waits. With scripts/test-shard-timings.json still stale for @objectstack/cli, wiring it today reds the shard carrying a CLI slice on every PR -- a true reading that blocks everything until the refresh. The file-level split already removes the urgent hazard on its own: the worst shard goes from ~1445s (80% of the 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, and the comment left in place says so rather than leaving the mode looking forgotten. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 52 +++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index decd38bd29..bfa2b9c62e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -693,38 +693,30 @@ jobs: if-no-files-found: ignore retention-days: 1 - # The durable half of #16173. The summary uploaded above is not only the - # input to the NEXT refresh of scripts/test-shard-timings.json — it is - # this shard measuring itself, right now, against the prediction the - # partition step printed minutes ago. Nothing compared those two numbers, - # and that is the whole defect: the dataset is generated, it rots in one - # direction only (suites get slower, the file stays put), and a shard that - # has drifted heavy reads as perfectly balanced until the job timeout - # kills it. Measured: @objectstack/cli was predicted 458.15s and ran - # 1231.52s — 2.69× — while this job's own max/mean banner read 1.00×. + # ⛔ THE DRIFT STEP IS DELIBERATELY NOT WIRED HERE YET (#16173). # - # ⛔ The failure mode being closed is NOT a slow shard. It is that a - # killed shard produces no reading at all while the rollup reads green - # (#16157 remains open on that half), so the cost of letting this rot is a - # PR that lands with a whole shard unmeasured. A comparison that only - # warned would inherit exactly that: something true, printed, and unread. + # 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. # - # No `if:` — a suite that already failed must not also be charged with - # drift, and the step is skipped for free when the job is already red. No - # `continue-on-error` either: the point is the red. It sits ABOVE the - # attestation pair for the #6082 reason documented on the upload above — - # anything below that pair can fail a job whose credential already counts - # as a pass — so a drift red also withholds the attestation, which is the - # fail-closed direction. - - name: Check this shard's predicted-vs-measured timing drift - run: | - if ! ls .turbo/runs/*.json > /dev/null 2>&1; then - echo "No turbo run summary — nothing was measured, so there is nothing to compare." - exit 0 - fi - node scripts/partition-test-shards.mjs --check-drift .turbo/runs/*.json \ - --label "Test Core (${{ matrix.shard }}/6)" - + # 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. From 860e25106153192effa5e0f52b648864016db803 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:54:11 +0000 Subject: [PATCH 10/10] docs(devx): separate the withheld-drift-step note from the completeness guard's The two comment blocks ran together, so the completeness guard's rationale read as part of the drift note. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bfa2b9c62e..08ad09856c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -717,6 +717,7 @@ jobs: # `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.