agent: preserve out-of-band file at CopyFromExisting destination (#62)#70
Merged
Conversation
Closes #62. When classify decides CopyFromExisting it inspects the index alone: "receiver has no live row at this path, but holds the requested blake3 elsewhere." The pre-stage then materialised the file via copyFileToPath, which ends in os.Rename(tmpPath, dstAbs) — an atomic overwrite that silently destroyed any file on disk at dstAbs the index hadn't yet observed (NAS web upload, direct scp, a sync aborted before MarkMissing fired). preStageCopyFromExisting now mirrors preMoveSupersedes: before copyFileToPath runs, os.Lstat dstAbs; if it exists, move it to .squirrel-history/run-<receiverRunID>/<relPath>. The receiver owns the move because rclone's --backup-dir doesn't apply to node syncs. Rollback path: when a later step in the same pre-stage fails, the existing rollback removed every dst we materialised; it now also attempts to rename the preserved files back from history to dstAbs. The restore is best-effort — if it fails the bytes are still under run-<id>/ for the user to recover from. Regression test: TestNodeSyncCopyFromExistingPreservesOutOfBandFile seeds an unindexed file at the receiver's pets/a.jpg, runs a sync where pets/a.jpg classifies as CopyFromExisting, and asserts the prior bytes survive at .squirrel-history/run-*/pets/a.jpg while the dedup'd content lands live.
There was a problem hiding this comment.
Pull request overview
This PR hardens node-to-node sync when a path is classified as CopyFromExisting (receiver-side dedup) by preserving any pre-existing, unindexed (“out-of-band”) file at the destination path instead of atomically overwriting it.
Changes:
- In
preStageCopyFromExisting, detect an existingdstAbsand move it into.squirrel-history/run-<receiverRunID>/...before materializing the deduped content. - Extend the pre-stage rollback to attempt restoring any preserved out-of-band files back to their original destinations.
- Add a regression test covering issue #62 to ensure out-of-band bytes are preserved under
.squirrel-history/run-*/....
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agent/sync.go | Preserve existing destination bytes for CopyFromExisting by moving them to history and restore them on rollback. |
| sync/node_test.go | Adds a regression test asserting out-of-band receiver bytes are preserved in .squirrel-history during CopyFromExisting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+782
to
+785
| rollback() | ||
| return fmt.Errorf("preserve out-of-band %s → %s: %w", relPath, histDst, err) | ||
| } | ||
| preserved = append(preserved, preservedMove{histAbs: histDst, dstAbs: dstAbs}) |
Comment on lines
+1704
to
+1708
| // The out-of-band bytes survive at | ||
| // .squirrel-history/run-<receiverRunID>/pets/a.jpg. The receiver | ||
| // run id isn't part of the report so glob the run-* directories | ||
| // and check there's exactly one history entry with our prior | ||
| // bytes. |
This was referenced May 22, 2026
- agent/sync.go preStageCopyFromExisting: tolerate ErrNotExist on the history-preserve rename. If the out-of-band file at dstAbs is unlinked between Lstat and Rename, the path is now clear so the CopyFromExisting copy can proceed as if nothing had been there. Mirrors preMoveSupersedes' "silently continue on ErrNotExist" contract. - sync/node_test.go: tighten the history-glob assertion to require exactly one match (was: len > 0) so the test's comment about "exactly one history entry" matches what it actually checks.
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.
Closes #62.
Summary
When
classifydecidesCopyFromExistingit inspects the index alone — "receiver has no live row at this path, but holds the requested blake3 elsewhere." The pre-stage then materialises the file viacopyFileToPath, which ends inos.Rename(tmpPath, dstAbs). That atomic overwrite silently destroys any file on disk atdstAbsthe index hadn't yet observed (NAS web upload, direct scp, a sync aborted beforeMarkMissingfired).preStageCopyFromExistingnow mirrorspreMoveSupersedes: beforecopyFileToPathruns, statdstAbs; if it exists, move it to.squirrel-history/run-<receiverRunID>/<relPath>first. The receiver owns the move because rclone's--backup-dirdoesn't apply to node syncs.Rollback path: the existing rollback removed every
dstwe materialised; it now also attempts to rename the preserved files back from history todstAbs. Best-effort — if it fails the bytes are still underrun-<id>/for the user to recover from.Test plan
TestNodeSyncCopyFromExistingPreservesOutOfBandFileseeds an unindexed file at the receiver'spets/a.jpg, runs a sync wherepets/a.jpgclassifies as CopyFromExisting, and asserts the prior bytes survive at.squirrel-history/run-*/pets/a.jpgwhile the dedup'd content lands live.go test ./...passes locally including the existingTestNodeSyncCopyFromExistingDedup.Notes