erneestoc
diff --git a/‎nativelink-util/src/fs_util.rs‎
Lines changed: 174 additions & 71 deletions b/‎nativelink-util/src/fs_util.rs‎
Lines changed: 174 additions & 71 deletions
@@ -51,16 +51,23 @@ 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, 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.
+///   `fs::hard_link`. `clonefile(2)` copies the source's modes verbatim, so
+///   the destination's directory/file modes mirror the source. For a directory
+///   cache entry locked down by [`set_readonly_recursive`], that means
+///   directories are writable (0o755) and files are read-only (0o555): the
+///   worker can create the action's declared outputs at any nested path, but
+///   the hardlinked input files stay immutable. 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. The destination root is additionally chmod'd to 0o755 as a
+///   defensive guarantee for callers that did not pre-mark the source.
 /// - Linux: Per-file `fs::hard_link` (directory hardlinks are not supported on
-///   ext4/btrfs without root). Always returns `CloneMethod::Hardlink`.
+///   ext4/btrfs without root). Directories at the destination are created
+///   fresh by this walk, so they are writable regardless of the source's
+///   directory modes; files are hardlinked and keep the source inode's mode.
+///   Always returns `CloneMethod::Hardlink`.
 /// - Windows: Per-file `fs::hard_link` (requires NTFS). Always returns
 ///   `CloneMethod::Hardlink`.
 ///
