Fix upstream deletes across independent peer revision spaces - #185
github-actions[bot] wants to merge 5 commits into
Conversation
Gardener-Operation: op_5e746d3660443aa9d21345f7e9a13aa41dc4605e4a9a3d8b3210451bd0331dd8:bc1b93cc3ac5de32532ffba0acf0db9b7c51d98bbdbe7fb2097be51daad0fade
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
commit: |
scuffi
left a comment
scuffi
left a comment
There was a problem hiding this comment.
Devin Review found 3 potential issues.
| } | ||
| const row = db.one<{ rev: number }>("SELECT rev FROM vfs_nodes WHERE inode = ?", live.inode); | ||
| return row !== undefined && row.rev > entry.rev; | ||
| return row !== undefined && row.rev > readWatermark(db, "pushRev", options.backend); |
There was a problem hiding this comment.
馃敶 Remote deletes resurrect pulled files
If a remote file is deleted before its earlier pull is echoed, tombstoneIsStale retains the pulled copy. The next push recreates the deleted file remotely.
Learn more
A pull applies a remote file through applyChanges, giving its local inode a fresh revision without advancing the push watermark. If the remote deletes that file before an echo push, the new guard mistakes the pulled copy for an unpushed local edit and skips the tombstone. The pull checkpoints past the deletion, and pushBlocks later exports the retained file as a live change. The remote consequently gets its deleted file back.
Example: The remote creates /x; the host pulls it at local rev 1 with pushRev 0. The remote deletes /x before the host's next push. The host ignores the delete, checkpoints it, then pushes /x back.
Recommended fix: Distinguish local authored changes from revisions minted by upstream apply when guarding pulls, or otherwise track the provenance of versions that have not yet been echoed. Ensure a later remote delete wins over an earlier pulled file while locally authored, unpushed edits remain protected.
| const row = db.one<{ rev: number }>("SELECT rev FROM vfs_nodes WHERE inode = ?", live.inode); | ||
| return row !== undefined && row.rev > entry.rev; | ||
| return row !== undefined && row.rev > readWatermark(db, "pushRev", options.backend); |
There was a problem hiding this comment.
馃敶 Directory deletes discard unpushed child edits
When a child changes beneath a previously pushed directory, tombstoneIsStale checks only the directory's old revision. A remote directory delete recursively removes the unpushed child edit.
Learn more
The guard compares the directory inode's revision against pushRev, but changing a child does not update that directory inode. Once the directory passes the guard, rm deletes its entire subtree, including children that were edited locally after the last push. The pull then checkpoints the remote tombstone, so those child changes cannot be sent upstream.
Example: Push /project and /project/draft at rev 2. Edit /project/draft locally at rev 3, without pushing it. A remote delete of /project passes because /project remains at rev 1 or 2 and pushRev is 2, destroying the draft.
Recommended fix: For a directory tombstone, inspect descendant live revisions or track dirty subtrees for the selected backend before calling recursive rm. Protect unpushed descendants without treating a clean directory as dirty.
| } | ||
| const row = db.one<{ rev: number }>("SELECT rev FROM vfs_nodes WHERE inode = ?", live.inode); | ||
| return row !== undefined && row.rev > entry.rev; | ||
| return row !== undefined && row.rev > readWatermark(db, "pushRev", options.backend); |
There was a problem hiding this comment.
馃敶 Partial pushes lose unshipped same-revision changes
After a partial same-revision push, tombstoneIsStale treats every path at pushRev as shipped. A remote delete can remove a later, unshipped path before its push.
Learn more
A directory rename stamps all moved nodes with one shared revision in stampRenamedSubtree. A bounded push block can checkpoint partway through that revision, recording both its rev and its last path. readWatermark(..., 'pushRev') discards the path component, so the guard treats later, still-unshipped paths at the same revision as clean. An incoming delete can erase one of those pending changes.
Example: Rename a directory with 3,000 files at rev 40 and ship only the first 2,000 paths, recording {rev:40,path:'/new/f1999'}. If the remote deletes /new/f2999 before the next push, the host accepts that deletion even though /new/f2999 was never shipped.
Recommended fix: Use readPushCursor and compare both revision and path for a live entry rather than comparing against the scalar push watermark. Account for paths whose inode revision was stamped by a rename.
- Let a remote delete win over a pulled version awaiting its echo push by recording revs minted by pull applies (_vfs_upstream_revs). - Check every node in a deleted directory's subtree, not only the directory, so unpushed descendant edits are protected. - Compare against the full push cursor (rev and path), so same-rev paths beyond a partial push are protected. Gardener-Operation: op_29ef360a754d42b37e0381356c12af6d78734c3a98f528dc18cd8bad69630a27:2fb95d57eb10338caf78fbb6584c0a03f9ecadd85a8ac4c0c72bf4de4f4fa155
|
I reproduced all three findings (each is reported twice, by Devin and again by hand) with failing tests on f78ff4b. The proposed commit fixes them in Remote deletes resurrect pulled files: fixed. Pull applies now record the local revs they mint, per backend, in a new
Directory deletes discard unpushed child edits: fixed. The guard now walks the whole subtree being deleted with a recursive CTE. It protects the delete if any node is past the push cursor and was not minted by a pull.
Partial pushes lose unshipped same-rev changes: fixed. The guard compares
Every Checks run, all passing:
|
| AND NOT EXISTS ( | ||
| SELECT 1 FROM _vfs_upstream_revs u WHERE u.backend = ? AND u.rev = n.rev | ||
| )`, |
There was a problem hiding this comment.
馃敶 Remote deletion of updated hardlink stays blocked
After a pull updates /x beside an unpushed hardlink /y, rewritesUnpushedHardlink leaves /x's revision unmarked. A later remote deletion of /x is rejected as local, although deleting /x leaves /y intact.
Learn more
Hardlinks share one inode and its revision, but deleting one name does not delete the other. tombstoneIsStale treats every name of that inode as locally changed when the pulled revision lacks upstream provenance. Skipping provenance preserves an unpushed /y against its own remote tombstone, but also blocks a later authoritative deletion of the updated /x.
Example: /x is already pushed, then the host links it locally as /y without pushing /y. A pull updates /x and a later pull deletes /x. The second pull retains /x because the update's revision was not marked as pulled, even though /y remains linked independently.
Recommended fix: Track pending local hardlink names separately from inode-wide pull provenance, and make the pull tombstone guard check whether the deleted name, rather than any alias's shared inode revision, has an unshipped local change. Cover an update followed by deletion of /x while /y remains unpushed in both apply variants.
Was this helpful? React with 馃憤 or 馃憥 to provide feedback.
A pulled write to one name of a hardlinked file no longer marks the shared inode's new rev as upstream while that inode holds an unpushed local version, so a later remote delete of another name cannot discard the local link. Gardener-Operation: op_789a00a9ac036c1a03cabf304c4c0ed4a78f5d010d9a28320a7422d516373113:b1b75f82ac55517fe6e1459d845c0232ecd34eb183e361f5c43e4bdb561d57e4
|
Unpushed hardlinks lost after remote writes: fixing in the proposed commit. I reproduced it on 4f13766. A pull that rewrites
The three duplicate threads by scuffi (remote deletes resurrect pulled files, directory deletes discard child edits, partial pushes lose same-rev changes) repeat Devin's round-1 findings. They were fixed in 4f13766, and Devin has resolved its copies. The round-1 tests still pass, so I made no further change for them. Checks I ran, all passing:
A bare |
1 participant
Summary
Addresses #167. Requested by #167 (comment).
The existing tombstone guard compares a local node revision with the sender's independent revision counter. Four new integration regressions fail against the original implementation: entry and pack pulls after a high-revision host push, and entry and pack pushes into a higher-revision receiver.
Validation
No real container was required or exercised. Pulls conservatively retain local versions not yet pushed to the selected backend.