db/snapshotsync: share segment open/close helpers with caplin snapshots#22172
Merged
Conversation
The two caplin snapshot containers re-implemented RoSnapshots' OpenList machinery: the find-open-segment walk, the optimistic-open error classification (twice inline in freezeblocks' CaplinSnapshots, once per its two segment types), and closeWhatNotInList. The copies had drifted unsafely: neither honored the refcount guard that keeps a segment alive while a View still reads it, and freezeblocks' close path used the embedded Decompressor's Close instead of DirtySegment.close, leaking index handles. Extract FindOpenSegment/ClassifyOpenErr/CloseSegmentsNotInList in snapshotsync, use them from RoSnapshots and both caplin containers, and drop the vestigial 'processed' flag and its stray comment. Caplin close paths now inherit the refcount guard (pinned by TestCloseAndDropNotProtected), full index+seg closing, and removal of stale unopened stubs.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR deduplicates and centralizes snapshot segment open/close logic in db/snapshotsync, then switches both Caplin snapshot containers and RoSnapshots to use the shared helpers to prevent drift and fix safety issues (notably refcount-guarded closes and full segment/index close).
Changes:
- Extracts shared helpers in
snapshotsyncfor finding an already-open segment, classifying open errors, and closing/dropping segments not present in a file list (with refcount guard). - Refactors
freezeblocks.CaplinSnapshotsandsnapshotsync.CaplinStateSnapshotsto use the shared helpers, collapsing duplicated open-list switch arms and reusing the close/drop logic. - Adds a regression test ensuring close/drop honors both the “protected list” and “segment still referenced by a live reader” refcount guard.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/snapshotsync/snapshots.go | Adds reusable helpers (FindOpenSegment, ClassifyOpenErr, CloseSegmentsNotInList) and refactors RoSnapshots to use them. |
| db/snapshotsync/snapshots_test.go | Adds TestCloseAndDropNotProtected to pin refcount-guard behavior. |
| db/snapshotsync/freezeblocks/caplin_snapshots.go | Replaces duplicated open/close machinery with calls into snapshotsync helpers. |
| db/snapshotsync/caplin_state_snapshots.go | Reuses shared open-error classification and close/drop helper logic for caplin state snapshots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 3, 2026
`bytes48.go` and `bytes96.go` were byte-identical whole files modulo the type name; `bytes4.go`, `bytes64.go`, `hash.go`, and `address.go` repeated most of the same method bodies. Since Go can't parameterize `[N]byte` by length and these types must stay comparable arrays, the bodies move to unexported helpers over `[]byte` (`fixedFormat`, `fixedSetBytes`, `fixedTerminalString`, `fixedGenerate`); each method becomes a one-liner passing `b[:]`. Net −169 production lines; no exported API change. Kept bespoke on purpose: `Address`'s EIP-55 checksummed `Hex`/`String`/`Format`; `Bytes4.TerminalString` (`%x` of the whole array — head…tail would overlap at N=4); `Hash.Generate` (`math/rand/v2`, incompatible with the v1 quick-check helper); all Hash/Address-only methods. Safety net: `TestFixedBytesGolden` (+377 lines) pins per type — every Format verb incl. bad-verb labels, String/Hex/MarshalText/Value, TerminalString, SetBytes placement, UnmarshalText/JSON error strings (incl. the `BLSSignature` label), seeded `Generate`, and the EIP-55 canonical example for Address. Goldens were captured from the pre-refactor implementation and stay green through the refactor. First commit is a cherry-pick of the `SetBytes` fix from erigontech#22165 (this consolidation builds on it); it will drop out on rebase once that PR merges. Verified: `go test ./common/...`, `go build ./...`, scoped `golangci-lint` clean (×2), `make erigon`. Part of a dedup series; siblings erigontech#22165–erigontech#22172. --------- Co-authored-by: awskii <artem.tsskiy@gmail.com>
…en ClassifyOpenErr The closeWhatNotInList nameOf closure called the promoted Decompressor.FilePath(), which nil-panics on an unopened stub; use the filePath field, which is set for every tree member. Pinned by TestCaplinStateCloseWhatNotInListDropsUnopenedStub. Also drop ClassifyOpenErr's unused skip return value.
AskAlexSharov
approved these changes
Jul 8, 2026
AskAlexSharov
added a commit
that referenced
this pull request
Jul 8, 2026
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.
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.
The two caplin snapshot containers (
freezeblocks.CaplinSnapshots,snapshotsync.CaplinStateSnapshots) re-implementedRoSnapshots' OpenList machinery — the find-open-segment walk, the optimistic-open error dance (duplicated twice more inline acrossCaplinSnapshots.OpenList's two identical switch arms), andcloseWhatNotInList.The copies had drifted in unsafe ways, which is the real payoff here:
CaplinSnapshots' close path called the embeddedDecompressor.Close()instead ofDirtySegment.close(), so recsplit index handles leaked on every file-list change;Decompressor == nil) were skipped rather than removed; to make that safe,CaplinStateSnapshotsnow derives the protect-list name from itsfilePathfield instead of the promotedDecompressor.FilePath(), which would nil-panic on such a stub (pinned byTestCaplinStateCloseWhatNotInListDropsUnopenedStub);View/RoTxstill references; contract pinned byTestCloseAndDropNotProtected). Caveat: the guard is inert for the caplin containers today — they mark every segmentfrozen, andVisibleSegments.BeginRodoesn't refcount frozen segments — so this is future-proofing rather than an active fix, and the pre-existing close-under-live-view hazard for frozen segments is unchanged by this PR.This extracts
FindOpenSegment/ClassifyOpenErr/CloseSegmentsNotInListin snapshotsync, convertsRoSnapshotsand both caplin containers onto them, collapses the two identical switch arms into one loop, and deletes the vestigialprocessedflag with its stray "Only bob sidecars count" comment (semantics preserved:segmentsMaxstill tracks BeaconBlocks only inCaplinSnapshots;CaplinStateSnapshotstracked it for every file and still does).Deliberately out of scope: making the caplin containers embed
RoSnapshotswholesale; un-freezing caplin segments so the refcount guard actually engages for their views (a refcount-contention trade-off deserving its own PR).Behavior changes (all safety-directional, called out above): full index+seg close, stub removal (incl. the nil-panic fix), refcount guard on caplin close paths (inert today). Everything else is behavior-preserving; snapshotsync/freezeblocks suites green, scoped
golangci-lintclean (×2),make erigonbuilds. Net −120 lines of non-test code (−50 including the two new tests).Part of a dedup series; siblings #22165–#22171.