Skip to content

Commit 6a1e382

Browse files
claude[bot]claude
andauthored
ci: red the Test Core shard when its predicted time stops matching the measured one (#16220)
* wip(devx): partition-test-shards gains --check-drift (predicted vs measured) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * wip(devx): wire --check-drift into the Test Core shard job Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * docs(devx): the log-assembly comment names the construct that is actually there Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8adb9e8 commit 6a1e382

4 files changed

Lines changed: 1175 additions & 30 deletions

File tree

.github/workflows/ci.yml

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ jobs:
539539
node scripts/partition-test-shards.mjs "$RUNNER_TEMP/turbo-ls.json" \
540540
--shard ${{ matrix.shard }}/6 --exclude @objectstack/dogfood \
541541
> "$RUNNER_TEMP/shard-packages.txt"
542-
echo "Packages on this shard:"
542+
echo 'Items on this shard (a package name, or a package plus a k/n file-level slice):'
543543
cat "$RUNNER_TEMP/shard-packages.txt"
544544
545545
# --concurrency=4: turbo's default (10) oversubscribes the 4-vCPU
@@ -587,11 +587,69 @@ jobs:
587587
# vitest's own "use the default" signal. Needs turbo.json's
588588
# globalPassThroughEnv entry or turbo strips it — see the script header.
589589
export VITEST_MAX_WORKERS="$(node scripts/vitest-worker-cap.mjs)"
590-
FILTERS=$(sed 's/^/--filter=/' "$RUNNER_TEMP/shard-packages.txt" | tr '\n' ' ')
591590
mkdir -p "$RUNNER_TEMP/stall-reports"
592-
node scripts/run-with-stall-guard.mjs --log "$RUNNER_TEMP/test-core.log" --stall-minutes 10 \
593-
--report-dir "$RUNNER_TEMP/stall-reports" -- \
594-
pnpm turbo run test $FILTERS --concurrency=4 --summarize --log-order=stream
591+
592+
# Split the shard's ITEMS into the whole packages, which share one
593+
# turbo run as they always have, and the file-level slices, which
594+
# cannot: `--shard=k/n` is passed through to vitest by turbo as a
595+
# RUN-level argument, so it would reach every package in the run —
596+
# and on any package with fewer test files than n that is a hard
597+
# vitest failure (or, with --passWithNoTests, silently no tests at
598+
# all). A slice therefore gets its own invocation, filtered to the one
599+
# package the partitioner sliced.
600+
FILTERS=""
601+
SLICES=""
602+
while read -r PKG SLICE; do
603+
[ -n "$PKG" ] || continue
604+
if [ -n "$SLICE" ]; then
605+
SLICES="$SLICES $PKG=$SLICE"
606+
else
607+
FILTERS="$FILTERS --filter=$PKG"
608+
fi
609+
done < "$RUNNER_TEMP/shard-packages.txt"
610+
611+
# Each leg tees to its OWN log — run-with-stall-guard opens the log
612+
# with 'w', so a second leg pointed at one path would truncate the
613+
# first leg's output and the completeness guard below would grade half
614+
# a shard. They are concatenated afterwards, and that concatenation
615+
# happens even when a leg failed, because a red suite is exactly when
616+
# the completeness guard earns its keep.
617+
#
618+
# ⛔ A failing leg STOPS the remaining ones, the same way turbo stops
619+
# scheduling on the first failure inside one run. Carrying on would add
620+
# a second full suite to a job that is already red and already inside a
621+
# 30-minute wall — turning an informative red into a killed job with no
622+
# attestation at all, which is the #16173 failure mode itself.
623+
STATUS=0
624+
LOGS=""
625+
for LEG in __whole__ $SLICES; do
626+
if [ "$LEG" = __whole__ ]; then
627+
[ -n "$FILTERS" ] || continue
628+
LOG="$RUNNER_TEMP/test-core-packages.log"
629+
set -- pnpm turbo run test $FILTERS --concurrency=4 --summarize --log-order=stream
630+
else
631+
PKG="${LEG%%=*}"
632+
SLICE="${LEG#*=}"
633+
LOG="$RUNNER_TEMP/test-core-slice-$(printf '%s' "$PKG" | tr -c 'A-Za-z0-9' '-').log"
634+
set -- pnpm turbo run test "--filter=$PKG" --concurrency=4 --summarize --log-order=stream -- "--shard=$SLICE"
635+
fi
636+
LOGS="$LOGS $LOG"
637+
node scripts/run-with-stall-guard.mjs --log "$LOG" --stall-minutes 10 \
638+
--report-dir "$RUNNER_TEMP/stall-reports" -- "$@" || { STATUS=$?; break; }
639+
done
640+
641+
# Only over logs that EXIST, and via an explicit `if` rather than a
642+
# `&&` chain: a leg whose guard never got far enough to open its log
643+
# would otherwise make `cat` non-zero, and under `set -e` that replaces
644+
# the SUITE's exit status with cat's — the step would report the wrong
645+
# reason for its own red.
646+
: > "$RUNNER_TEMP/test-core.log"
647+
for LOG in $LOGS; do
648+
if [ -f "$LOG" ]; then
649+
cat "$LOG" >> "$RUNNER_TEMP/test-core.log"
650+
fi
651+
done
652+
exit $STATUS
595653
596654
# --summarize above costs nothing at runtime and writes
597655
# `.turbo/runs/<id>.json`: one per-task record with the execution window
@@ -635,6 +693,31 @@ jobs:
635693
if-no-files-found: ignore
636694
retention-days: 1
637695

696+
# ⛔ THE DRIFT STEP IS DELIBERATELY NOT WIRED HERE YET (#16173).
697+
#
698+
# partition-test-shards.mjs carries a fully-tested `--check-drift` mode —
699+
# it reads the summary uploaded above back and reds when a shard's MEASURED
700+
# test total outruns its PREDICTED one past MAX_MEASURED_OVER_PREDICTED. The
701+
# code, its self-tests and its ablation are all on this branch; only this
702+
# invocation waits, and the wait is a SEQUENCING decision, not an oversight.
703+
#
704+
# Why: scripts/test-shard-timings.json is still stale for @objectstack/cli
705+
# (458.15s recorded, 1231.52s measured), so wiring the step today would red
706+
# the shard carrying a CLI slice on every single PR — a true reading, but one
707+
# that blocks everything until the dataset is refreshed. The file-level split
708+
# above already removes the urgent hazard on its own, taking the worst shard
709+
# from ~1445s (80% of this job's 30-minute wall) to ~1059s (59%) with the
710+
# dataset untouched, because the CLI is halved across two runners instead of
711+
# falling on one.
712+
#
713+
# ⇒ The refresh and this step land TOGETHER in the follow-up, in that order.
714+
# The refresh recipe is in PR #16220's body. When it lands, restore a step
715+
# here that runs `--check-drift` over `.turbo/runs/*.json` with
716+
# `--label "Test Core (${{ matrix.shard }}/6)"`, with NO `if:` and NO
717+
# `continue-on-error` (the point is the red), placed ABOVE the attestation
718+
# pair for the #6082 reason documented on the upload above — so a drift red
719+
# also withholds the attestation, which is the fail-closed direction.
720+
638721
# Runs even when the suite failed — that is when it earns its keep. It
639722
# answers TWO questions about a red suite, and needs both to be able to
640723
# say anything at all about a green one.

scripts/check-test-completeness.mjs

Lines changed: 106 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,12 @@ import path from 'node:path';
144144
import { fileURLToPath } from 'node:url';
145145
import process from 'node:process';
146146
import { isEntrypoint } from './invoked-as.mjs';
147+
// The shard-item grammar, from the script that WRITES the scheduled list. A
148+
// second reader here would be a second grammar, and the two would drift apart
149+
// silently -- an unparsed `@objectstack/cli 1/2` reaches describe() as a
150+
// package name the turbo ls document has never heard of, which this guard
151+
// (correctly, for its own contract) refuses the whole shard over.
152+
import { parseShardItem } from './partition-test-shards.mjs';
147153

148154
const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
149155

@@ -298,14 +304,49 @@ export function parseFailedTestPackages(text) {
298304
return failed;
299305
}
300306

307+
/**
308+
* The scheduled list, folded to PACKAGE names.
309+
*
310+
* `shard-packages.txt` holds shard ITEMS, and since #16173 an item can be a
311+
* file-level slice of a package (`@objectstack/cli 1/2`). Q2 is asked per
312+
* package -- did this package report at all on this shard -- and the reported
313+
* set is keyed by the package name turbo prints, which carries no slice, so an
314+
* unfolded item would be a name the `turbo ls` document does not list and the
315+
* shard would be refused whole. Duplicates collapse for the same reason: a
316+
* package is complete when every slice of it scheduled HERE has reported, and
317+
* partition-test-shards.mjs refuses any split that puts two slices of one
318+
* package on one shard, so "every slice here" is "the one slice here".
319+
*/
320+
export function scheduledPackages(lines) {
321+
const out = [];
322+
const seen = new Set();
323+
for (const line of lines) {
324+
const { name } = parseShardItem(line);
325+
if (seen.has(name)) continue;
326+
seen.add(name);
327+
out.push(name);
328+
}
329+
return out;
330+
}
331+
301332
// true = every task turbo scheduled succeeded, so nothing was cancelled.
302333
// false = it stopped early. null = turbo never printed its roster (it died, or
303334
// the log was truncated) -- unknown, and treated as "not completed" by Rule B.
335+
//
336+
// EVERY roster line is folded, not just the last one. A shard's log holds one
337+
// roster per `turbo run` invocation, and since #16173 a shard that carries a
338+
// file-level slice runs TWO (the sliced package needs its own passthrough, which
339+
// turbo applies run-wide). Reading only the last would let a completed second
340+
// invocation vouch for a first one that stopped early -- and Rule B turns that
341+
// into a red on every package the abort left unreached, which is precisely the
342+
// false-red machine this file's header warns about.
304343
export function parseRunCompleted(text) {
305344
let completed = null;
306345
for (const line of text.split('\n')) {
307346
const m = line.match(TASKS);
308-
if (m) completed = Number(m[1]) === Number(m[2]);
347+
if (!m) continue;
348+
const thisRun = Number(m[1]) === Number(m[2]);
349+
completed = completed === null ? thisRun : completed && thisRun;
309350
}
310351
return completed;
311352
}
@@ -596,7 +637,7 @@ function reportVerdict(verdict) {
596637
// not red. A battery BELOW its floor means cases stopped running; the remedy is
597638
// to find what stopped registering.
598639
const SELF_TEST_BATTERIES = Object.freeze({
599-
'check-test-completeness self-test': 67,
640+
'check-test-completeness self-test': 79,
600641
});
601642

602643
// DELETING an entry silences that battery's floor exactly as effectively as
@@ -956,6 +997,63 @@ function selfTest({ quiet = false } = {}) {
956997
'exit codes: the refusal code collides with a verdict code',
957998
);
958999

1000+
// -- File-level slice items in the scheduled list (#16173) ----------------
1001+
//
1002+
// `shard-packages.txt` stopped being a list of package names the day one
1003+
// package started being sharded below package granularity. Both halves below
1004+
// fail in the false-RED direction if they regress, which is the direction
1005+
// this file's header spends its length warning about.
1006+
eq(scheduledPackages(['@objectstack/spec', '@objectstack/core']), ['@objectstack/spec', '@objectstack/core'],
1007+
'scheduled: plain package names were disturbed');
1008+
eq(scheduledPackages(['@objectstack/cli 1/2']), ['@objectstack/cli'],
1009+
'scheduled: a slice did not fold to its package name');
1010+
eq(scheduledPackages(['@objectstack/cli 1/2', '@objectstack/cli 2/2']), ['@objectstack/cli'],
1011+
'scheduled: two slices of one package did not collapse');
1012+
eq(scheduledPackages(['@objectstack/spec', '@objectstack/cli 2/2', '@objectstack/core']),
1013+
['@objectstack/spec', '@objectstack/cli', '@objectstack/core'],
1014+
'scheduled: folding reordered the list');
1015+
// END-TO-END: an unfolded item reaches describe() as a name the turbo ls
1016+
// document cannot resolve, and classifyShard refuses the whole shard over it.
1017+
// This asserts the fold is what stops that, not that describe() got lenient.
1018+
eq(
1019+
classifyShard({
1020+
scheduled: scheduledPackages(['@objectstack/cli 1/2']),
1021+
reported: new Set(['@objectstack/cli']),
1022+
failed: new Set(),
1023+
runCompleted: true,
1024+
describe: describe({ '@objectstack/cli': { hasTestScript: true, testFileCount: 268 } }),
1025+
}).silent,
1026+
[],
1027+
'scheduled: a slice that DID report was still graded silent',
1028+
);
1029+
eq(
1030+
threw(() =>
1031+
classifyShard({
1032+
scheduled: ['@objectstack/cli 1/2'],
1033+
reported: new Set(['@objectstack/cli']),
1034+
failed: new Set(),
1035+
runCompleted: true,
1036+
describe: describe({ '@objectstack/cli': { hasTestScript: true, testFileCount: 268 } }),
1037+
}),
1038+
),
1039+
true,
1040+
'scheduled: an UNFOLDED item was accepted, so the fold is not what makes this work',
1041+
);
1042+
1043+
// -- One roster per turbo invocation, and a shard can now run two (#16173) --
1044+
const roster = (ok, total) => `Tasks: ${ok} successful, ${total} total`;
1045+
eq(parseRunCompleted(roster(4, 4)), true, 'runCompleted: a complete single roster');
1046+
eq(parseRunCompleted(roster(3, 5)), false, 'runCompleted: an incomplete single roster');
1047+
eq(parseRunCompleted('no roster here'), null, 'runCompleted: a log with no roster is unknown, not complete');
1048+
eq(parseRunCompleted(`${roster(4, 4)}\n${roster(2, 2)}`), true,
1049+
'runCompleted: two complete rosters');
1050+
// The load-bearing one: the packages the FIRST invocation never reached would
1051+
// otherwise be charged as silent under Rule B.
1052+
eq(parseRunCompleted(`${roster(3, 9)}\n${roster(1, 1)}`), false,
1053+
'runCompleted: a completed second invocation vouched for a first that stopped early');
1054+
eq(parseRunCompleted(`${roster(9, 9)}\n${roster(0, 1)}`), false,
1055+
'runCompleted: a second invocation that stopped early was overlooked');
1056+
9591057
// ⚠️ The floor is scoped to the LOUD run on purpose. `selfTest({ quiet: true })`
9601058
// also runs on EVERY production invocation of this gate (see `main()`), and
9611059
// there nothing claims a self-test verdict — the floor exists to stop a green
@@ -1059,10 +1157,12 @@ function main() {
10591157
if (scheduledPath) {
10601158
let scheduled;
10611159
try {
1062-
scheduled = readFileSync(scheduledPath, 'utf8')
1063-
.split('\n')
1064-
.map((l) => l.trim())
1065-
.filter(Boolean);
1160+
scheduled = scheduledPackages(
1161+
readFileSync(scheduledPath, 'utf8')
1162+
.split('\n')
1163+
.map((l) => l.trim())
1164+
.filter(Boolean)
1165+
);
10661166
} catch (err) {
10671167
console.error(`check-test-completeness: cannot read ${scheduledPath} -- ${err.message}`);
10681168
process.exit(1);

0 commit comments

Comments
 (0)