cockroachdb / cockroachdb/cockroach
kv: transaction is not rolled back when a panic unwinds through db.Txn
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
`kv.runTxn` calls `txn.Rollback` only inside `if err != nil`, not in a `defer`, and `Txn.exec` has no deferred rollback either. A panic in any `db.Txn` / `InternalDB.Txn` / `DescsTxn` closure therefore abandons a live `kv.Txn` without rolling it back.
The abandoned transaction does not simply expire. Its heartbeat loop deliberately runs on a context derived from `context.Background()` so it does not inherit caller cancellation (`kvcoord.txnHeartbeater.startHeartbeatLoopLocked`), and it exits only on EndTxn or stopper quiesce. So the intents stay live and heartbeated rather than becoming abandoned, and conflicting traffic blocks on them cluster-wide until the node restarts.
This is one function covering roughly 430 `.Txn(` call sites, and it is the highest-leverage item in the audit. Part of #174944.
**Findings:**
- `kv.runTxn`: the `*kv.Txn` is not rolled back on panic. Rollback is gated on the error return, which is never assigned during an unwind.
- `kvcoord.txnHeartbeater`: the heartbeat context is intentionally detached from the caller, so an abandoned transaction keeps heartbeating and its intents never expire. Equal-priority pushers escape only via `txnwait.IsExpired`, which never fires.
**Suggested fix**
Recover in `runTxn`, kick an abort, and re-panic immediately. The abort should be asynchronous rather than a synchronous `txn.Rollback`: a synchronous rollback issues an `EndTxn` on the unwinding stack, and because same-node requests are evaluated on the caller's goroutine, that can block indefinitely if the original panic left latches held on the very range being addressed. `kvcoord.txnHeartbeater.abortTxnAsyncLocked` already implements a bounded (`abortTxnAsyncTimeout`) asynchronous abort that interlocks with in-flight requests.
Worth doing independent of any panic-recovery work: today the process dies and intents clear lazily by expiry, so an eager bounded abort is an improvement in both cases.
**Next Steps:**
- [ ] Add a panic path to `runTxn` that triggers an async abort and re-panics
- [ ] Confirm the async abort path is safe to invoke from an unwinding goroutine
- [ ] Test: panic inside a `db.Txn` closure, assert no live transaction remains and no intent blocks a subsequent reader
Epic: none
Jira issue: CRDB-68116
Contributor guide
Research direction
Start at kv.runTxn and trace how txn.Rollback is currently gated on the returned error. Read txnHeartbeater.abortTxnAsyncLocked and its bounded timeout path, then verify it is safe during panic unwinding. Add coverage for a panic inside a db.Txn closure; done means the panic is re-raised, no live transaction remains, and a subsequent reader is not blocked by an intent.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100