@@ -101,14 +108,15 @@ pub async fn hardlink_directory_tree(src_dir: &Path, dst_dir: &Path) -> Result<C
 
         match try_clonefile(src_dir, dst_dir).await {
             Ok(()) => {
-                // 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.
+                // `clonefile(2)` copies the source's modes verbatim. A
+                // directory cache entry locked down by
+                // `set_readonly_recursive` already has writable directories
+                // (0o755) and read-only files (0o555), so the clone is
+                // immediately usable: the worker can create declared outputs
+                // at any nested path and the hardlinked inputs stay
+                // immutable. No per-directory chmod walk is needed. The root
+                // is still chmod'd here as a defensive guarantee for callers
+                // that pass a source whose root was not pre-marked writable.
                 chmod_dir_writable(dst_dir)
                     .await
                     .err_tip(|| "Failed to chmod cloned tree root")?;
@@ -188,11 +196,12 @@ async fn try_clonefile(src: &Path, dst: &Path) -> std::io::Result<()> {
 
 /// 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.
+/// destination root as a defensive guarantee: a directory cache entry locked
+/// down by [`set_readonly_recursive`] already has writable directories, so
+/// for those callers this is a no-op, but it keeps `hardlink_directory_tree`
+/// correct for any source whose root was not pre-marked writable. Existing
+/// entries inside `dir` are intentionally left at their cloned perms — files
+/// stay read-only (the hermeticity contract), directories stay writable.
 #[cfg(target_os = "macos")]
 async fn chmod_dir_writable(dir: &Path) -> Result<(), Error> {
     use std::os::unix::fs::PermissionsExt;
@@ -279,15 +288,33 @@ fn hardlink_directory_tree_recursive<'a>(
     })
 }
 
-/// Sets a directory tree to read-only recursively.
-/// This prevents actions from modifying cached directories.
+/// Locks down a directory tree as an immutable cache entry: every **file** is
+/// made read-only, every **directory** is left writable.
+///
+/// This is used by the worker's directory cache after it constructs a cache
+/// entry. Files must be read-only because they are hardlinked into the CAS
+/// (`FilesystemStore`) — keeping them immutable preserves the hermeticity
+/// contract (actions cannot mutate inputs) and avoids mutating the shared
+/// inode's mode for other in-flight actions.
+///
+/// Directories are deliberately left writable (0o755). Directories are *not*
+/// hardlink-shared between cache entries — only file content inodes are — so a
+/// writable directory mode is safe. Keeping cache-entry directories writable
+/// means the materialized destination tree (an APFS `clonefile(2)` clone,
+/// which copies modes verbatim, or a per-file hardlink walk, which creates
+/// fresh directories) already has writable directories. Bazel actions declare
+/// outputs at paths nested inside input subdirectories, so every directory in
+/// the materialized tree must be writable for the worker to create those
+/// outputs; doing it here, once per cache entry, removes the need for a
+/// separate per-materialization recursive chmod walk.
 ///
 /// # Arguments
-/// * `dir` - Directory to make read-only
+/// * `dir` - Directory tree to lock down
 ///
 /// # Platform Notes
-/// - Unix: Sets permissions to 0o555 (r-xr-xr-x)
-/// - Windows: Sets `FILE_ATTRIBUTE_READONLY`
+/// - Unix: files get 0o555 (r-xr-xr-x); directories get 0o755 (rwxr-xr-x).
+/// - Windows: files get `FILE_ATTRIBUTE_READONLY`; directories are left
+///   writable.
 pub async fn set_readonly_recursive(dir: &Path) -> Result<(), Error> {
     error_if!(!dir.exists(), "Directory does not exist: {}", dir.display());
 
@@ -321,19 +348,43 @@ fn set_readonly_one_path(
     metadata: Metadata,
 ) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send>> {
     Box::pin(async move {
-        // Set the file/directory to read-only
+        // Directories are left writable on purpose. They are not
+        // hardlink-shared between cache entries — only file content inodes
+        // are — so a writable directory mode cannot corrupt anything. Keeping
+        // them writable means the materialized destination tree already
+        // accepts the nested output files Bazel actions declare, with no
+        // separate per-materialization chmod walk.
+        if metadata.is_dir() {
+            #[cfg(unix)]
+            {
+                use std::os::unix::fs::PermissionsExt;
+                let mut perms = metadata.permissions();
+                perms.set_mode(0o755);
+
+                fs::set_permissions(&path, perms)
+                    .await
+                    .err_tip(|| format!("Failed to set permissions for: {}", path.display()))?;
+            }
+
+            // On Windows directories are already writable; clearing the
+            // read-only attribute here would be a no-op, so leave them alone.
+
+            return Ok(());
+        }
+
+        // Set the file to read-only.
         #[cfg(unix)]
         {
             use std::os::unix::fs::PermissionsExt;
             let mut perms = metadata.permissions();
 
-            // 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.
+            // 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)
@@ -567,11 +618,12 @@ mod tests {
 
     #[cfg(target_os = "macos")]
     #[nativelink_test("crate")]
-    async fn test_clonefile_root_writable_inputs_readonly() -> Result<(), Error> {
+    async fn test_clonefile_dirs_writable_files_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 and files.
+        // Source mimics a directory cache entry: writable dirs (0o755),
+        // read-only files (0o555).
         set_readonly_recursive(&src_dir).await?;
 
         let dst_dir = temp_dir.path().join("clone_dst");
@@ -582,20 +634,25 @@ mod tests {
         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.
+        // Nested subdir: writable too. `clonefile(2)` copies the source's
+        // modes verbatim and the source's directories were left writable by
+        // `set_readonly_recursive`. Bazel actions declare outputs at paths
+        // nested inside input subdirectories, so every directory in the
+        // materialized tree must be writable — no separate chmod walk needed.
         let dst_subdir_mode = fs::metadata(dst_dir.join("subdir"))
             .await?
             .permissions()
             .mode()
             & 0o777;
         assert_eq!(
-            dst_subdir_mode, 0o555,
-            "cloned subdirs must inherit source read-only mode"
+            dst_subdir_mode, 0o755,
+            "cloned subdirs must be writable so nested outputs can be created"
         );
 
+        // Existing file: stays 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_file_mode = fs::metadata(dst_dir.join("file1.txt"))
             .await?
             .permissions()
@@ -606,15 +663,24 @@ mod tests {
             "cloned files must inherit source read-only mode"
         );
 
-        // Source untouched.
+        // Source untouched: dirs writable, files read-only.
         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"
+            src_subdir_mode, 0o755,
+            "source dir should still be writable after clone"
+        );
+        let src_file_mode = fs::metadata(src_dir.join("file1.txt"))
+            .await?
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(
+            src_file_mode, 0o555,
+            "source file should still be read-only after clone"
         );
 
         Ok(())
@@ -688,78 +754,115 @@ 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.
+    /// Bazel actions declare outputs at paths nested inside input
+    /// subdirectories. Because `set_readonly_recursive` leaves directories
+    /// writable and `clonefile(2)` copies modes verbatim, the materialized
+    /// tree already accepts a nested output file with NO separate
+    /// `set_dir_writable_recursive` walk — that is the redundant work this
+    /// change removes from `prepare_action_inputs`.
     #[cfg(target_os = "macos")]
     #[nativelink_test("crate")]
-    async fn test_clonefile_nested_output_after_dir_writable() -> Result<(), Error> {
+    async fn test_clonefile_nested_output_without_dir_writable_walk() -> Result<(), Error> {
         use std::os::unix::fs::PermissionsExt;
 
         let (temp_dir, src_dir) = create_test_directory().await?;
+        // Lock the source down the way the directory cache does after
+        // constructing a cache entry: writable dirs, read-only files.
         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.
+        // Creating an output nested inside a cloned subdir succeeds straight
+        // away — no recursive chmod walk. This is the post-condition that
+        // lets `prepare_action_inputs` drop its `set_dir_writable_recursive`
+        // call.
         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.
+        // Files inside the tree stay read-only — hermeticity holds, and the
+        // CAS-hardlink inode invariant is preserved.
         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");
 
+        // A write to an input file still fails — actions cannot mutate inputs.
+        let err = fs::write(dst_dir.join("subdir").join("file2.txt"), b"mutated")
+            .await
+            .expect_err("input file write must fail (file is 0o555)");
+        assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied);
+
         Ok(())
     }
 
+    /// `set_readonly_recursive` locks a tree down as a cache entry: every
+    /// file is made read-only, every directory is left writable. Directories
+    /// stay writable because they are not hardlink-shared between cache
+    /// entries, and a writable directory mode lets the materialized
+    /// destination tree accept nested action outputs without a separate
+    /// chmod walk.
     #[nativelink_test("crate")]
     async fn test_set_readonly_recursive() -> Result<(), Error> {
         let (_temp_dir, test_dir) = create_test_directory().await?;
 
         set_readonly_recursive(&test_dir).await?;
 
-        // Verify files are read-only
+        // Files are read-only.
         let metadata = fs::metadata(test_dir.join("file1.txt")).await?;
         assert!(metadata.permissions().readonly());
 
         let metadata = fs::metadata(test_dir.join("subdir/file2.txt")).await?;
         assert!(metadata.permissions().readonly());
 
+        // Directories are left writable — root and every nested subdir.
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            for dir in [test_dir.clone(), test_dir.join("subdir")] {
+                let mode = fs::metadata(&dir).await?.permissions().mode() & 0o777;
+                assert_eq!(mode, 0o755, "{} must stay writable", dir.display());
+            }
+        }
+        #[cfg(windows)]
+        {
+            // On Windows directories carry no read-only attribute that would
+            // block creating children; assert they are not marked read-only.
+            for dir in [test_dir.clone(), test_dir.join("subdir")] {
+                assert!(
+                    !fs::metadata(&dir).await?.permissions().readonly(),
+                    "{} must stay writable",
+                    dir.display()
+                );
+            }
+        }
+
         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.
+    /// writable — including nested subdirs — so the eviction cleanup path can
+    /// `remove_dir_all` a cache entry. Files are left read-only because they
+    /// may share a CAS inode via hardlink. This walk runs on already-read-only
+    /// directory trees too, so the test first sets every file read-only with
+    /// `set_readonly_recursive`.
     #[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.
+        // Lock files down, then explicitly force every directory read-only so
+        // the walk has real work to do (the directory cache leaves dirs
+        // writable, but the eviction path must cope with any mode).
         set_readonly_recursive(&test_dir).await?;
+        for dir in [test_dir.clone(), test_dir.join("subdir")] {
+            fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o555)).await?;
+        }
+
         set_dir_writable_recursive(&test_dir).await?;
 
         // Every directory — the root and the nested subdir — must be writable.