cockroachdb / cockroachdb/cockroach

kv: transaction is not rolled back when a panic unwinds through db.Txn

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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.