Skip to content

Commit 9df0b71

Browse files
committed
ci(test-core): read an OS_TEST_SHARD slice back out of turbo run summaries
samplesFromSummary resolves the sha256 digest turbo records for OS_TEST_SHARD against the k/n the partitioner can emit for that package, keeps the cliArguments (passthrough) path, and refuses a digest that matches no candidate instead of reading the window as a whole-package sample. Comments that described the passthrough carrier are made true. Claude-Session: https://claude.ai/code/session_014EJ1ED8X4MMrT18BhVx4tx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 276b359 commit 9df0b71

4 files changed

Lines changed: 244 additions & 29 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -556,14 +556,19 @@ jobs:
556556
echo 'Items on this shard (a package name, or a package plus a k/n file-level slice):'
557557
cat "$RUNNER_TEMP/shard-packages.txt"
558558
559-
# ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN A RUN THAT
560-
# CARRIES NO PASSTHROUGH, so that the sharded run in the next step can be
561-
# `--only` (#16395).
562-
#
563-
# Turbo folds a run-level passthrough into the hash of EVERY task in the
564-
# run, not only the task that receives it -- and `-- "--shard=k/n"` is the
565-
# whole reason a slice gets its own invocation at all (the next step's
566-
# comment says why it cannot ride the shared run). Measured on turbo
559+
# ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN ITS OWN
560+
# GUARDED STEP, so the slice leg in the next step REPLAYS it. Since #19278
561+
# that leg carries its slice in `OS_TEST_SHARD`, which only the `test`
562+
# task declares, so its build tasks hash exactly as they do here (61 of 61
563+
# identical, measured) and it schedules its own closure: this step is no
564+
# longer what makes the leg correct, only what keeps the closure build off
565+
# the test step's stall-guard site (see the last paragraph below).
566+
#
567+
# Why the step first existed (#16395, when the slice was still a
568+
# passthrough and the leg was `--only`): turbo folds a run-level
569+
# passthrough into the hash of EVERY task in the run, not only the task
570+
# that receives it -- and `-- "--shard=k/n"` was then the whole reason a
571+
# slice got its own invocation at all. Measured on turbo
567572
# 2.10.10, `--filter=@objectstack/cli`, `turbo run test ... --dry=json`
568573
# (60 tasks: 59 `build` + 1 `test`):
569574
#
@@ -748,11 +753,12 @@ jobs:
748753
# package's own files plus the hand-declared `$TURBO_ROOT$`
749754
# inputs. A slice in the affected set (on a PR, queue or push run
750755
# this shard's set IS the affected set) could then match a
751-
# main-seeded entry across the very change that put it there. Run 34746808828 did: it replayed
752-
# `@objectstack/cli:test` on this leg -- `Cached: 1 cached, 1
753-
# total`, `73ms >>> FULL TURBO` -- out of a log a main push run
754-
# had produced ~15 minutes earlier, on a commit that did not
755-
# contain the PR under test. All six shards reported success,
756+
# main-seeded entry across the very change that put it there.
757+
# Run 34746808828 did: it replayed `@objectstack/cli:test` on
758+
# this leg -- `Cached: 1 cached, 1 total`, `73ms >>> FULL
759+
# TURBO` -- out of a log a main push run had produced ~15
760+
# minutes earlier, on a commit that did not contain the PR under
761+
# test. All six shards reported success,
756762
# `check-test-completeness` graded the replay OK, the shard
757763
# attestation said "ran to completion", and the red reached
758764
# `main`. Neither of those two can tell a run from a replay.
@@ -790,7 +796,11 @@ jobs:
790796
# never execute), where the passthrough without `--only` was 62 MISS.
791797
# The step above is no longer load-bearing for correctness -- this
792798
# run schedules its own closure -- but it keeps that build on its
793-
# own stall-guard site, and here it replays.
799+
# own stall-guard site, and here it replays. `--summarize` records
800+
# the slice only as the sha256 of `OS_TEST_SHARD` (`cliArguments`
801+
# is empty); measure-test-shard-timings.mjs resolves that digest
802+
# against the k/n the partitioner can emit and refuses one it
803+
# cannot, so a slice is never timed as the whole package.
794804
set -- env "OS_TEST_SHARD=$SLICE" pnpm turbo run test "--filter=$PKG" --concurrency=4 --summarize --log-order=stream
795805
fi
796806
LOGS="$LOGS $LOG"

‎scripts/measure-test-shard-timings.mjs‎

