cockroachdb / cockroachdb/cockroach

sqlstats: data race in Container.Add deletes from s.mu.txns without holding lock

Open
#166,638 1 comment 0 reactions 0 assignees View on GitHub
branch-master C-bug O-agent T-testeng
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.