fix(ci): pin count-invisible heavy test files apart, raise the shard timeout with real margin - #885
Merged
Merged
Conversation
…ap 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.
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI-only: every touched path is under
.github/ortests/(measured:git diff --name-only origin/main...HEAD=.github/scripts/shard-tests.sh,.github/workflows/tests.yml,tests/test_ci_sharding.bats). Nothing here reaches a path an existing user's install runs.Summary
Fixes the packing/ceiling half of #847 and #848 (the two issues describe the same underlying problem from different angles). Does not touch test content — the parallel effort to shorten the slow tests themselves is tracked separately and out of scope here.
.github/scripts/shard-tests.shbalances the bats suite across shards by@testcount (greedy LPT). Count is a loose proxy for wall-clock cost in general (already documented in the script's own header), and 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@testblocks it contains can sit near the bottom of the count-weighted sort while dominating its shard's actual runtime. Nothing then stops an unrelated test-count change elsewhere in the tree from repacking two such files onto the same shard — which is exactly #847's demonstrated failure mode: a 15-test addition to one file moved two entirely untouched, heavy files onto the same shard, which then exceeded the job'stimeout-minutescap (#848) — a wall-clock cancellation of a fully passing shard, not a hang, contrary to what the cap's own comment claimed.What was measured
2026-08-19, on a green
mainrun (head626a625b, run32193147987): correlated everyok N <desc>line's own GitHub Actions timestamp against which file's@testblock that description belongs to, across all four macOS shards, then ranked every file by seconds per test rather than raw duration — raw duration alone doesn't distinguish "slow because a few tests each wait a long time" from "slow because there are simply many tests," and only the former is invisible to the count weight:test_remote_engine_start_refusal.batstest_remote_status_liveness.batsagainst a whole-suite per-test cost the script's header already says runs ~0–8s. Both are 1.5×–10× above that on a low test count — invisible to the sort, while carrying two of the largest absolute durations in the suite.
Checked and deliberately not pinned: files that are merely large by test count (e.g. a 179-test file at an ordinary ~1.2s/test, a 139-test file at ~2.1s/test). Count already weights those correctly; pinning them would be redundant with what LPT already does.
Fix
shard-tests.sh: the two files above are now seeded into distinct shards before the ordinary count-weighted pass runs (full reasoning + numbers in the script's own comment). Their placement no longer depends on any other file's test count. Coverage (every file assigned exactly once, asserted bytest_ci_sharding.bats) is unaffected — verified for shard totals 1–8.tests.yml:bats-shard'stimeout-minutesmoves from 25 to 30, with the arithmetic in a comment (current heaviest shard 1230s / 270s headroom on the 25m cap → after the pin removes the two disproportionate files from the general pool, worst-case estimate ≈1420s → 1800s cap leaves real margin, not an asserted one). The comment claiming "a timeout is a genuine hang" is corrected — fix(watch): fold sentinel '-' into the empty session-id path #857 is quoted as the counter-example (335/335 tests ok, cancelled 12s after the last one).What this does not fix
The 722s file is heavy enough alone that no repacking of the rest of the suite moves its shard's floor by much — pinning prevents compounding (two heavies stacked), not that one file's own cost. A file not on the pin list could still turn out to be similarly disproportionate and collide with something else by chance; nothing here detects that case in general, only these two measured instances of it.
Positive control
Added two tests to
tests/test_ci_sharding.bats:the pinned-apart heavy files never share a shard, at any shard total >= 2(total=1is excluded: both pins trivially wrap into the same slot there, same as every other file, so the property does not claim to hold attotal=1) — checked pre-fix: fails ([ "$together" -eq 0 ]fails, confirming the two files do land together at some shard total under the old algorithm). Passes post-fix.adding tests to an unrelated file does not reunite the pinned-apart files— reproduces bats shards are balanced by test count, so adding tests can push one bucket past the job ceiling #847's actual trigger (append 20@tests totest_roster_journal.bats, re-derive the partition). Note this one happens to pass under the old algorithm too for this specific synthetic append — it's a supplementary check, not the primary discriminator; the first test above is what demonstrates the bug and the fix.Tests
tests/test_ci_sharding.bats: 11/11 passing (9 pre-existing + 2 new).Pre-existing, unrelated to this PR:
check-enforced-assertions.shreports 639 against a baseline of 638 — verified this is already true onmainat the commit this branch is based on, before any change here (639 both before and after this diff). Flagging so the CI red on this check isn't mistaken for something this PR caused.Not run locally: the full suite (left to CI, per team convention).