Clear a read-only tree without changing the mode of the files it links to

nativelink-util/src/fs.rs

@@ -513,6 +513,13 @@ pub async fn symlink_metadata(path: impl AsRef<Path>) -> Result<Metadata, Error>
 // We can't just use the stock remove_dir_all as it falls over if someone's set readonly
 // permissions. This version walks the directories and fixes the permissions where needed
 // before deleting everything.
+//
+// Only directories are touched. Unlinking a file needs write and search
+// permission on its directory, never on the file itself, and the files of
+// a worker's tree are hardlinks to CAS blobs and executable variants that
+// other actions share: a chmod on one of those changes the inode for every
+// tree it is linked into, and an executable variant would lose its execute
+// bit under the actions still running it.
 #[cfg(not(target_family = "windows"))]
 fn internal_remove_dir_all(path: impl AsRef<Path>) -> Result<(), Error> {
     // Because otherwise Windows builds complain about these things not being used
@@ -538,9 +545,6 @@ fn internal_remove_dir_all(path: impl AsRef<Path>) -> Result<(), Error> {
                 }
                 e @ Err(_) => e.err_tip(|| format!("Removing {}", entry.path().display()))?,
             }
-        } else if metadata.is_file() {
-            std::fs::set_permissions(entry.path(), Permissions::from_mode(0o600))
-                .err_tip(|| format!("Setting permissions for {}", entry.path().display()))?;
         }
     }
 

nativelink-util/tests/fs_test.rs

@@ -153,6 +153,35 @@ async fn remove_files_with_bad_permissions() -> Result<(), Box<dyn core::error::
     Ok(())
 }
 
+/// A worker's tree is hardlinks to blobs other actions share. Clearing a
+/// tree with a read-only directory in it must not change those inodes: an
+/// executable variant keeps its execute bit for the actions still running
+/// it, and the tree is still gone.
+#[nativelink_test]
+async fn remove_dir_all_leaves_shared_inodes_alone() -> Result<(), Box<dyn core::error::Error>> {
+    let dir = env::temp_dir().join("remove_dir_all_shared_inodes_test");
+    drop(remove_dir_all(&dir).await);
+    fs::create_dir_all(dir.join("tree/readonly"))?;
+    // The blob lives outside the tree, as a CAS blob does, linked into it.
+    let blob = dir.join("blob.exe");
+    fs::write(&blob, "#!/bin/sh\n")?;
+    fs::set_permissions(&blob, Permissions::from_mode(0o555))?;
+    fs::hard_link(&blob, dir.join("tree/readonly/tool"))?;
+    fs::hard_link(&blob, dir.join("tree/tool"))?;
+    fs::set_permissions(dir.join("tree/readonly"), Permissions::from_mode(0o500))?;
+
+    remove_dir_all(dir.join("tree")).await?;
+
+    assert!(!fs::exists(dir.join("tree"))?, "the tree is gone");
+    assert_eq!(
+        fs::metadata(&blob)?.permissions().mode() & 0o777,
+        0o555,
+        "the shared blob keeps its mode"
+    );
+    remove_dir_all(&dir).await?;
+    Ok(())
+}
+
 #[cfg(target_os = "linux")]
 #[nativelink_test]
 async fn freebind_allows_binding_unassigned_address() -> Result<(), Box<dyn core::error::Error>> {