From 50f81b8a1d99033cedb17c66b1fa136b9d33d9cb Mon Sep 17 00:00:00 2001 From: Chaser Huang Date: Thu, 8 Oct 2026 14:29:17 +0000 Subject: [PATCH 1/2] fix(fs): enforce POSIX unlink/rmdir semantics and reject tombstones over non-empty directories Assisted-by: pi deepseek-v4.1-flash --- src/branch.rs | 133 ++++++++++++++++++++- src/fs.rs | 235 +++++++++++++++++++++++--------------- tests/test_commit.sh | 39 +++++++ tests/test_integration.rs | 71 ++++++++++++ 4 files changed, 386 insertions(+), 92 deletions(-) diff --git a/src/branch.rs b/src/branch.rs index cc18a56..02fed69 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -97,6 +97,26 @@ fn remove_entry(path: &Path) -> std::io::Result<()> { } } +/// Whether `dest` contains a child of `src` that staging `src` as a deletion +/// would remove without the tombstone covering it. +/// +/// `deleted` reports whether a child name is tombstoned by the same branch. A +/// child that is neither tombstoned nor present in `src` (the commit's delta) +/// survives the tombstone and must not be discarded by `remove_dir_all`. +fn dest_has_uncovered_children(src: &Path, dest: &Path, deleted: impl Fn(&str) -> bool) -> bool { + let Ok(entries) = fs::read_dir(dest) else { + return false; + }; + for entry in entries.flatten() { + let name = entry.file_name().to_string_lossy().to_string(); + if deleted(&name) || src.join(&name).symlink_metadata().is_ok() { + continue; + } + return true; + } + false +} + /// An in-progress merge that prepares all of its destructive work as /// rollback-able side files, then publishes it in one near-infallible phase. /// @@ -820,6 +840,24 @@ impl BranchManager { if parent_name == "main" { // Direct child of main: apply to the base filesystem atomically. // + // A tombstone naming a directory cannot be replayed precisely: + // `stage_delete` renames the whole directory aside, discarding + // children the tombstone does not cover. Refuse before staging. + for path in &child_tombstones { + let dest = self.base_path.join(path.trim_start_matches('/')); + if dest.symlink_metadata().map(|m| m.is_dir()).unwrap_or(false) + && dest_has_uncovered_children( + &child_files_dir.join(path.trim_start_matches('/')), + &dest, + |name| child_tombstones.contains(&format!("{}/{}", path, name)), + ) + { + return Err(BranchError::Invalid(format!( + "tombstone '{path}' covers a non-empty base directory" + ))); + } + } + // Phase 1 (rollback-able): stage tombstone deletions (rename the // base entries aside) and copy every delta file to a temp sibling. // Deletions are staged before copies so a path whose type changed @@ -828,6 +866,7 @@ impl BranchManager { // here propagates, `staged` is dropped (temps removed, trashed // entries restored), and the branch + its delta are preserved for // retry or abort — never a false success or a partial mutation. + let mut staged = StagedMerge::new(); for path in &child_tombstones { let full_path = self.base_path.join(path.trim_start_matches('/')); @@ -890,6 +929,27 @@ impl BranchManager { let parent_files_dir = parent.files_dir.clone(); let mut parent_tombstones = parent.get_tombstones(); + // A child tombstone naming a directory in the parent's delta must + // not swallow that directory's surviving children. Check before + // any entry is renamed aside. + for tombstone in &child_tombstones { + let parent_delta = parent_files_dir.join(tombstone.trim_start_matches('/')); + if parent_delta + .symlink_metadata() + .map(|m| m.is_dir()) + .unwrap_or(false) + && dest_has_uncovered_children( + &child_files_dir.join(tombstone.trim_start_matches('/')), + &parent_delta, + |name| child_tombstones.contains(&format!("{}/{}", tombstone, name)), + ) + { + return Err(BranchError::Invalid(format!( + "tombstone '{tombstone}' covers a non-empty parent delta" + ))); + } + } + // Phase 1 (rollback-able): stage the merge into the parent's delta // directory. For each child tombstone, stage the deletion of any // matching parent-delta entry (renamed aside) and record the @@ -897,6 +957,7 @@ impl BranchManager { // Deletions are staged before copies so a type change at a path // (e.g. a directory replaced by a file) is cleared first. Any error // here drops `staged`, restoring the parent's delta untouched. + let mut staged = StagedMerge::new(); for tombstone in &child_tombstones { let parent_delta = parent_files_dir.join(tombstone.trim_start_matches('/')); @@ -1028,7 +1089,8 @@ impl BranchManager { #[cfg(test)] mod staged_merge_tests { - use super::{commit_side_path, StagedMerge}; + use super::{commit_side_path, dest_has_uncovered_children, BranchManager, StagedMerge}; + use crate::error::BranchError; use std::fs; use std::path::{Path, PathBuf}; @@ -1292,4 +1354,73 @@ mod staged_merge_tests { // Lossy conversion would map both to U+FFFD and collide; raw OsStr must not. assert_ne!(commit_side_path(&p1, "tmp"), commit_side_path(&p2, "tmp")); } + + #[test] + fn uncovered_children_detects_live_base_entries() { + let tmp = TmpDir::new(); + let src = tmp.path().join("src"); + let dst = tmp.path().join("dst"); + fs::create_dir_all(&src).unwrap(); + fs::create_dir_all(&dst).unwrap(); + write(&dst.join("keep"), b"live"); + + assert!( + dest_has_uncovered_children(&src, &dst, |_| false), + "a base child with no delta entry must be reported as uncovered" + ); + assert!( + !dest_has_uncovered_children(&src, &dst, |name| name == "keep"), + "a tombstoned child is covered by the tombstone" + ); + + write(&src.join("keep"), b"delta"); + assert!( + !dest_has_uncovered_children(&src, &dst, |_| false), + "a child present in the commit delta is covered" + ); + } + + #[test] + fn uncovered_children_ignores_covered_nested_child() { + let tmp = TmpDir::new(); + let src = tmp.path().join("src"); + let dst = tmp.path().join("dst"); + fs::create_dir_all(&src).unwrap(); + fs::create_dir_all(&dst).unwrap(); + write(&dst.join("gone"), b"live"); + + assert!( + !dest_has_uncovered_children(&src, &dst, |name| name == "gone"), + "a child tombstoned by the staging branch is covered" + ); + } + + /// Commit must refuse a branch whose tombstone names a directory that still + /// has visible children, rather than deleting the whole directory from the + /// destination. + #[test] + fn commit_refuses_tombstoned_nonempty_directory() { + let tmp = TmpDir::new(); + let storage = tmp.path().join("storage"); + let base = tmp.path().join("base"); + fs::create_dir_all(base.join("a/b")).unwrap(); + write(&base.join("a/keep"), b"keep"); + write(&base.join("a/b/keep"), b"keep"); + + let mgr = BranchManager::new(storage, base.clone(), base.clone(), None).unwrap(); + mgr.create_branch("feat", "main").unwrap(); + mgr.with_branch("feat", |b| b.add_tombstone("/a/b")) + .unwrap(); + + let err = mgr.commit("feat").unwrap_err(); + assert!( + matches!(err, BranchError::Invalid(_)), + "commit must reject the imprecise tombstone, got {err:?}" + ); + assert!( + base.join("a/b/keep").exists(), + "the destination subtree must survive a refused commit" + ); + assert!(mgr.is_branch_valid("feat"), "branch must remain for abort"); + } } diff --git a/src/fs.rs b/src/fs.rs index 0dec230..4c59d24 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -272,6 +272,148 @@ impl BranchFs { let path = self.inodes.get_path(ino)?; Some(classify_path(&path)) } + + /// Shared body of `unlink` and `rmdir`. `want_dir` selects the POSIX + /// rule: `unlink` rejects a directory with EISDIR, `rmdir` rejects a + /// missing path with ENOENT and a directory with visible children with + /// ENOTEMPTY. Only a genuinely empty directory is tombstoned. + fn remove_common(&mut self, parent: u64, name: &OsStr, want_dir: bool, reply: ReplyEmpty) { + let parent_path = match self.inodes.get_path(parent) { + Some(p) => p, + None => { + reply.error(libc::ENOENT); + return; + } + }; + + let name_str = name.to_string_lossy(); + + let branch_ctx = match classify_path(&parent_path) { + PathContext::BranchDir(b) => Some((b, "/".to_string())), + PathContext::BranchPath(b, rel) => Some((b, rel)), + _ => None, + }; + + if let Some((branch, parent_rel)) = branch_ctx { + // Can't unlink @child dirs or .branchfs_ctl + if name_str.starts_with('@') || *name_str == *CTL_FILE { + reply.error(libc::EPERM); + return; + } + + if !self.manager.is_branch_valid(&branch) { + reply.error(libc::ENOENT); + return; + } + + let rel_path = if parent_rel == "/" { + format!("/{}", name_str) + } else { + format!("{}/{}", parent_rel, name_str) + }; + + let resolved = self.manager.resolve_path(&branch, &rel_path).ok().flatten(); + if !want_dir { + if resolved + .as_ref() + .and_then(|p| p.symlink_metadata().ok()) + .map(|m| m.is_dir()) + .unwrap_or(false) + { + reply.error(libc::EISDIR); + return; + } + } else if resolved.is_none() { + reply.error(libc::ENOENT); + return; + } else if !self.branch_dir_is_empty(&branch, &rel_path) { + reply.error(libc::ENOTEMPTY); + return; + } + let result = self.manager.with_branch(&branch, |b| { + b.add_tombstone(&rel_path)?; + let delta = b.delta_path(&rel_path); + if delta.exists() { + let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); + std::fs::remove_file(&delta)?; + self.manager.quota.sub(freed); + } + Ok(()) + }); + + if result.is_err() { + reply.error(libc::EIO); + return; + } + + let inode_path = format!("/@{}{}", branch, rel_path); + self.inodes.remove(&inode_path); + reply.ok(); + } else { + // Root-path unlink (or EPERM for ctl files) + match classify_path(&parent_path) { + PathContext::BranchCtl(_) | PathContext::RootCtl => { + reply.error(libc::EPERM); + } + PathContext::RootPath(rp) => { + let path = if rp == "/" { + format!("/{}", name_str) + } else { + format!("{}/{}", rp, name_str) + }; + + let branch = self.get_branch_name(); + let resolved = self.manager.resolve_path(&branch, &path).ok().flatten(); + if !want_dir { + if resolved + .as_ref() + .and_then(|p| p.symlink_metadata().ok()) + .map(|m| m.is_dir()) + .unwrap_or(false) + { + reply.error(libc::EISDIR); + return; + } + } else if resolved.is_none() { + reply.error(libc::ENOENT); + return; + } else if !self.branch_dir_is_empty(&branch, &path) { + reply.error(libc::ENOTEMPTY); + return; + } + + let result = self.manager.with_branch(&self.get_branch_name(), |b| { + b.add_tombstone(&path)?; + let delta = b.delta_path(&path); + if delta.exists() { + let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); + std::fs::remove_file(&delta)?; + self.manager.quota.sub(freed); + } + Ok(()) + }); + + if result.is_err() || self.is_stale() { + reply.error(libc::ESTALE); + return; + } + + self.inodes.remove(&path); + reply.ok(); + } + _ => { + reply.error(libc::ENOENT); + } + } + } + } + + /// Whether the directory at `rel_path` reports no visible children. + fn branch_dir_is_empty(&self, branch: &str, rel_path: &str) -> bool { + self.collect_readdir_entries(branch, rel_path, 0, "") + .iter() + .all(|(_, _, name)| name == "." || name == "..") + } } impl Filesystem for BranchFs { @@ -1039,100 +1181,11 @@ impl Filesystem for BranchFs { } fn unlink(&mut self, _req: &Request, parent: u64, name: &OsStr, reply: ReplyEmpty) { - let parent_path = match self.inodes.get_path(parent) { - Some(p) => p, - None => { - reply.error(libc::ENOENT); - return; - } - }; - - let name_str = name.to_string_lossy(); - - let branch_ctx = match classify_path(&parent_path) { - PathContext::BranchDir(b) => Some((b, "/".to_string())), - PathContext::BranchPath(b, rel) => Some((b, rel)), - _ => None, - }; - - if let Some((branch, parent_rel)) = branch_ctx { - // Can't unlink @child dirs or .branchfs_ctl - if name_str.starts_with('@') || *name_str == *CTL_FILE { - reply.error(libc::EPERM); - return; - } - - if !self.manager.is_branch_valid(&branch) { - reply.error(libc::ENOENT); - return; - } - - let rel_path = if parent_rel == "/" { - format!("/{}", name_str) - } else { - format!("{}/{}", parent_rel, name_str) - }; - - let result = self.manager.with_branch(&branch, |b| { - b.add_tombstone(&rel_path)?; - let delta = b.delta_path(&rel_path); - if delta.exists() { - let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); - std::fs::remove_file(&delta)?; - self.manager.quota.sub(freed); - } - Ok(()) - }); - - if result.is_err() { - reply.error(libc::EIO); - return; - } - - let inode_path = format!("/@{}{}", branch, rel_path); - self.inodes.remove(&inode_path); - reply.ok(); - } else { - // Root-path unlink (or EPERM for ctl files) - match classify_path(&parent_path) { - PathContext::BranchCtl(_) | PathContext::RootCtl => { - reply.error(libc::EPERM); - } - PathContext::RootPath(rp) => { - let path = if rp == "/" { - format!("/{}", name_str) - } else { - format!("{}/{}", rp, name_str) - }; - - let result = self.manager.with_branch(&self.get_branch_name(), |b| { - b.add_tombstone(&path)?; - let delta = b.delta_path(&path); - if delta.exists() { - let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); - std::fs::remove_file(&delta)?; - self.manager.quota.sub(freed); - } - Ok(()) - }); - - if result.is_err() || self.is_stale() { - reply.error(libc::ESTALE); - return; - } - - self.inodes.remove(&path); - reply.ok(); - } - _ => { - reply.error(libc::ENOENT); - } - } - } + self.remove_common(parent, name, false, reply); } fn rmdir(&mut self, _req: &Request, parent: u64, name: &OsStr, reply: ReplyEmpty) { - self.unlink(_req, parent, name, reply); + self.remove_common(parent, name, true, reply); } fn rename( diff --git a/tests/test_commit.sh b/tests/test_commit.sh index 6c3e2d2..87c987c 100755 --- a/tests/test_commit.sh +++ b/tests/test_commit.sh @@ -177,6 +177,44 @@ test_commit_non_leaf_fails() { do_unmount } +# Regression for rmdir probing: removing a leaf must not tombstone non-empty +# ancestors, and committing must leave their surviving siblings in the base. +test_commit_preserves_ancestors_after_rmdir_probe() { + setup + mkdir -p "$TEST_BASE/a/b/c" + echo keep > "$TEST_BASE/a/keep" + echo keep > "$TEST_BASE/a/b/keep" + echo target > "$TEST_BASE/a/b/c/target" + do_mount + do_create "rmdir_probe" "main" + + rm -f "$TEST_MNT/@rmdir_probe/a/b/c/target" + rmdir "$TEST_MNT/@rmdir_probe/a/b/c" + + # Climbing must stop: both ancestors still hold live siblings. + if rmdir "$TEST_MNT/@rmdir_probe/a/b" 2>/dev/null; then + TESTS_RUN=$((TESTS_RUN + 1)) + TESTS_FAILED=$((TESTS_FAILED + 1)) + echo -e " ${RED}✗${NC} rmdir of non-empty a/b should fail" + else + TESTS_RUN=$((TESTS_RUN + 1)) + TESTS_PASSED=$((TESTS_PASSED + 1)) + echo -e " ${GREEN}✓${NC} rmdir of non-empty a/b refused" + fi + + assert_file_exists "$TEST_MNT/@rmdir_probe/a/keep" "a/keep survives the probe" + assert_file_exists "$TEST_MNT/@rmdir_probe/a/b/keep" "a/b/keep survives the probe" + + do_commit + + assert "[[ -d '$TEST_BASE/a' ]]" "a survives in base after commit" + assert_file_exists "$TEST_BASE/a/keep" "a/keep survives in base after commit" + assert_file_exists "$TEST_BASE/a/b/keep" "a/b/keep survives in base after commit" + assert "[[ ! -e '$TEST_BASE/a/b/c' ]]" "emptied a/b/c removed from base" + + do_unmount +} + # Run tests run_test "Commit New File" test_commit_new_file run_test "Commit Modified File" test_commit_modified_file @@ -185,5 +223,6 @@ run_test "Commit Switches to Main" test_commit_switches_to_main run_test "Commit Nested Branches" test_commit_nested_branches run_test "Commit Preserves Siblings" test_commit_preserves_siblings run_test "Commit Non-Leaf Fails" test_commit_non_leaf_fails +run_test "Commit Preserves Ancestors After rmdir Probe" test_commit_preserves_ancestors_after_rmdir_probe print_summary diff --git a/tests/test_integration.rs b/tests/test_integration.rs index 313db74..c1d2146 100644 --- a/tests/test_integration.rs +++ b/tests/test_integration.rs @@ -917,6 +917,77 @@ fn test_commit_deletion_visible_at_mount_root() { ); } +/// Regression: `rmdir` must return ENOTEMPTY for a directory that still has +/// visible children, and must not tombstone it. Removal tools probe upward +/// through parents and rely on ENOTEMPTY to stop climbing; without it they +/// tombstone non-empty ancestors, hiding subtrees from the branch and +/// replaying as a recursive delete of the base on commit. +#[test] +#[ignore] +fn test_rmdir_refuses_nonempty_directory_and_preserves_subtree() { + let fix = TestFixture::new("rmdir_nonempty"); + fs::create_dir_all(fix.base.join("a/b/c")).unwrap(); + fs::write(fix.base.join("a/keep"), "keep\n").unwrap(); + fs::write(fix.base.join("a/b/keep"), "keep\n").unwrap(); + fs::write(fix.base.join("a/b/c/target"), "target\n").unwrap(); + fix.mount(); + + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + + // Remove the leaf file, then remove the now-empty directory. + fs::remove_file(bdir.join("a/b/c/target")).unwrap(); + fs::remove_dir(bdir.join("a/b/c")).expect("a/b/c is empty and removable"); + + // `a/b` and `a` still contain visible children. + let err = fs::remove_dir(bdir.join("a/b")).unwrap_err(); + assert_eq!(err.raw_os_error(), Some(libc::ENOTEMPTY)); + let err = fs::remove_dir(bdir.join("a")).unwrap_err(); + assert_eq!(err.raw_os_error(), Some(libc::ENOTEMPTY)); + + // The surviving subtree must still resolve through the branch. + assert_eq!(fs::read_to_string(bdir.join("a/keep")).unwrap(), "keep\n"); + assert_eq!(fs::read_to_string(bdir.join("a/b/keep")).unwrap(), "keep\n"); + let entries: Vec = fs::read_dir(&bdir) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().to_string()) + .collect(); + assert!( + entries.contains(&"a".to_string()), + "non-empty ancestor must stay visible; got: {:?}", + entries + ); + + // Committing must not remove `a` from the base. + let bctl = fix.open_branch_ctl(&branch); + let ret = unsafe { ioctl_commit(bctl.as_raw_fd()) }; + assert_eq!(ret, 0, "commit should succeed"); + assert!(fix.base.join("a").exists(), "`a` must survive in the base"); + assert_eq!( + fs::read_to_string(fix.base.join("a/keep")).unwrap(), + "keep\n" + ); + assert!(!fix.base.join("a/b/c").exists(), "emptied dir is deleted"); +} + +/// `unlink` on a directory must fail with EISDIR rather than tombstoning it. +#[test] +#[ignore] +fn test_unlink_directory_returns_eisdir() { + let fix = TestFixture::new("unlink_dir"); + fix.mount(); + + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + + let err = fs::remove_file(bdir.join("subdir")).unwrap_err(); + assert_eq!(err.raw_os_error(), Some(libc::EISDIR)); + assert!(bdir.join("subdir/nested.txt").exists()); +} + /// Modifying a base file (not main's delta) in a branch and committing /// should update the file visible at the mount root. #[test] From fcbbb4901a3239a473e7eb2ccc34cb073d77fe3b Mon Sep 17 00:00:00 2001 From: Chaser Huang Date: Sun, 11 Oct 2026 22:17:16 -0400 Subject: [PATCH 2/2] fix(fs): remove directory deltas with directory-aware deletion Assisted-by: pi deepseek-v4.1-flash --- src/fs.rs | 104 ++++++++++++++++++++++++++++++---- tests/test_file_operations.sh | 48 ++++++++++++++++ tests/test_integration.rs | 38 +++++++++++++ 3 files changed, 178 insertions(+), 12 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 4c59d24..7cc1709 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -332,12 +332,8 @@ impl BranchFs { } let result = self.manager.with_branch(&branch, |b| { b.add_tombstone(&rel_path)?; - let delta = b.delta_path(&rel_path); - if delta.exists() { - let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); - std::fs::remove_file(&delta)?; - self.manager.quota.sub(freed); - } + let freed = Self::remove_delta_entry(&b.delta_path(&rel_path))?; + self.manager.quota.sub(freed); Ok(()) }); @@ -384,12 +380,8 @@ impl BranchFs { let result = self.manager.with_branch(&self.get_branch_name(), |b| { b.add_tombstone(&path)?; - let delta = b.delta_path(&path); - if delta.exists() { - let freed = delta.symlink_metadata().map(|m| m.len()).unwrap_or(0); - std::fs::remove_file(&delta)?; - self.manager.quota.sub(freed); - } + let freed = Self::remove_delta_entry(&b.delta_path(&path))?; + self.manager.quota.sub(freed); Ok(()) }); @@ -414,6 +406,46 @@ impl BranchFs { .iter() .all(|(_, _, name)| name == "." || name == "..") } + + /// A directory delta is stored as a real directory, so it must be removed + /// with a directory-aware primitive; a plain unlink fails with EISDIR. + /// A missing path releases nothing. + fn remove_delta_entry(path: &Path) -> std::io::Result { + let meta = match path.symlink_metadata() { + Ok(m) => m, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(0), + Err(e) => return Err(e), + }; + if meta.file_type().is_dir() { + let freed = Self::dir_file_bytes(path); + std::fs::remove_dir_all(path)?; + Ok(freed) + } else { + let freed = meta.len(); + std::fs::remove_file(path)?; + Ok(freed) + } + } + + /// Total size of regular files under `path`, used to release quota once a + /// directory delta is removed. Directory inode sizes are not counted, so + /// this mirrors how writes add to the quota. + fn dir_file_bytes(path: &Path) -> u64 { + let mut total = 0; + if let Ok(entries) = std::fs::read_dir(path) { + for entry in entries.flatten() { + let child = entry.path(); + if let Ok(m) = child.symlink_metadata() { + if m.file_type().is_dir() { + total += Self::dir_file_bytes(&child); + } else { + total += m.len(); + } + } + } + } + total + } } impl Filesystem for BranchFs { @@ -2029,4 +2061,52 @@ mod tests { "a foreign abort must make the mount stale via branch removal" ); } + + fn temp_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("branchfs-{tag}-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn remove_delta_entry_removes_directory_and_reports_file_bytes() { + let root = temp_dir("delta-dir"); + let dir = root.join("tree"); + std::fs::create_dir_all(dir.join("inner")).unwrap(); + std::fs::write(dir.join("inner/a"), b"abcd").unwrap(); + std::fs::write(dir.join("b"), b"ef").unwrap(); + + let freed = BranchFs::remove_delta_entry(&dir).unwrap(); + assert!( + !dir.exists(), + "a directory delta must be removed with a directory-aware primitive" + ); + assert_eq!( + freed, 6, + "regular file bytes under the directory are released" + ); + } + + #[test] + fn remove_delta_entry_removes_file_and_symlink() { + let root = temp_dir("delta-file"); + let file = root.join("f"); + std::fs::write(&file, b"12345").unwrap(); + assert_eq!(BranchFs::remove_delta_entry(&file).unwrap(), 5); + assert!(!file.exists()); + + let link = root.join("l"); + std::os::unix::fs::symlink("target", &link).unwrap(); + assert_eq!(BranchFs::remove_delta_entry(&link).unwrap(), 6); + assert!(!link.exists()); + } + + #[test] + fn remove_delta_entry_missing_path_releases_nothing() { + let root = temp_dir("delta-missing"); + assert_eq!( + BranchFs::remove_delta_entry(&root.join("absent")).unwrap(), + 0 + ); + } } diff --git a/tests/test_file_operations.sh b/tests/test_file_operations.sh index f1f7609..1be5096 100755 --- a/tests/test_file_operations.sh +++ b/tests/test_file_operations.sh @@ -234,6 +234,52 @@ test_synthetic_entry_ownership() { } # Run tests +# Regression: rmdir on a freshly created empty directory must succeed. The +# delta stores a directory as a real directory, so tombstoning it must remove +# that entry with a directory-aware primitive; a plain unlink fails with EISDIR +# and surfaced as EIO (branch path) or ENOENT (root path). +test_rmdir_fresh_directory_succeeds() { + setup + do_mount + do_create "rmdir_fresh" "main" + + mkdir "$TEST_MNT/root_fresh" + assert "rmdir '$TEST_MNT/root_fresh'" "rmdir of fresh root directory succeeds" + assert "[[ ! -e '$TEST_MNT/root_fresh' ]]" "fresh root directory removed" + + mkdir "$TEST_MNT/@rmdir_fresh/branch_fresh" + assert "rmdir '$TEST_MNT/@rmdir_fresh/branch_fresh'" \ + "rmdir of fresh branch directory succeeds" + assert "[[ ! -e '$TEST_MNT/@rmdir_fresh/branch_fresh' ]]" \ + "fresh branch directory removed" + + mkdir -p "$TEST_MNT/@rmdir_fresh/nested/inner" + assert "rmdir '$TEST_MNT/@rmdir_fresh/nested/inner'" \ + "rmdir of fresh nested directory succeeds" + assert "rmdir '$TEST_MNT/@rmdir_fresh/nested'" \ + "rmdir of emptied parent directory succeeds" + + do_unmount +} + +# Removing an emptied branch directory must also recreate cleanly, exercising +# the join of rmdir delta cleanup and the tombstone revival on mkdir. +test_recreate_dir_after_delete_in_branch() { + setup + do_mount + do_create "recreate_dir_b" "main" + + mkdir "$TEST_MNT/@recreate_dir_b/dir" + assert "rmdir '$TEST_MNT/@recreate_dir_b/dir'" "branch directory removed" + assert "[[ ! -e '$TEST_MNT/@recreate_dir_b/dir' ]]" "branch directory gone" + + mkdir "$TEST_MNT/@recreate_dir_b/dir" + assert "[[ -d '$TEST_MNT/@recreate_dir_b/dir' ]]" \ + "branch directory recreated with same name" + + do_unmount +} + run_test "Read Base Files" test_read_base_files run_test "Write New File in Branch" test_write_new_file_in_branch run_test "Modify Existing File (COW)" test_modify_existing_file_cow @@ -247,5 +293,7 @@ run_test "Chmod File" test_chmod_file run_test "Chmod Directory" test_chmod_directory run_test "Chmod Existing File (COW)" test_chmod_existing_file_cow run_test "Synthetic Entry Ownership" test_synthetic_entry_ownership +run_test "Rmdir Fresh Directory Succeeds" test_rmdir_fresh_directory_succeeds +run_test "Recreate Dir After Delete in Branch" test_recreate_dir_after_delete_in_branch print_summary diff --git a/tests/test_integration.rs b/tests/test_integration.rs index c1d2146..a1ea40e 100644 --- a/tests/test_integration.rs +++ b/tests/test_integration.rs @@ -988,6 +988,44 @@ fn test_unlink_directory_returns_eisdir() { assert!(bdir.join("subdir/nested.txt").exists()); } +/// Regression: `rmdir` of a freshly created empty directory must succeed. +/// The branch delta stores a directory as a real directory, so tombstoning it +/// must remove that entry with a directory-aware primitive. A plain unlink +/// fails with EISDIR, which surfaced as EIO on a branch path and ENOENT on a +/// root path even though the tombstone had already been recorded. +#[test] +#[ignore] +fn test_rmdir_fresh_directory_succeeds() { + let fix = TestFixture::new("rmdir_fresh"); + fix.mount(); + + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + + // Fresh directory through the mount root (root-path arm of remove_common). + fs::create_dir(fix.mnt.join("root_fresh")).unwrap(); + fs::remove_dir(fix.mnt.join("root_fresh")) + .expect("rmdir of a fresh root directory must succeed"); + assert!(!fix.mnt.join("root_fresh").exists()); + + // Fresh directory inside the branch (branch-path arm). + fs::create_dir(bdir.join("branch_fresh")).unwrap(); + fs::remove_dir(bdir.join("branch_fresh")) + .expect("rmdir of a fresh branch directory must succeed"); + assert!(!bdir.join("branch_fresh").exists()); + + // Nested: remove the emptied leaf, then its parent. + fs::create_dir_all(bdir.join("nested/inner")).unwrap(); + fs::remove_dir(bdir.join("nested/inner")).expect("rmdir of fresh nested leaf"); + fs::remove_dir(bdir.join("nested")).expect("rmdir of emptied parent"); + assert!(!bdir.join("nested").exists()); + + // The recreated name must be usable again (tombstone revival). + fs::create_dir(bdir.join("branch_fresh")).unwrap(); + assert!(bdir.join("branch_fresh").is_dir()); +} + /// Modifying a base file (not main's delta) in a branch and committing /// should update the file visible at the mount root. #[test]