celestiaorg / celestiaorg/celestia-node
Re-enable the -race unit-test CI job (disabled since #3147)
- Dominant language
- Go
- Stars
- 996
- Forks
- 1.1k
- Avg merge
- 1d 6h
- Merged PRs (30d)
- 34
Description
The `-race` CI job has been off for ~2 years. I investigated why, found
the blockers are 3 small test-side data races (not production bugs), fixed them,
and verified the full unit suite now passes under `-race`. This issue tracks
landing those fixes and turning the job back on.
## What happened
The unit-test race detector job was disabled in #3147 (Jan 2024), described there
as a *temporary* removal because it "intermittently fails across a handful of
tests". It's still commented out in `.github/workflows/go-ci.yml` today.
## Why it matters
With `-race` off, data races merge unnoticed. Re-enabling it closes a ~2-year gap
and protects the codebase going forward — every PR gets checked automatically.
## What I found
Ran the unit suite under `-race` locally, per-package: **36/41 packages already
clean**. Every failure is **test-side** (shared fixtures/mocks or racy test code),
not a production bug:
1. **`share/eds` + `store/file`** — `TestSuiteAccessor` runs parallel subtests on a
shared `*rsmt2d.ExtendedDataSquare`; rsmt2d caches row/col roots lazily without
locking, so the parallel first-access races.
2. **`shrex_getter`** — the test shares one `headertest.Subscriber` mock between two
managers that call `NextHeader` concurrently.
3. **`libs/utils`** — `TestSessions_ConcurrentDifferentKeys` updates a plain int from
many goroutines (the `Sessions` implementation itself is correct).
4. **`api/client` `TestSubmission`** — *not* a race; an intermittent embedded-testnode
failure ("nil block meta for height 2"), same framework as #4958. Pre-existing
flake, out of scope here.
## What I propose
Land the three test fixes as small PRs (each reproduces the race under `-race`),
then flip the job back on:
- [x] `libs/utils`: `maxActive` → atomic
- [x] `shrex_getter`: give each manager its own subscriber
- [x] `share/eds`: warm the EDS roots once before the parallel subtests
- [ ] re-enable the `-race` job in `go-ci.yml` (closes #3147)
## Verified
With all three fixes applied, the **full unit suite passes under `-race` — 41/41
packages**, locally. Re-enabling may still surface rarer intermittent races on CI
hardware; those can be handled iteratively.
I'd like to drive this. Separately, I've found several latent production races by
inspection that the current tests don't exercise — I'll send those as follow-ups
with regression tests once `-race` is back to guard against regressions.
Contributor guide
Research direction
Start with .github/workflows/go-ci.yml and review the commented unit-test race-detector job. Read the listed tests in libs/utils, shrex_getter, and share/eds, then run the full unit suite with -race. Done means the three test-side races remain fixed and all 41 packages pass with the CI job enabled.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, go
- Domain
- ci-cd, testing-qa
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100