cl/antiquary: remove overlapping caplin state segments after each dump

[awskii]

Caplin snapshot collections never removed overlapping segments on a running node. RemoveOverlaps was reachable only from cmd/utils/app/snapshots_cmd.go:3549,3556, both with onDelete == nil, so overlaps accumulated until someone ran seg retire by hand and the seeder was never told when that run removed them. EL has done this since BlockRetire.MergeBlocks (db/snapshotsync/freezeblocks/block_snapshots.go:333).

This wires it for caplin state, where overlaps genuinely accumulate: the node dumps at CaplinMergeLimit * 5 = 50,000 slots (cl/antiquary/state_antiquary.go:625), capcli at 10,000, and mainnet ships one ~10.5M-slot merged file per state table.

Changes

• call stateSn.RemoveOverlaps on every IncrementBeaconState, not only after a dump. --caplin.snapgen defaults false, so gating removal on a fresh dump left it unreachable on a normal node, which is where downloaded overlaps land. The dump moved into dumpCaplinStateIfDue so its early returns no longer skip the removal
• removal runs before Seed, so a subset the dump supersedes is not hashed and announced microseconds before it is unlinked
• CaplinStateSnapshots.RemoveOverlaps re-keys the reported names before passing them on. The base reports paths relative to the collection's dir, but the downloader is rooted at dirs.Snap and registers these as caplin/<name>, slash-separated (db/downloader/util.go:87) — a bare name matches nothing and Delete returns nil having done nothing. Putting the shim in the collection covers cmd/utils/app/snapshots_cmd.go:3556 too, and uses path.Join so the key does not become caplin\... on Windows
• RemoveOverlaps skips the callback when nothing is being removed, which is the steady state

What is left out, and why

Beacon blocks and blob sidecars stay unwired. Both collections live in dirs.Snap, shared with EL, and run into two defects in shared code that need their own fixes:

• db/snapshotsync: RemoveOverlaps cannot unlink caplin beacon-block overlaps, leaving them orphaned #23469 — OpenFolder's NoGaps filter drops a covered beaconblocks segment from the dirty set, and RemoveOverlaps can only unlink what is in that set. It would notify the seeder, dropping the .torrent, for a .seg that then stays on disk
• db/snapshotsync: RemoveOverlaps deletes every .tmp in its dir, racing concurrent merges #23470 — RemoveOverlaps unconditionally deletes every .tmp in its directory. A second collection on dirs.Snap calling it from an independent ticker can unlink an EL merge's in-flight compressor output

Caplin state avoids the first outright: its OpenFolder uses the unfiltered AllTypedSegments scan, so covered subsets reach the dirty set and are genuinely unlinked. It is not immune to the second — dir.CreateTemp writes into the output directory, so dirs.SnapCaplin holds in-flight .tmp files too. It is safe only because DumpCaplinState and RemoveOverlaps run sequentially on the single loopStates goroutine, which parallelising the per-table dump would break.

Part of #23412 and #23024, item 2.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[AskAlexSharov]

Review of the current head. The wiring is in the wrong place: on a default node the new RemoveOverlaps call can never run, and on a node where it does run it is ordered after Seed and can silently no-op.

Bugs

1. The call sits inside if s.snapgen, after three early returns — cl/antiquary/state_antiquary.go:625,669

caplin.snapgen defaults to false (cmd/utils/flags.go:1105), so on a normal node DumpCaplinState never runs and RemoveOverlaps is never reached — the download-created overlaps the PR targets stay forever. Even with --caplin.snapgen, the three return nil guards fire first: after a merged v1.x-000000-010500-*.seg lands, from = BlocksAvailable()+1 jumps past 10.5M and from+blocksPerStatefulFile+safetyMargin > currentSlot returns nil for the next ~50k slots (~7 days at 12s/slot), during which every covered subset stays on disk and stays seeded. Overlap removal should not be gated on a fresh dump.

2. Seed runs before RemoveOverlaps, on the dirty set — cl/antiquary/state_antiquary.go:660

SegFileNames(0, to) walks the dirty segments (db/snapshotsync/caplin_state_snapshots.go:205), so it includes exactly the segments RemoveOverlaps is about to unlink. On the first dump after a merged file lands, every covered subset goes through AddNewSeedableFile -> BuildTorrentIfNeed (hashes the whole .seg when the .torrent is missing) -> announce, and is then deleted microseconds later. A kill in that window leaves fresh .torrent files with no .seg, which AllTorrentPaths (db/downloader/util.go:285) re-adds on the next start. RemoveOverlaps first, then Seed the survivors, removes both the wasted hashing and the window.

3. With no downloader, segments are unlinked but their .torrent files are not — cl/antiquary/state_antiquary.go:658

onDelete stays nil under --snap.no-downloader (node/components/downloader/provider.go:51), so RemoveOverlaps unlinks v1.x-000000-000050-*.seg and leaves ...seg.torrent behind. Restart with downloading enabled and AllTorrentPaths re-adds a torrent whose data file is gone. Before this PR caplin state segments were never unlinked on a running node, so the orphan could not arise. The EL call site (db/snapshotsync/freezeblocks/block_snapshots.go:333) always passes a closure and relies on dbservices.NoopSeederClient{} for the no-downloader case — matching that removes the nil branch too.

4. The key form disagrees with the downloader on Windows — cl/antiquary/state_antiquary.go:680

