erigontech / erigontech/erigon
Shutdown race: temporal.DB.Close() tears down the state aggregator while the detached block-retire goroutine may still read it
- 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
Assessment
This issue has not been assessed yet.