You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Race Detection runs on every push and is the longest job in the workflow by a wide margin. The last run on main took 19m20s, and it is the floor on a review round — the recent connector work spent twenty-two of them.
That is what the card proposed, and measuring first says it buys about 4%. Of the 19m20s: 15s of setup, 82s compiling with -race, and 17m41s running — and inside that,
package
time
internal/connector
1031s
internal/connector/driver/acp
222s
internal/commands
146s
everything else (38 packages)
214s combined
Total package time is 1613s, go test ./... runs packages concurrently, so the floor is max(1031, 1613/lanes) — and the observed run phase was 1061s. One package is 97% of the critical path. Whichever shard gets it still takes seventeen minutes. So the unit has to be the test.
Jorge picked this over the package split once the measurement was on the card.
How the shards are built
Four of them. Each enumerates the suite with go test -list -json, which gives package-and-test pairs — a bare -list prints names with no package, and six names here exist in more than one. That is 4988 pairs across 41 packages, sorted, sliced by index, and run as one go test -race -run over ./... so Go's package parallelism survives inside the shard. The six duplicated names cost eight extra test runs in total.
The part that is the point
A shard scheme that silently stops running some tests is green and measures nothing. Two checks answer that, and neither is a warning.
Each shard compares what actually ran, from the run events, against what it was assigned, and fails on any shortfall. A -run regex that matches nothing exits 0 saying "no tests to run", which is exactly the shape this has to rule out.
That check earned itself on its first real run: the shard reported SHARD RAN 1247 OF ITS 1251 TESTS, and the four missing were benchmarks. -list enumerates them and -run does not run them without -bench, so they were being assigned to a shard that could not run them — sixteen across the suite, which the union check would have called complete, because the assignment was complete. They are out of the enumeration now.
The aggregate fails unless every shard passed, all four enumerations are byte-identical, the slices are pairwise disjoint, and their union is the whole list.
The required check keeps its name
Race Detection is now the aggregate job that needs: the shards, so the context never changes name and the ruleset is untouched however many shards there are. if: always(), because a failed shard would otherwise skip it — and a skipped required check is not a failed one. fail-fast: false, so the aggregate can tell a real gap from a cancelled sibling.
Proved red before trusted green
The union check has its own tests, in make check, because a check that cannot fail is worse than no check. Eight cases: a complete disjoint split accepted, and seven refused — a shard assigned nothing, a shard with no record, the same test in two shards, a test in no shard, shards that enumerated different sets, a test that does not exist, and an empty enumeration.
No wall-clock claim yet
Four shards is the approved starting point, not a target. What this is worth is the next thing to measure, on CI, and it will go on the card before the split is touched.
Race Detection is nineteen minutes and runs on every push, and seventeen
of those minutes are one package: internal/connector is 1031s of a 1061s
critical path. That is why sharding by package group, which the card
proposed, buys about 4% — whichever shard gets that package still takes
seventeen minutes. The unit has to be the test.
Four shards. Each enumerates the suite with `go test -list -json`, which
gives package-and-test pairs; a bare -list prints names with no package
and six names here exist in more than one. 4988 pairs across 41
packages, sorted, sliced by index, and run in one `go test -race -run`
over ./... so Go's own package parallelism survives inside the shard.
The six duplicated names cost eight extra test runs in total.
The hazard that comes with sharding is the one this repository keeps
finding: a split that silently stops running some tests is green and
measures nothing. Two checks answer it, and neither is a warning.
Each shard compares what actually ran, from the run events, against what
it was assigned, and fails on any shortfall. A -run regex that matches
nothing exits 0 saying "no tests to run", which is exactly the shape
this has to rule out. That check earned itself immediately: the first
real shard ran 1247 of its 1251 tests, and the four missing were
benchmarks — -list enumerates them and -run does not run them without
-bench, so they were being assigned to a shard that could not run them.
They are out of the enumeration now.
The aggregate then fails unless every shard passed, all four
enumerations are byte-identical, the slices are pairwise disjoint, and
their union is the whole list. It is the `Race Detection` job itself, so
the required status check keeps its name and the ruleset is untouched
however many shards there are; `if: always()` so a failed shard cannot
skip it, because a skipped required check is not a failed one.
The union check has its own tests, in make check, because a check that
cannot fail is worse than no check. Eight cases: one complete disjoint
split accepted, and seven refused — a shard assigned nothing, a shard
with no record, the same test in two shards, a test in no shard, shards
that enumerated different sets, a test that does not exist, and an empty
enumeration.
No wall-clock claim here yet. The point of this commit is that the split
is provably complete; what it is worth is the next thing to measure, on
CI, before the shard count is touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shards race detection by individual tests while preserving the required aggregate check.
Changes:
Adds four-way race-test sharding and coverage validation.
Adds union-check tests to the local CI gate.
Preserves the Race Detection status name.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
File
Description
scripts/race-shard.sh
Enumerates, assigns, runs, and verifies shard tests.
scripts/race-shard-union.sh
Validates complete, disjoint shard coverage.
scripts/race-shard-union-test.sh
Exercises union-check failure cases.
Makefile
Adds union tests to local checks.
.github/workflows/test.yml
Replaces the race job with shards and an aggregate.
Two from review, and the first is this change's own defect aimed back at
it. download-artifact extracts each artifact to <path>/<name>/, and the
uploads were named race-shard-N while the union check reads shard-N. So
the aggregate would have found no reports at all — on every run.
It would have failed rather than passed, which is the one mercy: the
check refuses a shard whose enumeration it cannot read. But a checker
that can never see its inputs is not a checker, and "shard 1 recorded no
enumeration" would have sent whoever met it looking in the wrong place.
The artifact is named for the directory the aggregate reads, and the
message now allows for a report that did not arrive.
Proved rather than reasoned about: the union check's tests gained the two
cases that would have caught this. A shard whose report never arrived is
refused, and so is a report that arrived under a name the checker does
not read — which is exactly the defect, kept as a test so it cannot come
back. Ten cases now, one accepted and nine refused.
And md5sum is not on stock macOS, which would have made check-race-shards
fail before testing anything for anyone not on Linux — a local check that
only runs on Linux quietly stops being run. The enumerations are compared
as sorted files with cmp instead, which is POSIX and needs no hash at
all. Nothing GNU-only is left in the script.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace seq with Bash arithmetic loops for macOS compatibility
scripts/race-shard-union.sh:38
check-race-shards now runs in the documented local bin/ci path, but stock macOS does not provide seq and the repository setup only installs project tools, not GNU coreutils. Thus macOS contributors still fail before this check executes, despite replacing md5sum; use Bash arithmetic loops for all three ranges instead.
The review flagged seq as unavailable on macOS. That part is wrong: macOS
ships /usr/bin/seq, and make check already runs seq twice in test-e2e long
before it reaches this script. But looking at the loop for that reason found
a worse bug behind it.
BSD seq reads "first larger than last" as a request to count down. GNU seq
prints nothing for `seq 5 4`; BSD seq prints "5" and "4". The overlap loop
ends on exactly that range — at i == total it asks for seq $((total+1)) total
— so on macOS the last iteration ran with j == total+1 and j == total, read a
shard directory that does not exist, and compared the last shard against
itself. Every correct split was refused, with the message "shards 4 and 4
were both assigned 1 test(s)".
So the fix the review asked for is right, for a reason it did not give. All
three ranges now count in the shell.
Proved it with a shim implementing BSD's documented rule: with seq the new
case fails exactly as macOS would, naming shards 4 and 4; counting in the
shell, it passes. The shim is the test, so a reintroduced seq cannot pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 Taken, though not for the reason given — and the real reason is worse.
The premise is wrong. macOS does ship seq at /usr/bin/seq; it is BSD userland, not GNU coreutils, which is what separates it from md5sum. And make check already runs seq twice in test-e2e (e2e/schedule.bats:52, e2e/errors.bats:604), which comes before check-race-shards in the target list — so if stock macOS lacked seq, make check would already have been broken there, with or without this PR.
But going to the loop to check that found a genuine defect sitting behind the wrong reason.
BSD seq(1): "If first is larger than last the default incr is -1." So seq 5 4 prints nothing on GNU and prints 5 then 4 on macOS. The overlap loop ended on exactly that range — at i == total it asked for seq $((total + 1)) "$total". On macOS the final iteration ran with j == 5 and j == 4, read a shard-5 directory that does not exist, and compared shard 4 against itself:
sort: cannot read: .../shard-5/shard.5.txt: No such file or directory
SHARD COVERAGE FAILED: shards 4 and 4 were both assigned 1 test(s), e.g. pkg/b TestFour
That is a correct, complete, disjoint split being refused. Every macOS run of make check would have failed here, and the message would have sent the reader looking for a duplicate assignment that does not exist.
All three ranges now count in the shell.
Evidence. I shimmed a seq implementing BSD's documented rule and put it first on PATH. With seq, the new case fails exactly as macOS would, naming shards 4 and 4; counting in the shell, it passes. I mutated the fix back out to confirm the case is actually load-bearing rather than passing over nothing. The shim is written by the test itself, so this is now pinned permanently — a reintroduced seq cannot go green:
ok - accepts a complete, disjoint split under BSD seq semantics
I also swept both scripts again for GNU-only tools and BSD-divergent flags; there are none left. shellcheck -S style is clean on all three.
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
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.
Race Detection runs on every push and is the longest job in the workflow by a wide margin. The last run on main took 19m20s, and it is the floor on a review round — the recent connector work spent twenty-two of them.
Originally tracked in Race detection takes 16 minutes and gates every push.
Why not shard by package group
That is what the card proposed, and measuring first says it buys about 4%. Of the 19m20s: 15s of setup, 82s compiling with
-race, and 17m41s running — and inside that,internal/connectorinternal/connector/driver/acpinternal/commandsTotal package time is 1613s,
go test ./...runs packages concurrently, so the floor is max(1031, 1613/lanes) — and the observed run phase was 1061s. One package is 97% of the critical path. Whichever shard gets it still takes seventeen minutes. So the unit has to be the test.Jorge picked this over the package split once the measurement was on the card.
How the shards are built
Four of them. Each enumerates the suite with
go test -list -json, which gives package-and-test pairs — a bare-listprints names with no package, and six names here exist in more than one. That is 4988 pairs across 41 packages, sorted, sliced by index, and run as onego test -race -runover./...so Go's package parallelism survives inside the shard. The six duplicated names cost eight extra test runs in total.The part that is the point
A shard scheme that silently stops running some tests is green and measures nothing. Two checks answer that, and neither is a warning.
Each shard compares what actually ran, from the run events, against what it was assigned, and fails on any shortfall. A
-runregex that matches nothing exits 0 saying "no tests to run", which is exactly the shape this has to rule out.That check earned itself on its first real run: the shard reported
SHARD RAN 1247 OF ITS 1251 TESTS, and the four missing were benchmarks.-listenumerates them and-rundoes not run them without-bench, so they were being assigned to a shard that could not run them — sixteen across the suite, which the union check would have called complete, because the assignment was complete. They are out of the enumeration now.The aggregate fails unless every shard passed, all four enumerations are byte-identical, the slices are pairwise disjoint, and their union is the whole list.
The required check keeps its name
Race Detectionis now the aggregate job thatneeds:the shards, so the context never changes name and the ruleset is untouched however many shards there are.if: always(), because a failed shard would otherwise skip it — and a skipped required check is not a failed one.fail-fast: false, so the aggregate can tell a real gap from a cancelled sibling.Proved red before trusted green
The union check has its own tests, in
make check, because a check that cannot fail is worse than no check. Eight cases: a complete disjoint split accepted, and seven refused — a shard assigned nothing, a shard with no record, the same test in two shards, a test in no shard, shards that enumerated different sets, a test that does not exist, and an empty enumeration.No wall-clock claim yet
Four shards is the approved starting point, not a target. What this is worth is the next thing to measure, on CI, and it will go on the card before the split is touched.