Lines changed: 206 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,12 +81,17 @@
8181
// --run <id> <summary.json>... [--merge-into <dataset>] [--out <path>]
8282
// node scripts/measure-test-shard-timings.mjs --self-test
8383

84+
import { createHash } from 'node:crypto';
8485
import { existsSync, readFileSync, readdirSync, writeFileSync } from 'node:fs';
8586
import path from 'node:path';
8687
import { fileURLToPath } from 'node:url';
8788
import process from 'node:process';
8889

89-
import { countTestFiles } from './partition-test-shards.mjs';
90+
// FILE_SHARDED_PACKAGES and SLICE_ENV are read only inside functions: this
91+
// import is circular (the partitioner imports samplesFromSummary from here), so
92+
// a top-level read would meet an uninitialised binding when the partitioner is
93+
// the entry point.
94+
import { countTestFiles, FILE_SHARDED_PACKAGES, SLICE_ENV } from './partition-test-shards.mjs';
9095
import { isEntrypoint } from './invoked-as.mjs';
9196

9297
const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
@@ -140,6 +145,79 @@ export function sliceOfCliArguments(args) {
140145
return null;
141146
}
142147

148+
// Which file-level slice a task ran as when the slice travelled in
149+
// `OS_TEST_SHARD` rather than as a passthrough (#19278), or null.
150+
//
151+
// Test Core's slice leg now runs `OS_TEST_SHARD=k/n turbo run test` with NO
152+
// passthrough -- a passthrough is folded into the hash of every task in the run,
153+
// which forced `--only`, which dropped the build closure out of the test's hash
154+
// -- so `cliArguments` is `[]` on that leg and sliceOfCliArguments() above sees
155+
// a WHOLE package. turbo does record the variable, but only as a digest: the
156+
// task's `environmentVariables.configured` carries `OS_TEST_SHARD=` followed by
157+
// the sha256 hex of the value (measured on turbo 2.10.10, executed summaries
158+
// and `--dry=json` alike: `1/16` -> `ece2d97a…`, `1/2` -> `d939926f…`), and
159+
// `OS_TEST_SHARD=` with nothing after it when the value is empty.
160+
//
161+
// So the digest is matched against every slice the partitioner CAN emit for
162+
// that package, and nothing else: `k/n` for 1 <= k <= n, where n is
163+
// FILE_SHARDED_PACKAGES[package] from partition-test-shards.mjs. The bound is
164+
// that map -- n candidates for a package it slices (2 for @objectstack/cli
165+
// today), none for one it does not.
166+
//
167+
// ⛔ A digest that matches no candidate is REFUSED, never read as a whole
168+
// package. That is the #16173 direction exactly: one slice's window recorded as
169+
// the package's whole cost, a number that reads right and is n times too small.
170+
// It happens when FILE_SHARDED_PACKAGES changed since the run that wrote the
171+
// summary, or when the variable was set by hand to a slice the partitioner does
172+
// not emit; either way the remedy is a summary from a run under the current map.
173+
//
174+
// An empty value is not a slice: the package's vitest config hands vitest an
175+
// empty `shard`, which it ignores, and the package runs whole. A task with no
176+
// `environmentVariables` record at all carries no evidence either way and is
177+
// read as carrying no env slice -- turbo 2.10.10 writes one on every task, and
178+
// the self-test fixtures predate it.
179+
export function sliceOfEnvironment(environmentVariables, name, label, sliced = FILE_SHARDED_PACKAGES) {
180+
const configured = environmentVariables?.configured;
181+
if (configured === undefined || configured === null) return null;
182+
if (!Array.isArray(configured)) {
183+
throw new Error(`${label}: environmentVariables.configured is not an array -- did the summary format change?`);
184+
}
185+
const prefix = `${SLICE_ENV}=`;
186+
const entry = configured.find((e) => typeof e === 'string' && e.startsWith(prefix));
187+
if (entry === undefined) return null;
188+
const digest = entry.slice(prefix.length);
189+
if (digest === '') return null;
190+
const count = Object.hasOwn(sliced, name) ? sliced[name] : 1;
191+
const candidates = [];
192+
for (let index = 1; count > 1 && index <= count; index++) {
193+
const spec = `${index}/${count}`;
194+
if (createHash('sha256').update(spec).digest('hex') === digest) return { index, count };
195+
candidates.push(spec);
196+
}
197+
throw new Error(
198+
`${label}: ${name} ran with ${SLICE_ENV} set (digest ${digest.slice(0, 16)}…), and that digest ` +
199+
`matches no slice the partitioner can emit for it (${candidates.length ? candidates.join(', ') : 'none: FILE_SHARDED_PACKAGES does not slice it'}). ` +
200+
'Refusing to read the window as a whole-package sample: it is one slice of the suite, and ' +
201+
'recording it as the whole cost is the #16173 defect. If FILE_SHARDED_PACKAGES changed since ' +
202+
'this run, refresh from a run made under the current map.'
203+
);
204+
}
205+
206+
// The slice one measured leg ran as, from either carrier. Both present and
207+
// disagreeing is not a reading to pick from: vitest would have run the CLI's,
208+
// but no workflow here sets both, so it is refused rather than resolved.
209+
function sliceOfLeg(leg, name, label, sliced) {
210+
const fromArgs = sliceOfCliArguments(leg.cliArguments);
211+
const fromEnv = sliceOfEnvironment(leg.environmentVariables, name, label, sliced);
212+
if (fromArgs && fromEnv && (fromArgs.index !== fromEnv.index || fromArgs.count !== fromEnv.count)) {
213+
throw new Error(
214+
`${label}: ${name} carries two different slices -- --shard=${fromArgs.index}/${fromArgs.count} ` +
215+
`in cliArguments and ${fromEnv.index}/${fromEnv.count} in ${SLICE_ENV}. Refusing to guess.`
216+
);
217+
}
218+
return fromArgs ?? fromEnv;
219+
}
220+
143221
// The task names whose windows this file counts as a package's test cost.
144222
// #16550: since #16466, six packages (core, objectql, rest, runtime, spec,
145223
// types) split their suite into `test` and `test:repo` (the repo-scanning
@@ -172,7 +250,7 @@ const SAMPLED_TASKS = ['test', 'test:repo'];
172250
// Recording one leg's seconds alone (because the other was cached or failed)
173251
// would write a partial suite's cost as the package's whole cost, a reading
174252
// worse than today's undercount by #16466's own defect this card fixes.
175-
export function samplesFromSummary(parsed, label) {
253+
export function samplesFromSummary(parsed, label, sliced = FILE_SHARDED_PACKAGES) {
176254
const tasks = parsed?.tasks;
177255
if (!Array.isArray(tasks)) {
178256
throw new Error(
@@ -182,7 +260,7 @@ export function samplesFromSummary(parsed, label) {
182260
}
183261
// One entry per package, holding whichever of its sampled tasks this
184262
// summary carries (almost always just `test`; `test` + `test:repo` for a
185-
// split package). Each leg is recorded as EITHER a seconds+cliArguments
263+
// split package). Each leg is recorded as EITHER a seconds+slice-carrier
186264
// reading, OR a `cached`/`failed` flag -- never both -- so the fold below
187265
// can tell "this leg disqualifies the package" from "this leg is a real
188266
// measurement" without re-reading the raw task.
@@ -211,7 +289,7 @@ export function samplesFromSummary(parsed, label) {
211289
}
212290
const seconds = (endTime - startTime) / 1000;
213291
if (!(seconds >= 0)) throw new Error(`${label}: ${name}#${taskName} measured ${seconds}s`);
214-
legs.set(taskName, { seconds, cliArguments: task.cliArguments });
292+
legs.set(taskName, { seconds, cliArguments: task.cliArguments, environmentVariables: task.environmentVariables });
215293
}
216294

217295
const samples = new Map();
@@ -230,13 +308,14 @@ export function samplesFromSummary(parsed, label) {
230308
}
231309
if (readings.some((leg) => leg.failed)) continue;
232310
let seconds = 0;
233-
let cliArguments;
311+
let slice = null;
234312
for (const leg of readings) {
235313
seconds += leg.seconds;
236-
cliArguments ??= leg.cliArguments;
314+
// A passthrough slice (`cliArguments`, the nightly tiers) or an
315+
// `OS_TEST_SHARD` one (Test Core, #19278) -- see sliceOfEnvironment().
316+
slice ??= sliceOfLeg(leg, name, label, sliced);
237317
}
238318
samples.set(name, seconds);
239-
const slice = sliceOfCliArguments(cliArguments);
240319
if (slice) slices.set(name, slice);
241320
}
242321
return { samples, skippedCached, slices };
@@ -480,11 +559,12 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n
480559
// remedy is to find what stopped registering, never to lower the number.
481560
const SELF_TEST_BATTERIES = Object.freeze({
482561
'measure-test-shard-timings self-test': 56,
562+
'env-carried slices (#19278)': 10,
483563
});
484564

485565
// DELETING an entry silences that battery's floor exactly as effectively as
486566
// zeroing it, so the roster's own size is pinned too.
487-
const SELF_TEST_BATTERY_FLOOR = 1;
567+
const SELF_TEST_BATTERY_FLOOR = 2;
488568

489569
// The key an assertion is filed under when no battery is open. It is not a
490570
// declared battery, so it reds by the same set difference rather than silently
@@ -1066,6 +1146,124 @@ function selfTest() {
10661146
if (flat === null || path.basename(flat) !== 'spec') throw new Error('workspace: a depth-1 package stopped resolving');
10671147
});
10681148

1149+
// -- ENV-CARRIED SLICES (#19278) --------------------------------------------
1150+
//
1151+
// Test Core's slice leg carries k/n in OS_TEST_SHARD, which a summary records
1152+
// only as a sha256 digest in `environmentVariables.configured`. Each case
1153+
// below fails in the #16173 direction if the digest path is dropped: a slice
1154+
// read as a whole package. `sliced` stands in for FILE_SHARDED_PACKAGES so the
1155+
// fixtures keep the short names above, and one case reads the REAL map.
1156+
battery('env-carried slices (#19278)');
1157+
const digestOf = (value) => createHash('sha256').update(value).digest('hex');
1158+
const envTask = (pkg, start, end, value, extra = {}) => ({
1159+
...testTask(pkg, start, end),
1160+
cliArguments: [],
1161+
environmentVariables: {
1162+
specified: { env: ['OS_TEST_SHARD', 'OS_TEST_TIERS'], passThroughEnv: null },
1163+
configured: [`OS_TEST_SHARD=${value === '' ? '' : digestOf(value)}`],
1164+
inferred: [],
1165+
passthrough: null,
1166+
},
1167+
...extra,
1168+
});
1169+
const map3 = Object.freeze({ cli: 3 });
1170+
1171+
check(() => {
1172+
const r = samplesFromSummary(summary([envTask('cli', 0, 118_073, '2/3')]), 'f', map3);
1173+
const s = r.slices.get('cli');
1174+
if (!s || s.index !== 2 || s.count !== 3) {
1175+
throw new Error(`env slice: an OS_TEST_SHARD digest was read as ${JSON.stringify(s ?? null)}, not 2/3`);
1176+
}
1177+
if (r.samples.get('cli') !== 118.073) throw new Error('env slice: the slice window was not kept as the sample');
1178+
});
1179+
check(() => {
1180+
// The REAL map, the real package name, a digest of the form turbo writes.
1181+
const n = FILE_SHARDED_PACKAGES['@objectstack/cli'];
1182+
const r = samplesFromSummary(summary([envTask('@objectstack/cli', 0, 1000, `${n}/${n}`)]), 'f');
1183+
const s = r.slices.get('@objectstack/cli');
1184+
if (!s || s.index !== n || s.count !== n) {
1185+
throw new Error(`env slice: the live FILE_SHARDED_PACKAGES did not resolve ${n}/${n} (got ${JSON.stringify(s ?? null)})`);
1186+
}
1187+
});
1188+
check(() => {
1189+
// The passthrough carrier is untouched: the nightly tiers still use it.
1190+
const r = samplesFromSummary(summary([slicedTask('cli', 0, 400_000, 1, 3)]), 'f', map3);
1191+
const s = r.slices.get('cli');
1192+
if (!s || s.index !== 1 || s.count !== 3) throw new Error('env slice: the cliArguments carrier stopped working');
1193+
});
1194+
check(() => {
1195+
// ⛔ The refusal: a digest no candidate matches must never become a sample.
1196+
let message = '';
1197+
try {
1198+
samplesFromSummary(summary([envTask('cli', 0, 118_073, '1/16')]), 'f', map3);
1199+
} catch (e) {
1200+
message = e.message;
1201+
}
1202+
if (!message.includes('matches no slice the partitioner can emit') || !message.includes('1/3, 2/3, 3/3')) {
1203+
throw new Error(`env slice: an unmatched digest was not refused naming its candidates (got ${JSON.stringify(message)})`);
1204+
}
1205+
});
1206+
check(() => {
1207+
// A package the partitioner never slices has NO candidates, so any slice
1208+
// digest on it is refused as well.
1209+
if (!threw(() => samplesFromSummary(summary([envTask('a', 0, 10_000, '1/2')]), 'f', map3))) {
1210+
throw new Error('env slice: a slice digest on a package the partitioner does not slice was accepted');
1211+
}
1212+
});
1213+
check(() => {
1214+
// A refusal is a refusal of the whole summary: buildDataset never sees it.
1215+
if (!threw(() =>
1216+
buildDataset({
1217+
perSummary: [samplesFromSummary(summary([envTask('cli', 0, 400_000, '9/9')]), 'f', map3)],
1218+
fileCounts: new Map([['cli', 300]]),
1219+
provenance: {},
1220+
})
1221+
)) {
1222+
throw new Error('env slice: an unmatched digest reached the dataset');
1223+
}
1224+
});
1225+
check(() => {
1226+
// An empty value is no slice: vitest ignores an empty `shard`.
1227+
const r = samplesFromSummary(summary([envTask('cli', 0, 10_000, '')]), 'f', map3);
1228+
if (r.slices.size !== 0 || r.samples.get('cli') !== 10) {
1229+
throw new Error('env slice: an empty OS_TEST_SHARD was read as a slice');
1230+
}
1231+
});
1232+
check(() => {
1233+
// Other declared variables are not the slice.
1234+
const task = {
1235+
...testTask('cli', 0, 10_000),
1236+
environmentVariables: { configured: [`OS_TEST_TIERS=${digestOf('queue')}`] },
1237+
};
1238+
if (samplesFromSummary(summary([task]), 'f', map3).slices.size !== 0) {
1239+
throw new Error('env slice: an unrelated configured variable was read as a slice');
1240+
}
1241+
});
1242+
check(() => {
1243+
const agree = envTask('cli', 0, 10_000, '1/3', { cliArguments: ['--shard=1/3'] });
1244+
const s = samplesFromSummary(summary([agree]), 'f', map3).slices.get('cli');
1245+
if (!s || s.index !== 1) throw new Error('env slice: two agreeing carriers were not read as that slice');
1246+
const disagree = envTask('cli', 0, 10_000, '2/3', { cliArguments: ['--shard=1/3'] });
1247+
if (!threw(() => samplesFromSummary(summary([disagree]), 'f', map3))) {
1248+
throw new Error('env slice: two disagreeing carriers were resolved instead of refused');
1249+
}
1250+
});
1251+
check(() => {
1252+
// The load-bearing case, env-carried: three slices are ONE package.
1253+
const ds = buildDataset({
1254+
perSummary: [
1255+
samplesFromSummary(summary([envTask('cli', 0, 400_000, '1/3')]), 'f', map3),
1256+
samplesFromSummary(summary([envTask('cli', 0, 380_000, '2/3')]), 'g', map3),
1257+
samplesFromSummary(summary([envTask('cli', 0, 420_000, '3/3'), testTask('a', 0, 10_000)]), 'h', map3),
1258+
],
1259+
fileCounts: new Map([['cli', 300], ['a', 5]]),
1260+
provenance: {},
1261+
});
1262+
if (ds.packages.cli !== 1200) {
1263+
throw new Error(`env slice: three env-carried slices summed to ${ds.packages.cli}, expected 1200`);
1264+
}
1265+
});
1266+
10691267
// -- The floor: every declared battery RAN, and ran its cases (#13489) ----
10701268
//
10711269
// Evaluated after every battery has had its chance and BEFORE the verdict, so

‎scripts/partition-test-shards.mjs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,9 @@ export const MAX_MEASURED_OVER_PREDICTED = 1.5;
190190
// suite below package granularity.
191191
//
192192
// THIS IS THAT SPLIT, and it is the shape the Dogfood job has run since #4859:
193-
// vitest's own `--shard=k/n` passthrough applied to ONE named package. The
193+
// vitest's own `--shard=k/n` applied to ONE named package (carried to it in
194+
// `OS_TEST_SHARD` rather than as a passthrough since #19278 -- see SLICE_ENV
195+
// below; the argument that follows is about vitest's shard, not the carrier). The
194196
// objection this file records against passthrough is specific and it does not
195197
// reach here -- `--shard` on a package with fewer test files than the shard
196198
// count hard-fails on vitest 4, and `--passWithNoTests` converts that into
@@ -1639,7 +1641,8 @@ function checkDrift(argv) {
16391641
const merged = new Map();
16401642
// What the summaries say each package was RUN as. A shard that carries a
16411643
// file-level slice writes two summaries -- one per turbo invocation -- and
1642-
// only the slice leg's tasks carry `--shard=k/n`, so this is per package and
1644+
// only the slice leg's tasks carry the slice (an `OS_TEST_SHARD` digest, or
1645+
// `--shard=k/n` on a passthrough run), so this is per package and
16431646
// comes from the run rather than from FILE_SHARDED_PACKAGES. A package absent
16441647
// here ran whole; that is a reading, not a default.
16451648
const observedSlices = new Map();

0 commit comments

Comments
 (0)