seg: add rm-blocks command to remove latest block snapshot files#20554
seg: add rm-blocks command to remove latest block snapshot files#20554sudeepdino008 wants to merge 4 commits intomainfrom
Conversation
Mirrors seg rm-state but targets block/header/transaction segments and their index files in <datadir>/snapshots. Removes the file set at the highest From value (the most recent, smallest, unmerged range), with an interactive KEEP/REMOVE menu and --dry-run support. Accessor index files with composite type names like transactions-to-block.idx are included.
sudeepdino008
requested review from
AskAlexSharov and
Giulio2002
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a new seg rm-blocks subcommand to the snapshots CLI tooling to remove the most recent (highest-From) block snapshot file range from <datadir>/snapshots, including transactions-to-block accessor indexes, with interactive confirmation and --dry-run support.
Changes:
- Register new
rm-blocks(rm-block-snapshots,rm-block-segments) CLI subcommand. - Implement block snapshot filename parsing that accepts composite type names (e.g.
transactions-to-block). - Implement selection/display and deletion flow for the latest block snapshot range, with optional prompting and dry-run output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
DeleteStateSnapshots unconditionally cleans up .tmp artifacts across snapshot directories (including dirs.Snap). rm-blocks currently leaves .tmp files in dirs.Snap untouched, even though they can be created by incomplete snapshot/torrent operations. Consider reusing the same .tmp cleanup logic (at least for dirs.Snap) to keep recovery commands consistent and avoid leaving behind confusing temp files.
| } | |
| return nil | |
| } | |
| } | |
| tmpRemoved, err := cleanupTmpArtifactsInDir(args.Dirs.Snap, args.DryRun) | |
| if err != nil { | |
| return err | |
| } | |
| if args.DryRun { | |
| fmt.Printf("[dry-run] would remove %d .tmp artifacts from %s\n", tmpRemoved, args.Dirs.Snap) | |
| } else if tmpRemoved > 0 { | |
| fmt.Printf("removed %d .tmp artifacts from %s\n", tmpRemoved, args.Dirs.Snap) | |
| } | |
| return nil | |
| } | |
| func cleanupTmpArtifactsInDir(root string, dryRun bool) (uint64, error) { | |
| if root == "" { | |
| return 0, nil | |
| } | |
| if _, err := os.Stat(root); err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return 0, nil | |
| } | |
| return 0, err | |
| } | |
| var tmpPaths []string | |
| if err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if path == root { | |
| return nil | |
| } | |
| if strings.HasSuffix(d.Name(), ".tmp") { | |
| tmpPaths = append(tmpPaths, path) | |
| if d.IsDir() { | |
| return filepath.SkipDir | |
| } | |
| } | |
| return nil | |
| }); err != nil { | |
| return 0, err | |
| } | |
| for _, path := range tmpPaths { | |
| if dryRun { | |
| fmt.Printf("[dry-run] rm %s\n", path) | |
| continue | |
| } | |
| if err := os.RemoveAll(path); err != nil { | |
| return 0, err | |
| } | |
| } | |
| return uint64(len(tmpPaths)), nil | |
| } |
| func DeleteBlockSnapshots(args DeleteBlockSnapshotsArgs) error { | ||
| dirs := args.Dirs | ||
|
|
||
| filePaths, err := dir2.ListFiles(dirs.Snap) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| fmt.Printf("snapshot directory does not exist: %s\n", dirs.Snap) | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| allFiles := make([]blockFileInfo, 0, len(filePaths)) | ||
| var maxFrom uint64 | ||
| for _, p := range filePaths { | ||
| _, name := filepath.Split(p) | ||
| bf, ok := parseBlockFileName(name) | ||
| if !ok { | ||
| continue | ||
| } | ||
| bf.Path = p | ||
| allFiles = append(allFiles, bf) | ||
| if bf.From > maxFrom { | ||
| maxFrom = bf.From | ||
| } | ||
| } |
There was a problem hiding this comment.
DeleteBlockSnapshots introduces new behavior (parsing/selecting latest block snapshot range and deleting associated files) but there are no unit tests alongside the existing snapshots_cmd_test.go coverage for DeleteStateSnapshots. Add tests that create a temp dirs.Snap with multiple ranges/types (including transactions-to-block.idx), verify the highest-From range is selected, and verify DryRun vs non-dry-run behavior.
| if args.DryRun { | ||
| fmt.Printf("[dry-run] rm %s\n", f.Path) | ||
| fmt.Printf("[dry-run] rm %s\n", f.Path+".torrent") | ||
| continue | ||
| } | ||
| dir2.RemoveFile(f.Path) | ||
| dir2.RemoveFile(f.Path + ".torrent") |
There was a problem hiding this comment.
This command only deletes the exact companion file path + ".torrent". Snapshot dirs can also contain partial/temporary torrent files with extensions like .torrent<suffix> (see snaptype.IsTorrentPartial / tests). Consider also deleting any files matching path + ".torrent*" (or using existing snaptype helpers) so rm-blocks doesn't leave torrent artifacts behind for the removed snapshot range.
| if args.DryRun { | |
| fmt.Printf("[dry-run] rm %s\n", f.Path) | |
| fmt.Printf("[dry-run] rm %s\n", f.Path+".torrent") | |
| continue | |
| } | |
| dir2.RemoveFile(f.Path) | |
| dir2.RemoveFile(f.Path + ".torrent") | |
| torrentArtifacts, globErr := filepath.Glob(f.Path + ".torrent*") | |
| if globErr != nil || len(torrentArtifacts) == 0 { | |
| torrentArtifacts = []string{f.Path + ".torrent"} | |
| } | |
| if args.DryRun { | |
| fmt.Printf("[dry-run] rm %s\n", f.Path) | |
| for _, torrentArtifact := range torrentArtifacts { | |
| fmt.Printf("[dry-run] rm %s\n", torrentArtifact) | |
| } | |
| continue | |
| } | |
| dir2.RemoveFile(f.Path) | |
| for _, torrentArtifact := range torrentArtifacts { | |
| dir2.RemoveFile(torrentArtifact) | |
| } |
…nd .tmp Address review feedback on #20554: - Drop the local parseBlockFileName/blockFileInfo; snaptype.ParseFileName already handles composite type strings like transactions-to-block, and returns block-scaled From/To so the "step in thousands of blocks" caveat goes away. - Sweep .torrent<suffix> partials (snaptype.IsTorrentPartial) via glob, not just the exact .torrent companion. - Unconditionally clean .tmp artifacts in dirs.Snap via a new sweepTmpFilesInSnapDir helper, mirroring DeleteStateSnapshots. - Replace os.Exit(0) in DeleteBlockSnapshots with a normal return so the caller's defer l.Unlock() runs. - Extract a shared promptRemoveFiles helper for the confirmation prompt. - Surface os.Stat / dir2.RemoveFile errors instead of swallowing them. - Add unit tests covering: latest-range removal (incl. transactions-to-block.idx), dry-run, no-files, torrent partial sweep, and .tmp sweep.
Merge the six block-snapshot tests down to three: a single happy-path test that exercises latest-range removal, transactions-to-block.idx, .torrent / partial .torrent<suffix> companions, and .tmp sweep all at once; a dry-run test; and a no-block-files-still-sweeps-tmp test.
Summary
seg rm-blocks(aliases:rm-block-snapshots,rm-block-segments) alongsideseg rm-state..seg+.idx, includingtransactions-to-block.idx) from<datadir>/snapshots, plus their.torrentcompanions.Fromvalue — the most recent, unmerged range.rm-statedisplay,1) RemoveFile / 4) Exitprompt, and--dry-runsupport.A local parser is used in place of
snaptype.ParseFileNamebecause the latter rejects composite type names liketransactions-to-block, which live next to block segments and must be removed alongside them.Test plan
make erigonbuilds cleanmake lintcleanhoodi_arch_bakcorrectly identifies range002593-002594(3.seg+ 4.idx= 7 files).torrent); previous range002592-002593untouched002592-002593