db/state: move `refcnt` to `visibleFiles` object by AskAlexSharov · Pull Request #21397 · erigontech/erigon

AskAlexSharov · 2026-05-25T06:37:38Z

Context

Aggregator.BeginFilesRo() was made lock-free in #20462/#20490, but physical
file deletion stayed gated by two per-FilesItem atomics (refcount + canDelete).
Two atomics guarding one destructive action (closeFilesAndRemove) is the TOCTOU
double-free behind #21384, and a per-file refcount taken after the snapshot
pointer is loaded can't protect the load→pin window by itself.

What this does

Replaces per-file refcount/canDelete with MVCC reclamation gated by a refcount
on the published bundle (aggregatorVisible) — the MDBX freelist model (a page
freed at txnid T is reclaimable once the oldest live reader's txnid > T),
realized in Go by reference-counting the generation object instead of each file.

Published bundles form an oldest→newest chain; a reader pins exactly one via
refcnt. refcnt only grows while a bundle is current, only shrinks once superseded.
BeginFilesRo does validate-after-pin (one atomic add + re-check), closing the
load→pin window. One add instead of dozens of per-file increments.
Files removed from dirtyFiles by a merge/prune are attached to the outgoing
generation's retired set and physically deleted only once that generation
(and every older one) drains — reclaimed oldest-first, single owner of
closeFilesAndRemove, no per-file flag, no double-free.
DebugBeginDirtyFilesRo (BuildMissedAccessors) pins the generation the same way,
so its captured dirty files — including unindexed ones absent from the visible
set — are protected for the duration of the accessor build.

FilesItem.refcount/canDelete are now used only by the forkable subsystem
(out of scope here).

Design + file lifecycle: docs/plans/20260525-lockfree-file-reclamation-spec.md.

Status

WIP. Validated locally: db/state/... under -race (no data races), make lint,
make erigon integration.

Copilot

Pull request overview

This PR refactors db/state snapshot-file deletion to avoid TOCTOU/double-free hazards by moving from per-FilesItem refcount/canDelete gating to generation-based (bundle) refcounting with oldest-first reclamation of retired files.

Changes:

Add generation-chained aggregatorVisible bundles with a single refcnt pin per reader, plus a retired set reclaimed when the oldest drained generation advances.
Update merge/cleanup paths to “retire” files (remove from dirtyFiles) and defer physical deletion to the bundle reclaimer.
Update debug pinning and GC tests to validate deferred deletion behavior under both normal readers and debug accessor-building readers.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/plans/20260525-lockfree-file-reclamation-spec.md	New design spec describing generation-based reclamation and invariants.
db/state/aggregator.go	Implements bundle `refcnt` pinning, generation chain, retired-file reclamation, and updated merge cleanup plumbing.
db/state/dirty_files.go	Replaces `deleteMergeFile` with `retireMergeFiles` and removes per-visible-file refcount increment/decrement helpers.
db/state/merge.go	Threads “retired files” up from domain/history/II cleanup so aggregator can publish+defer deletion.
db/state/domain.go	Removes per-file refcount pin/unpin in domain RO tx lifecycle (now covered by bundle pin).
db/state/history.go	Removes per-file refcount pin/unpin in history RO tx lifecycle (now covered by bundle pin).
db/state/inverted_index.go	Removes per-file refcount pin/unpin in II RO tx lifecycle (now covered by bundle pin).
db/state/aggregator_debug.go	Debug dirty-files RO tx now pins the current bundle generation instead of per-file refcounting.
db/state/gc_test.go	New tests verifying deferred deletion semantics for normal readers, debug pins, and concurrent reclaim.
db/state/aggregator_debug_test.go	Updates test to assert debug Close is not a deleter (reclaimer is sole deleter).
db/state/squeeze.go	Wraps `recalcVisibleFiles(nil)` with `dirtyFilesLock` where needed and updates signature calls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

TestDirtyFilesRoTx_CloseIsNotADeleter leaked the seg.Decompressor FD, so on Windows t.TempDir cleanup could not unlink the still-open file. Close it via defer (the Close() under test must not delete the file, only release the FD). Also refresh a stale "via FilesItem.refcount" comment — the debug RoTx now pins the bundle generation.

yperbasis

Approving — single-owner reclamation is the right fix, and TestHistoryVerification_SimpleBlocks is clean under -race -count=5 locally. Three non-blocking nits:

The refcount==0 && canDelete → closeFilesAndRemove() TOCTOU (this PR's whole point) still lives in the out-of-scope forkable subsystem — snap_repo.go:119/:123 and proto_forkable.go:332-333, plus the FilesItem.refcount/canDelete fields they keep alive. The double-free class survives there; worth a follow-up to fix/remove forkable or a comment asserting it's dead.
checkForVisibility's canDelete guard (dirty_files.go:685) is now dead on the agg path (canDelete is never set after this PR) — the spec listed it for removal.
openFolder() calls recalcVisibleFiles relying on the caller holding dirtyFilesLock, but unlike reclaimRetiredLocked nothing in its name/comment signals that — a future caller could regress it. A one-line comment or a Locked suffix would pin it down.

`frozen` cached `stepCount >= stepsInFrozenFile` only to skip per-file refcount atomics in BeginFilesRo(). Since reader pinning moved to a single bundle-level refcount (aggregatorVisible, #21397), that optimization is obsolete. This removes the field and the guards that depended on it: the per-file refcount `!frozen` skip in the forkable/snapshot begin+close paths, the "don't delete frozen files" guard in closeFilesAndRemove, and the frozen skip in garbage(). newFilesItem() drops its now-unused stepSize/ stepsInFrozenFile params; newFilesItemWithSnapConfig and the orphaned SnapshotConfig.StepsInFrozenFile() are removed. Step 2 of #21306.

…t retention window Frozen History/InvertedIndex files (Accounts/Storage/Code/Receipt domains plus the standalone log/trace inverted indices) were never deleted once merged, so long-running non-archive nodes grew disk usage unbounded regardless of --prune.mode. Adds Aggregator.RetireOldHistoryFiles, wired into PruneExecutionStage via the existing --prune.distance retention window, and reuses the lock-free file-reclamation mechanism (PR #21397) for safe deferred deletion while readers may still be pinning the old generation. CommitmentDomain, download-time filtering, and block/tx segment files are intentionally out of scope (follow-up PR) — see #21306.

…t retention window Frozen History/InvertedIndex files (Accounts/Storage/Code/Receipt domains, plus the standalone log/trace inverted indices) were never deleted once merged, so a long-running non-archive node's disk usage grew unbounded regardless of --prune.mode. Adds Aggregator.RetireOldHistoryFiles, wired into PruneExecutionStage via the existing --prune.distance retention window, and reuses the lock-free file-reclamation mechanism (#21397) for safe deferred deletion while readers may still be pinning the old generation. CommitmentDomain, download-time filtering, and block/tx segment files are intentionally out of scope (follow-up PR) — see #21306. Also fixes staticFilesInRange: it selected merge source files by range membership alone, with no check that consecutive files were contiguous. A gap between selected files would silently produce a shorter file list that mergeFiles would still merge into one output file named for the full claimed range, permanently losing the data in the gap.

Step 2 of erigontech#21306 (prune frozen files). `frozen` (a cached `stepCount >= stepsInFrozenFile`) existed only to skip per-file refcount atomics in `BeginFilesRo()`. erigontech#21397 moved reader pinning to a single bundle-level refcount (`aggregatorVisible`) — "one atomic add instead of dozens of per-file adds" — so the field is no longer needed for that optimization. ### Removed - the `frozen` field on `FilesItem` - the per-file refcount `!frozen` skip in the forkable / snapshot begin+close paths (files are now always refcounted) - the "paranoic-mode: don't delete frozen files" guard in `closeFilesAndRemove` and the `frozen` skip in `garbage()` — this is the unprotection Step 3 (prune state history files) needs - `newFilesItem()`'s now-unused `stepSize`/`stepsInFrozenFile` params, `newFilesItemWithSnapConfig`, and the orphaned `SnapshotConfig.StepsInFrozenFile()` ### Behavior note The merged domain-II (`dIdx`) downloader-seed filter in `MergeResult.FilePaths` previously depended on `frozen`; it is now unconditional, matching how domain-value and domain-history merged files were already announced. ### Out of scope (follow-ups) - The RoTx/entity `stepsInFrozenFile` fields are now write-only; removing them cleanly unwinds the exported `NewDomain`/`NewHistory`/`NewInvertedIndex` signatures (external caller in `cmd/integration` + tests), so it is left for a separate change. - `db/snapshotsync`'s own `frozen` (on `DirtySegment`) is untouched — its begin path still uses per-file refcounting (Step 1 not yet done there).

…r readers Aggregator.OpenFolder invalidated files that vanished from disk via closeWhatNotInList -> FilesItem.closeFiles(), nil-ing the shared decompressor in place. A concurrent RO tx that captured the same *FilesItem in DomainRoTx.files then panicked when it next dereferenced the nil decompressor (e.g. txpool sender lookups run in parallel with execution). Route these files through the same visible-generation reclamation the merge/prune path already uses (#21397). detachFilesNotInList removes them from dirtyFiles without closing; openList/openFolder thread the invalidated slice up; Aggregator.openFolder attaches it to the outgoing generation via recalcVisibleFiles(retired). reclaimRetiredLocked closes them only once the last reader pinning that generation drains, so readers never observe a nil decompressor. The Close() teardown paths keep the in-place closeWhatNotInList. Adds a deterministic (non -race) reproduction: a reader pins the generation, a file is removed from disk, OpenFolder runs, and the reader reads again. Fixes #22646.

…r readers Aggregator.OpenFolder invalidated files that vanished from disk in place via closeWhatNotInList -> FilesItem.closeFiles(), nil-ing the shared decompressor. A concurrent RO tx that captured the same *FilesItem in DomainRoTx.files then panicked dereferencing the nil decompressor (e.g. txpool sender lookups run in parallel with execution; OpenFolder runs each sync cycle in stage_snapshots). Route these files through the same visible-generation reclamation the merge/ prune path uses (#21397): detachFilesNotInList removes them from dirtyFiles without closing, openList/openFolder thread the invalidated slice up, and Aggregator.openFolder attaches it to the outgoing generation via recalcVisibleFiles. Files are physically reclaimed only once the last reader pinning that generation drains, so a reader never observes a nil decompressor. Reclaim distinguishes close-only from close-and-delete via FilesItem.canDelete, set once in retire() from the retire reason: merge/prune output is still on disk and must be deleted, but files an external actor already removed (retireReasonDeletedFromDisk) are close-only — never re-deleted, so a same-name recreation (e.g. a re-download) before the readers drain is not clobbered. Adds two deterministic (non -race) tests: a reader survives OpenFolder removing a file it still references, and reclaiming a vanished-then-recreated file does not delete the recreation. Fixes #22646.

…r readers Aggregator.OpenFolder invalidated files that vanished from disk in place via closeWhatNotInList -> FilesItem.closeFiles(), nil-ing the shared decompressor. A concurrent RO tx that captured the same *FilesItem in DomainRoTx.files then panicked dereferencing the nil decompressor (e.g. txpool sender lookups run in parallel with execution; OpenFolder runs each sync cycle in stage_snapshots). Route these files through the same visible-generation reclamation the merge/ prune path uses (#21397): retireFilesNotInList removes them from dirtyFiles without closing, openList/openFolder thread the invalidated slice up, and Aggregator.openFolder attaches it to the outgoing generation via recalcVisibleFiles. Files are physically reclaimed only once the last reader pinning that generation drains, so a reader never observes a nil decompressor. Reclaim closes each retired file and, only if FilesItem.canDelete is set, also deletes it from disk. canDelete is decided per retire reason in retire(): merge/prune output is still on disk and must be removed, but a file an external actor already deleted (retireReasonWasDeleted) is close-only — never re-deleted, so a same-name recreation (e.g. a re-download) before readers drain is not clobbered. Attach the retired slice even when a sibling open errors (only ctx cancellation), so already-detached files are never stranded/leaked. Adds two deterministic (non -race) tests: a reader survives OpenFolder removing a file it still references, and reclaiming a vanished-then-recreated file does not delete the recreation. Fixes #22646.

…r readers Aggregator.OpenFolder invalidated files that vanished from disk in place via closeWhatNotInList -> FilesItem.closeFiles(), nil-ing the shared decompressor. A concurrent RO tx that captured the same *FilesItem in DomainRoTx.files then panicked dereferencing the nil decompressor (e.g. txpool sender lookups run in parallel with execution; OpenFolder runs each sync cycle in stage_snapshots). Route these files through the same visible-generation reclamation the merge/ prune path uses (#21397): retireFilesNotInList removes them from dirtyFiles without closing, openList/openFolder thread the invalidated slice up (typed as retiredFiles), and Aggregator.openFolder attaches it to the outgoing generation via recalcVisibleFiles. Files are physically reclaimed only once the last reader pinning that generation drains, so a reader never observes a nil decompressor. Reclaim closes each retired file and, only if FilesItem.canDelete is set, also deletes it from disk. canDelete is decided per mvcc.RetireReason in retire(): merge/prune output is still on disk and must be removed, but a file an external actor already deleted (RetireReasonWasDeletedFromDisk) is close-only — never re-deleted, so a same-name recreation (e.g. a re-download) before readers drain is not clobbered. Attach the retired slice even when a sibling open errors (only ctx cancellation), so already-detached files are never stranded/leaked. Adds two deterministic (non -race) tests: a reader survives OpenFolder removing a file it still references, and reclaiming a vanished-then-recreated file does not delete the recreation. Fixes #22646.

…r readers Aggregator.OpenFolder invalidated files that vanished from disk in place via closeWhatNotInList -> FilesItem.closeFiles(), nil-ing the shared decompressor. A concurrent RO tx that captured the same *FilesItem in DomainRoTx.files then panicked dereferencing the nil decompressor (e.g. txpool sender lookups run in parallel with execution; OpenFolder runs each sync cycle in stage_snapshots). Route these files through the same visible-generation reclamation the merge/ prune path uses (#21397): retireFilesNotInList detaches them from dirtyFiles without closing, openList/openFolder thread the invalidated slice up (typed retiredFiles), and Aggregator.openFolder attaches it to the outgoing generation via recalcVisibleFiles. Files are reclaimed only once the last reader pinning that generation drains, so a reader never observes a nil decompressor. Reclaim closes each retired file and, only if FilesItem.canDelete is set, also deletes it from disk. canDelete is decided per mvcc.RetireReason in retire(): merge/prune output is still on disk and must be removed, but a file an external actor already deleted (RetireReasonWasDeletedFromDisk) is close-only — never re-deleted, so a same-name recreation before readers drain is not clobbered. Attach the retired slice even when a sibling open errors (only ctx cancellation). Fixes #22646.

AskAlexSharov added 3 commits May 25, 2026 12:14

save

bdaec6c

save

fcd0ed1

save

0f133d7

AskAlexSharov requested review from bloxster, sudeepdino008 and yperbasis as code owners May 25, 2026 06:37

sudeepdino008 requested a review from Copilot May 25, 2026 07:40

Copilot started reviewing on behalf of sudeepdino008 May 25, 2026 07:40 View session

Copilot AI reviewed May 25, 2026

View reviewed changes

Comment thread db/state/aggregator_debug_test.go

Comment thread db/state/gc_test.go Outdated

AskAlexSharov changed the title ~~[wip] db/state: bundle-refcount snapshot file reclamation~~ [wip] db/state: move refcnt to visibleFiles object May 25, 2026

AskAlexSharov mentioned this pull request May 26, 2026

[wip] db/state: only last reader deletes merged files #21395

Closed

AskAlexSharov added 10 commits May 26, 2026 15:06

Merge remote-tracking branch 'origin/main' into alex/think_on_begin_35

1354415

save

e6d934a

save

23af195

save

b0d9c3c

save

0193d53

save

8d58e28

save

87a30b1

save

da54a82

save

5586213

save

7423d3a

AskAlexSharov changed the title ~~[wip] db/state: move refcnt to visibleFiles object~~ db/state: move refcnt to visibleFiles object May 27, 2026

Merge branch 'main' into alex/think_on_begin_35

cd6949a

AskAlexSharov changed the title ~~db/state: move refcnt to visibleFiles object~~ [wip] db/state: move refcnt to visibleFiles object May 27, 2026

AskAlexSharov changed the title ~~[wip] db/state: move refcnt to visibleFiles object~~ db/state: move refcnt to visibleFiles object May 27, 2026

yperbasis approved these changes May 27, 2026

View reviewed changes

yperbasis requested a review from Copilot May 27, 2026 09:55

Copilot started reviewing on behalf of yperbasis May 27, 2026 09:56 View session

yperbasis requested a review from awskii May 27, 2026 09:56

taratorio deleted the alex/think_on_begin_35 branch May 27, 2026 11:46

erigon-copilot Bot mentioned this pull request May 28, 2026

[SharovBot] db/state: fix DATA RACE in closeFilesAndRemove #21465

Closed

3 tasks

AskAlexSharov mentioned this pull request May 28, 2026

[SharovBot] fix data race in closeFilesAndRemove and Decompressor.Close #21477

Closed

3 tasks

AskAlexSharov mentioned this pull request Jul 1, 2026

--prune.mode=minimal to delete old state-history files #22123

Merged

AskAlexSharov mentioned this pull request Jul 1, 2026

db/state: remove FilesItem.frozen field #22126

Merged

AskAlexSharov mentioned this pull request Jul 1, 2026

--prune.mode=minimal must delete old files #21306

Closed

mh0lt mentioned this pull request Jul 21, 2026

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

Closed

AskAlexSharov mentioned this pull request Jul 22, 2026

[r3.6] db/state: retire OpenFolder-invalidated files instead of closing under readers #22657

Closed

AskAlexSharov mentioned this pull request Jul 22, 2026

[r3.6] db/state: retire OpenFolder-invalidated files instead of closing under readers #22661

Merged

awskii mentioned this pull request Jul 26, 2026

db/snapshotsync: refcounted visible-generation retirement core; adopt in caplin state #22400

Closed

awskii mentioned this pull request Aug 5, 2026

Epic: caplin snapshots on the erigondb segment lifecycle #23024

Open

30 tasks

Navigation Menu

db/state: move `refcnt` to `visibleFiles` object - #21397

db/state: move `refcnt` to `visibleFiles` object#21397
taratorio merged 15 commits into
mainfrom
alex/think_on_begin_35

AskAlexSharov commented May 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

yperbasis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

AskAlexSharov commented May 25, 2026

Context

What this does

Status

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

yperbasis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants