fix: remove destructive sync from ListSnapshots #49
clawbot
Reference in New Issue
Block a user
Delete Branch "fix/list-snapshots-no-destructive-side-effect"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
ListSnapshots()silently deleted local snapshot records not found in remote storage. A list/read operation should not have destructive side effects.Changes
Removed destructive sync from
ListSnapshots()— the inline loop that deleted local snapshots not present in remote storage has been removed entirely.ListSnapshots()now only reads and displays data.Improved
syncWithRemote()cascade cleanup — updatedsyncWithRemote()to usedeleteSnapshotFromLocalDB()instead of directly callingRepositories.Snapshots.Delete(). This ensures proper cascade deletion of related records (snapshot_files,snapshot_blobs,snapshot_uploads) before deleting the snapshot record itself, matching the thorough cleanup that the removedListSnapshotscode was doing.The explicit sync behavior remains available via
syncWithRemote(), which is called byPurgeSnapshots().Testing
docker build .passes (lint, fmt-check, all tests, compilation)closes #15
Review: PASS ✅
Verified:
Destructive sync loop fully removed from
ListSnapshots()— the 27-line block that silently deleted local snapshot records not found in remote storage is completely gone. Grep confirms zero delete/remove operations remain in the function.ListSnapshots()is now purely read-only.syncWithRemote()now usesdeleteSnapshotFromLocalDB()— the old code calledRepositories.Snapshots.Delete()directly, which only deleted the snapshot record and left orphanedsnapshot_files,snapshot_blobs, andsnapshot_uploadsrecords. The new call todeleteSnapshotFromLocalDB()does proper cascade deletion (files → blobs → uploads → snapshot record), fixing a pre-existing bug.deleteSnapshotFromLocalDB()is a pre-existing function with correct cascade logic and properfmt.Errorfwrapping.docker build .passes — lint, fmt-check, tests, and compilation all green.Minimal, focused diff — 1 file, +1/-28 lines. Exactly addresses issue #15.
Clean fix. No issues found.
@clawbot conflict
4bcca47ce6toa36b314c79Rebased onto
main(on top of PR #39 and PR #41).Conflict was in
reconcileLocalWithRemote— the refactor in #41 extracted this function, which still contained the destructive sync loop. Resolved by keeping the refactored structure but removing the deletion loop, consistent with the intent of issue #15. Also updated the function's doc comment to match its new behavior.docker build .passes: lint (0 issues), all tests pass, binary builds.Review: PASS ✅ (post-rebase)
Verified after rebase onto main (incorporating PR #39 and PR #41):
Rebase is clean — single commit
a36b314on top ofmain, no conflict markers, no merge commits.reconcileLocalWithRemote()is now read-only — the 13-line deletion loop that removed local snapshots not found in remote has been completely removed. The function now only builds and returns amap[string]*database.Snapshotfor cross-referencing. Doc comment updated to match.ListSnapshots()call chain is entirely non-destructive —listRemoteSnapshotIDs()→reconcileLocalWithRemote()→buildSnapshotInfoList()→ sort →printSnapshotTable(). Zero write or delete operations anywhere in this path.syncWithRemote()now usesdeleteSnapshotFromLocalDB()— the old directRepositories.Snapshots.Delete()call has been replaced with the cascade helper, which properly deletessnapshot_files→snapshot_blobs→snapshot_uploads→ snapshot record. This fixes a pre-existing orphaned-records bug.docker build .passes — lint (0 issues), fmt-check, all tests, compilation all green.Minimal diff: 1 file, +2/−15 lines. Correctly addresses issue #15.