Fixes#2237.
Ingest appends to `tx.Observations` and rewrites a transmission's header
fields (`LatestSeen`, observer, path, counts) under `s.mu.Lock`. Several
readers touched that state without the lock. This PR follows the
approach of dborup/CoreScope#475, the reporter's fix: copy what each
reader needs under the RLock it already takes, and sort or encode
outside it. That fix does not apply to upstream as is (the fork has
diverged), so this is a rewrite against current master.
## What changes, per site
| Site | Before | After |
|---|---|---|
| `QueryGroupedPackets` (`store.go`) | Sort read `tx.LatestSeen` after
`RUnlock`; the page (`groupedTxsToPage`, `storeTxDistinctIatas`) was
built from the shared cache with no lock | The sort key is copied under
the existing RLock (`groupedEntry`). The cache holds those entries.
`groupedPage` copies only the requested page's rows under a short RLock;
the maps are built after it |
| `txGetParsedPath` (`store.go`) | Filled `tx.parsedPath` lazily under
RLock, so concurrent readers wrote the same fields |
`pickBestObservation` fills it through `setTxPath` where the path is
written, under the write lock or before the tx is shared |
| `BuildFromStoreWithOptions` (`neighbor_graph.go`) | Ranged
`tx.Observations` after releasing the lock | Each tx's `Observations`
slice header is copied with the packet list |
| `/api/audio-lab/buckets` (`routes.go`) | Read path, observer and count
after `RUnlock` | The sort stays outside the lock (`RawHex` never
changes); the picked rows are read under a second short RLock |
| `trackedBytes` (`/api/stats`, perf stats) | Read with no lock | Read
under `s.mu` |
| Observer analytics (`routes.go`, `observer_analytics.go`) | Read
`tx.Hash` after `RUnlock`; the content-hash migration rewrites it |
`txByID` holds copies made under the existing RLock
(`txAnalyticsFields`) |
`/api/nodes/{pk}/paths` and `GetNodeHopAnalytics`, items 2 in the issue,
are not changed: on current master they already copy under the lock
(`snapshotRPObs`) or run inside it.
Appending to `tx.Observations` never changes the elements an already
copied slice header covers, so copying the header is enough there.
## Tests
`store_read_race_2237_test.go` adds one `-race` regression test per
site. Each runs an ingest-style writer under `s.mu.Lock` against the
reader:
- `TestQueryGroupedPacketsIngestRace2237` (cold and cached paths)
- `TestTxGetParsedPathReadersRace2237`
- `TestBuildFromStoreIngestRace2237`
- `TestAudioLabBucketsIngestRace2237`
- `TestPerfStoreStatsTrackedBytesRace2237`
- `TestStatsTrackedBytesRace2237`
- `TestObserverAnalyticsHashMigrationRace2237`
Plus `TestGroupedPageReflectsHeaderAfterIngest2237` (no race detector):
header fields, `distinct_iatas`, count and order after ingest.
How I checked them:
- With the fix, `go test -race -count=3 -run 'Race2237'` passes.
- Each site reverted on its own, one at a time, makes its test report
`DATA RACE` (between 1 and 24 reports per site).
- `go test ./...` in `cmd/server` passes; `go vet` and `gofmt -l` are
clean.
## Performance
`QueryGroupedPackets` is on the packets page's hot path.
`BenchmarkQueryGroupedPackets2237`, 30K transmissions, median of 8
alternating runs per binary (Windows, noisy: single runs varied up to
2x):
| Case | Before (ns/op) | After (ns/op) | B/op before → after |
|---|---|---|---|
| cold, limit 50 | 1,015,512 | 1,022,879 (+1%) | 1.62 MB → 2.10 MB |
| cold, limit 1000 | 1,711,237 | 1,793,032 (+5%) | 2.93 MB → 3.64 MB |
| cached, limit 50 | 29,439 | 32,608 (+11%, about 3 µs) | 69 KB → 82 KB
|
| cached, limit 1000 | 606,970 | 662,204 (+9%) | 1.38 MB → 1.61 MB |
The extra cost is copying the page's rows. The lock is held for that
copy only (at most `limit` rows), never for the sort or for building the
maps.
## Not done
- `BuildFromStoreWithOptions` still calls `tx.ParsedDecoded()` outside
the lock, and `tx_rewrite_feed.go` resets `decodedOnce` /
`parsedDecoded` under the write lock. That pair is still a race. Fixing
it means parsing under the RLock on the first build, so I left it for a
follow-up rather than widen this PR.
- I ran the new tests under `-race`, not the whole package.
- No load test against a live server; backend only, no browser check.
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
