From 8f9e8d4d185abdbd3a48e5b00b68f187913d405c Mon Sep 17 00:00:00 2001 From: Kinfe123 Date: Thu, 29 Oct 2026 19:08:55 -0700 Subject: [PATCH] perf(core): change only directory permissions before deleting a tree Unlinking an entry on Unix needs write and search permission on its parent directory, never on the entry. remove_tree_if_present used to lstat and chmod every file, directory, and symlink before remove_dir_all walked the tree again. The new riftri_storage::make_tree_removable walks directories only, takes child types from readdir, and chmods a directory only when it lacks an owner read, write, or search bit. Symlinks are never followed or changed. Windows keeps the per-entry pass, because the read-only attribute blocks deletion there. --- CHANGELOG.md | 10 + crates/riftri-core/src/worktree.rs | 2 +- .../riftri-core/tests/native_cow_removal.rs | 102 ++++++++ crates/riftri-storage/src/lib.rs | 27 ++ crates/riftri-storage/src/tree_removal.rs | 243 ++++++++++++++++++ .../riftri-storage/tests/native_deep_walk.rs | 14 + docs/architecture.md | 11 + 7 files changed, 408 insertions(+), 1 deletion(-) create mode 100644 crates/riftri-storage/src/tree_removal.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e0fb17d..fb9eafd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,16 @@ for its Rust CLI and npm distribution packages as one synchronized release. each prune pass still re-reads the state directories and Git's worktrees. Removing or moving a path Riftri does not manage while `git worktree list` fails now reports the path as unmanaged instead of the Git failure. +- On macOS and Linux, change only directory permissions before deleting a + tree. Unlinking a file needs write access to its directory, never to the + file, so removal, rollback, compaction, and `riftri gc --apply` no longer + stat and chmod every file first. A directory is changed only when it lacks + owner read, write, or search permission, and symlinks are never followed. + Removing a view of 25,000 files (5,000 tracked, 20,000 ignored) now makes + 913 `lstat` calls and no `chmod` calls instead of 25,915 of each, and took + 1.15 s instead of 1.85 s in an interleaved local run; collecting a + 5,000-file base took 0.34 s instead of 0.52 s. Windows still clears the + read-only attribute on every entry, which deletion there requires. ### Fixed diff --git a/crates/riftri-core/src/worktree.rs b/crates/riftri-core/src/worktree.rs index b9f59d8c..903d6c8c 100644 --- a/crates/riftri-core/src/worktree.rs +++ b/crates/riftri-core/src/worktree.rs @@ -13424,7 +13424,7 @@ fn remove_tree_if_present(path: &Path) -> Result<(), WorktreeError> { path.display() ))); } - NativeCowCloner::make_tree_owner_writable(path)?; + riftri_storage::make_tree_removable(path)?; fs::remove_dir_all(path).map_err(|source| io("remove rollback directory", path, source)) } diff --git a/crates/riftri-core/tests/native_cow_removal.rs b/crates/riftri-core/tests/native_cow_removal.rs index 5690cc83..e0bcbfd3 100644 --- a/crates/riftri-core/tests/native_cow_removal.rs +++ b/crates/riftri-core/tests/native_cow_removal.rs @@ -492,3 +492,105 @@ fn garbage_collection_requires_apply_and_never_collects_an_in_use_base() { assert_eq!(rebuilt.base_path, first_add.base_path); assert!(git(&rebuilt_path, &["status", "--porcelain=v1"]).is_empty()); } + +/// Deletion grants permission only on directories. A clean removal must still +/// delete ignored read-only files under read-only directories, without +/// following a symlink out of the view, and collection must still delete an +/// immutable base whose nested directories and files are all read-only. +#[cfg(unix)] +#[test] +fn removal_and_collection_delete_read_only_trees_without_following_links() { + use std::os::unix::fs::PermissionsExt; + + let mode = |path: &Path| fs::symlink_metadata(path).unwrap().permissions().mode() & 0o7777; + let set_mode = |path: &Path, mode: u32| { + fs::set_permissions(path, fs::Permissions::from_mode(mode)).unwrap(); + }; + let fixture = tempdir().expect("fixture directory"); + let repository = fixture.path().join("repository"); + let state = fixture.path().join("state"); + let worktree = fixture.path().join("worktree"); + let outside = fixture.path().join("outside"); + fs::create_dir(&repository).expect("create repository"); + git(&repository, &["init", "--quiet"]); + git(&repository, &["config", "user.name", "Riftri Tests"]); + git( + &repository, + &["config", "user.email", "riftri@example.invalid"], + ); + git(&repository, &["config", "core.autocrlf", "false"]); + fs::create_dir_all(repository.join("src/nested/deeper")).unwrap(); + fs::write(repository.join("src/nested/deeper/leaf.txt"), "leaf\n").unwrap(); + fs::write(repository.join("src/tool.sh"), "#!/bin/sh\n").unwrap(); + set_mode(&repository.join("src/tool.sh"), 0o755); + fs::write(repository.join(".gitignore"), "node_modules/\n").unwrap(); + git(&repository, &["add", "--all"]); + git(&repository, &["commit", "--quiet", "-m", "initial"]); + + fs::create_dir(&outside).unwrap(); + fs::write(outside.join("target.txt"), "outside\n").unwrap(); + set_mode(&outside.join("target.txt"), 0o444); + set_mode(&outside, 0o555); + let outside_modes = (mode(&outside), mode(&outside.join("target.txt"))); + + let added = add_worktree(AddWorktreeRequest { + repository: repository.clone(), + destination: worktree.clone(), + revision: OsString::from("HEAD"), + mode: WorktreeMode::NewBranch(OsString::from("feature/read-only-trees")), + state_dir: Some(state.clone()), + sparse_directories: Vec::new(), + }) + .expect("create Riftri worktree"); + // The immutable base is read-only all the way down. + for relative in ["src", "src/nested", "src/nested/deeper"] { + assert_eq!( + mode(&added.base_path.join(relative)) & 0o222, + 0, + "{relative}" + ); + } + assert_eq!( + mode(&added.base_path.join("src/nested/deeper/leaf.txt")) & 0o222, + 0 + ); + + let package = worktree.join("node_modules/package/lib"); + fs::create_dir_all(&package).unwrap(); + fs::write(package.join("index.js"), "module.exports = 1;\n").unwrap(); + set_mode(&package.join("index.js"), 0o444); + symlink(&outside, worktree.join("node_modules/outside-dir")).unwrap(); + symlink(outside.join("target.txt"), package.join("outside-file")).unwrap(); + symlink("index.js", package.join("inside")).unwrap(); + set_mode(&package, 0o555); + set_mode(&worktree.join("node_modules/package"), 0o500); + assert!(git(&worktree, &["status", "--porcelain=v1"]).is_empty()); + + remove_worktree(RemoveWorktreeRequest { + repository: repository.clone(), + destination: worktree.clone(), + state_dir: Some(state.clone()), + }) + .expect("remove a clean view holding read-only ignored files"); + assert!(!worktree.exists()); + assert_eq!( + (mode(&outside), mode(&outside.join("target.txt"))), + outside_modes + ); + assert_eq!( + fs::read_to_string(outside.join("target.txt")).unwrap(), + "outside\n" + ); + + let applied = garbage_collect(&state, true).expect("collect the read-only base"); + assert_eq!( + applied.collected.as_slice(), + std::slice::from_ref(&added.base_path) + ); + assert!(!added.base_path.exists()); + assert_eq!( + (mode(&outside), mode(&outside.join("target.txt"))), + outside_modes + ); + set_mode(&outside, 0o755); +} diff --git a/crates/riftri-storage/src/lib.rs b/crates/riftri-storage/src/lib.rs index e1c5cbe0..df387b54 100644 --- a/crates/riftri-storage/src/lib.rs +++ b/crates/riftri-storage/src/lib.rs @@ -9,6 +9,33 @@ use thiserror::Error; mod exact_copy; #[cfg(any(test, target_os = "linux", target_os = "macos", target_os = "windows"))] mod parallel; +#[cfg(unix)] +mod tree_removal; + +/// Grant what deleting the directory tree at `path` needs, without deleting +/// anything. Symlinks are never followed or changed. +/// +/// On Unix, unlinking an entry needs write and search permission on its +/// parent directory, never on the entry, so only directories missing an owner +/// `rwx` bit are changed and files are not inspected at all. On Windows the +/// read-only attribute blocks deletion itself, so every directory and file +/// becomes writable, as [`RefsBlockCloner::make_tree_owner_writable`] does. +#[cfg(unix)] +pub fn make_tree_removable(path: &Path) -> Result<(), StorageError> { + tree_removal::make_tree_removable(path) +} + +#[cfg(target_os = "windows")] +pub fn make_tree_removable(path: &Path) -> Result<(), StorageError> { + refs::make_tree_owner_writable(path) +} + +#[cfg(not(any(unix, target_os = "windows")))] +pub fn make_tree_removable(_path: &Path) -> Result<(), StorageError> { + Err(StorageError::UnsupportedPlatform { + backend: "tree removal", + }) +} /// Errors from concrete native storage operations. #[derive(Debug, Error)] diff --git a/crates/riftri-storage/src/tree_removal.rs b/crates/riftri-storage/src/tree_removal.rs new file mode 100644 index 00000000..1fac25a0 --- /dev/null +++ b/crates/riftri-storage/src/tree_removal.rs @@ -0,0 +1,243 @@ +//! Permissions a Unix tree needs before `remove_dir_all` can delete it. +//! +//! Unlinking an entry needs write and search permission on its parent +//! directory and nothing on the entry itself, so a read-only file or symlink +//! never blocks deletion. Listing a directory, which recursive deletion must do +//! first, also needs read permission on it. Only directories are therefore +//! inspected and changed: the entry type comes from `readdir`, so files and +//! symlinks are neither stat'ed nor chmod'ed, and a directory is chmod'ed only +//! when it lacks one of the owner `rwx` bits. Symlinks are never followed. + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; + +use crate::StorageError; + +/// Owner read, write, and search: what listing a directory and unlinking its +/// entries require. +const OWNER_REMOVAL_BITS: u32 = 0o700; + +pub(crate) fn make_tree_removable(path: &Path) -> Result<(), StorageError> { + let metadata = symlink_metadata(path)?; + if !metadata.file_type().is_dir() { + // A file or symlink is deleted through its parent's permissions. + return Ok(()); + } + make_directory_removable(path, metadata.permissions().mode()) +} + +fn make_directory_removable(path: &Path, mode: u32) -> Result<(), StorageError> { + if mode & OWNER_REMOVAL_BITS != OWNER_REMOVAL_BITS { + set_mode(path, mode | OWNER_REMOVAL_BITS)?; + } + // Collect child directory names and close the handle before descending, + // so a deep tree holds one directory handle at a time. + let mut directories = Vec::new(); + for entry in fs::read_dir(path).map_err(|source| io("read tree for removal", path, source))? { + let entry = entry.map_err(|source| io("read tree entry for removal", path, source))?; + let file_type = entry + .file_type() + .map_err(|source| io("inspect tree entry for removal", &entry.path(), source))?; + if file_type.is_dir() { + directories.push(entry.file_name()); + } + } + let mut child = path.to_path_buf(); + for name in directories { + child.push(name); + // Re-read without following links: the entry type above came from + // `readdir`, and only a real directory's own mode is ever changed. + let metadata = symlink_metadata(&child)?; + if metadata.file_type().is_dir() { + make_directory_removable(&child, metadata.permissions().mode())?; + } + child.pop(); + } + Ok(()) +} + +fn symlink_metadata(path: &Path) -> Result { + #[cfg(test)] + counters::record(&counters::LSTATS, path); + fs::symlink_metadata(path).map_err(|source| io("inspect tree for removal", path, source)) +} + +fn set_mode(path: &Path, mode: u32) -> Result<(), StorageError> { + #[cfg(test)] + counters::record(&counters::CHMODS, path); + fs::set_permissions(path, fs::Permissions::from_mode(mode)) + .map_err(|source| io("prepare directory for removal", path, source)) +} + +fn io(operation: &'static str, path: &Path, source: std::io::Error) -> StorageError { + StorageError::Io { + operation, + path: path.to_path_buf(), + source, + } +} + +/// Per-thread records of the paths this module stat'ed and chmod'ed. +#[cfg(test)] +mod counters { + use std::cell::RefCell; + use std::path::{Path, PathBuf}; + use std::thread::LocalKey; + + thread_local! { + pub(super) static LSTATS: RefCell> = const { RefCell::new(Vec::new()) }; + pub(super) static CHMODS: RefCell> = const { RefCell::new(Vec::new()) }; + } + + pub(super) fn record(counter: &'static LocalKey>>, path: &Path) { + counter.with(|paths| paths.borrow_mut().push(path.to_path_buf())); + } + + pub(super) fn take(counter: &'static LocalKey>>) -> Vec { + counter.with(|paths| std::mem::take(&mut *paths.borrow_mut())) + } +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::os::unix::fs::{PermissionsExt, symlink}; + use std::path::{Path, PathBuf}; + + use tempfile::tempdir; + + use super::{counters, make_tree_removable}; + + fn mode(path: &Path) -> u32 { + fs::symlink_metadata(path).unwrap().permissions().mode() & 0o7777 + } + + fn set_mode(path: &Path, mode: u32) { + fs::set_permissions(path, fs::Permissions::from_mode(mode)).unwrap(); + } + + fn sorted(mut paths: Vec) -> Vec { + paths.sort(); + paths + } + + #[test] + fn read_only_tree_is_deleted_without_touching_files_or_link_targets() { + let root = tempdir().unwrap(); + let outside = root.path().join("outside"); + fs::create_dir(&outside).unwrap(); + let outside_file = outside.join("target"); + fs::write(&outside_file, b"keep").unwrap(); + set_mode(&outside_file, 0o444); + set_mode(&outside, 0o555); + + let tree = root.path().join("tree"); + let nested = tree.join("a").join("b"); + fs::create_dir_all(&nested).unwrap(); + let writable = tree.join("writable"); + fs::create_dir(&writable).unwrap(); + fs::write(tree.join("top"), b"top").unwrap(); + fs::write(nested.join("leaf"), b"leaf").unwrap(); + fs::write(writable.join("plain"), b"plain").unwrap(); + symlink("leaf", nested.join("inside-link")).unwrap(); + symlink(&outside, tree.join("a").join("outside-dir-link")).unwrap(); + symlink(&outside_file, writable.join("outside-file-link")).unwrap(); + set_mode(&tree.join("top"), 0o444); + set_mode(&nested.join("leaf"), 0o444); + set_mode(&nested, 0o555); + set_mode(&tree.join("a"), 0o555); + set_mode(&tree, 0o555); + let outside_modes = (mode(&outside), mode(&outside_file)); + let writable_mode = mode(&writable); + + counters::take(&counters::LSTATS); + counters::take(&counters::CHMODS); + make_tree_removable(&tree).unwrap(); + let chmods = sorted(counters::take(&counters::CHMODS)); + let lstats = sorted(counters::take(&counters::LSTATS)); + + // Only the three directories missing owner bits were changed; the + // already-writable directory, every file, and every link were not. + assert_eq!( + chmods, + sorted(vec![tree.clone(), tree.join("a"), nested.clone()]) + ); + // Files and symlinks are typed by `readdir` and never stat'ed. + assert_eq!( + lstats, + sorted(vec![ + tree.clone(), + tree.join("a"), + nested.clone(), + writable.clone() + ]) + ); + assert_eq!(mode(&tree.join("top")), 0o444); + assert_eq!(mode(&nested.join("leaf")), 0o444); + assert_eq!(mode(&writable), writable_mode); + + fs::remove_dir_all(&tree).unwrap(); + assert!(!tree.exists()); + assert_eq!((mode(&outside), mode(&outside_file)), outside_modes); + assert_eq!(fs::read(&outside_file).unwrap(), b"keep"); + set_mode(&outside, 0o755); + } + + #[test] + fn writable_tree_needs_no_permission_changes() { + let root = tempdir().unwrap(); + let tree = root.path().join("tree"); + for directory in ["one/two", "three"] { + fs::create_dir_all(tree.join(directory)).unwrap(); + } + for (index, file) in ["one/a", "one/two/b", "three/c", "d"].iter().enumerate() { + fs::write(tree.join(file), index.to_string()).unwrap(); + set_mode(&tree.join(file), 0o444); + } + counters::take(&counters::LSTATS); + counters::take(&counters::CHMODS); + make_tree_removable(&tree).unwrap(); + assert!(counters::take(&counters::CHMODS).is_empty()); + assert_eq!(counters::take(&counters::LSTATS).len(), 4); + fs::remove_dir_all(&tree).unwrap(); + } + + #[test] + fn missing_owner_read_or_search_is_restored() { + let root = tempdir().unwrap(); + let tree = root.path().join("tree"); + let unreadable = tree.join("unreadable"); + let unsearchable = tree.join("unsearchable"); + fs::create_dir_all(&unreadable).unwrap(); + fs::create_dir_all(&unsearchable).unwrap(); + fs::write(unreadable.join("file"), b"x").unwrap(); + fs::write(unsearchable.join("file"), b"y").unwrap(); + set_mode(&unreadable, 0o300); + set_mode(&unsearchable, 0o600); + make_tree_removable(&tree).unwrap(); + assert_eq!(mode(&unreadable) & 0o700, 0o700); + assert_eq!(mode(&unsearchable) & 0o700, 0o700); + fs::remove_dir_all(&tree).unwrap(); + } + + #[test] + fn a_link_or_file_root_is_left_alone() { + let root = tempdir().unwrap(); + let target = root.path().join("target"); + fs::create_dir(&target).unwrap(); + set_mode(&target, 0o555); + let link = root.path().join("link"); + symlink(&target, &link).unwrap(); + let file = root.path().join("file"); + fs::write(&file, b"x").unwrap(); + set_mode(&file, 0o444); + counters::take(&counters::CHMODS); + make_tree_removable(&link).unwrap(); + make_tree_removable(&file).unwrap(); + assert!(counters::take(&counters::CHMODS).is_empty()); + assert_eq!(mode(&target), 0o555); + assert_eq!(mode(&file), 0o444); + set_mode(&target, 0o755); + } +} diff --git a/crates/riftri-storage/tests/native_deep_walk.rs b/crates/riftri-storage/tests/native_deep_walk.rs index 7cad7ba7..fb3eae37 100644 --- a/crates/riftri-storage/tests/native_deep_walk.rs +++ b/crates/riftri-storage/tests/native_deep_walk.rs @@ -52,6 +52,15 @@ fn exercise(phase: &str, test: &str) { Native::make_tree_owner_writable(&source).unwrap(); fs::write(source.join(&relative), b"updated").unwrap(); } + "removable" => { + riftri_storage::make_tree_removable(&source).unwrap(); + let leaf = source.join(&relative); + let mode = |path: &std::path::Path| { + fs::symlink_metadata(path).unwrap().permissions().mode() & 0o777 + }; + assert_eq!(mode(leaf.parent().unwrap()) & 0o700, 0o700); + assert_eq!(mode(&leaf) & 0o222, 0); + } _ => unreachable!(), } return; @@ -101,3 +110,8 @@ fn deep_readonly_walk_bounds_directory_handles() { fn deep_writable_walk_bounds_directory_handles() { exercise("writable", "deep_writable_walk_bounds_directory_handles"); } + +#[test] +fn deep_removal_walk_bounds_directory_handles() { + exercise("removable", "deep_removal_walk_bounds_directory_handles"); +} diff --git a/docs/architecture.md b/docs/architecture.md index 31768a94..8626d118 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -410,6 +410,17 @@ Git still registers the view, recovery renames it back and decides again as if it had never moved. Otherwise recovery finishes deleting it. A quarantine that exists beside a reappeared destination is preserved together with it. +Every tree Riftri deletes, such as a removal or compaction quarantine, a +collected base, or an add's rollback staging, is prepared and then deleted +with a recursive delete that does not follow symlinks. On macOS and Linux, +unlinking an entry needs write and search permission on its parent directory, +never on the entry, so preparation changes only real directories that lack +owner read, write, or search permission. Files are neither inspected nor +changed, and symlinks are never followed. On Windows the read-only attribute +itself blocks deletion, so every file and directory is made writable first. A +delete that stops partway leaves a partial tree under the same journaled name, +which a retry or recovery finishes deleting. + An explicit forced removal snapshots the complete native view, or the complete OverlayFS private layer, before recording durable intent. The journal records both the force choice and snapshot. The snapshot includes Git HEAD, its symbolic branch, and structured