cockroachdb / cockroachdb/cockroach
*: panic recovery and unwind-safety
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Definition used.** A path is unwind safe if, after a panic unwinds through it, the process is left in a state where continued operation is correct: no lock left held, no accounting left un-released, no shared structure left half-mutated, no goroutine left blocked forever.
CockroachDB has historically let a panic crash the process rather than recovering it, on the grounds that neither our own code nor our dependencies are known to be *unwind safe* — a recovered panic could leave the process holding a lock, a memory-monitor reservation, or a half-mutated shared structure, and continuing in that state is worse than restarting into a known-good one.
However as we push more separate workloads, jobs, background processes, observability tools, multi-tenancy, etc all into one single, easy-to-deploy and resource-managing cockroach node, the cost of letting any one of those features bring down the whole node has grown, and particularly when so much of this code may be "nice to have" features rather than critical to serving queries immediately, letting it crash the whole node has started to feel less justified.
However, recovering panics means we need to be confident the code which crashed was unwound safely before we can proceed.
An AI-assisted audit identified some cases where our code is or could be _not_ unwind safe. The worst of these are when a) a failure becomes a silent permanent hang rather than a crash, which is often worse than the behaviour it replaces (if the hang is wide blast radius) such as #174953, #174954, #174960, and the `Wait` cases in #174949 or b) if recovery could cause silent/quiet corruption later, e.g. #174948, and the pooled-buffer case in #174957.
## Identified Likely Unwind-unsafe code by package (for SQL request evaluation and job execution reachable code)
| Issue | Area | Worst case in it |
|---|---|---|
| #174945 | `kv` | `db.Txn` never rolls back on panic; the abandoned txn keeps heartbeating so its intents never expire and block conflicting traffic cluster-wide. ~430 call sites, one function |
| #174946 | `kvclient` | `DistSender.Send` skips draining `responseChs` on panic, so per-range goroutines keep mutating a request the caller has released |
| #174947 | `storage` | Unguarded double close on the temp-store iterator hands one pooled pebble alloc to two owners; leaked iterators also pin sstables and prevent disk reclamation |
| #174948 | `util/span` | A frontier entry is pooled before it is unlinked, so two frontiers alias one entry and can emit a resolved timestamp ahead of unemitted data. Silent |
| #174949 | `sql/flowinfra` | Flow teardown hangs: nothing cancels the flow context, `Wait` has no deadline, `RowChannel.Push` has no context escape, and two registry lock sites give a self-deadlock and an unrecoverable fatal error |
| #174950 | `sql/colexec` | One panicking closer skips every subsequent closer; a leaked FD semaphore token permanently reduces the node's disk-spilling capacity |
| #174951 | `sql` | An abandoned internal-executor iterator wedges a `connExecutor` goroutine and holds a stopper task, so the node can never drain |
| #174952 | `sql/catalog` | A leaked lease refcount stalls DDL indefinitely rather than expiring, because `CountLeases` filters on session liveness |
| #174953 | `backup` | The progress fan-in erases the panic entirely — no crash, no error, no log — and the job hangs forever |
| #174954 | `crosscluster` | Same fan-in shape in PCR |
| #174955 | `kv/bulk` | `SSTBatcher` cleanup re-panics from a deferred `Close` during an unwind, replacing the original panic payload |
| #174956 | `changefeedccl` | A node-lifetime descriptor collection leaks leases on both error returns; unbounded wait in aggregator close blocks drain |
| #174957 | `sqlstats`, `contention` | A panicking callback leaves a node-global buffer guard stuck, blocking every subsequent writer |
| #174958 | `geo/geos` | Three GEOS allocations leaked with no finalizer backstop, invisible to Go memory accounting |
| #174959 | `jobs` | A failed resumer is reused for `OnFailOrCancel` while its goroutines are still writing through it |
| #174960 | `util` | A panic in `singleflight.Do` wedges every later caller for that key forever; `ctxgroup.Go` has no recover; the stopper handle's `recover()` never fires, so async-task panics crash without a report |
Some error paths leak on ordinary error returns and can be fixed on their own merits: #174948, #174952 (both branches), #174956, #174958, and parts of #174953.
Note: Some conditions are fatal and are handled by calling log.Fatal, not recoverable panics, so we do not need to worry about unwind-safety around those.
Test builds deliberately re-panic from the existing recovery helpers to ensure panics are identified and fixed. This however means CI does not exercise any recover-and-continue path today. We should consider a fault-injection mechanism.
## Not covered
`pkg/kv/kvserver` findings are excluded from this round — the filing scope was SQL request evaluation and job execution. Static-analysis proposals for detecting these shapes seem unlikely to be reliable beyond the existing deferunlock check.
Epic: none
Contributor guide
Research direction
This is an umbrella audit covering SQL request evaluation and job execution across the packages and issues listed in the table, with existing recovery helpers and test-build re-panics as the named entry points. Start by reading the referenced issues, then determine a scoped change and completion criteria; the issue currently does not identify a single file, test, or bounded definition of done.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, databases, distributed-systems, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100