Skip to content

ci: shard Go race tests into a round-robin 3-way matrix (PLT-959) #3874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amir-deris wants to merge 12 commits into from
+96 −13
Closed

ci: shard Go race tests into a round-robin 3-way matrix (PLT-959) #3874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d0e45a3
ci: shard Go race tests into a 3-way matrix (PLT-959)
amir-deris Aug 7, 2026
8b92414
ci: fix shard-file lookup to not assume split's suffix width
amir-deris Aug 7, 2026
34d246c
ci: balance race-detection shards by historical package duration
amir-deris Aug 7, 2026
ea6cf7c
ci: prefer own-branch timing history over main-only lookup
amir-deris Aug 7, 2026
ccafa3d
ci: trigger a re-run to pick up this branch's timing artifact
amir-deris Aug 7, 2026
d8b3396
ci: revert to plain round-robin sharding, drop testsplit tool
amir-deris Aug 7, 2026
ec8c6ac
Merge branch 'main' into amir/plt-959-shard-go-race-tests
amir-deris Aug 7, 2026
9cb1953
fix: restore full package list for race sharding, not just test-havin…
amir-deris Aug 7, 2026
b88efd4
Merge remote-tracking branch into amir/plt-959-shard-go-race-tests
amir-deris Aug 7, 2026
cedd009
fix: make packages.txt generation fail on a broken go list
amir-deris Aug 10, 2026
b4793b4
Split heavy tests in the shards first before other tests
amir-deris Aug 10, 2026
f2a1122
Merge branch 'main' into amir/plt-959-shard-go-race-tests
amir-deris Aug 10, 2026
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
47 changes: 43 additions & 4 deletions .github/workflows/go-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,32 @@ env:
# state_db tests run in their own workflow (sei-db-tests.yml); exclude
# that subtree everywhere in this workflow to avoid double-running them.
STATE_DB_PKG_PREFIX: github.com/sei-protocol/sei-chain/sei-db/state_db
# Number of race-detection shards. Matches `NUM_SPLIT` passed to
# `make test-group-N` so shards are locally reproducible.
NUM_SPLIT: 3

jobs:
shard-indexes:

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This job exists only to convert the literal NUM_SPLIT: 3 (line 23) into [0,1,2], but it costs a full serialized runner acquisition (~20–40s) on the critical path of a PR that's optimizing wall-clock, plus a needs: edge.

Since NUM_SPLIT is a static literal in this same file (not derived from repo state the way a dynamically-discovered test-case count would be), matrix: shard: [0, 1, 2] inline is equivalent and free. The tradeoff is having to edit two places when changing the shard count — a comment on NUM_SPLIT pointing at the matrix would cover that.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This provisions a full runner and ~30s of setup on every run purely to turn NUM_SPLIT=3 into [0,1,2]. Since NUM_SPLIT is a hardcoded workflow-level env var (not an input), a literal matrix: shard: [0, 1, 2] would do the same with zero jobs. Worth the tradeoff only if you expect NUM_SPLIT to become dynamic; otherwise the eth_blocktests.yml pattern is more machinery than this case needs.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This job provisions a full runner and shells out to jq purely to turn the static literal 3 into [0,1,2], and it sits on the critical path ahead of every shard. Since NUM_SPLIT is a hardcoded env constant rather than something computed at run time, matrix: shard: [0, 1, 2] is equivalent and removes a job plus a needs hop. If keeping the single-source-of-truth on NUM_SPLIT is the point, that's a fair trade — worth a one-line comment saying so, since the eth_blocktests.yml precedent it mirrors is genuinely dynamic and this one isn't.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This whole job exists only to turn NUM_SPLIT into [0,1,2], because env isn't available in strategy. That's a real constraint, but GitHub already exposes the matrix length as strategy.job-total, so you can drop both the job and the workflow-level NUM_SPLIT and keep a single source of truth:

test:
  strategy:
    fail-fast: false
    matrix:
      shard: [0, 1, 2]   # length must equal NUM_SPLIT below
  ...
      run: make split-test-packages NUM_SPLIT=${{ strategy.job-total }}

That removes an extra runner + queue wait from the critical path of every race run. If you keep the generator job, jq -nc --argjson n "$NUM_SPLIT" '[range($n)]' is a good deal clearer than '. |= [inputs]' over an unquoted here-string.

name: Generate race shard indexes
runs-on: ubuntu-latest
outputs:
json: ${{ steps.generate-index-list.outputs.json }}
steps:
- id: generate-index-list
run: |
MAX_INDEX=$((${{ env.NUM_SPLIT }}-1))
INDEX_LIST=$(seq 0 ${MAX_INDEX})
INDEX_JSON=$(jq --null-input --compact-output '. |= [inputs]' <<< ${INDEX_LIST})

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Two small hardening points in this step:

  1. No set -euo pipefail (the Go test step has it). If jq fails, INDEX_JSON is empty, the step still exits 0, and the failure surfaces later as an opaque fromJson error on the matrix rather than here.
  2. <<< ${INDEX_LIST} is unquoted. It happens to work — word-splitting collapses the newlines to spaces and jq's inputs reads whitespace-separated values either way — but quoting it (<<< "$INDEX_LIST") removes the dependence on that.

