cockroachdb / cockroachdb/cockroach
backup: cleanup skipped and panics erased on backup/restore processor paths
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary:**
Cleanup on several backup and restore processor paths does not run when a panic unwinds through it, and one fan-in pattern erases the panic entirely. Go runs deferred functions during unwinding, so the gap is cleanup that is either not deferred, or deferred behind an `if err != nil` guard — `err` is nil during a panic. Several of these also leak on ordinary error returns today. Part of #174944.
**Findings:**
- `backup.distBackup` / `backup.distRestore` progress fan-in: `progCh`, `tracingAggCh` — these pair a context-guarded consumer loop with an *unguarded* producer send. `ctxgroup.GoCtx` swallows a consumer panic and cancels the group context, but the producer is parked on a bare channel send that ignores the context, so the `defer close(progCh)` never runs and `Wait` never returns. **The panic is erased: no crash, no error, no log line, and the job hangs forever.** This is live today and needs no cluster setting. `restore_processor_planning.go` already does the correct `select` on `ctx.Done()` for one of its sends — the fix is to copy it to the others.
- `backup.restoreDataProcessor.ConsumerClosed`: `rd.qp` (`MemoryBackedQuotaPool`), `rd.aggTimer` — `cancelWorkersAndWait()`, which re-panics by design, runs before the quota pool close, the timer stop and `InternalClose`, none of which are deferred. A recovered worker panic permanently leaks `numWorkers × minWorkerMemReservation` from a monitor that is never stopped.
- `backupinfo.openSSTWriter`: `w` (`objstorage.Writable`) — `cloud.OpenAbortableWriter` is called, then three subsequent error returns abort neither the writer nor its context. Each leaks a cloud writer, an unreachable `context.CancelFunc`, and for S3 a background goroutine blocked on the pipe.
- `backup.newFileSpanStartKeyIterator`: the already-appended `FileIter`s are guarded by an error-only defer, and `fileSpanStartKeyIterator.Close` closes them in an unguarded loop, so one panicking `Close` leaks the rest for the process lifetime.
- `backup.compactionProcessor`: `iter` — `NewBackupCompactionIterator`'s error path returns without closing the `ExternalSSTReader` opened just above. Fires on ordinary errors today.
- `backupsink.FileSSTSink.Close`: `s.ctx`, `s.cancel` — `Close` cancels but never resets them, while `open` only builds a context `if s.ctx == nil`, so reuse after close silently writes through a cancelled context.
- `backup.backupProcessor` cleanup: the deferred cleanup calls `sink.Flush(ctx)` before closing, i.e. it *commits* the in-flight SST rather than aborting it, publishing a partial object on the abandonment path.
- `backup.runBackupProcessor`: `bp.progCh` — `cancel()` and `close(bp.progCh)` are un-deferred, and `cancelAndWaitForWorker` drains the channel.
- `backup.backupResumer` / `backup.restoreResumer` tracing-aggregator update: the `mu.Lock(); map[k] = *ptr; mu.Unlock()` sequence is not deferred here, while the identical logic is deferred in the LDR and changefeed siblings.
**Next Steps:**
- [ ] Make every producer send in the progress fan-ins a `select` on `ctx.Done()`
- [ ] Defer the three cleanup steps in `restoreDataProcessor.ConsumerClosed` before calling `cancelWorkersAndWait`
- [ ] Defer an abort of `w` in `openSSTWriter`, disarmed on success
- [ ] Per-item recover in `fileSpanStartKeyIterator.Close`; fix the error-only defer in `newFileSpanStartKeyIterator`
- [ ] Defer the iterator close in `compactionProcessor`, disarmed once ownership passes
- [ ] Nil out `s.ctx` / `s.cancel` in `FileSSTSink.Close`
- [ ] Abort rather than flush on the backup processor's error/panic path
- [ ] Defer `close(bp.progCh)`
- [ ] Defer the tracing-aggregator update to match the siblings
Epic: none
Jira issue: CRDB-68124
Contributor guide
Research direction
Start with restore_processor_planning.go for the existing context-aware producer-send pattern, then inspect the listed entry points: distBackup, distRestore, ConsumerClosed, openSSTWriter, newFileSpanStartKeyIterator, compactionProcessor, FileSSTSink.Close, backupProcessor, runBackupProcessor, and the resumers. Work through the checklist for each path, verifying cleanup survives errors and panics, producers cannot hang, and abandoned backup output is aborted rather than committed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100