cockroachdb / cockroachdb/cockroach
sqlstats: data race in Container.Add deletes from s.mu.txns without holding lock
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
## Summary
`(*Container).Add` in `pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go:729` performs `delete(s.mu.txns, k)` without holding `s.mu.Lock()` when memory accounting (`s.acc.Grow`) fails for a newly created transaction fingerprint entry. This is a data race: the same map is concurrently accessed under `s.mu` by `RecordTransaction`, `Clear`, `DrainStats`, iterators, and other `Add` calls. The statement stats path at lines 657-662 correctly acquires `s.mu.Lock()` before the delete, confirming this is an omission in the transaction stats path.
## Affected code
- **Primary:** `pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go:729` -- `delete(s.mu.txns, k)` without lock
- **Correct pattern (for comparison):** `pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go:657-662` -- stmt path wraps delete in `s.mu.Lock()`
- **Also correct:** `pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go:234-236` -- `RecordTransaction` locks before delete
## Reproduction
This bug requires internal/unit-test-level access to trigger (memory-limited monitor + concurrent access + OOM during `Add`). No SQL repro is feasible.
**Unit test sketch:**
```
// Setup: Container with memory-limited monitor (small budget)
// and non-nil uniqueServerCount.
container := New(settings, atomicCounters, limitedMonitor, "app", nil)
// Pre-fill container with enough txn entries to nearly exhaust memory.
// Spawn N goroutines calling RecordTransaction on the container.
// Spawn 1 goroutine calling container.Add(ctx, otherWithManyTxns)
// where otherWithManyTxns has enough txn entries to cause Grow to fail.
// Run with -race flag.
// Assert: race detector does NOT fire.
// Current behavior: race detector fires on delete(s.mu.txns, k) at line 729.
```
## Secondary issue: `uniqueServerCount` counter leak
When a new fingerprint entry is created (incrementing the atomic counter) but then deleted due to OOM, the counter is never decremented. This affects all four OOM-delete sites:
- `ss_mem_storage.go:661` (stmt path in Add)
- `ss_mem_storage.go:729` (txn path in Add)
- `ss_mem_writer.go:166` (RecordStatement)
- `ss_mem_writer.go:236` (RecordTransaction)
Over time under memory pressure, this causes premature fingerprint throttling.
## Suggested fix direction
1. **Data race fix:** Wrap lines 726-731 in `s.mu.Lock()`/`defer s.mu.Unlock()`, mirroring the stmt path pattern at lines 656-663.
2. **Counter leak fix:** In all four OOM-delete sites, after deleting the map entry, decrement the corresponding atomic counter if `s.uniqueServerCount != nil`.
3. **Stale comment:** Update or remove the comment at lines 707-709 which incorrectly states memory is already accounted for.
_This issue was found via automated deep static analysis._
Jira issue: CRDB-62044
Contributor guide
Assessment
This issue has not been assessed yet.