Optionally, jq -nc --argjson n ${{ env.NUM_SPLIT }} '[range($n)]' drops the seq round-trip entirely.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] <<< ${INDEX_LIST} is unquoted, so it relies on word-splitting collapsing seq's newlines into spaces. jq's inputs handles newline-separated values identically, so quoting (<<< "${INDEX_LIST}") is strictly safer and won't trip shellcheck's SC2086.

echo "json=${INDEX_JSON}" >> "$GITHUB_OUTPUT"

test:
name: Race Detection
name: "Race Detection (shard ${{ matrix.shard }})"
runs-on: uci-default
needs: shard-indexes
strategy:
fail-fast: false
matrix:
shard: ${{ fromJson(needs.shard-indexes.outputs.json) }}
env:
GOFLAGS: -race -tags=ledger,test_ledger_mock
steps:
Expand Down Expand Up @@ -52,9 +73,27 @@ jobs:
- name: Go test
run: |
set -euo pipefail
PKGS=$(go list ./... | grep -v "^${STATE_DB_PKG_PREFIX}")
echo "$PKGS" | xargs go test \
-timeout=${{ env.GO_TEST_TIMEOUT }}
make split-test-packages NUM_SPLIT=${{ env.NUM_SPLIT }}

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This step hand-reimplements the shard-file selection and occ_tests detection that Makefile:591-603 already does — in a PR whose stated goal is removing local/CI drift. The two copies already diverge:

Makefile test-group-% this step
non-occ parallelism -parallel=4 unset (GOMAXPROCS)
timeout 10m 30m
build tags none ledger,test_ledger_mock (via GOFLAGS)
coverage -coverprofile/-coverpkg=./... none
occ match grep -q (substring) grep -qx (exact)

A test-group-race-% target in the Makefile (same selection logic, no coverage flags, tags applied) invoked from here would keep the two genuinely in lockstep and make the AGENTS.md reproducibility claim true by construction.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] Each shard recomputes the split independently, so the three shards can end up with different partitions of the same package list — and the failure is silent.

make split-test-packages runs inside the matrix job, so every shard does its own fetchTimings API call and its own binPack. The plan is only deterministic if all three shards observe identical inputs. Two realistic ways they don't:

  • A transient API error / rate-limit / timeout on one shard's fetch. That shard falls back to roundRobin while the others bin-pack (plan.go:53-62 only logs to stderr and continues).
  • Shards start minutes apart when the capped uci-default pool queues them. If a main push run completes in between, the later shard fetches a newer package-timings artifact and packs differently.

When the partitions disagree, some packages run twice and some run zero times — with every shard green and test-check reporting success. That's untested code merging behind a passing required check.

Suggest computing the plan exactly once and sharing it: run testsplit plan in the existing shard-indexes job, upload build/packages.txt.* as an artifact, and have each shard download its own file instead of re-planning. That also removes N-1 redundant API calls and N-1 redundant go list invocations. If you'd rather keep per-shard planning, the fallback needs to be all-or-nothing across shards rather than per-shard best-effort.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] Each of the N matrix shards runs make split-test-packages independently, so testsplit plan makes its own GitHub API call and computes its own partition. Nothing guarantees the N jobs agree.

Divergence is reachable in normal operation:

  • Shard 0's fetch succeeds and bin-packs; shard 1 hits a transient 5xx / rate limit / 30s timeout and falls back to round-robin.
  • A newer successful run on the same branch lands between two shards' latestSuccessfulRun queries (shards can start minutes apart under uci-default queuing), so they read different timing artifacts and bin-pack differently.
  • Timing coverage sits near the 0.5 threshold and one shard's artifact differs enough to flip minTimingCoverage.

Because round-robin and bin-packing produce completely unrelated partitions, the union of the shards is then neither complete nor disjoint: some packages run 2–3×, and some packages run zero times — with every shard green and the aggregate "Race Detection" check passing. That is a silent loss of race-detector coverage on the gate that exists to catch races, and it fails invisibly rather than loudly.

Compute the plan once per run and distribute it: a plan job that runs testsplit plan and uploads build/packages.txt.* as an artifact, with each shard downloading its own file. Every shard then provably reads one consistent partition, and the fallback decision is logged once instead of N times.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] Race CI no longer compiles packages that have no test files.

The previous step built its list from go list ./..., so every package was handed to go test — packages without tests print no test files but are still compiled under -race -tags=ledger,test_ledger_mock.

build/packages.txt is produced by Makefile:583, whose template ({{ if (or .TestGoFiles .XTestGoFiles) }}) deliberately keeps only packages that have tests. So test-less packages are now never compiled by this job.

What still covers them, and what doesn't:

  • golangci.yml type-checks everything, but with default build tags — not ledger,test_ledger_mock.
  • The coverage job's full path (go-test.yml:218-232) does pass all of go list ./..., but it's gated to github.event_name != 'pull_request', so it only runs post-merge.

