cockroachdb / cockroachdb/cockroach

sqlstats,contention: node-global buffer guards not restored when a callback panics

Open
#174,957 1 comment 0 reactions 0 assignees View on GitHub
C-bug O-agent T-observability
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.