Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 117 additions & 11 deletions .github/scripts/shard-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,76 @@
#
# 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 <desc>` 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.
#
# 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
# 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##*/} <shard-index> <shard-total> [tests-dir]" >&2
echo " shard-index is 1-based and must be <= shard-total" >&2
Expand All @@ -62,12 +129,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 <<EOF
$files
EOF
[ -n "$match" ] || continue
s=$((slot % total))
load[s]=$((load[s] + $(file_weight "$match")))
if [ "$s" -eq "$((index - 1))" ]; then
printf '%s\n' "$match"
fi
pinned_paths="${pinned_paths}${match} "
slot=$((slot + 1))
done

# Weight every remaining (non-pinned) file the same way as before.
weighted=""
while IFS= read -r f; do
[ -n "$f" ] || continue
n="$(grep -c '^[[:space:]]*@test' "$f" || true)"
[ -n "$n" ] || n=0
weighted="${weighted}${n} ${f}
case "$pinned_paths" in
*" $f "*) continue ;;
esac
weighted="${weighted}$(file_weight "$f") ${f}
"
done <<EOF
$files
Expand All @@ -76,13 +187,8 @@ EOF
# Heaviest first; ties broken by path so the order is total, not incidental.
sorted="$(printf '%s' "$weighted" | LC_ALL=C sort -t' ' -k1,1nr -k2,2)"

# Greedy LPT: hand each file to the currently lightest shard.
i=0
while [ "$i" -lt "$total" ]; do
load[i]=0
i=$((i + 1))
done

# Greedy LPT: hand each remaining file to the currently lightest shard. `load`
# already carries the pinned seeds from above, not reset here.
while IFS=' ' read -r n f; do
[ -n "$f" ] || continue
best=0
Expand Down
23 changes: 21 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,27 @@ jobs:
# block the PR). When changes failed, docs_only is empty → heavy steps run.
if: ${{ !cancelled() }}
runs-on: ${{ matrix.os }}
# A shard has generous headroom under this cap; a timeout is a genuine hang.
timeout-minutes: 25
# NOT "a timeout is a genuine hang" (#848): a cancellation here is a wall-
# clock cap being reached, and a shard that is still passing tests right up
# to the cap looks identical to one that is actually stuck. #857 hit this
# on macOS at 25m29s with all 335 of its tests ok and the last one landing
# at 24m53s -- a shard that finished normally a heartbeat too late, not a
# hang. Distinguishing the two needs the job's own log (does the last `ok`
# land seconds before cancellation, or does output stop cold with time
# left on the cap), not this number.
#
# Sized with real margin, not "generous" by assertion. Measured 2026-08-19
# (run 32193147987, macOS): the four shards ran 890s/824s/540s/1230s, and
# the heaviest (1230s) had 270s of the old 1500s cap left -- 18%. After
# #847's pin (shard-tests.sh) removes the two disproportionate files
# (722s, 380s) from the ordinary weighted pool, the worst remaining shard
# is estimated at the heavier pin's own floor (722s) plus a fair share of
# what is left of the suite (~2382s / 4 shards =~ 595s, generously ~700s
# unevenly) =~ 1420s, plus runner-speed variance -- call it 1700s. 1800s
# (30m) leaves real headroom over that estimate, not merely over last
# week's number. The 2 known intermittent reds (ubuntu 4/4's
# `_wait_pidfile`, windows runtime #567) are unrelated to this cap.
timeout-minutes: 30
strategy:
# Cover both GNU (Linux) and BSD (macOS) userlands — the scripts shell out
# to sed/stat/mktemp/ps etc. whose flags differ between them.
Expand Down
55 changes: 55 additions & 0 deletions tests/test_ci_sharding.bats
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,61 @@ union_of_shards() {
done
}

# The test above catches drift in the TOP TWO BY COUNT — exactly the metric
# that misses the files pinned below (#847, #848): both are near the bottom
# of the count-weighted sort (9 and 31 tests) despite carrying some of the
# 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 >= 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
for total in 2 3 4 5 8; do
together=0
for ((i = 1; i <= total; i++)); do
shard_files="$(cd "$REPO_ROOT" && bash "$SHARD" "$i" "$total")"
if grep -qF "$pin1" <<<"$shard_files" && grep -qF "$pin2" <<<"$shard_files"; then
together=1
fi
done
[ "$together" -eq 0 ]
done
}

@test "adding tests to an unrelated file does not reunite the pinned-apart files (#847 positive control)" {
# Reproduces #847's actual trigger, not a stand-in for it: that issue's
# collision was caused by appending @test cases to ONE file
# (test_roster_journal.bats) and having the repack land two OTHER, entirely
# untouched files in the same shard. This does the same append and checks
# the two files pinned above specifically, since a pin is exactly the part
# of the fix that is supposed to make this kind of drift unable to reunite
# them, whatever else in the tree changes shape.
local pin1="test_remote_engine_start_refusal.bats"
local pin2="test_remote_status_liveness.bats"
local dir="$BATS_TEST_TMPDIR/tests-plus"
mkdir -p "$dir"
cp "$REPO_ROOT"/tests/*.bats "$dir"/
local i
for i in $(seq 1 20); do
printf '\n@test "synthetic case %d" {\n true\n}\n' "$i" >> "$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 ]
Expand Down
Loading