cockroachdb / cockroachdb/cockroach

storage: temp-store and SST iterators leaked or double-closed when a panic unwinds

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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.