cockroachdb / cockroachdb/cockroach
storage: temp-store and SST iterators leaked or double-closed when a panic unwinds
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
Iterator and reader cleanup on the temp-store and external-SST paths does not run when a panic unwinds through it. These are the `pkg/storage` paths reachable from SQL disk spilling and from bulk jobs; request-evaluation paths are out of scope for this round. Part of #174944.
A leaked pebble iterator pins a `Version` and its memtable, so beyond the memory cost it prevents sstables from being reclaimed — the leak has a disk-full endpoint, not just a memory one.
**Findings:**
- `storage.pebbleMapIterator.Close`: the pebble iterator — `_ = i.iter.Close()` with no `closed`/`inuse` guard, while `pebble.Iterator.Close` returns its `iterAlloc` to a pool. A double close hands one pooled alloc to two owners with no assertion to catch it. `storage.pebbleIterator` already has the guard to copy. Same shape in `backupinfo` (`file_sst.go`).
- `storage.ExternalSSTReader`: `openedReadersByLevel` is set to nil, disarming an otherwise-correct unconditional cleanup defer, *before* the `NewSSTIterator` call it needs to protect. A panic there leaks every opened cloud reader. Apart from this ordering the function is the model shape.
- Non-deferred `iter.Close()`: `storage.CheckSSTConflicts` (three create/seek/`Valid()`/close sequences), `storage.MVCCIncrementalIterator`, `storage.pebbleBatch`, `bulk.sstAdder`, `bulkmerge.suffixingIterator`. 18 such sites against 118 deferred ones, so the convention is already overwhelmingly the right one. The `CheckSSTConflicts` sequences use `MVCCKeyAndIntentsIterKind`, whose `intentInterleavingIter` has reachable assertion panics.
**Next Steps:**
- [ ] Add the `closed` guard to `pebbleMapIterator.Close`, matching `pebbleIterator`
- [ ] Move the `openedReadersByLevel` disarm after `NewSSTIterator` returns
- [ ] Convert the 18 non-deferred `iter.Close()` sites, using the `iter = nil`-before-reopen idiom from `rangefeed.catchUpScan`
Epic: none
Jira issue: CRDB-68118
Contributor guide
Research direction
Start in pkg/storage with pebbleMapIterator.Close and compare it with pebbleIterator, then inspect storage.ExternalSSTReader and backupinfo/file_sst.go. Review the listed non-deferred iter.Close sites, using rangefeed.catchUpScan as the reference for reopen handling. Done means all listed cleanup paths remain safe during panic unwinding and repeated Close calls, with relevant tests passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100