Skip to content

aggregator: OpenFolder → closeWhatNotInList bypasses the visible-generation retirement path, races concurrent RO tx into nil *Decompressor #22646

Description

@mh0lt

Summary

Aggregator.OpenFolder closes files invalidated by a disk-side change directly through dirtyFiles.CloseIf → FilesItem.closeFiles(), which does i.decompressor.Close(); i.decompressor = nil. Any concurrent RO tx that captured the affected visibleFile.src in DomainRoTx.files before that runs panics as soon as it dereferences dt.files[i].src.decompressor — typical crash site is Decompressor.FileName(). Reachable today whenever Aggregator.OpenFolder() is called at runtime while active RO tx readers exist.

The generation-based reclamation infrastructure introduced by #21397 (aggregatorVisible.retired + reclaimRetiredLocked) already solves this correctly for the merge/prune path (cleanAfterMerge → retireMergeFiles → recalcVisibleFiles(retired)), but closeWhatNotInList and the openFolder → openDirtyFiles(invalidFileItems) path never got rewired onto it.

Where the bypass lives

  • db/state/dirty_files.go:854 — closeWhatNotInList(dirtyFiles, fNames) — the single helper all four domain-level closeWhatNotInList methods call.
  • Callers: db/state/domain.go:382, db/state/history.go:159, db/state/inverted_index.go:293, db/state/snap_repo.go (analogous). Each is invoked from its owner's openFolder/OpenList and directly from openDirtyFiles's invalidFileItems collection path.
  • dirtyFiles.CloseIf(predicate) calls item.closeFiles() immediately on every invalidated item.

Public entry point: Aggregator.OpenFolder() at db/state/aggregator.go:521. Runtime callers that can reach it while other readers hold an aggregator RO tx include (at least):

  • execution/stagedsync/stage_snapshots.go:268 — snapshot fetch/reopen inside the stage loop.
  • cmd/rpcdaemon/cli/config.go:478 and :511 — rpcdaemon reopen on new-snapshot events.
  • cmd/integration/commands/commitment.go:391 — comment: "reopen after snapshot file deletions".
  • db/state/aggregator.go:610 (OptimisticalOptionalOpenFolder) and :883 — internal reopens.
  • The debug_setHead unwind machinery in execution/execmodule/set_head.go — if any downstream caller re-opens the aggregator's file list after an unwind that unlinks files, the same bypass is hit.

Failure signature

panic: runtime error: invalid memory address or nil pointer dereference

... senders.go               (reader path)
... kv_temporal.go            (BeginTemporalRo / view lifetime)
... aggregator.go             (GetAsOf / domain.getLatest)
... domain.go   (~1611–1790)  (dataReader → reusableReader → dt.files[i].src.decompressor)
... db/seg/decompress.go      (Decompressor.FileName)

A reader has captured dt.files[i].src via DomainRoTx (db/state/domain.go:552-573); a concurrent OpenFolder runs after some files are unlinked on disk, so those files are absent from scanResult.domainFiles, so their FilesItems are matched by the closeWhatNotInList predicate and their decompressor field is nulled — while the reader still holds visibleFile.src.

Concrete reader that hits this: the txpool

The txpool runs in parallel to execution — a separate goroutine tree with no synchronising lock — and drains remote-txn batches on its own schedule. Each batch opens a temporal RO tx and reads through the aggregator to resolve sender nonce/balance for validation. That's exactly what an aggregator RO tx is meant to allow. But because execution and the txpool intentionally operate in parallel for throughput, there is no coordination point that prevents execution from calling Aggregator.OpenFolder() (directly or transitively, e.g. after an unwind that unlinks files) while a txpool RO tx is mid-read. When those two overlap, the reader hits a nil *Decompressor and panics.

The panic is caught by a deferred recover on the txpool goroutine, so it presents as a caught error rather than a process crash, but the batch is lost and the goroutine bounces.

Code path (origin/main):

  • txnprovider/txpool/pool.go:468 — func (p *TxPool) processRemoteTxns(ctx context.Context) (err error).
    • pool.go:475-479 — the deferred recover() that converts the panic into a returned error.
    • pool.go:483 — coreTx, err := coreDB.BeginTemporalRo(ctx) — the RO tx whose lifetime overlaps the file-swap window.
    • pool.go:488 — cacheView, err := cache.View(ctx, coreTx) — cacheView backed by the temporal RO tx.
  • txnprovider/txpool/senders.go:215 — func (sc *sendersBatch) info(cacheView kvcache.CacheView, id uint64) ….
    • senders.go:220 — encoded, err := cacheView.Get(addr[:]) — the read that eventually descends through the aggregator to Decompressor.FileName and dereferences the nulled pointer.

The txpool is just the most visible reader. Any long-running consumer that holds a temporal RO tx across a runtime Aggregator.OpenFolder() is exposed to the same panic — the parallelism between the reader and execution is the invariant that makes this reachable, not the identity of the reader.

Why the merge/prune path is safe

Aggregator.recalcVisibleFiles(retired []*FilesItem) (db/state/aggregator.go:1930) attaches invalidated items to the outgoing generation's retired slice. Physical deletion runs only in reclaimRetiredLocked (db/state/aggregator.go:2569), which walks oldest→current while refcnt == 0. Any RO tx that pinned an older generation blocks reclamation of files it references. cleanAfterMerge → retireMergeFiles → recalcVisibleFiles(retired) uses this correctly; closeWhatNotInList does not.

Proposed fix

Rewire closeWhatNotInList (and analogous invalidFileItems handling in each domain's openDirtyFiles) so it returns the invalidated []*FilesItem instead of calling item.closeFiles() directly. Callers thread the returned slice into Aggregator.recalcVisibleFiles(retired) via each domain's openFolder and the top-level Aggregator.openFolder orchestrator. reclaimRetiredLocked closes them oldest-first when the last generation that pinned them drains — same semantic as the merge/prune path.

Scope estimate: four closeWhatNotInList call sites plus their openFolder wrappers plus the top-level Aggregator.openFolder orchestrator (~120 loc net). Merge-driven deletion is untouched (already correct).

Related work

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions