Fix atomic commit: prevent silent data loss on merge failure (closes #29)#34
Merged
Conversation
Signed-off-by: Cong Wang <cwang@multikernel.io>
Signed-off-by: Cong Wang <cwang@multikernel.io>
Copy delta files to temp siblings first and only publish via atomic rename once all copies succeed. Propagate copy errors instead of swallowing them, and preserve the branch + delta on failure so a failed merge can never be reported as success or destroy the only delta copy. Signed-off-by: Cong Wang <cwang@multikernel.io>
congwang-mk
force-pushed
the
fix-atomic-commit
branch
from
52e219f to
f7aa10b
Compare
Re-review of the first cut found that publishing copies before applying deletions broke replacing a base directory with a file: the publish rename hit EISDIR and the whole commit failed with EIO (regression vs the pre-fix behavior). Two changes: - StagedMerge now stages deletions too: each tombstoned entry is renamed aside to a .branchfs-trash sibling and only discarded on commit, so a failed merge restores it on Drop. Deletions are staged before copies, and stage_copy clears a directory sitting at a copy's destination (reachable via rename, which leaves no tombstone) so publishing a file over it succeeds. Commit is now all-or-nothing for deletions as well as copies. - Build temp/trash side-paths from the raw OsStr instead of a lossy String so distinct non-UTF8 filenames never collide on one temp path. Adds unit tests for staged deletions, dir->file replacement (with and without an explicit tombstone) and its rollback, nested paths, and non-UTF8 naming; adds an ioctl integration test that replaces a base directory with a file and commits (fails on the previous code). Signed-off-by: Cong Wang <cwang@multikernel.io>
The project had no dev-dependencies and the integration tests already manage their own temp directories with manual Drop cleanup. Replace tempfile::tempdir() in the StagedMerge unit tests with a small RAII TmpDir helper that reuses the uuid crate already in [dependencies], removing tempfile and its transitive deps from the lockfile. Signed-off-by: Cong Wang <cwang@multikernel.io>
congwang-mk
force-pushed
the
fix-atomic-commit
branch
from
e5e9c8e to
bbd928b
Compare
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
1 participant
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.
Summary
Fixes #29.
BranchManager::commitpreviously swallowed copy errors (let _ = fs::create_dir_all(...)/let _ = storage::copy_entry(...)) and then deleted the branch's delta directory unconditionally, so a failed merge (ENOSPC, EACCES, EIO, quota) was reported asOkwhile the only copy of the data was destroyed — contradicting the "Atomic Commit" claim in the README.StagedMergeRAII helper that copies each delta file to a temp sibling of its final destination (the only fallible step), then publishes them with atomic same-filesystemfs::renameonly after all copies succeed. If dropped withoutcommit()(e.g. an error propagates), it removes the staged temps and leaves destination data untouched.walk_files' callback returnResult<()>so copy failures propagate instead of being structurally un-reportable.commitpaths (parent-is-main→ base filesystem; nested → parent delta dir) to stage-then-publish. On failure the branch and its delta are preserved for retry or abort — never a false success.Scope: this delivers error-atomicity within a running daemon. Crash-atomicity is out of scope because branches are in-memory only (no on-disk recovery), so a daemon crash discards the branch map regardless. A rolled-back merge may leave benign empty directories in the destination (documented in code).
Test Plan
cargo test --lib— 3 newStagedMergeunit tests (publish-all, rollback-on-drop, failure-cleanup) passcargo test --test test_ioctl -- --ignored— 7/7 pass (incl. commit + nested commit)cargo test --test test_integration -- --ignored— 22/22 pass (commit/deletion/symlink/rename happy paths unchanged)🤖 Generated with Claude Code