retire only visible files#22365
Open
AskAlexSharov wants to merge 61 commits into
Open
Conversation
db/snapshotsync conflict resolution: main's #22172 extracted the per-segment `DirtySegment.refcount` close model into shared helpers and wired caplin to them. This branch removed per-segment refcount in favour of the generation model (snapshotVisible.refcnt), so the RoSnapshots close path keeps detachNotInList + generation reclamation, while the shared CloseSegmentsNotInList/closeAndDropNotProtected helpers are retained (they dedup caplin's own close code) minus the refcount check, matching caplin's existing no-per-reader-protection behaviour. Kept main's generic FindOpenSegment/ClassifyOpenErr helpers and the OpenList dedup.
Close() deferred recalcVisibleFiles, so the empty generation was published last — after the refcnt check and the segment-close loop. Since View() pins the current generation lock-free (acquireVisible's hazard-pointer re-check), a concurrent View() could load the still-current outgoing generation, increment its refcnt, pass the re-check (s.visible unchanged because the swap was deferred), and be handed segments Close was closing → use-after-close. Publish the empty generation synchronously before reading the outgoing generation's refcnt (captured as prev): a concurrent View() that pinned the outgoing generation now fails its re-check and retries onto the empty one, and any reader that pinned it earlier is counted in prev.refcnt, which we honor. Matches the synchronous mutate-then-publish order Delete already uses.
…ots package Renames snapshotsync.RoSnapshots to BaseRoSnapshots to free up the RoSnapshots name, then moves the freezeblocks-specific RoSnapshots/View wrapper into db/snapshotsync/freezeblocks/blocksnapshots, mirroring the db/rawdb/blockio precedent of extracting a type from its parent package into a nested subpackage that's imported back.
…hots Sibling of freezeblocks rather than nested under it, matching how freezeblocks itself sits directly under snapshotsync.
… fix RemoveBlockSnapshotsBelow view lifetime - temporal.DB gains blockFiles services.BlockSnapshots (the block-data peer of stateFiles); set via New(), wired to the real *RoSnapshots at SetUpBlockReader and nil at state-only/seg/test sites. Accessor BlockSnapshots(). - RemoveBlockSnapshotsBelow now holds the View across Delete (defer Close), like Aggregator.cleanAfterMerge, so the physical unlink defers to reclaimRetired at Close (off the dirtyFilesLock) instead of running mid-Delete under the lock.
…ve_36 Adopts the RoSnapshots package split (snapshotsync.RoSnapshots -> BaseRoSnapshots; freezeblocks.RoSnapshots -> blocksnapshots.RoSnapshots) on top of this branch's blk_rc_36 generation-refcount reclamation. Conflict resolution: kept this branch's reclamation model (detachNotInList, delete->*DirtySegment, recalcVisibleFiles(alignMin, retired), generation-pinned View) and applied the PR's rename to it in snapshots.go and merger.go. RemoveBlockSnapshotsBelow now reaches the base view via sn.BaseRoSnapshots.View().
Mirrors aggtx: BeginTemporalRo/newRwTx pin db.blockFiles.View() for the tx lifetime (nil for state-only DBs), autoClose releases it on every Commit/Rollback, and AsyncClone shares the master's pin. Ties block-file reclamation to the transaction lifetime — a long-running temporal tx now defers block-file unlink through the same blk_rc_36 watermark that already governs state files.
…e_36 Picks up blk_rc_36 now merged to main (#22246) plus Gloas CL (#22091). The three db/snapshotsync conflicts were purely the #22343 rename (this branch renamed snapshotsync.RoSnapshots -> BaseRoSnapshots; main's finalized blk_rc_36 kept the old name): took main's canonical reclamation code (which already includes the Close TOCTOU fix) and re-applied the rename in snapshots.go, merger.go and snapshots_race_test.go.
…le unlink Exercises step 2 end-to-end: with a real block RoSnapshots wired into temporal.DB, an open temporal RO tx pins the block-files view, so RemoveBlockSnapshotsBelow retires the aged tx segment but the file stays on disk until the tx closes, at which point the generation drains and reclamation unlinks it.
…al tx's pinned view (opt-in) Step 3: reads prefer the block-files view pinned by the temporal tx over opening a fresh one, when the tx carries it. - temporal.tx.BlockFilesRo() exposes the tx's blocktx view. - blocksnapshots.View gains a generic Segment(t, blockNum). - BlockReader.viewSingleFile(tx, ...) uses the tx-pinned view when the tx exposes BlockFilesRo() (non-nil), else falls back to r.sn.ViewSingleFile — so non-temporal txs and state-only DBs keep today's behavior exactly. 12 read sites routed through it. Tests: a retired segment vanishes from the live set but the still-open tx keeps resolving it via its pinned view (TestBlockReaderReadsThroughTemporalTxView).
…th a stub tx Replaces the temporal-integration tests (which need the setter enabled) with a lightweight stub-tx test of viewSingleFile's tx-view preference. Temporal integration is covered in the follow-up that enables SetBlockSnapshots.
…irstTxnNumNotInSnapshots; add BeginTemporalRo bench - IterateFrozenBodies and FirstTxnNumNotInSnapshots now take a tx, so BlockReader reads through the tx's pinned view (view(tx)/viewSingleFile(tx)) instead of hardcoding view(nil). FillDBFromSnapshots passes its tx; the download-planning (getMinimumBlocksToDownload) and dump (DumpBlocks) paths have no tx and pass nil. - BenchmarkBeginTemporalRo guards the per-tx pin. vs main: sec/op and allocs/op unchanged (p=1.000), +16 B/op (the one blocktx pointer field).
…umNotInSnapshots in DumpBlocks
…ck; drop merged-only limit
AskAlexSharov
changed the title
The enginextests-benchmark-150m shards intermittently OOM-kill their
16GB hosted runner ("The runner has received a shutdown signal") — 5 of
the last 54 instances, always near the end of the test phase, on
unrelated branches. Peak demand measured for 150m: ~16.8GB = 9.9GB evm
process (live heap 4.9GB doubled by default GOGC; dominated by opReturn
return-data buffers and TemporalMemBatch write-set clones during the
unchunkified_bytecode tests) + 6.9GB MDBX datadir on the 8GB ramdisk,
whose tmpfs bytes compete with the process for the same RAM.
The ramdisk exists for shards that churn hundreds of short-lived
datadirs; the benchmark shards create 3 long-lived ones, so it buys no
wall time there. A/B on identical runners (run 29073818835): test phase
17m09s vs a 16m40s-17m06s five-run ramdisk baseline for 150m-parallel,
13m22s vs 11m14s-13m06s for 150m-sequential; both jobs passed with ~7GB
more headroom.
New manifest key no-ramdisk (opt-in true — a false-valued key would be
invisible to both jq's // default and GitHub expressions' loose ==, so
the enabled state must be the truthy one): the workflow skips the tmpfs
and puts those shards' datadirs under $RUNNER_TEMP, and
run-eest-spec-test.sh skips the Darwin auto-ramdisk, whose 2GB default
the 150m datadir could not fit anyway.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines snapshot file lifecycle handling so that Merge/Retire operate only on the pinned visible set, while cleanup of invisible/garbage leftovers is delegated to dedicated post-merge cleanup paths. It also extends “retire”/prune behavior to cover small aged files that previously could be stuck forever.
Changes:
- Move block transaction snapshot pruning to a visible-only retirement path (
BlockRetire.RetireTransactionFiles) and use it from snapshot stage pruning. - Make state history retirement (
AggregatorRoTx.Retire) select over visible files only, then detach the corresponding sources fromdirtyFilesunder lock. - Introduce visible-only retirement helpers for block snapshots (
RetireFilesBelow/RetireFilesAbove) and update call sites + tests; document lifecycle vocabulary indb/kv.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| execution/stagedsync/stage_snapshots.go | Switch temporal reopen hook and route block tx pruning through BlockRetire.RetireTransactionFiles. |
| db/state/squeeze.go | Use the now-internal filesInRange helper for visible-only selection. |
| db/state/retire_history.go | Rework retirement to select over visible files, defer dirty detachment under lock, add safety checks. |
| db/state/retire_history_test.go | Update tests for AggregatorRoTx.Retire and add coverage for visible-only selection and dirty detachment. |
| db/state/aggregator.go | Update merge path to use filesInRange and new merge input type. |
| db/state/aggregator_files.go | Rename/privatize merge selection container and filesInRange helper. |
| db/snaptype/files.go | Remove unused constant. |
| db/snapshotsync/snapshotsync.go | Replace manual “download past target” cleanup with RetireFilesAbove. |
| db/snapshotsync/snapshots.go | Add RetireFilesBelow/Above, rename internal delete to retireFiles, add trace logging for retire. |
| db/snapshotsync/snapshots_test.go | Update tests for retireFiles and add coverage for new retire semantics and safety checks. |
| db/snapshotsync/snapshots_race_test.go | Update to use retireFiles in race test. |
| db/snapshotsync/freezeblocks/block_snapshots.go | Add BlockRetire.RetireTransactionFiles wrapper over snapshots retire. |
| db/snapshotsync/freezeblocks/block_snapshots_test.go | Add/adjust tests validating tx-segment retirement behavior. |
| db/snapshotsync/blocksnapshots/block_snapshots.go | Make View.Close() nil-safe (supports nil block view). |
| db/services/interfaces.go | Extend BlockRetire interface and replace BlockSnapshots.Delete with RetireFilesAbove. |
| db/kv/temporal/kv_temporal.go | Rename reopen method and make Retire operate on the pinned aggtx; reopen block+state file txs. |
| db/kv/kv_interface.go | Document canonical lifecycle terms and the “visible-only” invariant for Merge/Retire. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Don't export ERIGON_EXECUTION_TESTS_TMPDIR for them at all: with the setup-erigon ramdisk step skipped nothing else sets it (the TMPDIR override in setup-erigon is Windows-gated), so the shard exercises the same env-var-unset path as local runs and the datadirs land in the runner's default temp dir. /tmp and RUNNER_TEMP sit on the same root SSD on hosted runners, so the A/B timing evidence carries over.
Review follow-up: interpolate matrix.shard through a step env var instead of directly into the run block, matching the cache-warming step's TARGET pattern and the template-injection hardening direction of #22363. matrix.shard is repo-controlled, so hygiene rather than a vulnerability fix.
AskAlexSharov
changed the title
…no-ramdisk' into alex/blk_rc_retire_blks_36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Realized next invariants:
visibleFiles. and delete aged.visibleFiles. and delete small merged to big.Such invariants help:
files rotx- means any files visible for them will be visible/alive until theirrotxend.dirtyFiles mutexfrom hot-path, also reduce amount of places where we readingdirtyFilesoverallPR:
pruneBlockSnapshotsmethod intoblockRetireobjectif sn.To()-sn.From() != snaptype.Erigon2MergeLimit { continue }check which prevented removal of small aged files. For example: Minimal node may download very small piece of history (small files) and this files will never get merged to big (because not enough small files)db/kvnaming block