diff --git a/.github/workflows/shard-timings-refresh.yml b/.github/workflows/shard-timings-refresh.yml index 2b997d121b1..5e45a000268 100644 --- a/.github/workflows/shard-timings-refresh.yml +++ b/.github/workflows/shard-timings-refresh.yml @@ -98,6 +98,28 @@ # accumulated runs gets the median of three observations, which is the property # the dataset's own merge rule always claimed and could not previously deliver. # +# ⚠ AND THE ACCUMULATION USED TO STOP AT ONE (#22014). The loop broke at the +# first accumulation that COVERED the workspace — and coverage counts a carried +# weight as covered, so the newest run always covered it. Every refresh since +# the hourly run went live was therefore one run: the dataset on `main` named +# `provenance.runs = ['37262126122']`, and recorded `@objectstack/spec` at +# 1134.86s against 1573-1651s on the executed windows after it. The loop now +# stops only when every package has been EXECUTED by at least the generator's +# MINIMUM_EXECUTED_RUNS (three) of the accumulated runs — executed, not +# replayed: an hourly run replays a different part of the workspace each time, +# so three runs is not three samples of `spec`. Measured over the 20 eligible +# hourly runs retained on 2026-10-06 (17:01 the day before to 15:01): every one +# of the 72 packages was executed by at least three of them, the newest twelve +# already sufficed, and `spec` was the tightest at 7 of 20. +# +# A refresh whose retained runs cannot give some package three executions is +# NOT refused: it is opened PROVISIONAL, with those packages named in the +# dataset's `provisional` list and in the PR body. Refusing would discard every +# other package's measurement over one under-sampled one — the silent-rot +# failure this lane exists to end — while the list keeps the shortfall +# machine-readable for anything that needs a baseline rather than a balancing +# input (a per-package ceiling must refuse a provisional weight). +# # ⚠ AND ACCUMULATION ALONE STILL FALLS SHORT, so the pass MERGES rather than # replaces. Measured on the same live runs: the union of all seven retained runs # reaches 57 of 71 packages and converges there, because 14 packages are cache @@ -266,11 +288,12 @@ jobs: # 24, not 15 (#16467). The window that matters is the artifact # retention window — 1 day — and the runs inside it are now the # hourly scheduled ones, so 24 is "everything still downloadable" - # rather than an arbitrary depth. Coverage is ACCUMULATED across - # runs (see the next step), so examining fewer than the window holds - # is coverage left on the table; the cost is two API reads per run - # examined and the loop below still stops at the first accumulation - # that covers the workspace. + # rather than an arbitrary depth. Coverage and sample depth are both + # ACCUMULATED across runs (see the next step), so examining fewer + # than the window holds is samples left on the table; the cost is two + # API reads per run examined, and the loop below stops at the first + # accumulation in which every package has been executed by the + # minimum number of runs (#22014). # # ⭐ THREE EXITS, THREE READINGS (#16467). The selector answers 0, 1 # or 3, and this step must not flatten them: @@ -369,9 +392,16 @@ jobs: # observations rather than whichever run happened to be read last. Feeding # several runs WITHOUT that grouping is refused by the generator by name. # - # The loop stops at the first accumulation that covers the workspace, so a - # quiet week costs one run's download and a busy one costs a few. - - name: Regenerate the dataset, accumulating runs until the workspace is covered + # WHEN IT STOPS (#22014). Not at the first accumulation that covers the + # workspace: coverage counts a carried weight as covered, so the newest + # run alone always passed it and every refresh was one run deep. The loop + # stops at the first accumulation that BOTH covers the workspace AND + # leaves the dataset's `provisional` list empty — every package executed + # by at least the generator's MINIMUM_EXECUTED_RUNS of the runs fed. + # When the candidates run out first, the last covering accumulation is + # kept and opened PROVISIONAL, its short packages named; only a coverage + # shortfall is still a refusal. + - name: Regenerate the dataset, accumulating runs until every package is measured enough times id: generate # Skipped on the NOT MEASURED reading: there is nothing to download. # Every step after `compare` is already gated on @@ -384,7 +414,11 @@ jobs: set -euo pipefail WORK="$RUNNER_TEMP/refresh" mkdir -p "$WORK" - CHOSEN='' + # Both judged on the LAST accumulation the generator accepted, which + # is the one `refreshed.json` holds: a run the generator refuses is + # dropped and leaves the previous accumulation's file in place. + COVERED='' + SHORT='' ACCEPTED=() RUN_COUNT=$(node -e 'console.log(JSON.parse(require("fs").readFileSync(process.env.RUNNER_TEMP + "/candidates.json", "utf8")).length)') @@ -445,9 +479,13 @@ jobs: # HIT witnesses that its inputs are unchanged; anything else is left # out for the coverage check below to name. The workflow still writes # no byte the generator did not emit — the merge happens inside it. + # `--spread-out`: every package's executed-run count and + # min/median/max, for the PR body. Written beside the dataset, + # never into it — nothing machine-reads a spread. if ! node scripts/measure-test-shard-timings.mjs "${ARGS[@]}" \ --merge-into scripts/test-shard-timings.json \ - --out "$WORK/refreshed.json"; then + --out "$WORK/refreshed.json" \ + --spread-out "$WORK/spread.md"; then echo "::warning::The generator refused the set including run $RUN_ID; dropping that run and continuing." unset 'ACCEPTED[-1]' echo "::endgroup::"; continue @@ -459,17 +497,41 @@ jobs: --refreshed "$WORK/refreshed.json" \ --workspace "$RUNNER_TEMP/turbo-ls.json" \ --exclude @objectstack/dogfood; then - CHOSEN="$RUN_ID" + COVERED=1 + else + COVERED='' + fi + # How many weights still rest on fewer than the minimum executed + # runs. Read from the generator's own `provisional` list rather than + # recounted here, and an absent list is an error, not a zero: a + # zero read from nothing would end the loop at one run again. + SHORT=$(node -e ' + const d = JSON.parse(require("fs").readFileSync(process.argv[1], "utf8")); + if (!Array.isArray(d.provisional)) throw new Error(process.argv[1] + " carries no `provisional` list"); + console.log(d.provisional.length); + ' "$WORK/refreshed.json") + if [ -n "$COVERED" ] && [ "$SHORT" -eq 0 ]; then + echo "Every package is covered and executed by at least the minimum number of these runs; stopping." echo "::endgroup::" break fi + if [ -n "$COVERED" ]; then + echo "Covered, but $SHORT package weight(s) rest on fewer than the minimum executed runs; adding the next older run." + fi echo "::endgroup::" done - if [ -z "$CHOSEN" ]; then + if [ -z "$COVERED" ]; then echo "::error::The ${#ACCEPTED[@]} eligible run(s) on main, accumulated and merged with the committed dataset, still leave a package that HAD a measured weight with neither a fresh measurement nor a turbo cache HIT to witness that it is unchanged — the shortfall above names them. Each would drop to the test-file-count ESTIMATE, which is the silent degradation this lane exists to prevent. NOTHING was regenerated and no PR was opened: this is a refusal, not a quiet success. A package that merely went unmeasured is NOT this error — that case is carried on its cache-hit witness — so a shortfall here means a suite failed, a package was renamed or removed, or its slices could not be assembled in any run." exit 1 fi + # PROVISIONAL, not refused (#22014): the candidates ran out before + # every package reached the minimum. The measurement is kept, the + # short packages are named in the dataset and the PR body, and this + # annotation makes the reading visible on a green job. + if [ "$SHORT" -gt 0 ]; then + echo "::warning::Shard timings PROVISIONAL: $SHORT package weight(s) rest on fewer than the minimum executed runs across all ${#ACCEPTED[@]} retained run(s). They are named in the dataset's \`provisional\` list; read them as balancing inputs, not baselines." + fi # Candidates arrive newest-first, so ACCEPTED[0] is the most recent run # in the set and its commit is the one the refresh is dated from. The # full list travels alongside it: every run in it contributed @@ -553,6 +615,25 @@ jobs: } ') + # Sample depth (#22014), read out of the dataset like the carry line + # above. An absent list is an error rather than "none provisional". + DEPTH_LINE=$(node -e ' + const d = JSON.parse(require("fs").readFileSync(process.env.RUNNER_TEMP + "/refresh/refreshed.json", "utf8")); + if (!Array.isArray(d.provisional)) throw new Error("the refreshed dataset carries no `provisional` list"); + const min = d.provenance.minimumRuns; + const total = Object.keys(d.packages).length; + if (d.provisional.length === 0) { + console.log(`Every one of the ${total} weights rests on at least ${min} executed runs; none is provisional.`); + } else { + console.log( + `⚠️ PROVISIONAL: ${d.provisional.length} of the ${total} weights rest on fewer than ${min} executed ` + + `runs, because the retained runs replayed them more often than they executed them. They are ` + + `named in the dataset \`provisional\` list and are fit to balance on, NOT to set a per-package ` + + `ceiling from: ${d.provisional.join(", ")}.` + ); + } + ') + SHARDS=$(node -e ' const rs = JSON.parse(require("fs").readFileSync(process.env.RUNNER_TEMP + "/candidates.json", "utf8")); const r = rs.find((x) => String(x.run_id) === process.env.RUN_ID); @@ -576,8 +657,8 @@ jobs: echo "per shard and only main pushes write it, so a" echo "package whose inputs have not changed is a HIT and the generator refuses hits rather" echo "than recording a replay as a duration. Runs are therefore accumulated, each fenced by" - echo "its own \`--run\` group, until every package the committed dataset holds is measured" - echo "again; a package seen in several of them gets the median of those observations." + echo "its own \`--run\` group, until every package has been EXECUTED by at least the" + echo "generator's minimum number of them, and each weight is the median of those executions." echo for R in $RUNS; do echo "- https://github.com/$GITHUB_REPOSITORY/actions/runs/$R" @@ -594,6 +675,12 @@ jobs: echo "because a weekly lane cannot know which cards a given run ought to retire. If this is" echo "the first refresh to land, retire those two by hand as part of merging it." echo + echo "## Sample depth and run-to-run spread" + echo + echo "$DEPTH_LINE" + echo + cat "$RUNNER_TEMP/refresh/spread.md" + echo echo "## Measured per-shard suite time on the newest run in the set" echo echo "\`\`\`" diff --git a/scripts/ci/select-shard-timings-run.mjs b/scripts/ci/select-shard-timings-run.mjs index 4d75df1177e..36f28e1d39f 100644 --- a/scripts/ci/select-shard-timings-run.mjs +++ b/scripts/ci/select-shard-timings-run.mjs @@ -90,10 +90,20 @@ // partitioner already estimated them and this refresh did not change that. import { spawnSync } from 'node:child_process'; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import process from 'node:process'; +import { fileURLToPath } from 'node:url'; import { EXIT_FINDINGS, EXIT_PREREQUISITE_NOT_MET } from '../import-prerequisite.mjs'; import { isEntrypoint } from '../invoked-as.mjs'; @@ -225,6 +235,14 @@ export function runIsEligible({ jobs, artifacts }, shardCount = SHARD_COUNT) { // cannot be a regression — but it is NAMED rather than passed over // in silence, because "estimated" must never be something a reader // has to infer from an absence. +// +// ⛔ Coverage is NECESSARY, NOT SUFFICIENT, for the refresh to stop adding runs +// (#22014). A carried weight counts as covered, so the newest run alone always +// passes this check, and a loop that stopped here was one run deep on every +// refresh. The regeneration step also waits for the dataset's `provisional` +// list to empty -- every package executed by the generator's +// MINIMUM_EXECUTED_RUNS -- and the 'accumulation depth' battery below drives +// that step to prove it. export function coverageReport({ committed, refreshed, workspace, exclude = [] }) { const excluded = new Set(exclude); const inWorkspace = new Set(workspace); @@ -452,6 +470,10 @@ export async function runCandidates({ repo, token, limit, event, workflow = 'ci. // this self-test. export const REFRESH_WORKFLOW = '.github/workflows/shard-timings-refresh.yml'; +// The regeneration step whose accumulation loop the depth battery drives +// (#22014). Spelled once: the NOT MEASURED battery's YAML half names it too. +export const GENERATE_STEP = 'Regenerate the dataset, accumulating runs until every package is measured enough times'; + // The `run:` script of one named step, de-indented, so the self-test can drive // the real branch instead of reading it. Text rather than a YAML parse: this // file is dependency-free by design, and one block scalar at a known indent is @@ -499,8 +521,13 @@ const SELF_TEST_BATTERIES = Object.freeze({ // `run:` block, lifted out of the YAML and driven under `bash -e` against a // stub `node`. An exit code nothing reads is not a distinction. 'shard-timings-refresh NOT MEASURED path': 18, + // #22014. The regeneration step's own loop, driven like the battery above: + // it must stop at the minimum depth rather than at the first covering run, + // count executed runs rather than runs, and keep a short refresh as + // PROVISIONAL rather than refusing it. + 'shard-timings-refresh accumulation depth': 11, }); -const SELF_TEST_BATTERY_FLOOR = 4; +const SELF_TEST_BATTERY_FLOOR = 5; const UNATTRIBUTED_BATTERY = '(no battery open)'; // Returned by `selfTest()` only after its verdict is printed, so a `return` that @@ -1169,7 +1196,7 @@ async function selfTest() { // carry the guard. Everything after `compare` is already gated on // `steps.compare.outputs.changed`, which is '' when `compare` is skipped. for (const stepName of [ - 'Regenerate the dataset, accumulating runs until the workspace is covered', + GENERATE_STEP, 'Compare against the committed dataset', ]) { await check(() => { @@ -1182,6 +1209,243 @@ async function selfTest() { }); } + // ------------------------------------------------------------------------- + battery('shard-timings-refresh accumulation depth'); + // ------------------------------------------------------------------------- + // #22014. The loop in the regeneration step used to `break` at the first + // accumulation that COVERED the workspace, and coverage counts a carried + // weight as covered -- so the newest run always passed it, and every refresh + // was one run deep. What is pinned here is the LOOP, not a helper: the step's + // own `run:` block is lifted out of the YAML and driven under `bash -e`, with + // `curl` and `unzip` stubbed to serve fixture run summaries and the REAL + // generator and coverage check doing the rest. The tools are reached through + // symlinks from a scratch tree whose `scripts/test-shard-timings.json` is a + // two-package fixture, so nothing in the repository is read as the + // committed dataset or written. + // + // Four legs: depth reached (stops at three, not one), executed-not-replayed + // (a run that replayed a package does not count for it), PROVISIONAL when the + // runs run out (kept and named, not refused), and the coverage refusal left + // exactly as it was. + const generateScript = extractStepScript(REFRESH_YAML, GENERATE_STEP); + const SPEC = '@objectstack/spec'; + const TYPES = '@objectstack/types'; + const toolPath = (rel) => fileURLToPath(new URL(rel, import.meta.url)); + + // runs: newest first, each { id, tasks: [[package, seconds, cacheStatus]] }. + // Every task lands in shard 1's summary; shards 2-6 carry an empty task list, + // which is a valid summary that measures nothing. + const driveGenerate = (runs, committedPackages, workspace = Object.keys(committedPackages)) => { + const dir = mkdtempSync(join(tmpdir(), 'os-timings-generate-')); + try { + const runnerTemp = join(dir, 'runner-temp'); + const bin = join(dir, 'bin'); + const fixtures = join(dir, 'fixtures'); + const tree = join(dir, 'tree'); + for (const d of [runnerTemp, bin, fixtures, join(tree, 'scripts', 'ci')]) mkdirSync(d, { recursive: true }); + symlinkSync(toolPath('../measure-test-shard-timings.mjs'), join(tree, 'scripts', 'measure-test-shard-timings.mjs')); + symlinkSync(toolPath('./select-shard-timings-run.mjs'), join(tree, 'scripts', 'ci', 'select-shard-timings-run.mjs')); + writeFileSync(join(tree, 'scripts', 'test-shard-timings.json'), JSON.stringify({ packages: committedPackages })); + writeFileSync( + join(runnerTemp, 'turbo-ls.json'), + JSON.stringify({ packages: { items: workspace.map((name) => ({ name })) } }) + ); + const candidatesList = runs.map((r) => ({ + run_id: r.id, + head_sha: String(r.id).padStart(40, 'c'), + created_at: '2026-10-06T00:00:00Z', + artifact_ids: Object.fromEntries([1, 2, 3, 4, 5, 6].map((n) => [n, Number(`${r.id}${n}`)])), + })); + writeFileSync(join(runnerTemp, 'candidates.json'), JSON.stringify(candidatesList)); + for (const r of runs) { + for (const n of [1, 2, 3, 4, 5, 6]) { + const tasks = + n !== 1 + ? [] + : r.tasks.map(([pkg, seconds, status = 'MISS']) => ({ + taskId: `${pkg}#test`, + task: 'test', + package: pkg, + cache: { status }, + execution: { startTime: 0, endTime: status === 'MISS' ? seconds * 1000 : 90, exitCode: 0 }, + })); + writeFileSync(join(fixtures, `${r.id}${n}.json`), JSON.stringify({ tasks })); + } + } + // curl: write the artifact id named in the URL into the `-o` file. + writeFileSync( + join(bin, 'curl'), + [ + '#!/usr/bin/env bash', + "out=''; id=''", + 'while [ $# -gt 0 ]; do', + ' case "$1" in', + ' -o) out="$2"; shift 2; continue ;;', + ' */actions/artifacts/*/zip) id="${1%/zip}"; id="${id##*/}" ;;', + ' esac', + ' shift', + 'done', + '[ -n "$out" ] && [ -n "$id" ] || exit 22', + 'printf %s "$id" > "$out"', + ].join('\n') + ); + // unzip: the "zip" holds an artifact id; serve that artifact's fixture. + writeFileSync( + join(bin, 'unzip'), + [ + '#!/usr/bin/env bash', + "zip=''; dest=''", + 'while [ $# -gt 0 ]; do', + ' case "$1" in', + ' -d) dest="$2"; shift 2; continue ;;', + ' -*) ;;', + ' *) zip="$1" ;;', + ' esac', + ' shift', + 'done', + 'mkdir -p "$dest"', + `cp ${JSON.stringify(fixtures)}/"$(cat "$zip")".json "$dest/summary.json"`, + ].join('\n') + ); + chmodSync(join(bin, 'curl'), 0o755); + chmodSync(join(bin, 'unzip'), 0o755); + const outputFile = join(dir, 'github-output'); + writeFileSync(outputFile, ''); + const run = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', generateScript], { + cwd: tree, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: runnerTemp, + GITHUB_OUTPUT: outputFile, + GITHUB_TOKEN: 'stub-token', + GITHUB_REPOSITORY: 'o/r', + }, + }); + const refreshedPath = join(runnerTemp, 'refresh', 'refreshed.json'); + const spreadPath = join(runnerTemp, 'refresh', 'spread.md'); + const output = readFileSync(outputFile, 'utf8'); + return { + status: run.status, + log: `${run.stdout ?? ''}${run.stderr ?? ''}`, + output, + runCount: /^run_count=(\d+)$/m.exec(output)?.[1] ?? null, + refreshed: existsSync(refreshedPath) ? JSON.parse(readFileSync(refreshedPath, 'utf8')) : null, + spread: existsSync(spreadPath) ? readFileSync(spreadPath, 'utf8') : null, + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const committedTwo = { [SPEC]: 1134.86, [TYPES]: 50 }; + const both = (spec, types) => [ + [SPEC, spec], + [TYPES, types], + ]; + + // LEG 1 -- DEPTH REACHED. Four eligible runs, each executing both packages. + // The first already covers the workspace, which is where the old loop + // stopped; the depth rule takes exactly three, and the weight is their median. + const deep = driveGenerate( + [ + { id: 901, tasks: both(1600, 60) }, + { id: 902, tasks: both(1500, 62) }, + { id: 903, tasks: both(1700, 61) }, + { id: 904, tasks: both(9999, 99) }, + ], + committedTwo + ); + await check(() => { + if (deep.status !== 0) throw new Error(`depth leg: the step failed (exit ${deep.status}):\n${deep.log}`); + }); + await check(() => { + if (deep.runCount !== '3') { + throw new Error(`depth leg: expected the loop to stop at 3 runs, it fed ${deep.runCount} (1 is the coverage-only loop this replaced)`); + } + }); + await check(() => { + if (deep.refreshed?.packages?.[SPEC] !== 1600 || deep.refreshed?.provenance?.runs?.join(',') !== '901,902,903') { + throw new Error(`depth leg: spec is not the median of the three newest runs (${JSON.stringify(deep.refreshed?.provenance?.runs)}, ${deep.refreshed?.packages?.[SPEC]})`); + } + }); + await check(() => { + if (!Array.isArray(deep.refreshed?.provisional) || deep.refreshed.provisional.length !== 0) { + throw new Error(`depth leg: a dataset at full depth was marked provisional (${JSON.stringify(deep.refreshed?.provisional)})`); + } + }); + await check(() => { + if (!deep.spread?.includes(`| \`${SPEC}\` | 3 | 1500.00 | 1600.00 | 1700.00 |`)) { + throw new Error(`depth leg: the spread table for the PR body is missing or wrong:\n${deep.spread}`); + } + }); + + // LEG 2 -- EXECUTED, NOT REPLAYED. Run 912 replayed spec, so three runs give + // spec only two samples and the loop has to take a fourth. + const replayed = driveGenerate( + [ + { id: 911, tasks: both(1600, 60) }, + { id: 912, tasks: [[SPEC, 0, 'HIT'], [TYPES, 62]] }, + { id: 913, tasks: both(1500, 61) }, + { id: 914, tasks: both(1700, 63) }, + { id: 915, tasks: both(9999, 99) }, + ], + committedTwo + ); + await check(() => { + if (replayed.status !== 0 || replayed.runCount !== '4') { + throw new Error(`replay leg: expected 4 runs (spec replayed in one of the first three), got ${replayed.runCount} (exit ${replayed.status}):\n${replayed.log}`); + } + }); + await check(() => { + if (replayed.refreshed?.provisional?.length !== 0 || replayed.refreshed?.packages?.[SPEC] !== 1600) { + throw new Error(`replay leg: spec is not the median of its three executions (${replayed.refreshed?.packages?.[SPEC]}, ${JSON.stringify(replayed.refreshed?.provisional)})`); + } + }); + + // LEG 3 -- PROVISIONAL, NOT REFUSED. Only two runs are retained. The step + // stays green, keeps the measurement, names both packages, and says so on + // the job with an annotation. + const shallow = driveGenerate( + [ + { id: 921, tasks: both(1600, 60) }, + { id: 922, tasks: both(1500, 62) }, + ], + committedTwo + ); + await check(() => { + if (shallow.status !== 0 || shallow.runCount !== '2') { + throw new Error(`provisional leg: two runs were refused or not fed (exit ${shallow.status}, runs ${shallow.runCount}):\n${shallow.log}`); + } + }); + await check(() => { + if (shallow.refreshed?.provisional?.join(',') !== `${SPEC},${TYPES}` || shallow.refreshed?.packages?.[SPEC] !== 1550) { + throw new Error(`provisional leg: the shallow dataset was not kept and named (${JSON.stringify(shallow.refreshed?.provisional)}, ${shallow.refreshed?.packages?.[SPEC]})`); + } + }); + await check(() => { + if (!shallow.log.includes('::warning::Shard timings PROVISIONAL')) { + throw new Error('provisional leg: no annotation, so a provisional refresh reads as an ordinary green job'); + } + }); + + // LEG 4 -- CONTROL: the coverage refusal is untouched. A package that HAD a + // weight and that no run measured or replayed still refuses the refresh, so + // the depth rule cannot have turned every shortfall into "provisional". + const lost = driveGenerate( + [ + { id: 931, tasks: both(1600, 60) }, + { id: 932, tasks: both(1500, 62) }, + { id: 933, tasks: both(1700, 61) }, + ], + { ...committedTwo, '@objectstack/core': 300 } + ); + await check(() => { + if (lost.status !== 1 || !lost.log.includes('::error::') || lost.runCount !== null) { + throw new Error(`coverage control: a lost package did not refuse the refresh (exit ${lost.status}, runs ${lost.runCount}):\n${lost.log}`); + } + }); + // -- The floor: every declared battery ran, and ran its cases. Evaluated // before the verdict, so the success line can only be printed by a run in // which the set of batteries that registered EQUALS the set declared. diff --git a/scripts/measure-test-shard-timings.mjs b/scripts/measure-test-shard-timings.mjs index 2619d974d69..43b32dd6bb1 100644 --- a/scripts/measure-test-shard-timings.mjs +++ b/scripts/measure-test-shard-timings.mjs @@ -49,6 +49,13 @@ // treatment as every other package (#16473); undeclared multi-run input is // REFUSED rather than resolved by guessing. // +// ⚠ AND SEVERAL MEANS THREE PER PACKAGE, NOT THREE RUNS (#22014). A run +// executes a different slice of the workspace each time, so the count that +// matters is how many runs EXECUTED a given package. One with fewer than +// MINIMUM_EXECUTED_RUNS is kept and named in the dataset's `provisional` list; +// `--spread-out` writes every package's run count and min/median/max for the +// refresh PR to carry. +// // `--merge-into` is what makes a partial measurement sound. A package this // pass did not measure keeps its previous weight -- but ONLY when a cache HIT // witnesses that its inputs are unchanged, which is the evidence that the old @@ -78,7 +85,8 @@ // Usage: // node scripts/measure-test-shard-timings.mjs ... [--out ] // node scripts/measure-test-shard-timings.mjs --run ... \ -// --run ... [--merge-into ] [--out ] +// --run ... [--merge-into ] [--out ] \ +// [--spread-out ] // node scripts/measure-test-shard-timings.mjs --self-test import { createHash } from 'node:crypto'; @@ -117,6 +125,36 @@ export function median(values) { return sorted.length % 2 === 1 ? sorted[mid] : (sorted[mid - 1] + sorted[mid]) / 2; } +// The fewest EXECUTED runs a package's weight may rest on before it stops +// being PROVISIONAL (#22014). +// +// "Median, not mean and not max" above is a promise about several samples, and +// for a while the file kept it on paper only: the refresh lane stopped at the +// first run that covered the workspace, so every weight in the dataset was ONE +// observation wearing the median's name. A single run is a sample, not a +// baseline -- measured, `@objectstack/cli` read 1033.74s and 1723.05s an hour +// apart, and `@objectstack/spec` was recorded at 1134.86s while three executed +// windows after it read 1573-1651s. A ceiling set at that weight plus 25% sits +// below every one of them. +// +// Three, and per PACKAGE rather than per refresh, because the unit that varies +// is the package: a scheduled run executes a different slice of the workspace +// each hour (the rest are turbo cache HITs, which this file refuses), so "three +// runs accumulated" can still leave `spec` with one sample if it was replayed in +// the other two. A package is counted once per run that EXECUTED it -- a MISS +// with exit 0, and for a file-sharded package a complete slice set from that +// one run. +// +// ⛔ A weight below this is not REFUSED. It is kept and named in the dataset's +// `provisional` list, beside `carriedOver`: the partitioner still balances on +// it (a fresh single sample is still a measurement of code as it stands), and +// anything that needs a BASELINE rather than a balancing input -- a per-package +// ceiling above all -- reads `provisional` and refuses those entries. Refusing +// the whole refresh instead would throw away every other package's measurement +// over one under-sampled package, which is the silent-rot failure the refresh +// lane exists to end. +export const MINIMUM_EXECUTED_RUNS = 3; + // Which file-level slice a summary's tasks were run as, or null for a whole // package (#16173). // @@ -370,7 +408,12 @@ export function fallbackRate(measured, fileCounts) { return Math.round(median(rates) * 1000) / 1000; } -export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = null }) { +// Every EXECUTED per-run reading of every package, before any median is taken: +// name -> [seconds, ...], one entry per run that executed the package (#22014). +// buildDataset() medians these into the weights; the refresh PR's spread table +// (renderSpread below) prints them, so the two can never be computed from +// different readings. +export function collectSamples(perSummary) { const bySample = new Map(); const cachedNames = new Set(); const push = (name, seconds) => { @@ -481,6 +524,54 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n } } incompleteSlices.sort((a, b) => a.localeCompare(b, 'en')); + return { bySample, cachedNames, incompleteSlices }; +} + +// The packages whose weight rests on fewer than `minimumRuns` executed runs +// (#22014). Two ways in, and both are named rather than inferred: +// +// MEASURED in this pass with fewer readings than the minimum -- the ordinary +// case, a package the accumulated runs mostly replayed. +// +// CARRIED from the prior dataset, when that dataset did not establish that +// the carried weight met the minimum. A carry moves a weight forward +// unchanged, so it moves its depth forward unchanged too: provisional there +// stays provisional here. A prior dataset with no `provisional` list at all +// predates this rule and never recorded a depth -- the one on `main` when it +// landed rested on a single run -- so its carried weights are provisional +// too. ⛔ An unrecorded depth is not evidence of meeting the bar. +export function provisionalPackages({ bySample, carriedOver, carryProvisional, minimumRuns }) { + const names = []; + for (const [name, values] of bySample) { + if (values.length < minimumRuns) names.push(name); + } + if (carriedOver.length > 0) { + if (carryProvisional !== null && !Array.isArray(carryProvisional)) { + throw new Error( + `the prior dataset's \`provisional\` is ${JSON.stringify(carryProvisional)}, not a list of package names. ` + + 'Refusing to guess which carried weights met the minimum.' + ); + } + const priorShort = carryProvisional === null ? null : new Set(carryProvisional); + for (const name of carriedOver) { + if (priorShort === null || priorShort.has(name)) names.push(name); + } + } + return [...new Set(names)].sort((a, b) => a.localeCompare(b, 'en')); +} + +// `carryProvisional` is the prior dataset's own `provisional` list, or null when +// that dataset has none (see provisionalPackages above). It matters only for +// carried packages, so a call with no `carryFrom` never reads it. +export function buildDataset({ + perSummary, + fileCounts, + provenance, + carryFrom = null, + carryProvisional = null, + minimumRuns = MINIMUM_EXECUTED_RUNS, +}) { + const { bySample, cachedNames, incompleteSlices } = collectSamples(perSummary); if (bySample.size === 0) { throw new Error( @@ -534,14 +625,21 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n for (const name of [...measured.keys()].sort((a, b) => a.localeCompare(b, 'en'))) { packages[name] = measured.get(name); } + if (!Number.isInteger(minimumRuns) || minimumRuns < 1) { + throw new Error(`minimumRuns must be a positive integer, got ${JSON.stringify(minimumRuns)}`); + } return { note: 'GENERATED by scripts/measure-test-shard-timings.mjs -- do not hand-edit. Per-package ' + '`turbo run test` durations in seconds, the balancing input for the Test Core shard ' + 'split (scripts/partition-test-shards.mjs). See `provenance.refresh` to regenerate. ' + 'Every weight is a real measurement: the ones in `carriedOver` were measured by an earlier ' + - 'refresh and re-confirmed unchanged by a turbo cache HIT in this one.', - provenance, + 'refresh and re-confirmed unchanged by a turbo cache HIT in this one. A weight is the median ' + + 'of the executed runs that measured it; the ones in `provisional` rest on fewer than ' + + '`provenance.minimumRuns` of them -- fit to balance on, NOT a baseline to set a ceiling from.', + // The bar `provisional` was judged against travels with the list, so a + // reader never has to know which version of this script wrote the file. + provenance: { ...provenance, minimumRuns }, secondsPerTestFileFallback: fallbackRate(measured, fileCounts), packages, skippedAsCached: [...cachedNames].sort((a, b) => a.localeCompare(b, 'en')), @@ -554,9 +652,70 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n // `provenance.measuredAt` paired with this list carries the same // information and needs no change in the partitioner. carriedOver, + // #22014. The same shape and the same reason as `carriedOver`: a list + // beside the weights, never a change to them, because the partitioner and + // the drift gate read `packages` as name -> number. Always present -- an + // empty list and an absent key must not read the same way, since an absent + // key is exactly how a dataset that predates this rule is recognised. + provisional: provisionalPackages({ bySample, carriedOver, carryProvisional, minimumRuns }), }; } +// The run-to-run spread of every package, as the Markdown table the refresh +// PR's body carries (#22014). One row per package in the dataset: +// +// package | runs | min | median | max | max/min +// +// where `runs` counts the executed runs behind the weight and the three +// seconds columns are those runs' readings. A carried package has no reading in +// this pass, so it says `carried` instead of inventing numbers; a provisional +// one is marked in its own column. Rows sort by max/min, widest first, so the +// noisiest suites are the ones a reviewer meets first. +// +// ⛔ Kept OUT of the dataset file. Nothing machine-reads a spread -- the +// partitioner and the drift gate read the weights, and the one machine fact a +// baseline needs (did this weight meet the minimum?) is `provisional` -- so the +// numbers go where their only reader is: the PR a person reviews. +export function renderSpread({ bySample, dataset }) { + const fmt = (n) => n.toFixed(2); + const provisional = new Set(dataset.provisional); + const rows = []; + for (const name of Object.keys(dataset.packages)) { + const values = bySample.get(name); + const flag = provisional.has(name) ? 'provisional' : ''; + if (!values) { + rows.push({ ratio: -1, name, cells: [`\`${name}\``, '0', 'carried', 'carried', 'carried', '—', flag] }); + continue; + } + const lo = Math.min(...values); + const hi = Math.max(...values); + const ratio = lo > 0 ? hi / lo : Number.POSITIVE_INFINITY; + rows.push({ + ratio, + name, + cells: [ + `\`${name}\``, + String(values.length), + fmt(lo), + fmt(Math.round(median(values) * 100) / 100), + fmt(hi), + Number.isFinite(ratio) ? `${ratio.toFixed(2)}×` : '∞', + flag, + ], + }); + } + rows.sort((a, b) => b.ratio - a.ratio || a.name.localeCompare(b.name, 'en')); + const minimumRuns = dataset.provenance?.minimumRuns; + const lines = [ + `Each package's executed runs in this refresh (minimum ${minimumRuns}), and the spread of its readings in seconds.`, + '', + '| package | runs | min s | median s | max s | max/min | |', + '|---|--:|--:|--:|--:|--:|---|', + ...rows.map((r) => `| ${r.cells.join(' | ')} |`), + ]; + return `${lines.join('\n')}\n`; +} + // -- The self-test's own battery roster and floor (#13489) ------------------ // // `--self-test` reaching its verdict used to be this self-test's ONLY success @@ -587,11 +746,13 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n const SELF_TEST_BATTERIES = Object.freeze({ 'measure-test-shard-timings self-test': 56, 'env-carried slices (#19278)': 11, + // #22014: the per-package minimum of executed runs, and the spread table. + 'minimum executed runs per package': 14, }); // 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 = 2; +const SELF_TEST_BATTERY_FLOOR = 3; // 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 @@ -1355,6 +1516,162 @@ function selfTest() { } }); + // -- MINIMUM EXECUTED RUNS PER PACKAGE (#22014) ---------------------------- + // + // The pin triage asked for: a refresh carrying fewer than the minimum runs is + // REPORTED AS PROVISIONAL, never refused and never passed off as a baseline. + // Every case below fails one of three ways if the rule is dropped -- a short + // weight not named, a full one named, or the measurement thrown away -- and + // the counting cases separate RUNS from EXECUTED RUNS, which is the whole + // defect: the dataset on `main` when this landed carried one run. + battery('minimum executed runs per package'); + const runOf = (run, tasks) => ({ ...samplesFromSummary(summary(tasks), `${run}`), run }); + const depthSet = (runs, extra = {}) => + buildDataset({ + perSummary: runs, + fileCounts: new Map([['a', 5], ['b', 5], ['cli', 300]]), + provenance: {}, + ...extra, + }); + + check(() => { + // The ruled floor is "at least three". Lowering it is a ruling, not a tune. + if (!(MINIMUM_EXECUTED_RUNS >= 3)) throw new Error(`depth: MINIMUM_EXECUTED_RUNS is ${MINIMUM_EXECUTED_RUNS}, below the ruled three`); + }); + check(() => { + // THE DEFECT'S OWN SHAPE: one run, everything executed. Kept, and named. + const one = depthSet([runOf('37262126122', [testTask('a', 0, 10_000), testTask('b', 0, 20_000)])]); + if (one.packages.a !== 10 || one.packages.b !== 20) throw new Error('depth: a one-run refresh lost its measurement'); + if (one.provisional.join(',') !== 'a,b') { + throw new Error(`depth: a one-run refresh was not reported provisional (${JSON.stringify(one.provisional)})`); + } + }); + check(() => { + // Two runs: still short, and the weight is still the median of what exists. + const two = depthSet([runOf('r1', [testTask('a', 0, 10_000)]), runOf('r2', [testTask('a', 0, 30_000)])]); + if (two.packages.a !== 20 || two.provisional.join(',') !== 'a') { + throw new Error(`depth: two runs read ${two.packages.a}, provisional ${JSON.stringify(two.provisional)}`); + } + }); + check(() => { + // Three runs: the control leg. The list is EMPTY and present. + const three = depthSet([ + runOf('r1', [testTask('a', 0, 10_000)]), + runOf('r2', [testTask('a', 0, 30_000)]), + runOf('r3', [testTask('a', 0, 20_000)]), + ]); + if (three.packages.a !== 20) throw new Error(`depth: three runs did not median to 20 (${three.packages.a})`); + if (!Array.isArray(three.provisional) || three.provisional.length !== 0) { + throw new Error(`depth: a package measured by three runs was reported provisional (${JSON.stringify(three.provisional)})`); + } + }); + check(() => { + // RUNS ARE NOT EXECUTED RUNS. Three runs accumulated, `b` replayed in two + // of them: `a` has met the minimum, `b` has one reading and is named. + const mixed = depthSet([ + runOf('r1', [testTask('a', 0, 10_000), testTask('b', 0, 50_000)]), + runOf('r2', [testTask('a', 0, 11_000), testTask('b', 0, 90, 'HIT')]), + runOf('r3', [testTask('a', 0, 12_000), testTask('b', 0, 90, 'HIT')]), + ]); + if (mixed.provisional.join(',') !== 'b') { + throw new Error(`depth: three runs were counted as three samples of a package two of them replayed (${JSON.stringify(mixed.provisional)})`); + } + }); + check(() => { + // A failed suite is not an execution that counts either. + const red = depthSet([ + runOf('r1', [testTask('a', 0, 10_000)]), + runOf('r2', [testTask('a', 0, 10_000)]), + runOf('r3', [testTask('a', 0, 500, 'MISS', 1)]), + ]); + if (red.provisional.join(',') !== 'a') throw new Error(`depth: a failed run counted toward the minimum (${JSON.stringify(red.provisional)})`); + }); + check(() => { + // A sliced package counts a run only when THAT run assembled every slice: + // two complete runs plus one with a slice missing is two, not three. + const sliceDepth = depthSet([ + runOf('r1', [slicedTask('cli', 0, 300_000, 1, 2)]), + runOf('r1', [slicedTask('cli', 0, 300_000, 2, 2)]), + runOf('r2', [slicedTask('cli', 0, 310_000, 1, 2)]), + runOf('r2', [slicedTask('cli', 0, 310_000, 2, 2)]), + runOf('r3', [slicedTask('cli', 0, 320_000, 1, 2), testTask('a', 0, 10_000)]), + ]); + if (sliceDepth.packages.cli !== 610 || sliceDepth.provisional[0] !== 'a' || !sliceDepth.provisional.includes('cli')) { + throw new Error( + `depth: an incomplete slice set counted toward the minimum (cli ${sliceDepth.packages.cli}, ` + + `provisional ${JSON.stringify(sliceDepth.provisional)})` + ); + } + }); + check(() => { + // The bar travels with the list, so a reader needs no knowledge of which + // version of this script wrote the file. + const withBar = depthSet([runOf('r1', [testTask('a', 0, 10_000)])], { provenance: { measuredAt: 'x' } }); + if (withBar.provenance.minimumRuns !== MINIMUM_EXECUTED_RUNS || withBar.provenance.measuredAt !== 'x') { + throw new Error(`depth: provenance does not record the bar beside the caller's fields (${JSON.stringify(withBar.provenance)})`); + } + }); + check(() => { + // The rule reads the bar it is given -- a minimum of one makes the same + // one-run set non-provisional, so the default is what is being pinned above. + const lax = depthSet([runOf('r1', [testTask('a', 0, 10_000)])], { minimumRuns: 1 }); + if (lax.provisional.length !== 0) throw new Error(`depth: the minimum is not the one passed in (${JSON.stringify(lax.provisional)})`); + }); + // The carry: a weight moved forward unchanged keeps the depth it was measured at. + const carryCase = (carryProvisional) => + depthSet( + [ + runOf('r1', [testTask('a', 0, 10_000), testTask('cached', 0, 90, 'HIT')]), + runOf('r2', [testTask('a', 0, 10_000), testTask('cached', 0, 90, 'HIT')]), + runOf('r3', [testTask('a', 0, 10_000), testTask('cached', 0, 90, 'HIT')]), + ], + { carryFrom: { a: 9, cached: 500 }, carryProvisional } + ); + check(() => { + // A prior dataset with NO `provisional` key never recorded a depth (the one + // this landed on rested on a single run), so its carried weight is not shown + // to meet the minimum. + const legacy = carryCase(null); + if (legacy.packages.cached !== 500 || legacy.provisional.join(',') !== 'cached') { + throw new Error(`depth: a weight carried from a depth-less dataset was not provisional (${JSON.stringify(legacy.provisional)})`); + } + }); + check(() => { + const met = carryCase([]); + if (met.provisional.length !== 0) { + throw new Error(`depth: a weight carried from a dataset that met the minimum was marked provisional (${JSON.stringify(met.provisional)})`); + } + }); + check(() => { + const stillShort = carryCase(['cached']); + if (stillShort.provisional.join(',') !== 'cached') { + throw new Error(`depth: a provisional weight lost the mark by being carried (${JSON.stringify(stillShort.provisional)})`); + } + }); + check(() => { + if (!threw(() => carryCase('cached'))) throw new Error('depth: a malformed prior `provisional` was guessed at instead of refused'); + }); + check(() => { + // The spread table: a measured row carries its run count and min/median/max, + // a carried row says so instead of inventing numbers, the provisional mark + // is in the row, and the noisiest package is the first row. + const runs = [ + runOf('r1', [testTask('a', 0, 10_000), testTask('b', 0, 20_000), testTask('cached', 0, 90, 'HIT')]), + runOf('r2', [testTask('a', 0, 30_000), testTask('cached', 0, 90, 'HIT')]), + runOf('r3', [testTask('a', 0, 20_000), testTask('cached', 0, 90, 'HIT')]), + ]; + const ds = depthSet(runs, { carryFrom: { cached: 500 }, carryProvisional: [] }); + const table = renderSpread({ bySample: collectSamples(runs).bySample, dataset: ds }); + const body = table.split('\n').filter((l) => l.startsWith('| `')); + if (body.length !== 3) throw new Error(`spread: expected one row per package, got ${body.length}:\n${table}`); + if (body[0] !== '| `a` | 3 | 10.00 | 20.00 | 30.00 | 3.00× | |') throw new Error(`spread: the widest row is wrong: ${body[0]}`); + if (!body.some((l) => l.startsWith('| `b` | 1 | 20.00 | 20.00 | 20.00 | 1.00× | provisional |'))) { + throw new Error(`spread: the provisional row is wrong:\n${table}`); + } + if (!body.some((l) => l.startsWith('| `cached` | 0 | carried |'))) throw new Error(`spread: the carried row is wrong:\n${table}`); + if (!table.includes(`minimum ${MINIMUM_EXECUTED_RUNS}`)) throw new Error('spread: the table does not state the minimum'); + }); + // -- The floor: every declared battery RAN, and ran its cases (#13489) ---- // // Evaluated after every battery has had its chance and BEFORE the verdict, so @@ -1469,6 +1786,10 @@ function main() { } let out = DEFAULT_OUT; let mergeInto = null; + // Where the per-package run-to-run spread is written, as Markdown for the + // refresh PR's body (#22014). Optional, and never into the dataset itself -- + // see renderSpread() for why. + let spreadOut = null; // Each input carries the run it belongs to (#16473). `--run ` opens a // group and every summary AFTER it belongs to that run, so one CI run's six // artifacts are named together the way they are fetched together. Summaries @@ -1485,6 +1806,12 @@ function main() { throw new Error('--merge-into needs the path of the dataset to carry unchanged weights from'); } mergeInto = path.resolve(value); + } else if (argv[i] === '--spread-out') { + const value = argv[++i]; + if (typeof value !== 'string' || value.length === 0 || value.startsWith('--')) { + throw new Error('--spread-out needs the path to write the per-package run-to-run spread table to'); + } + spreadOut = path.resolve(value); } else if (argv[i] === '--run') { const value = argv[++i]; if (typeof value !== 'string' || value.length === 0 || value.startsWith('--')) { @@ -1497,7 +1824,7 @@ function main() { if (inputs.length === 0) { console.error( 'usage: measure-test-shard-timings.mjs [--run ] ... ' + - '[--merge-into ] [--out ]' + '[--merge-into ] [--out ] [--spread-out ]' ); process.exit(1); } @@ -1512,6 +1839,7 @@ function main() { // exactly like a freshly measured one, which is what keeps that rate derived // from the numbers actually in the file rather than from a subset of them. let carryFrom = null; + let carryProvisional = null; if (mergeInto !== null) { const prior = JSON.parse(readFileSync(mergeInto, 'utf8')); if (!prior || typeof prior.packages !== 'object' || prior.packages === null) { @@ -1521,6 +1849,10 @@ function main() { ); } carryFrom = prior.packages; + // A carried weight keeps the depth it was measured at (#22014). A prior + // dataset with no `provisional` key never recorded one, which buildDataset + // reads as "not shown to meet the minimum" -- null, deliberately not []. + carryProvisional = Object.hasOwn(prior, 'provisional') ? prior.provisional : null; } const fileCounts = new Map(); @@ -1537,6 +1869,7 @@ function main() { perSummary, fileCounts, carryFrom, + carryProvisional, provenance: { measuredAt: new Date().toISOString().slice(0, 10), summaries: inputs.map((i) => path.basename(i.file)), @@ -1562,11 +1895,15 @@ function main() { '`pnpm exec turbo run test --concurrency=4 --summarize`. ⚠ ONE RUN COVERS ONLY 2-52 of ~71 ' + 'packages depending on cache warmth, so feed SEVERAL runs -- a `--run ` before each ' + 'run\'s summaries is REQUIRED, so a sliced package is assembled per run and then medianed ' + - 'like every other package -- and `--merge-into` to carry the cache-hit remainder. ' + - '`.github/workflows/shard-timings-refresh.yml` does all of this weekly.)', + 'like every other package -- and `--merge-into` to carry the cache-hit remainder. A package ' + + 'measured by fewer than `minimumRuns` executed runs is named in `provisional`, so feed runs until ' + + 'that list is empty. `.github/workflows/shard-timings-refresh.yml` does all of this weekly.)', }, }); writeFileSync(out, `${JSON.stringify(dataset, null, 2)}\n`); + if (spreadOut !== null) { + writeFileSync(spreadOut, renderSpread({ bySample: collectSamples(perSummary).bySample, dataset })); + } const n = Object.keys(dataset.packages).length; const total = Object.values(dataset.packages).reduce((a, b) => a + b, 0); console.error( @@ -1578,6 +1915,17 @@ function main() { // 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. + // Named, never inferred (#22014): a provisional weight balances fine and must + // not be read as a baseline, and the reader who needs to know is the one + // holding this output, not the one who opens the file next month. + if (dataset.provisional.length > 0) { + console.error( + `measure-test-shard-timings: ${dataset.provisional.length} package weight(s) are PROVISIONAL -- ` + + `measured by fewer than ${dataset.provenance.minimumRuns} executed runs (or carried from a ` + + `dataset that never showed they were): ${dataset.provisional.join(', ')}. Feed more runs, or ` + + 'read them as balancing inputs only.' + ); + } if (dataset.skippedIncompleteSlices.length > 0) { console.error( `measure-test-shard-timings: ⚠ ${dataset.skippedIncompleteSlices.length} package(s) reported an ` +