Skip to content

Commit de74f3c

Browse files
erneestocclaude
andcommitted
fs_util: remove dead set_readwrite_recursive after post-TraceMachina#2347 cleanup
After TraceMachina#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 TraceMachina#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>
1 parent 28422b2 commit de74f3c

2 files changed

Lines changed: 2 additions & 52 deletions

File tree

nativelink-util/src/fs_util.rs

Lines changed: 0 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -294,21 +294,6 @@ pub async fn set_readonly_recursive(dir: &Path) -> Result<(), Error> {
294294
set_perms_recursive_impl(dir.to_path_buf(), set_readonly_one_path).await
295295
}
296296

297-
/// Sets a directory tree to read-write for the current user recursively.
298-
/// This is done so we can delete directories we're evicting.
299-
///
300-
/// # Arguments
301-
/// * `dir` - Directory to make read-write
302-
///
303-
/// # Platform Notes
304-
/// - Unix: Sets permissions to 0o755 (rwxr-xr-x)
305-
/// - Windows: Clears `FILE_ATTRIBUTE_READONLY`
306-
pub async fn set_readwrite_recursive(dir: &Path) -> Result<(), Error> {
307-
error_if!(!dir.exists(), "Directory does not exist: {}", dir.display());
308-
309-
set_perms_recursive_impl(dir.to_path_buf(), set_readwrite_one_path).await
310-
}
311-
312297
/// Sets only the **directories** in a tree to writable for the current user,
313298
/// leaving files untouched. This is the safe variant for cleanup paths that
314299
/// need to delete a tree containing CAS-hardlinked files.
@@ -366,41 +351,6 @@ fn set_readonly_one_path(
366351
})
367352
}
368353

369-
fn set_readwrite_one_path(
370-
path: PathBuf,
371-
metadata: Metadata,
372-
) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send>> {
373-
Box::pin(async move {
374-
// Set the file/directory to read-write for the current user
375-
#[cfg(unix)]
376-
{
377-
use std::os::unix::fs::PermissionsExt;
378-
let mut perms = metadata.permissions();
379-
380-
// If it's a directory, set to rwxr-xr-x (755)
381-
// If it's a file, set to rw-r--r-- (644)
382-
let mode = if metadata.is_dir() { 0o755 } else { 0o644 };
383-
perms.set_mode(mode);
384-
385-
fs::set_permissions(&path, perms)
386-
.await
387-
.err_tip(|| format!("Failed to set permissions for: {}", path.display()))?;
388-
}
389-
390-
#[cfg(windows)]
391-
{
392-
let mut perms = metadata.permissions();
393-
perms.set_readonly(false);
394-
395-
fs::set_permissions(&path, perms)
396-
.await
397-
.err_tip(|| format!("Failed to set permissions for: {}", path.display()))?;
398-
}
399-
400-
Ok(())
401-
})
402-
}
403-
404354
fn set_dir_writable_one_path(
405355
path: PathBuf,
406356
metadata: Metadata,

nativelink-worker/src/directory_cache.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,8 +451,8 @@ impl DirectoryCache {
451451
// CRITICAL: only chmod directories writable, never files. Cached
452452
// files share an inode with FilesystemStore CAS entries via
453453
// hardlink (see `download_to_directory` in running_actions_manager).
454-
// Calling `set_readwrite_recursive` here would chmod those files
455-
// to 0o644, mutating the CAS inode's mode for every other in-flight
454+
// A naive recursive chmod here would mutate those files to 0o644,
455+
// changing the CAS inode's mode for every other in-flight
456456
// action that has hardlinked the same blob and causing EACCES on
457457
// exec (e.g. cc_wrapper.sh) or EPERM on open. Directory write
458458
// permission is sufficient on unix to unlink files inside.

0 commit comments

Comments
 (0)