Net effect: a compile break in a test-less package gated behind ledger/test_ledger_mock now escapes PR CI entirely and only surfaces on main. The PR says it "preserves -race, -tags=ledger,test_ledger_mock" — this is the one place it doesn't.

Cheapest fix is a separate step in this job that keeps the old guarantee without affecting the split:

- name: Build all packages
  run: go build ./...

(Codex flagged this as P1; I agree it's real, though the exposed surface is narrow.)

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Coverage regression vs. the previous command (raised by the Codex pass). The old step used go list ./..., which fed every package to go test — and go test builds a package even when it reports [no test files], so this job was compile-checking the whole tree under -race -tags=ledger,test_ledger_mock. split-test-packages derives from packages.txt, which filters to packages having .TestGoFiles/.XTestGoFiles, so test-less packages are now never built here.

The gap is narrower than it first looks: cross-arch-build.yml runs make build, which compiles the cmd/seid dependency tree with the ledger tag, and golangci.yml type-checks the tree (though with tests: false and build-tags: [codeanalysis], per .golangci.yml). What's left uncovered is test_ledger_mock-gated code and any test-less package not reachable from cmd/seid.

If you want the old guarantee back cheaply, add a go build -tags=ledger,test_ledger_mock ./... step to one shard (or to the aggregate job) rather than reverting the split.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] make will emit go list's warnings but the recipe's success is what gates the step — worth echoing the shard's package count here (e.g. wc -l < "$SHARD_FILE") so a truncated or empty shard is visible in the log rather than showing up as a suspiciously fast green run. Cheap insurance regardless of how the pipefail issue on Makefile:585 is resolved.

SHARD_FILE=build/packages.txt.${{ matrix.shard }}
Comment thread
cursor[bot] marked this conversation as resolved.
PARALLEL=()
if grep -qx "github.com/sei-protocol/sei-chain/occ_tests" "$SHARD_FILE"; then

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] -parallel=1 applies to the whole shard, not just occ_tests. With ~420 test packages split three ways, roughly 140 packages get their subtests serialized.

Note this isn't actually "preserving" prior behavior of this job: the old race job passed no -parallel flag at all (it ran at the GOMAXPROCS default). The -parallel=1 special case comes from the Makefile's coverage-flavored test-group-%, which CI never invoked. So this PR adds a serialization constraint to ~1/3 of the suite in a PR whose goal is wall-clock, and that shard will likely become the long pole.

Consider splitting the invocation instead:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=...
grep -qx "$OCC" "$SHARD_FILE" && go test -parallel=1 -timeout=... "$OCC"

(occ_tests is the only test-bearing package under that tree — occ_tests/utils and occ_tests/messages have no test files.)

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] -parallel=1 is applied to the entire shard, not just occ_tests. Whichever shard draws occ_tests runs all ~140 of its packages with in-package t.Parallel() subtests serialized, which is likely to make that shard the straggler and erode the wall-clock win this PR is chasing.

Note also that the old single-job CI never passed -parallel at all (that special case lived only in the Makefile's test-group-%), so this is a newly-added constraint on CI rather than a preserved one.

Splitting the invocation keeps the constraint where it's actually needed:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=${{ env.GO_TEST_TIMEOUT }}
if grep -qx "$OCC" "$SHARD_FILE"; then
  go test -parallel=1 -timeout=${{ env.GO_TEST_TIMEOUT }} "$OCC"
fi

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This reimplements the occ_tests → -parallel=1 special case that already lives in test-group-% (Makefile:596-605), and the two copies already disagree: CI uses -timeout=30m / default -parallel / no coverage, while the Makefile uses -timeout=10m / -parallel=4 / -coverprofile. So a shard that passes in CI can time out locally under the command the docs tell you to run.

Per AGENTS.md's "guard at the choke point, never at each caller": the shard-running policy should live in exactly one place. Suggest either invoking make test-group-${{ matrix.shard }} here (parameterizing the timeout/coverage flags), or extracting a run-test-shard target the workflow calls, so the special case can't be forgotten or drift on the next edit.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Two things here:

  1. Scope: -parallel=1 is applied to the entire shard, not just occ_tests — roughly a third of the tree loses in-package t.Parallel() concurrency because one package needs serialization. Since the shard file is already in hand, splitting it is cheap: run occ_tests on its own with -parallel=1 and the rest at default parallelism. That also removes the reason this shard will be the slow one.

  2. Drift: the package path is hardcoded here while Makefile:589 already holds it as TARGET_PACKAGE, and the two matchers disagree — this uses grep -qx (exact) where the Makefile uses grep -q (substring). occ_tests/messages and occ_tests/utils are now in packages.txt, so a shard containing only a helper subpackage triggers -parallel=1 locally but not in CI. Pick one matcher and one source of truth.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This applies -parallel=1 to the entire shard, not just to occ_tests.

Note that the previous CI step passed no -parallel flag at all — the -parallel=1 special case only existed in the Makefile's test-group-%, which CI never invoked. So this isn't preserved behavior for CI; it's newly imposed on roughly a third of the repo's packages. Every package that lands in the occ_tests shard now runs its tests serially within each binary, which risks making that shard the long pole and eating into the wall-clock win the sharding is meant to buy.

Consider splitting the invocation instead — occ_tests at -parallel=1, the rest of the shard at the default:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=...
if grep -qx "$OCC" "$SHARD_FILE"; then go test -parallel=1 -timeout=... "$OCC"; fi

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This applies -parallel=1 to the entire shard (~140 packages), not just occ_tests. Note the old single-job command passed no -parallel at all, so this isn't "preserving" a CI behavior — it's newly constraining roughly a third of the tree, and is a plausible reason shard 0 is the slowest at 13m.

Scoping it to the one package that needs it recovers that time:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=${{ env.GO_TEST_TIMEOUT }}
if grep -qx "$OCC" "$SHARD_FILE"; then
  go test -parallel=1 -timeout=${{ env.GO_TEST_TIMEOUT }} "$OCC"
fi

echo "occ_tests present in this shard; forcing -parallel=1"
PARALLEL=(-parallel=1)
fi
xargs go test "${PARALLEL[@]}" -timeout=${{ env.GO_TEST_TIMEOUT }} < "$SHARD_FILE"

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] xargs without -r/--no-run-if-empty still runs the command once when stdin is empty, so an empty shard file executes bare go test -timeout=30m, which tests the repo-root package (mod_test.go, link_directives_test.go) rather than nothing.