absPaths joins with filepath.Join, and RpcClient.fixPath (db/downloader/client.go:20) then returns filepath.Rel, so on Windows the name reaching Downloader.Delete is caplin\v1.1-...seg. The registered key is path.Join("caplin", name) = caplin/v1.1-...seg (db/downloader/util.go:87-88; filePathForName at downloader.go:1764 calls filepath.FromSlash, confirming the key is slash-separated). d.torrentsByName misses, t.Drop() never runs, and the node keeps seeding a file it just unlinked — the same silent no-op the PR set out to fix.

The new test hides this: require.Equal(t, filepath.Join("caplin", name), rel) (state_overlap_paths_test.go:41) compares two filepath results, so it passes on Windows while asserting a key the downloader never stores, despite the comment calling it "the torrent key the downloader stores".

5. One failed Delete RPC skips all cleanup until the next dump — cl/antiquary/state_antiquary.go:669

RemoveOverlaps returns fmt.Errorf("onDelete: %w", err) (db/snapshotsync/snapshots.go:1590) before retireSegmentsNotInList and before the .tmp sweep, and the new call site only logs a warning. With an external downloader restarting, or s.ctx already cancelled during shutdown, nothing is removed. Combined with the once-per-50k-slot cadence in the first point, one transient error costs ~7 days of retained overlap on mainnet.

6. onDelete uses s.ctx, not the ctx the function threads everywhere else — cl/antiquary/state_antiquary.go:666

IncrementBeaconState(ctx, to) passes ctx to DumpCaplinState and pruneFrozenStateTables, but the closure captures s.ctx (copying the adjacent Seed line). On shutdown s.ctx cancels first, Delete returns Canceled, and the previous point wipes out the whole removal; meanwhile a caller cancelling ctx cannot interrupt the RPC at all.

Placement

The shim belongs in CaplinStateSnapshots.RemoveOverlaps (db/snapshotsync/caplin_state_snapshots.go:232), which already exists as an override for the nil guard. Re-rooting the reported names there fixes every caller at once; as written, cmd/utils/app/snapshots_cmd.go:3556 (caplinStateSnaps.RemoveOverlaps(nil)) still deletes caplin state segments with no seeder notification, and the next caller that passes a callback repeats the mistake. The root asymmetry is that BaseRoSnapshots.RemoveOverlaps reports paths relative to s.dir while the downloader is rooted at dirs.Snap — EL only works because those happen to be the same directory. absPaths also duplicates the join already in SegFileNames and inverts toRelativePaths (snapshots.go:1618).

Smaller

• The PR body's reason that caplin state is immune to the .tmp blanket-delete is not right: db/seg/compress.go:321 and db/recsplit/recsplit.go:947 both write <name>.<rand>.tmp into dirs.SnapCaplin, which is exactly what RemoveOverlaps sweeps — and the code there already carries a TODO saying so. It is safe today only because DumpCaplinState and RemoveOverlaps run sequentially on the single loopStates goroutine. Worth recording as "single-goroutine", not "different directory", since parallelising the per-table dump jobs would break it.
• onDelete is called even when relativePaths is empty (snapshots.go:1585), which is the steady state once overlaps are gone — a len(relativePaths) > 0 guard skips a pointless gRPC round-trip per dump.

Test

TestAbsPathsReRootOntoTheDownloaderKey restates the function body — delete the RemoveOverlaps call from state_antiquary.go:669 and it still passes. Nothing pins that removal is invoked after the dump, that onDelete is wired only when a downloader exists, or that the names arrive in the downloader's key form. A fake dbservices.DownloaderClient recording its Delete argument, driven through stateSn.RemoveOverlaps(onDelete) on a fixture dir with a real overlap (the pattern in db/snapshotsync/caplin_state_overlap_test.go:143), would go red before the fix and green after.

Also, cl/antiquary already has state_antiquary_test.go covering this file — a 42-line new file for one assertion about a package-private helper is worth folding in there.

[awskii]

Reworked in 4b6de38. All six confirmed.

1 — and it made the PR inert, since snapgen defaults false. The dump moved to dumpCaplinStateIfDue so its early returns are local; removal now runs on every IncrementBeaconState.

2 — reordered: removal first, then Seed on the survivors.

3, 6 — the closure is always passed and captures the threaded ctx; a nil downloader is a no-op inside it.

4 — right, and my test couldn't see it: it compared two filepath results. The shim moved to CaplinStateSnapshots.RemoveOverlaps as you suggested, keyed with path.Join + ToSlash, so snapshots_cmd.go:3556 is covered too. absPaths is gone.

5 — mitigated, not fixed. With removal off the dump cadence a transient error costs one cycle instead of ~7 days. I left the onDelete-before-removal ordering in BaseRoSnapshots alone since EL shares it.

Empty-list guard added.

The .tmp point is yours: dir.CreateTemp writes into the output dir, so dirs.SnapCaplin gets them too. Corrected the body to say single-goroutine, and #23470 carried the same wrong rationale — fixing it there.

Tests in caplin_state_overlap_test.go: ...ReportsDownloaderKeys goes red without the shim, ...SkipsTheCallbackWithNothingToRemove red without the guard. Still nothing pins the call site — driving IncrementBeaconState needs a 50k-slot fixture, so I am flagging that rather than claiming coverage.