erigontech / erigontech/erigon

Shutdown race: temporal.DB.Close() tears down the state aggregator while the detached block-retire goroutine may still read it

Open
#22,115 2 comments 0 reactions 1 assignee Claimed by @AskAlexSharov View on GitHub
ErigonDB
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Summary

Once `temporal.DB.Close()` closes the state aggregator at node shutdown (enabled in #21973, to release snapshot mmaps so the datadir can be deleted on Windows), there is a shutdown-time data race between the aggregator teardown and the **detached background block-retire goroutine**, which is never joined before `chainDB.Close()`.

Found by code inspection while reviewing #21973; **not yet reproduced empirically**. It is `-race`-detectable and requires a block-snapshot retire to be in flight at the moment of shutdown, so it is not triggered by #21973's own tests (their block counts are far below a retire boundary).

## Mechanism

- `Ethereum.Stop()` (`node/eth/backend.go`) cancels `ctx`, then waits on `bgComponentsEg.Wait()`, `execModule.WaitIdle()`, `readAheader.WaitForWarmup()` and the KZG warmup — **none of which join the block-retire goroutine** — before calling `s.chainDB.Close()`.
- The retire goroutine is spawned detached by `BlockRetire.RetireBlocksInBackground` (`db/snapshotsync/freezeblocks/block_snapshots.go:435`), guarded only by a `working` CAS flag; `BlockRetire` exposes no wait/drain method. `ctx` cancellation is cooperative and in-progress compression/dump work does not poll it promptly.
- `BlockRetire` is constructed with the **temporal** chain DB (`node/components/storage/provider.go:142`, `ChainDB kv.TemporalRwDB`), so each `br.db.View(...)` opens a temporal RO tx whose `BeginRo` unconditionally creates an `aggtx` via `stateFiles.BeginFilesRo()` (`db/kv/temporal/kv_temporal.go`).
- `temporal.DB.Close()` closes the aggregator **before** the MDBX handle. `Aggregator.Close()` → `closeDirtyFiles()` (`db/state/aggregator.go`) force-closes the underlying files and does **not** honor the `aggtx` reader refcount (that refcount only guards the normal merge-reclaim path, not `Close`).

So a retire `View` in flight during `stateFiles.Close()` races the file teardown: a concurrent `BeginFilesRo` / aggtx-close against `closeDirtyFiles` (`-race` report; worst case a double-close or read of an unmapped file).

## Why the MDBX side is safe but the aggregator side is not

`RwDB.Close()` drains open read transactions (`waitTxsAllDoneOnClose`), so the retire goroutine's MDBX access is already protected. But `stateFiles.Close()` runs **before** `RwDB.Close()` (intentionally — so the aggregator's *own* background goroutines release their read txns first), and there is no equivalent drain for external `aggtx` holders. The "agg before RwDB" ordering is correct for the aggregator's own goroutines but leaves the external retire goroutine unprotected.

## Proposed fix

Join/drain the block-retire goroutine in `Ethereum.Stop()` **before** `s.chainDB.Close()` — e.g. add a `Wait`/drain to `BlockRetire` (block until the `working` flag clears, or back it with a `WaitGroup`/channel) and call it alongside the other shutdown barriers. After that, the "agg before RwDB" close order is safe.

## References

- Introduced/exposed by #21973 (enables `temporal.DB.Close()` → `stateFiles.Close()`).
- Key sites: `node/eth/backend.go` `Ethereum.Stop()`; `db/snapshotsync/freezeblocks/block_snapshots.go:435`; `node/components/storage/provider.go:142`; `db/kv/temporal/kv_temporal.go` `DB.Close()`; `db/state/aggregator.go` `Aggregator.Close()`/`closeDirtyFiles()`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.