This is only reachable when NUM_SPLIT exceeds the package count, so it's not a practical concern — but it does mean the Makefile:591-592 comment ("a shard with zero packages ... still gets an (empty) file instead of breaking test-group-%") doesn't hold as stated. Adding -r here and to Makefile:605 would make it true.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] xargs without -r/--no-run-if-empty still runs the command once on empty input, so an empty shard file invokes go test -timeout=30m with no package arguments — which resolves to the current directory rather than being a no-op. The Makefile:590-592 comment claims pre-touching the shard files keeps NUM_SPLIT > package count working, but the failure just moves to this line (and to Makefile:605, which has the same issue). Adding -r in both places would make the comment true. Unreachable in practice at NUM_SPLIT=3 with ~420 packages, but cheap to close.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] xargs without -r still runs the command once on empty input, so an empty shard file invokes go test -timeout=30m with no package arguments (which resolves to the current directory) rather than being a no-op. This makes the rationale in Makefile:595-596 — pre-touching empty files so an empty shard doesn't break things — only half true; it holds for file existence, not for the invocation. xargs -r closes it on both paths.

Also note "${PARALLEL[@]}" on an empty array under set -u requires bash ≥ 4.4; fine on any current Ubuntu image, but uci-default is a custom pool, so ${PARALLEL[@]+"${PARALLEL[@]}"} would be immune if that image ever lags.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Add -r (--no-run-if-empty). With an empty $SHARD_FILE, xargs still invokes the command once with no arguments, so this becomes go test -timeout=30m in the repo root — testing the root package instead of being the no-op it looks like.

This interacts with the pre-touch loop the Makefile added at line 619: its comment says an empty shard file avoids "breaking test-group-%", but test-group-%'s own cat ... | xargs go test has the same behavior, so an empty shard doesn't skip — it silently retargets. Not reachable at NUM_SPLIT=3, but worth closing since the whole point of the pre-touch is to make empty shards safe.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] GNU xargs runs the command once with no arguments when input is empty, so a zero-package shard would invoke bare go test, which tests the repo-root package (gen.go/mod_test.go/link_directives_test.go) rather than nothing. The Makefile explicitly handles this case ("a shard with zero packages (NUM_SPLIT > package count) still gets an (empty) file"); xargs -r makes the CI side agree.


test-check:
name: Race Detection
runs-on: ubuntu-latest
needs: test
if: always()

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] if: always() also runs this job when the workflow is cancelled. This workflow sets cancel-in-progress: true (line 12), so every superseded PR run will post a red ✗ on the required Race Detection check rather than a cancelled one.

if: '!cancelled()' is the usual fix, or handle the state explicitly:

if: ${{ !cancelled() }}

The skipped case (e.g. shard-indexes fails) is correctly caught by the != "success" comparison and should stay a failure.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] if: always() also runs this job when the workflow is cancelled, in which case needs.test.result is 'cancelled' and the aggregate Race Detection check reports a hard failure instead of a cancellation. Given concurrency.cancel-in-progress: true at the top of this file, pushes to an open PR will hit this routinely and leave a red required check on a superseded run.

if: ${{ !cancelled() }} preserves the "run even when shards fail" behaviour without converting cancellations into failures.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] if: always() also runs this job when the workflow run is cancelled, where needs.test.result is cancelled and the check fails with the misleading message "One or more Race Detection shards failed". With concurrency.cancel-in-progress: true set at the top of this file, cancellation happens on every force-push to an open PR. if: !cancelled() gives the same failure propagation without the spurious red on superseded runs.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] if: always() also runs this job when the workflow run itself is cancelled. Combined with cancel-in-progress: true in the concurrency block at the top of this file, every superseded run will report the branch-protection check named "Race Detection" as a hard failure (needs.test.result is cancelled, which is != "success") rather than as cancelled.

!cancelled() is the usual guard here — it still catches failure and skipped (so a failed shard-indexes job correctly fails the aggregate), but lets genuine cancellations propagate as cancellations:

if: ${{ !cancelled() }}

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] always() also runs when the run is cancelled, and needs.test.result is then cancelled, so this exits 1 and reports a red "Race Detection". With cancel-in-progress: true at the top of the workflow, every superseded run will now show a failing required check (on the stale SHA, so not blocking — just noise). if: !cancelled() gives you the failure-propagation you want without that.

steps:
- name: Check shard results
run: |
if [[ "${{ needs.test.result }}" != "success" ]]; then

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] != "success" also fires on cancelled, so a run cancelled by the cancel-in-progress concurrency group reports the required "Race Detection" check as failed rather than cancelled. Treating skipped as failure is right; consider special-casing cancelled so superseded runs don't leave a red required check.

echo "One or more Race Detection shards failed"
exit 1
fi

coverage:
name: Coverage
Expand Down
11 changes: 7 additions & 4 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,15 @@ make build # build the seid binary into ./build/seid
make install # install seid into $GOBIN
```

Tests run with the race detector and coverage. CI shards them into groups; while
iterating, run a single package directly:
Tests run with the race detector and coverage. `go-test.yml`'s Race Detection job
shards into `NUM_SPLIT` (currently 3) parallel matrix jobs, round-robin split
(package `i` goes to shard `i % NUM_SPLIT`, not a contiguous chunk — see the
`split-test-packages` Makefile target). `make test-group-N` reproduces a given

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] "reproduces a given shard locally with the same package split" is stronger than what's actually guaranteed. packages.txt is produced by go list, which honours GOFLAGS; the CI job sets GOFLAGS: -race -tags=ledger,test_ledger_mock at the job level, whereas a bare local make test-group-0 does not. If any package's test files are gated behind those tags, the package list differs — and because the split is round-robin, one added or dropped package reshuffles the shard assignment of every package after it, not just its own.

Suggest either pinning the tags inside the Makefile target (so the list is tag-independent of the caller), or documenting the invocation as GOFLAGS='-tags=ledger,test_ledger_mock' NUM_SPLIT=3 make test-group-0 and softening the claim to "the same package split, given the same build tags".

shard locally with the same package split:

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] "reproduces a given shard locally with the same package split" is precise and correct — worth keeping that qualifier, because the invocations genuinely differ: test-group-% uses -timeout=10m (vs CI's 30m), pins -parallel=4, adds -coverprofile/-coverpkg=./..., and omits -tags=ledger,test_ledger_mock. Consider one more clause noting the split is what's reproduced, not the flags, so the next reader doesn't infer a failing shard reproduces byte-for-byte.


```bash
make test-group-0 # one CI test shard (race + coverage)
go test ./<pkg>/... # run a single package
NUM_SPLIT=3 make test-group-0 # reproduce CI race shard 0 locally

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] "reproduce CI race shard 0 locally" oversells what test-group-N does. The prose above correctly qualifies it as "the same package split," but this comment reads as a full reproduction — and test-group-% does not set -tags=ledger,test_ledger_mock, which CI sets via GOFLAGS. That tag difference changes what compiles, so a local run can fail (or pass) on packages CI treats differently, which is the most confusing possible way for this to drift. Suggest # same package set as CI race shard 0 plus a note that CI additionally builds with -tags=ledger,test_ledger_mock.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] "reproduce CI race shard 0 locally" overstates what test-group-% does — it reproduces the package split, but not the flags. Concretely, test-group-% (Makefile:635) runs:

  • no -tags=ledger,test_ledger_mock, while the CI job sets it via GOFLAGS. There are real build-tag-gated tests behind it (sei-cosmos/crypto/ledger/ledger_secp256k1_test.go, crypto/keyring/keyring_ledger_test.go, client/keys/add_ledger_test.go, types/bech32/legacybech32/pk_test.go), so the local run silently compiles and runs a different set of files. This is Codex's finding and I agree with it.
  • -timeout=10m vs CI's GO_TEST_TIMEOUT: 30m
  • -parallel=4 vs CI's default (GOMAXPROCS)
  • -coverprofile/-covermode=atomic/-coverpkg=./..., which CI's race job doesn't use and which materially changes runtime

Either add the tags and align the timeout in test-group-%, or soften this line to say it reproduces the package split rather than the shard.

