From fbdd761f955100d6e9321b8dbbaccc2b2b013da7 Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 18 Aug 2026 16:20:17 -0700 Subject: [PATCH 1/2] fix(ci): pin count-invisible heavy files apart, and raise the shard cap with real margin (#847, #848) @test count is a loose proxy for wall-clock cost in general, and blind for a specific shape of file: one whose cost is almost entirely waiting rather than how many @test blocks it contains. Measured 2026-08-19 (run 32193147987): test_remote_engine_start_refusal.bats (9 tests, 722s, ~80s/test) and test_remote_status_liveness.bats (31 tests, 380s, ~12s/test) both sit near the bottom of the count-weighted sort while carrying two of the largest absolute durations in the suite -- invisible to the weight the greedy partition uses, so nothing stops an unrelated test-count change elsewhere from repacking them onto the same shard (#847's demonstrated failure mode). shard-tests.sh now seeds these two into distinct shards before the ordinary count-weighted pass runs, so their placement no longer depends on any other file's test count. tests.yml's bats-shard timeout-minutes moves from 25 to 30, with the arithmetic behind that number in a comment, and its comment claiming a timeout is always a genuine hang is corrected (#848) -- #857 hit exactly the opposite: 335/335 tests ok, cancelled 12s after its last one, at the cap. Files that are merely large by test count (e.g. a 179-test file at an ordinary ~1.2s/test) were checked and deliberately left unpinned: count already weights them correctly. --- .github/scripts/shard-tests.sh | 119 ++++++++++++++++++++++++++++++--- .github/workflows/tests.yml | 23 ++++++- tests/test_ci_sharding.bats | 51 ++++++++++++++ 3 files changed, 180 insertions(+), 13 deletions(-) diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index 4abc1a79c..249620801 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -33,9 +33,67 @@ # # Whatever the weights, the property that matters is coverage, not balance: the # worst case of a bad weight is an unevenly filled shard, never a missing file. - +# +# --- Pinned-apart files (#847, #848) --------------------------------------- +# +# @test count is a loose proxy for runtime in general (above), but for a +# specific shape of file it is not loose, it is blind: a file whose cost is +# almost entirely waiting (background processes, poll loops with +# hundred-plus-iteration bounds) rather than how many @test blocks it +# contains can carry a tiny weight here while dominating its shard's actual +# wall clock. A file that is merely large -- many @test blocks, ordinary +# per-test cost -- is NOT this case; count already weights it correctly, and +# it is not pinned. +# +# Measured 2026-08-19 on a green main run (head 626a625b, run 32193147987) by +# correlating each `ok N ` line's own GitHub Actions timestamp against +# which file's `@test` block that description belongs to, then ranking every +# file by seconds-per-test rather than by raw duration (raw duration alone +# does not distinguish "slow because few tests wait a long time" from "slow +# because there are simply many tests", and only the former is what count +# weighting misses): +# +# tests/test_remote_engine_start_refusal.bats 722s / 9 tests = ~80s/test +# tests/test_remote_status_liveness.bats 380s / 31 tests = ~12s/test +# +# against a whole-suite per-test cost this script's own header already says +# runs ~0.0s-8s. Both are 1.5x-10x above that ceiling on a low test count, so +# both rank near the bottom of the count-weighted sort while carrying some of +# the largest absolute durations in the suite. (Files that are merely large in +# absolute terms -- e.g. a 179-test file at a very ordinary ~1.2s/test -- were +# checked and excluded: their weight already reflects their real cost.) +# +# #847's own trigger was exactly this class of file landing next to another +# heavy one purely because an unrelated 15-test addition elsewhere repacked +# the partition — the count weight cannot tell "heavy because slow" from +# "heavy because voluminous", so nothing stops two slow-but-few-tests files +# from drifting onto the same shard as the tree changes shape. Pinning these +# apart, in fixed shard slots decided before the ordinary weighted pass runs, +# means no future change to any OTHER file's test count can put two of them +# together again — that was the actual, demonstrated failure, not merely a +# theoretical one. +# +# This does not bound a shard's total duration: the heavier entry above +# (722s) is heavy enough on its own that no repacking of the rest of the +# suite moves its shard's floor by much. See tests.yml's bats-shard +# timeout-minutes for the ceiling this is paired with, sized to cover that +# floor plus a fair share of everything else with real margin. And a file NOT +# on this list can still turn out to be similarly disproportionate and land +# next to another one by chance — nothing here detects that case in general, +# only these two measured instances of it. Revisit alongside the counting +# scheme itself once the concurrent effort to shorten these files (tracked +# separately from #847/#848) lands and the numbers above are stale. +# +# Matched by basename, not by the `$dir`-relative path `files` below uses, so +# the pin still resolves when this script is invoked against a different +# tests-dir (as several of this file's own tests do). A pinned name that no +# longer exists in the tree (renamed, removed) is silently skipped rather +# than treated as an error: the partition's correctness (full coverage, +# asserted by tests/test_ci_sharding.bats) never depended on it. set -euo pipefail +PINNED_APART="test_remote_engine_start_refusal.bats test_remote_status_liveness.bats" + usage() { echo "usage: ${0##*/} [tests-dir]" >&2 echo " shard-index is 1-based and must be <= shard-total" >&2 @@ -62,12 +120,56 @@ files="$(find "$dir" -maxdepth 1 -name '*.bats' | LC_ALL=C sort)" # Weight each file by its number of test cases. `grep -c` exits 1 on no match # after printing 0, which set -e would otherwise treat as fatal. +file_weight() { + local n + n="$(grep -c '^[[:space:]]*@test' "$1" || true)" + [ -n "$n" ] || n=0 + printf '%s' "$n" +} + +# Seed the pinned files into distinct shards first, in PINNED_APART's own +# (measured-heaviest-first) order — not the order they happen to sort in +# below, which is by count and is exactly the metric these files defeat. Each +# consumes one shard slot (wrapping if there are more pinned files than +# shards); everything else is decided by the ordinary weighted pass afterward, +# which never reconsiders a file placed here. +i=0 +while [ "$i" -lt "$total" ]; do + load[i]=0 + i=$((i + 1)) +done + +pinned_paths=" " +slot=0 +for p in $PINNED_APART; do + match="" + while IFS= read -r f; do + [ -n "$f" ] || continue + if [ "$(basename "$f")" = "$p" ]; then + match="$f" + break + fi + done <> "$dir/test_roster_journal.bats" + done + + local shard1="" shard2="" shard_files + for i in 1 2 3 4; do + shard_files="$(cd "$REPO_ROOT" && bash "$SHARD" "$i" 4 "$dir")" + grep -qF "$pin1" <<<"$shard_files" && shard1="$i" + grep -qF "$pin2" <<<"$shard_files" && shard2="$i" + done + [ -n "$shard1" ] + [ -n "$shard2" ] + [ "$shard1" != "$shard2" ] +} + @test "shard-tests.sh rejects out-of-range and non-numeric arguments" { run bash "$SHARD" 0 4 [ "$status" -eq 2 ] From 34e37770ae0aa1cc30f1e93228a41c6265f5809e Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 18 Aug 2026 16:27:13 -0700 Subject: [PATCH 2/2] address co1's static review P2s (#847, #848) - test name now says >= 2 (total=1 trivially wraps both pins into the same slot, same as every other file) instead of claiming a property that does not hold there - shard-tests.sh's pin comment now names a concrete follow-up: re-run the same seconds-per-test measurement once #876 et al. land, since they change these two files' real cost and can make the pin list stale or incomplete --- .github/scripts/shard-tests.sh | 9 +++++++++ tests/test_ci_sharding.bats | 6 +++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index 249620801..13db2f598 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -84,6 +84,15 @@ # scheme itself once the concurrent effort to shorten these files (tracked # separately from #847/#848) lands and the numbers above are stale. # +# FOLLOW-UP: whichever of that effort's PRs (#876 et al.) touches either file +# named below changes its real cost, possibly enough to make pinning it +# pointless or to make some other, currently-unremarkable file the next +# hidden outlier. Re-run this script's own measurement method (correlate a +# green run's `ok N` timestamps against each file, rank by seconds-per-test) +# on main once that work lands, and drop or replace entries here based on +# what it says then — this list is not meant to be permanent. The 30-minute +# job cap in tests.yml is a separate decision and does not depend on this one. +# # Matched by basename, not by the `$dir`-relative path `files` below uses, so # the pin still resolves when this script is invoked against a different # tests-dir (as several of this file's own tests do). A pinned name that no diff --git a/tests/test_ci_sharding.bats b/tests/test_ci_sharding.bats index 5029b121d..98bf510b0 100644 --- a/tests/test_ci_sharding.bats +++ b/tests/test_ci_sharding.bats @@ -91,7 +91,11 @@ union_of_shards() { # largest measured durations in the suite (722s and 380s; see # shard-tests.sh's own comment for the measurement). This test guards the # actual fix, not the metric that already worked. -@test "the pinned-apart heavy files never share a shard, at any shard total (#847, #848)" { +@test "the pinned-apart heavy files never share a shard, at any shard total >= 2 (#847, #848)" { + # total=1 is deliberately not checked: with one shard both pins wrap into + # slot 0 and land together by construction, same as every other file — the + # pin has nothing to separate them FROM at total=1, so that is not a case + # this property claims to hold. local pin1="test_remote_engine_start_refusal.bats" local pin2="test_remote_status_liveness.bats" local total i shard_files together