Sync → Manifest can no longer record content that was never uploaded#107
Merged
Conversation
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.
Resolves #84
Problem
state.jsonand the manifest already agreed, and a later sync from another device could push the stale bucket copy back over it; the edit was lost with no conflict copy and no errorChanges
Why
state.json, so the next pass sees a plain local change and pushes itGreptile Summary
This PR fixes a data-loss bug (#84) where the manifest uploaded at the end of a sync was derived from a fresh disk snapshot rather than from what the plan actually did to the bucket. A file edited mid-sync could land in the manifest with content the bucket never received, silently stalling all future uploads of that edit and leaving the vault vulnerable to a remote device pushing a stale copy back over it.
manifestAfterSync(new,plan.ts) derives the post-sync bucket state directly from the plan's actions applied to the pre-sync remote snapshot, so the manifest can never claim content the bucket doesn't hold.adoptLiveStats(new,sync.ts) follows up with a re-snapshot purely to refreshsize/mtimefor stat-skipping on the next pass, only adopting live entries whose hash still matches the manifest; mid-sync edits and creations are left as they appear in the bucket.manifestAfterSync, all fouradoptLiveStatsbranches, and an end-to-end regression test that intercepts aputObjectcall to simulate a mid-sync edit and verifies both the manifest written and the successful re-push on the next pass.Confidence Score: 5/5
Safe to merge — the change is a well-scoped fix to a clearly understood bug, guarded by both targeted unit tests and an end-to-end regression test that reproduces the exact failure mode.
The new
manifestAfterSynclogic is straightforward: it starts from the remote snapshot and applies only the plan's actions, so every branch (push, pushDelete, pull, pullDelete, and all three conflict variants) has a verified correct outcome. TheadoptLiveStatsfunction iterates only manifest entries, so it cannot accidentally add mid-sync creations to the manifest. The e2e test'sputObjectinterception closely mirrors the real race, and the assertions cover both the manifest written after the first pass and the successful catchup on the second pass. No pre-existing invariants are broken.No files require special attention.
Important Files Changed
manifestAfterSync, which correctly derives the post-sync bucket state from plan actions against the pre-sync remote snapshot, handling all five action kinds without re-snapshotting disk.takeSnapshotused for the manifest withmanifestAfterSync+adoptLiveStats; exportsadoptLiveStatsfor unit testing. Logic is correct — stat adoption only fires when hashes match, so mid-sync edits keep the bucket entry.manifestAfterSyncunit tests covering push, pushDelete, pull/pullDelete (no-op), and all three conflict variants; assertions match the expected bucket state for each case.putObject), plus four isolatedadoptLiveStatsunit tests covering the content-match, mid-sync edit, mid-sync deletion, and mid-sync creation branches.Reviews (2): Last reviewed commit: "Address comments" | Re-trigger Greptile