Fix CAS inode corruption: directory cache cleanup must not chmod files (#2347)

Port of TraceMachina/nativelink PR #2243 commit a47774d.

Root cause: when DirectoryCache evicts an entry, the cleanup path calls
`set_readwrite_recursive` on the cached tree before `remove_dir_all`. That
helper chmods every entry — including files — to 0o755/0o644. Files in
a cached entry are hardlinked into in-flight action workspaces (via
`hardlink_directory_tree` in `get_or_create`) and ultimately share an
inode with the underlying `FilesystemStore` CAS blob (via `fs::hard_link`
in `download_to_directory`). Chmoding the cached-side file therefore
silently mutates the shared inode's mode for every other in-flight
action holding a hardlink to the same blob.

Production symptom: EACCES on exec for `cc_wrapper.sh`. The CAS mode of
0o555 (r-xr-xr-x) gets clobbered to 0o644 (rw-r--r--), dropping the +x
bit while an unrelated action is mid-exec.

Fix: introduce `set_dir_writable_recursive` which only chmods
directories, never files. On unix, write permission on the parent
directory is sufficient to unlink files inside; the files' own modes
are irrelevant for unlinking. Switch the eviction cleanup path in
`DirectoryCache::evict_lru` to the new helper.

Empirically verified: a regression test that hardlinks a 0o555 file
into a cached tree and runs the cleanup helper FAILS on pre-fix code
(file mode mutated to 0o644) and PASSES on post-fix code.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: erneestoc

nativelink-util/src/fs_util.rs

@@ -286,6 +286,28 @@ pub async fn set_readwrite_recursive(dir: &Path) -> Result<(), Error> {
     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.
+///
+/// On unix, write permission on the parent directory is sufficient to unlink
+/// files inside it — the files' own modes are irrelevant for unlinking. Chmoding
+/// a CAS-hardlinked file would silently mutate the shared inode's permissions
+/// for every other in-flight action that has hardlinked the same blob, leading
+/// to EACCES on exec or EPERM on open in unrelated actions.
+///
+/// # Arguments
+/// * `dir` - Directory whose directories should be made writable
+///
+/// # Platform Notes
+/// - Unix: Sets directory permissions to 0o755 (rwxr-xr-x); files are NOT touched.
+/// - Windows: Clears `FILE_ATTRIBUTE_READONLY` on directories only; files are NOT touched.
+pub async fn set_dir_writable_recursive(dir: &Path) -> Result<(), Error> {
+    error_if!(!dir.exists(), "Directory does not exist: {}", dir.display());
+
+    set_perms_recursive_impl(dir.to_path_buf(), set_dir_writable_one_path).await
+}
+
 fn set_readonly_one_path(
     path: PathBuf,
     metadata: Metadata,
@@ -356,6 +378,43 @@ fn set_readwrite_one_path(
     })
 }
 
+fn set_dir_writable_one_path(
+    path: PathBuf,
+    metadata: Metadata,
+) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send>> {
+    Box::pin(async move {
+        // Files are intentionally skipped here. They may be hardlinked into
+        // the CAS (FilesystemStore); chmoding them would corrupt the shared
+        // inode's mode for every other in-flight action.
+        if !metadata.is_dir() {
+            return Ok(());
+        }
+
+        #[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()))?;
+        }
+
+        #[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_perms_recursive_impl<'a, F>(
     path: PathBuf,
     perms_fn: F,

nativelink-worker/src/directory_cache.rs

@@ -28,7 +28,7 @@ use nativelink_store::ac_utils::get_and_decode_digest;
 use nativelink_store::cas_utils::is_zero_digest;
 use nativelink_util::common::DigestInfo;
 use nativelink_util::fs_util::{
-    CloneMethod, hardlink_directory_tree, set_readonly_recursive, set_readwrite_recursive,
+    CloneMethod, hardlink_directory_tree, set_dir_writable_recursive, set_readonly_recursive,
 };
 use nativelink_util::store_trait::{Store, StoreKey, StoreLike};
 use tokio::fs;
@@ -448,12 +448,20 @@ impl DirectoryCache {
         {
             debug!(?digest, size = metadata.size, "Evicting cached directory");
 
-            if let Err(e) = set_readwrite_recursive(&metadata.path).await {
+            // 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
+            // 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.
+            if let Err(e) = set_dir_writable_recursive(&metadata.path).await {
                 warn!(
                     ?digest,
                     path = ?metadata.path,
                     error = ?e,
-                    "Unable to mark evicted directory as read/write, will probably fail to remove"
+                    "Unable to mark evicted directory as writable, will probably fail to remove"
                 );
             }
 
@@ -680,4 +688,116 @@ mod tests {
 
         Ok(())
     }
+
+    /// Regression test for CAS inode corruption during directory cache
+    /// eviction.
+    ///
+    /// Background: a cached directory's files share an inode (via hardlink)
+    /// with both the `FilesystemStore` CAS entry and every action workspace
+    /// that has consumed this cached directory. The cleanup path that runs
+    /// before `fs::remove_dir_all` used to call `set_readwrite_recursive`,
+    /// which chmods every file in the tree to 0o644 — silently mutating the
+    /// shared inode's mode for every in-flight action holding a hardlink to
+    /// the same blob. In production this surfaced as EACCES on exec for
+    /// `cc_wrapper.sh` (whose CAS mode is 0o555, but eviction turned it into
+    /// 0o644, dropping the +x bit).
+    ///
+    /// This test models the real scenario: a "cached" directory tree whose
+    /// files are hardlinked to a still-active "action workspace" file. The
+    /// eviction cleanup runs on the cached tree, and we assert the active
+    /// workspace file's mode is untouched. Before the fix this test fails
+    /// because `set_readwrite_recursive` chmods the cached-side file, and
+    /// the same inode underlies the workspace file.
+    #[cfg(unix)]
+    #[nativelink_test]
+    async fn test_eviction_cleanup_preserves_hardlinked_file_mode() -> Result<(), Error> {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+
+        use nativelink_util::fs_util::set_dir_writable_recursive;
+
+        let temp_dir = TempDir::new().unwrap();
+
+        // Build a "cached" directory tree mimicking what DirectoryCache
+        // produces: a top-level dir with a nested subdir and an executable
+        // file inside. Mode 0o555 matches the CAS mode of an executable like
+        // `cc_wrapper.sh`.
+        let cache_entry_dir = temp_dir.path().join("cache_entry");
+        let nested_dir = cache_entry_dir.join("nested");
+        fs::create_dir_all(&nested_dir).await.unwrap();
+        let cached_file = nested_dir.join("cc_wrapper.sh");
+        fs::write(&cached_file, b"#!/bin/sh\necho hi\n")
+            .await
+            .unwrap();
+        fs::set_permissions(&cached_file, std::fs::Permissions::from_mode(0o555))
+            .await
+            .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.
+        set_readonly_recursive(&cache_entry_dir).await?;
+
+        // Simulate an in-flight action workspace that has hardlinked the
+        // cached file. This is what `hardlink_directory_tree` does in
+        // `get_or_create` after the cached tree is built and locked down.
+        let action_workspace = temp_dir.path().join("action_workspace");
+        fs::create_dir_all(&action_workspace).await.unwrap();
+        let workspace_file = action_workspace.join("cc_wrapper.sh");
+        fs::hard_link(&cached_file, &workspace_file).await.unwrap();
+
+        // Sanity check: cached file and workspace file share the same inode.
+        let cached_ino = fs::metadata(&cached_file).await.unwrap().ino();
+        let workspace_ino = fs::metadata(&workspace_file).await.unwrap().ino();
+        assert_eq!(
+            cached_ino, workspace_ino,
+            "workspace file must share inode with cached file (hardlinked)",
+        );
+        let workspace_mode_before = fs::metadata(&workspace_file)
+            .await
+            .unwrap()
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(workspace_mode_before, 0o444);
+
+        // Run the cleanup that `evict_lru` runs before removing the tree.
+        // After the fix this only chmods directories; before the fix this
+        // chmoded every file to 0o644, mutating the shared inode.
+        set_dir_writable_recursive(&cache_entry_dir).await?;
+
+        // Critical assertion: the action workspace file's mode (i.e. the
+        // shared inode's mode) MUST be unchanged. If this fails, the cleanup
+        // path corrupted the inode for an in-flight action — that is the
+        // bug we are guarding against.
+        let workspace_mode_after = fs::metadata(&workspace_file)
+            .await
+            .unwrap()
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(
+            workspace_mode_after, workspace_mode_before,
+            "eviction cleanup mutated the inode mode of an active workspace \
+             file (was 0o{workspace_mode_before:o}, now 0o{workspace_mode_after:o}); \
+             this is the CAS inode corruption bug",
+        );
+
+        // We should still be able to remove the cached tree. This proves
+        // directory writability alone is sufficient to unlink files on unix.
+        fs::remove_dir_all(&cache_entry_dir).await.unwrap();
+        assert!(!cache_entry_dir.exists());
+
+        // The workspace's file is still intact and the mode survives even
+        // after the cached tree is gone.
+        assert!(workspace_file.exists());
+        let workspace_mode_after_remove = fs::metadata(&workspace_file)
+            .await
+            .unwrap()
+            .permissions()
+            .mode()
+            & 0o777;
+        assert_eq!(workspace_mode_after_remove, workspace_mode_before);
+
+        Ok(())
+    }
 }