cockroachdb / cockroachdb/cockroach
sqlstats,contention: node-global buffer guards not restored when a callback panics
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
Two node-global buffer guards run cleanup as plain statements after calling into code they do not own, so a panic in the callback leaves the guard in a state that affects every subsequent writer on the node. Part of #174944.
**Findings:**
- `contentionutils.ConcurrentBufferGuard.syncLocked`: `rewindBuffer()` is a non-deferred statement after the installed `onBufferFullSync` callback and `flushDone.Broadcast()`. A panic in the callback publishes a guard whose `atomicIdx` is stuck past `size`, so every subsequent writer on the node falls through to `flushDone.Wait()`. It self-heals after one flush interval, with silently double-counted statistics in between. Currently benign only because of what the two callers happen to contain.
- `sslocal.sqlStatsIngestor.flushBuffer`: `statements` (a pooled `*statementBuf`) — `defer statements.release()` is registered and then the buffer is handed to `sink.ObserveTransaction`, an interface call into code the ingester does not own. If a sink panics *and* any sink retained the slice, the pooled buffer is reissued under the retained reference and statement statistics and insights are silently overwritten.
**Next Steps:**
- [ ] `defer c.rewindBuffer()` at the top of `syncLocked`
- [ ] In `flushBuffer`, either copy before handing to sinks or release after the sink loop
Epic: none
Jira issue: CRDB-68128
Contributor guide
Research direction
Start with contentionutils.ConcurrentBufferGuard.syncLocked and sslocal.sqlStatsIngestor.flushBuffer, tracing the callback and sink ownership paths described in the findings. Verify cleanup remains safe when either call panics, and confirm pooled statement data cannot be reused while retained by a sink; run the relevant package tests and ensure subsequent writers and statement statistics remain correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 65/100