macOS: skip set_readwrite_recursive walk after clonefile (~1.85x materialization speedup) (#2349)

erneestoc · claude · MarcusSorealheis · web-flow · commit 6b17b68a5c08 · 2026-05-21T17:12:02.000Z
* fs_util: skip set_readwrite_recursive walk after clonefile The macOS clonefile fast path was followed by a recursive chmod walk that made every file in the cloned tree writable (0o644 / 0o755). On real Bazel input shapes (~2000-file SwiftCompile) that walk accounted for ~46% of materialization time — ~33 µs per file, ~67 ms per action. Replace the walk with a single chmod(2) on the destination root. Existing entries inherit the source's read-only mode (0o555 dirs, 0o444 files). The worker can still create the action's declared output files inside the root because the root itself is 0o755. This matches the hermeticity contract enforced by Bazel's local sandbox (linux-sandbox bind-mounts inputs read-only; darwin-sandbox / sandbox-exec denies writes outside declared output paths) and the REAPI Action.output_files / output_directories semantics: actions write only to declared outputs, never mutate inputs. An action that does try to mutate an input now hits EACCES, which is the correct REAPI behavior — same failure mode as on Bazel's own sandbox. Bench (nativelink-util/benches/chmod_strategy.rs on the bench branch), toplevel_only vs full walk: shape walk toplevel_only speedup small_flat (64 files) 4.66 ms 2.61 ms 1.79x pcm_cluster (219 files) 15.17 ms 8.19 ms 1.85x medium_flat (635 files) 46.36 ms 25.10 ms 1.85x large_flat (1978 files) 147.39 ms 80.17 ms 1.84x set_readwrite_recursive stays public — directory_cache.rs:451 still uses it on the source side during eviction. Tests: - test_clonefile_root_writable_inputs_readonly: root 0o755, subdirs 0o555, files 0o444 (replaces the old test_clonefile_dest_is_writable which assumed subdirs would be made writable). - test_clonefile_root_accepts_new_files: worker can create outputs at the root even though everything inside the clone is read-only. - test_clonefile_input_mutation_fails: writes to existing input files fail with PermissionDenied — encodes the hermeticity contract. * fs_util: remove dead set_readwrite_recursive after post-#2347 cleanup After #2347 (DirectoryCache cleanup uses set_dir_writable_recursive) and this PR (post-clonefile path uses chmod_dir_writable), the generic set_readwrite_recursive helper has zero remaining callers. Both replacements are intentionally narrower - set_dir_writable_recursive chmods only directories so file inodes aren't mutated, and chmod_dir_writable chmods only the destination root so the clone tree stays read-only inside. Delete the old helper and its private companion set_readwrite_one_path. Addresses palfrey's review feedback on #2347 ("set_readwrite_recursive and set_readwrite_one_path can be dropped as part of this") - sequenced on this PR instead because both PRs had to land before the helpers became dead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * worker: make the APFS clonefile directory-cache path work end-to-end PR #2338's clonefile fast path is effectively dead in production — it materialized 1 of 1771 actions in a real build. Three coupled bugs kept the directory cache silently falling back to the slow download path, and two of them only surface once the fast path actually fires, so they must land together. Bug 1: prepare_action_inputs received a work directory the caller had already created, but hardlink_directory_tree and clonefile(2) both require the destination to NOT exist. Every cache attempt failed its precondition and fell back to download_to_directory. Fix: remove the empty pre-created directory before invoking the cache, and recreate it on cache failure so the download fallback (which needs an existing destination) still works. Adds fs::remove_dir for the empty-dir removal. Bug 2: set_readonly_recursive chmod'd files to 0o444, stripping the execute bit from cached executables. Once a tree is cloned into a workspace this makes an action's interpreter/wrapper script fail with EACCES. Fix: mark files 0o555 instead of 0o444 — read + execute, still no write bit, so the hermeticity contract is unchanged. Bug 3: the clonefile path chmods only the destination root writable; cloned subdirectories keep the source's 0o555 mode. Bazel actions declare outputs at paths nested inside input subdirectories, and creating those files needs write permission on the parent directory. Fix: after a cache hit, set_dir_writable_recursive makes every directory in the materialized tree writable. Files stay read-only — they may be CAS-hardlinked and chmoding them would corrupt the shared inode. Adds regression tests for nested output creation, which the existing root-only clonefile tests did not cover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Marcus Eagan <marcuseagan@gmail.com>
diff --git a/nativelink-util/src/fs.rs b/nativelink-util/src/fs.rs
@@ -369,6 +369,13 @@ pub async fn remove_file(path: impl AsRef<Path>) -> Result<(), Error> {
     call_with_permit(move |_| std::fs::remove_file(path).map_err(Into::<Error>::into)).await
 }
 
+/// Removes an empty directory. Errors if the directory is not empty; use
+/// [`remove_dir_all`] when the contents should be removed too.
+pub async fn remove_dir(path: impl AsRef<Path>) -> Result<(), Error> {
+    let path = path.as_ref().to_owned();
+    call_with_permit(move |_| std::fs::remove_dir(path).map_err(Into::<Error>::into)).await
+}
+
 pub async fn canonicalize(path: impl AsRef<Path>) -> Result<PathBuf, Error> {
     let path = path.as_ref().to_owned();
     call_with_permit(move |_| std::fs::canonicalize(path).map_err(Into::<Error>::into)).await
diff --git a/nativelink-util/src/fs_util.rs b/nativelink-util/src/fs_util.rs
@@ -51,10 +51,14 @@ pub enum CloneMethod {
 /// # Platform Support
 /// - macOS: Tries APFS `clonefile(2)` first (O(1), copy-on-write). On failure
 ///   (e.g., cross-volume EXDEV, or any unexpected errno) falls back to per-file
-///   `fs::hard_link`. After a successful clone, the destination tree is made
-///   writable (0o755 / 0o644) because the clone inherits the source's
-///   permissions, and cached subtrees are 0o555 / 0o444. The COW semantics of
-///   `clonefile(2)` mean writes to the destination do not affect the source.
+///   `fs::hard_link`. After a successful clone, only the destination root is
+///   chmod'd to 0o755 so the worker can create the action's declared output
+///   files inside it. Existing entries inherit the source's read-only mode
+///   (0o555) — this matches the hermeticity contract
+///   enforced by Bazel's local sandbox and the REAPI `Action.output_files`
+///   semantics: actions can only write to declared outputs, never mutate
+///   inputs. The COW semantics of `clonefile(2)` mean any writes the worker
+///   does make to the destination do not affect the source.
 /// - Linux: Per-file `fs::hard_link` (directory hardlinks are not supported on
 ///   ext4/btrfs without root). Always returns `CloneMethod::Hardlink`.
 /// - Windows: Per-file `fs::hard_link` (requires NTFS). Always returns
@@ -97,13 +101,17 @@ pub async fn hardlink_directory_tree(src_dir: &Path, dst_dir: &Path) -> Result<C
 
         match try_clonefile(src_dir, dst_dir).await {
             Ok(()) => {
-                // The clone inherits the source's permissions. Cached subtrees
-                // are 0o555 / 0o444, but actions need to write outputs into
-                // their input tree, so make the clone writable. COW means
-                // these writes do not affect the source.
-                set_readwrite_recursive(dst_dir)
+                // Only chmod the destination root so the worker can create
+                // the action's declared output files inside it. Existing
+                // entries (subdirs and files) inherit the source's
+                // read-only mode (0o555) — that's the hermeticity
+                // contract. Skipping the per-file chmod walk avoids an
+                // O(N) syscall sweep that, on real Bazel SwiftCompile
+                // shapes (~2000 inputs), accounts for ~46% of
+                // materialization time.
+                chmod_dir_writable(dst_dir)
                     .await
-                    .err_tip(|| "Failed to make cloned tree writable")?;
+                    .err_tip(|| "Failed to chmod cloned tree root")?;
                 return Ok(CloneMethod::Clonefile);
             }
             Err(e) => {
@@ -178,6 +186,21 @@ async fn try_clonefile(src: &Path, dst: &Path) -> std::io::Result<()> {
     .map_err(std::io::Error::other)?
 }
 
+/// Sets the directory `dir`'s mode to 0o755 so callers can create new
+/// entries inside it. Used after `clonefile(2)` on the materialized
+/// destination root: the clone inherits the source's read-only mode
+/// (0o555) but the worker needs to drop the action's declared output
+/// files into the root. Existing entries inside `dir` are intentionally
+/// left at their cloned read-only perms — that's the hermeticity
+/// contract.
+#[cfg(target_os = "macos")]
+async fn chmod_dir_writable(dir: &Path) -> Result<(), Error> {
+    use std::os::unix::fs::PermissionsExt;
+    fs::set_permissions(dir, std::fs::Permissions::from_mode(0o755))
+        .await
+        .err_tip(|| format!("Failed to chmod {} to 0o755", dir.display()))
+}
+
 /// Internal recursive function to hardlink directory contents
 fn hardlink_directory_tree_recursive<'a>(
     src: &'a Path,
@@ -271,21 +294,6 @@ pub async fn set_readonly_recursive(dir: &Path) -> Result<(), Error> {
     set_perms_recursive_impl(dir.to_path_buf(), set_readonly_one_path).await
 }
 
-/// Sets a directory tree to read-write for the current user recursively.
-/// This is done so we can delete directories we're evicting.
-///
-/// # Arguments
-/// * `dir` - Directory to make read-write
-///
-/// # Platform Notes
-/// - Unix: Sets permissions to 0o755 (rwxr-xr-x)
-/// - Windows: Clears `FILE_ATTRIBUTE_READONLY`
-pub async fn set_readwrite_recursive(dir: &Path) -> Result<(), Error> {
-    error_if!(!dir.exists(), "Directory does not exist: {}", dir.display());
-
-    set_perms_recursive_impl(dir.to_path_buf(), set_readwrite_one_path).await
-}
-
 /// Sets only the **directories** in a tree to writable for the current user,
 /// leaving files untouched. This is the safe variant for cleanup paths that
 /// need to delete a tree containing CAS-hardlinked files.
@@ -319,10 +327,14 @@ fn set_readonly_one_path(
             use std::os::unix::fs::PermissionsExt;
             let mut perms = metadata.permissions();
 
-            // If it's a directory, set to r-xr-xr-x (555)
-            // If it's a file, set to r--r--r-- (444)
-            let mode = if metadata.is_dir() { 0o555 } else { 0o444 };
-            perms.set_mode(mode);
+            // Both directories and files get r-xr-xr-x (0o555): read and
+            // execute for everyone, write for no one. Files use 0o555 rather
+            // than 0o444 so the execute bit survives on cached executables —
+            // a stripped +x bit makes an action's interpreter or wrapper
+            // script fail with EACCES once the tree is materialized into a
+            // workspace. The write bit stays cleared, so the hermeticity
+            // contract (inputs are immutable) is unchanged.
+            perms.set_mode(0o555);
 
             fs::set_permissions(&path, perms)
                 .await
@@ -343,41 +355,6 @@ fn set_readonly_one_path(
     })
 }
 
-fn set_readwrite_one_path(
-    path: PathBuf,
-    metadata: Metadata,
-) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send>> {
-    Box::pin(async move {
-        // Set the file/directory to read-write for the current user
-        #[cfg(unix)]
-        {
-            use std::os::unix::fs::PermissionsExt;
-            let mut perms = metadata.permissions();
-
-            // If it's a directory, set to rwxr-xr-x (755)
-            // If it's a file, set to rw-r--r-- (644)
-            let mode = if metadata.is_dir() { 0o755 } else { 0o644 };
-            perms.set_mode(mode);
-
-            fs::set_permissions(&path, perms)
-                .await
-                .err_tip(|| format!("Failed to set permissions for: {}", path.display()))?;
-        }
-
-        #[cfg(windows)]
-        {
-            let mut perms = metadata.permissions();
-            perms.set_readonly(false);
-
-            fs::set_permissions(&path, perms)
-                .await
-                .err_tip(|| format!("Failed to set permissions for: {}", path.display()))?;
-        }
-
-        Ok(())
-    })
-}
-
 fn set_dir_writable_one_path(
     path: PathBuf,
     metadata: Metadata,
@@ -590,39 +567,103 @@ mod tests {
 
     #[cfg(target_os = "macos")]
     #[nativelink_test("crate")]
-    async fn test_clonefile_dest_is_writable() -> Result<(), Error> {
+    async fn test_clonefile_root_writable_inputs_readonly() -> Result<(), Error> {
         use std::os::unix::fs::PermissionsExt;
 
         let (temp_dir, src_dir) = create_test_directory().await?;
-        // Source mimics the directory cache: 0o555 dirs, 0o444 files.
+        // Source mimics the directory cache: 0o555 dirs and files.
         set_readonly_recursive(&src_dir).await?;
 
         let dst_dir = temp_dir.path().join("clone_dst");
         hardlink_directory_tree(&src_dir, &dst_dir).await?;
 
-        let src_subdir_mode = fs::metadata(src_dir.join("subdir"))
+        // Root: writable, so the worker can drop the action's declared
+        // outputs inside it.
+        let root_mode = fs::metadata(&dst_dir).await?.permissions().mode() & 0o777;
+        assert_eq!(root_mode, 0o755, "destination root must be writable");
+
+        // Subdir and existing file: stay read-only. Hermeticity contract —
+        // inputs are not writable. Matches Bazel's local-sandbox model and
+        // REAPI Action.output_files semantics: actions can only write to
+        // declared outputs, not mutate inputs.
+        let dst_subdir_mode = fs::metadata(dst_dir.join("subdir"))
             .await?
             .permissions()
             .mode()
             & 0o777;
         assert_eq!(
-            src_subdir_mode, 0o555,
-            "source dir should still be readonly after clone"
+            dst_subdir_mode, 0o555,
+            "cloned subdirs must inherit source read-only mode"
         );
 
-        let dst_subdir_mode = fs::metadata(dst_dir.join("subdir"))
+        let dst_file_mode = fs::metadata(dst_dir.join("file1.txt"))
             .await?
             .permissions()
             .mode()
             & 0o777;
         assert_eq!(
-            dst_subdir_mode, 0o755,
-            "cloned dir should be writable so actions can write outputs"
+            dst_file_mode, 0o555,
+            "cloned files must inherit source read-only mode"
+        );
+
+        // Source untouched.
+        let src_subdir_mode = fs::metadata(src_dir.join("subdir"))
+            .await?
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(
+            src_subdir_mode, 0o555,
+            "source dir should still be readonly after clone"
         );
 
         Ok(())
     }
 
+    #[cfg(target_os = "macos")]
+    #[nativelink_test("crate")]
+    async fn test_clonefile_root_accepts_new_files() -> Result<(), Error> {
+        let (temp_dir, src_dir) = create_test_directory().await?;
+        set_readonly_recursive(&src_dir).await?;
+
+        let dst_dir = temp_dir.path().join("clone_dst");
+        hardlink_directory_tree(&src_dir, &dst_dir).await?;
+
+        // The worker creates declared output files at the action's
+        // working directory root. Verify a new file can be created there
+        // even though everything inside the clone is read-only (0o555).
+        let new_output = dst_dir.join("new_output.bin");
+        fs::write(&new_output, b"action output").await?;
+        assert_eq!(fs::read(&new_output).await?, b"action output");
+
+        Ok(())
+    }
+
+    #[cfg(target_os = "macos")]
+    #[nativelink_test("crate")]
+    async fn test_clonefile_input_mutation_fails() -> Result<(), Error> {
+        let (temp_dir, src_dir) = create_test_directory().await?;
+        set_readonly_recursive(&src_dir).await?;
+
+        let dst_dir = temp_dir.path().join("clone_dst");
+        hardlink_directory_tree(&src_dir, &dst_dir).await?;
+
+        // Hermeticity: actions cannot mutate inputs. A write to an input
+        // file in the cloned tree must fail with EACCES, mirroring what
+        // Bazel's linux-sandbox / darwin-sandbox would do.
+        let input_file = dst_dir.join("file1.txt");
+        let err = fs::write(&input_file, b"mutated")
+            .await
+            .expect_err("input file write should fail (file is 0o555, no write bit)");
+        assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied);
+
+        // Source must be untouched.
+        let src_content = fs::read_to_string(src_dir.join("file1.txt")).await?;
+        assert_eq!(src_content, "Hello, World!");
+
+        Ok(())
+    }
+
     #[cfg(target_os = "macos")]
     #[nativelink_test("crate")]
     async fn test_clonefile_cow_isolation() -> Result<(), Error> {
@@ -647,6 +688,50 @@ mod tests {
         Ok(())
     }
 
+    /// Regression test for the clonefile directory-cache path. Bazel actions
+    /// declare outputs at paths nested inside input subdirectories, but
+    /// `hardlink_directory_tree` chmods only the destination *root* writable
+    /// — cloned subdirs keep the source's read-only mode (0o555). This test
+    /// mirrors the two-step `prepare_action_inputs` performs after a cache
+    /// hit (materialize, then `set_dir_writable_recursive`) and proves a
+    /// nested output can be created only after the recursive walk.
+    #[cfg(target_os = "macos")]
+    #[nativelink_test("crate")]
+    async fn test_clonefile_nested_output_after_dir_writable() -> Result<(), Error> {
+        use std::os::unix::fs::PermissionsExt;
+
+        let (temp_dir, src_dir) = create_test_directory().await?;
+        set_readonly_recursive(&src_dir).await?;
+
+        let dst_dir = temp_dir.path().join("clone_dst");
+        hardlink_directory_tree(&src_dir, &dst_dir).await?;
+
+        // The clone leaves "subdir" at 0o555, so creating an output nested
+        // inside it fails — this is the gap a root-only chmod ships.
+        let nested_output = dst_dir.join("subdir").join("nested_output.o");
+        let err = fs::write(&nested_output, b"x")
+            .await
+            .expect_err("nested write must fail while the subdir is 0o555");
+        assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied);
+
+        // This is what prepare_action_inputs does after a cache hit: make
+        // every directory in the materialized tree writable.
+        set_dir_writable_recursive(&dst_dir).await?;
+
+        fs::write(&nested_output, b"action output").await?;
+        assert_eq!(fs::read(&nested_output).await?, b"action output");
+
+        // Files inside the tree stay read-only — hermeticity holds.
+        let file_mode = fs::metadata(dst_dir.join("subdir").join("file2.txt"))
+            .await?
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(file_mode, 0o555, "input files must remain read-only");
+
+        Ok(())
+    }
+
     #[nativelink_test("crate")]
     async fn test_set_readonly_recursive() -> Result<(), Error> {
         let (_temp_dir, test_dir) = create_test_directory().await?;
@@ -663,6 +748,37 @@ mod tests {
         Ok(())
     }
 
+    /// `set_dir_writable_recursive` must make *every* directory in a tree
+    /// writable — including nested subdirs — so actions can create outputs
+    /// at nested declared-output paths. Files are left read-only because
+    /// they may share a CAS inode via hardlink.
+    #[cfg(unix)]
+    #[nativelink_test("crate")]
+    async fn test_set_dir_writable_recursive_walks_nested_dirs() -> Result<(), Error> {
+        use std::os::unix::fs::PermissionsExt;
+
+        let (_temp_dir, test_dir) = create_test_directory().await?;
+        // Lock the tree down the way the directory cache does post-construction.
+        set_readonly_recursive(&test_dir).await?;
+        set_dir_writable_recursive(&test_dir).await?;
+
+        // Every directory — the root and the nested subdir — must be writable.
+        for dir in [test_dir.clone(), test_dir.join("subdir")] {
+            let mode = fs::metadata(&dir).await?.permissions().mode() & 0o777;
+            assert_eq!(mode, 0o755, "{} must be writable", dir.display());
+        }
+
+        // Files stay read-only — chmoding them would corrupt a shared CAS inode.
+        let file_mode = fs::metadata(test_dir.join("subdir/file2.txt"))
+            .await?
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(file_mode, 0o555, "files must remain read-only");
+
+        Ok(())
+    }
+
     #[nativelink_test("crate")]
     async fn test_calculate_directory_size() -> Result<(), Error> {
         let (_temp_dir, test_dir) = create_test_directory().await?;
diff --git a/nativelink-worker/src/directory_cache.rs b/nativelink-worker/src/directory_cache.rs
@@ -451,8 +451,8 @@ impl DirectoryCache {
             // CRITICAL: only chmod directories writable, never files. Cached
             // files share an inode with FilesystemStore CAS entries via
             // hardlink (see `download_to_directory` in running_actions_manager).
-            // Calling `set_readwrite_recursive` here would chmod those files
-            // to 0o644, mutating the CAS inode's mode for every other in-flight
+            // A naive recursive chmod here would mutate those files to 0o644,
+            // changing the CAS inode's mode for every other in-flight
             // action that has hardlinked the same blob and causing EACCES on
             // exec (e.g. cc_wrapper.sh) or EPERM on open. Directory write
             // permission is sufficient on unix to unlink files inside.
@@ -733,8 +733,8 @@ mod tests {
             .unwrap();
 
         // Mark the cached tree read-only the way DirectoryCache does after
-        // construction (set_readonly_recursive). This drops the file to 0o444
-        // and the directories to 0o555.
+        // construction (set_readonly_recursive). This sets every file and
+        // directory to 0o555 (read + execute, no write).
         set_readonly_recursive(&cache_entry_dir).await?;
 
         // Simulate an in-flight action workspace that has hardlinked the
@@ -758,7 +758,7 @@ mod tests {
             .permissions()
             .mode()
             & 0o777;
-        assert_eq!(workspace_mode_before, 0o444);
+        assert_eq!(workspace_mode_before, 0o555);
 
         // Run the cleanup that `evict_lru` runs before removing the tree.
         // After the fix this only chmods directories; before the fix this
diff --git a/nativelink-worker/src/running_actions_manager.rs b/nativelink-worker/src/running_actions_manager.rs