go test ./<pkg>/... # run a single package
```

CI mirrors these checks: `.github/workflows/golangci.yml` runs golangci-lint
Expand Down
51 changes: 46 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,19 +569,60 @@ GO_TEST_FILES != find $(CURDIR) -name "*_test.go"
# default to four splits by default
NUM_SPLIT ?= 4

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Default is 4 but CI is now pinned to 3, so a bare make test-group-0 does not reproduce a CI shard — hence AGENTS.md having to spell out NUM_SPLIT=3. Defaulting to 3 here would make the documented command the natural one and remove a footgun (see the stale-shard-file note below).

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The default is 4 but CI now runs 3. Anyone who runs make test-group-0 without the NUM_SPLIT=3 prefix gets a silently different package set than the shard they're trying to reproduce. Defaulting to 3 here would make the documented command work bare.


# state_db tests run in their own workflow (sei-db-tests.yml); exclude that
# subtree here too so local shards match the CI shards exactly.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] STATE_DB_PKG_PREFIX is now defined in two places that must stay in sync: here, and as a workflow-level env in .github/workflows/go-test.yml:20. After this PR the race job's exclusion comes from this copy (via make split-test-packages) while the coverage job still uses the workflow copy — so if the sei-db/state_db path ever moves, updating only one silently double-runs or silently drops that subtree in one of the two jobs. Worth a cross-reference comment on each, at minimum, noting the other must be updated too.

STATE_DB_PKG_PREFIX := github.com/sei-protocol/sei-chain/sei-db/state_db

$(BUILDDIR):
mkdir -p $@

# The format statement filters out all packages that don't have tests.
# Note we need to check for both in-package tests (.TestGoFiles) and
# out-of-package tests (.XTestGoFiles).
# Includes every package, not just ones with test files: `go test` on a
# package with no tests still compiles it (reported as "no test files"),
# which is how go-test.yml's Race Detection job also acts as a compile
# check under -race -tags=ledger,test_ledger_mock for the whole tree.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This comment justifies including test-less packages by pointing at the compile check CI gets "under -race -tags=ledger,test_ledger_mock" — but test-group-% (line 609) doesn't pass those tags, so the local target compiles a different set of files than the shard it's meant to reproduce. Worth either adding -tags=ledger,test_ledger_mock to the test-group-% invocation, or narrowing the comment to say the compile-check rationale holds for the CI path only.

# Filtering to test-only packages here would silently drop that coverage.
$(BUILDDIR)/packages.txt:$(GO_TEST_FILES) $(BUILDDIR)

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The prerequisite list no longer matches the target's inputs. When packages.txt held only test-bearing packages, $(GO_TEST_FILES) (find -name "*_test.go") was a reasonable proxy. Now that it lists every package, adding a new package with no test file doesn't touch any *_test.go, so a stale build/packages.txt is reused and that package is silently never compiled locally — exactly the compile coverage the comment above says this list exists to provide. CI is unaffected (fresh build/), but make test-group-N drifts.

Consider depending on all *.go files, or making the target .PHONY/always-regenerated since go list is cheap relative to the test run.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Prerequisites no longer match what the recipe reads (also raised by Codex). $(GO_TEST_FILES) is find -name "*_test.go", but as of this change the recipe runs a bare go list ./... — the output no longer derives from test files at all. Adding or deleting a package that contains only production .go files won't invalidate the cached list, so a local shard can miss the new compile check or try to test a package that no longer exists.

The $(BUILDDIR) prerequisite only rescues this intermittently: creating packages.txt.N bumps the directory mtime, but the pre-touch loop on line 598 truncates existing files, which doesn't — so the staleness reappears on subsequent runs. CI is unaffected (fresh checkout), this is a local-dev papercut.

Either widen the prerequisite to all .go files, or drop the file-list prerequisite and mark the target .PHONY — go list is fast enough that unconditional regeneration is cheaper than reasoning about when the cache is valid.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The contents of packages.txt no longer depend only on test files, but the prerequisite list still does.

Before this change the rule emitted only packages with .TestGoFiles/.XTestGoFiles, so keying the target on $(GO_TEST_FILES) (find . -name "*_test.go") was exactly right. Now it emits go list ./... — every package. Add a brand-new package that has no _test.go file (or a new package whose first file isn't a test), and none of the prerequisites change, so make considers build/packages.txt up to date and the package is silently missing from every local shard. The -race compile-check coverage that the new comment above explicitly calls out as the reason for including test-less packages is the exact thing that gets dropped.

CI is unaffected (fresh checkout each run), but make test-group-N locally will quietly under-test. Either widen the prerequisite to all Go files, or just always regenerate:

.PHONY: $(BUILDDIR)/packages.txt

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Switching to go list ./... changes what lands in packages.txt in a way that breaks the occ_tests detection in test-group-% below (line 628), which uses an unanchored grep -q "$(TARGET_PACKAGE)".

occ_tests/messages and occ_tests/utils contain no _test.go files, so they were excluded by the old .TestGoFiles/.XTestGoFiles filter and are now included. All three sort consecutively, so round-robin at NUM_SPLIT=3 puts one on each shard — and the substring grep -q matches all three. Result: every local make test-group-N now runs with -parallel=1.

The workflow got this right with grep -qx; the Makefile should match:

@if grep -qx "$(TARGET_PACKAGE)" $(BUILDDIR)/packages.txt.$*; then \

Separately (Codex's point, and it's a real one now): this target's only file prerequisite is $(GO_TEST_FILES), but the recipe no longer derives from test files at all. Adding, removing, or renaming a package with no tests won't invalidate the cached packages.txt, so the whole-tree compile coverage this comment claims can go stale locally. Depending on go.mod/go.sum in addition, or just accepting a go list on every invocation, would keep the prerequisite honest.

go list -f "{{ if (or .TestGoFiles .XTestGoFiles) }}{{ .ImportPath }}{{ end }}" ./... | sort > $@
go list ./... > $@.tmp

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Non-atomic write leaves a poisoned cache on failure. If grep matches nothing (or the recipe is killed between line 585 and 586), $@ exists but is empty or partial — and because its mtime is now the newest, the next make split-test-packages considers it up to date, produces three empty shard files, and CI goes green having tested nothing.

Build the whole thing into the temp file and rename it into place, so the target only ever appears complete:

$(BUILDDIR)/packages.txt:$(GO_TEST_FILES) $(BUILDDIR)
	go list ./... | grep -v "^$(STATE_DB_PKG_PREFIX)" | LC_ALL=C sort > $@.tmp
	mv $@.tmp $@

(Adding .DELETE_ON_ERROR: at the top of the Makefile would also work, but the rename is local to this rule.)

grep -v "^$(STATE_DB_PKG_PREFIX)" $@.tmp | sort > $@

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] sort here is locale-dependent, which undermines the shard-reproducibility goal. Import paths contain /, -, _, and ., and en_US.UTF-8 collation ignores punctuation on the first pass where C does not — so the same package list sorts differently on a dev machine than on the CI runner, which shifts every package's i % NUM_SPLIT assignment. make test-group-0 then runs a different package set than CI shard 0, silently.

Pin it: LC_ALL=C sort.

@rm -f $@.tmp

TARGET_PACKAGE := github.com/sei-protocol/sei-chain/occ_tests

# Packages whose test suite alone regularly runs 1-4+ minutes under -race.
# Plain i%N round-robin assigns these by their position in the full,
# alphabetically-sorted package list, so several of them can land on the
# same shard by coincidence. Splitting them into their own round-robin
# pass, ahead of the rest of the list, guarantees consecutive heavy
# packages rotate across shards instead of clustering. Re-derive this list
# occasionally from a race job's `ok <pkg> <secs>s` log lines.
HEAVY_TEST_PACKAGES := \
github.com/sei-protocol/sei-chain/sei-db/db_engine/litt/disktable \
github.com/sei-protocol/sei-chain/sei-cosmos/storev2/rootmulti \
github.com/sei-protocol/sei-chain/sei-db/db_engine/litt/test \
github.com/sei-protocol/sei-chain/sei-ibc-go/modules/core/04-channel/keeper \
github.com/sei-protocol/sei-chain/giga/tests \
github.com/sei-protocol/sei-chain/sei-cosmos/x/staking/keeper \
github.com/sei-protocol/sei-chain/evmrpc/tests \
github.com/sei-protocol/sei-chain/sei-ibc-go/modules/core/03-connection/keeper \
github.com/sei-protocol/sei-chain/sei-ibc-go/modules/apps/transfer/keeper \
github.com/sei-protocol/sei-chain/sei-cosmos/x/bank/keeper

# Round-robin split: package i goes to shard i%N.
# Interleaving avoids dumping a whole cluster of alphabetically-adjacent
# (and often runtime-correlated, e.g. a module's many keeper packages)
# packages into one shard, unlike a straight `split -d -n l/N` chunk split.
# Pre-touch all N files first so a shard with zero packages (NUM_SPLIT >
# package count) still gets an (empty) file instead of breaking test-group-%.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The comment's claim doesn't quite hold: an empty shard file doesn't stop test-group-% from running go test. GNU xargs invokes the command once even on empty input unless given -r/--no-run-if-empty, so both cat $(BUILDDIR)/packages.txt.$* | xargs go test … (line 607) and the workflow's xargs go test … < "$SHARD_FILE" would run go test with no package arguments, testing the repo-root package instead. Adding -r to both xargs calls would make the comment true. Not reachable at NUM_SPLIT=3 with ~420 packages, but the mitigation is what the comment is documenting.

# HEAVY_TEST_PACKAGES is round-robined separately, and first, so its own
# i%N indexing can't collide with the coincidental clustering above.
split-test-packages:$(BUILDDIR)/packages.txt
split -d -n l/$(NUM_SPLIT) $< $<.
@for i in $$(seq 0 $$(($(NUM_SPLIT)-1))); do : > $(BUILDDIR)/packages.txt.$$i; done

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The pre-touch loop truncates packages.txt.0..N-1 but doesn't remove shard files left over from a larger previous NUM_SPLIT. Since this PR moves CI from 4 to 3, anyone with an existing build/ directory keeps a stale packages.txt.3, and make test-group-3 will silently run a package list from the old split rather than erroring.

@rm -f $(BUILDDIR)/packages.txt.*
@for i in $$(seq 0 $$(($(NUM_SPLIT)-1))); do : > $(BUILDDIR)/packages.txt.$$i; done

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Two staleness edges around shard files, both local-only:

  1. Lowering NUM_SPLIT (e.g. the default 4 → 3) leaves an orphaned build/packages.txt.3 from the previous run, since the pre-touch loop only covers 0..N-1. make test-group-3 then silently runs a stale package set instead of erroring. rm -f $(BUILDDIR)/packages.txt.[0-9]* before the loop would close it.
  2. $(BUILDDIR)/packages.txt is a timestamp-based file target whose prerequisites ($(GO_TEST_FILES), $(BUILDDIR)) are unchanged by this PR, so an existing local packages.txt generated before the new grep -v filter won't be regenerated and will keep listing state_db packages until some _test.go file's mtime changes. Adding $(MAKEFILE_LIST) (or just Makefile) as a prerequisite makes recipe changes invalidate it.

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Only shards 0..NUM_SPLIT-1 are truncated, so shard files from a previous run with a larger NUM_SPLIT survive with stale contents. This is reachable by default: the Makefile default is NUM_SPLIT ?= 4 while CI uses 3, so make test-group-3 after a NUM_SPLIT=3 run silently re-tests a stale package set. Prepend rm -f $(BUILDDIR)/packages.txt.[0-9]* before the pre-touch loop.

@printf '%s\n' $(HEAVY_TEST_PACKAGES) > $(BUILDDIR)/heavy-packages.txt
@grep -Fxf $(BUILDDIR)/heavy-packages.txt $< > $(BUILDDIR)/packages.txt.heavy || true

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Drift in HEAVY_TEST_PACKAGES is silent. If one of these packages is renamed, moved, or deleted, grep -Fxf simply doesn't match it, the entry falls out of the heavy pass with no diagnostic, and the shard balancing quietly degrades back toward the clustering this pass exists to prevent. Given the comment above already asks people to "re-derive this list occasionally," a warning is what would actually prompt that:

@comm -23 <(sort $(BUILDDIR)/heavy-packages.txt) <(sort $<) \
  | sed 's/^/warning: HEAVY_TEST_PACKAGES entry not found: /' >&2

Separately, || true on these two grep calls masks real errors (exit 2 — unreadable/missing pattern file) as well as the intended "no matches" (exit 1). || [ $$? -eq 1 ] is tighter.

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] grep -Fxf emits matches in the order of the searched file, not the pattern file — so this re-sorts HEAVY_TEST_PACKAGES alphabetically (because $< is sorted), discarding the declared order of the list.

That matters: the list is written in what looks like descending-cost order (disktable, rootmulti, litt/test, …, bank/keeper), and the round-robin (NR-1)%n in the following awk depends on that order to interleave expensive packages across shards. After the alphabetical re-sort the actual assignment at NUM_SPLIT=3 is:

  • shard 0: evmrpc/tests, sei-cosmos/x/bank/keeper, sei-db/.../litt/test, ibc .../04-channel/keeper
  • shard 1: giga/tests, staking/keeper, transfer/keeper
  • shard 2: storev2/rootmulti, litt/disktable, 03-connection/keeper

i.e. shard 0 gets 4 heavy packages and the cost ordering is back to alphabetical accident — which is what the separate heavy pass was introduced to avoid.

Swapping the arguments preserves the declared order while still filtering to packages that exist:

@grep -Fxf $< $(BUILDDIR)/heavy-packages.txt > $(BUILDDIR)/packages.txt.heavy || true

@grep -Fxvf $(BUILDDIR)/heavy-packages.txt $< > $(BUILDDIR)/packages.txt.rest || true
@awk -v n=$(NUM_SPLIT) -v dir=$(BUILDDIR) '{print >> (dir "/packages.txt." (NR-1)%n)}' $(BUILDDIR)/packages.txt.heavy
@awk -v n=$(NUM_SPLIT) -v dir=$(BUILDDIR) '{print >> (dir "/packages.txt." (NR-1)%n)}' $(BUILDDIR)/packages.txt.rest
@rm -f $(BUILDDIR)/heavy-packages.txt $(BUILDDIR)/packages.txt.heavy $(BUILDDIR)/packages.txt.rest
test-group-%:split-test-packages
@echo "🔍 Checking for special package: $(TARGET_PACKAGE)"
@if grep -q "$(TARGET_PACKAGE)" $(BUILDDIR)/packages.txt.$*; then \

@seidroid seidroid Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This substring grep -q now diverges from CI's grep -qx (.github/workflows/go-test.yml:79), and the divergence is newly triggered by this PR.

occ_tests/messages and occ_tests/utils contain only non-test .go files, so the old go list -f "{{if (or .TestGoFiles .XTestGoFiles)}}" filter excluded them; the new go list ./... includes them. They sort adjacent to occ_tests (/ < _, and nothing else shares the prefix), so round-robin with NUM_SPLIT=3 puts the three in three different shards. Substring grep -q then matches all three → every local shard runs at -parallel=1, while CI's grep -qx correctly restricts it to the one shard holding occ_tests itself.

Switching this to grep -qx "$(TARGET_PACKAGE)" closes the last piece of the local/CI drift this PR set out to fix.

Expand Down
Loading