Skip to content

Remove folders with bad permissions#1980

Merged
palfrey merged 1 commit intoTraceMachina:mainfrom
palfrey:remove-with-bad-permissions
Oct 16, 2025
Merged

Remove folders with bad permissions#1980
palfrey merged 1 commit intoTraceMachina:mainfrom
palfrey:remove-with-bad-permissions

Conversation

@palfrey
Copy link
Member

@palfrey palfrey commented Oct 15, 2025

Description

Running abseil-py gets the error
2025-10-15T10:42:08.665910Z ERROR nativelink_worker::running_actions_manager: Error removing working directory, operation_id: Uuid(9843cbd8-8842-40bc-8055-4ed42ecdb1d5), err: Error { code: PermissionDenied, messages: ["Permission denied (os error 13)", "Could not remove working directory /tmp/nativelink/work/9843cbd8-8842-40bc-8055-4ed42ecdb1d5"] }

The reason for this is it makes a directory called test_create_file_fails_cleanup with execute-only permissions

ls -l /tmp/nativelink/work/74e09c35-5de0-4710-9b1d-ecbb7232b2cf/work/_tmp/ab852d4144af12b81ada3000195799c8/TempFileTest/                             
total 4
d--x------ 2 palfrey users 4096 Oct 15 11:44 test_create_file_fails_cleanup

The user is able to delete the file, but the standard Rust remove_dir_all fails to do so, so I've written new code to walk the directory. Tried using the rm_rf crate first, but it has a bug in our test scenario.

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

bazel test //...

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@palfrey palfrey force-pushed the remove-with-bad-permissions branch from 367a934 to c855ff9 Compare October 15, 2025 12:21
@palfrey palfrey marked this pull request as draft October 15, 2025 12:21
@palfrey palfrey marked this pull request as ready for review October 15, 2025 12:52
@palfrey palfrey requested a review from amankrx October 15, 2025 13:30
@palfrey palfrey marked this pull request as draft October 15, 2025 13:57
@palfrey palfrey force-pushed the remove-with-bad-permissions branch from c855ff9 to 58a33ae Compare October 15, 2025 15:38
@palfrey palfrey force-pushed the remove-with-bad-permissions branch from 58a33ae to d8a2a94 Compare October 15, 2025 15:41
@palfrey palfrey force-pushed the remove-with-bad-permissions branch from d8a2a94 to 9c0a0d4 Compare October 15, 2025 16:02
Copy link
Contributor

@chrisstaite chrisstaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 LGTMs obtained, and 0 of 9 files reviewed


nativelink-util/src/fs.rs line 375 at r1 (raw file):

    for entry in WalkDir::new(&path) {
        let entry = match &entry {

Since you're ignoring the Err value the usual way to do this would be:

let Ok(entry) = &entry else {
    debug!("Can't get into {entry:?}, assuming already deleted");
    continue;
};

nativelink-util/src/fs.rs line 386 at r1 (raw file):

            match std::fs::remove_dir_all(entry.path()) {
                Ok(()) => {}
                Err(e) => {

You could use a matcher here to avoid the nested if:

Err(e) if e.kind() == ErrorKind::PermissionDenied => {...}
e @ Err(_) => e.err_tip(|| format!("Removing {}", entry.path().display()))?,

nativelink-util/src/fs.rs line 403 at r1 (raw file):

        }
    }
    if exists(&path)? {

Rather than the extra exists check, you could just call remove_dir_all and ignore the NotFound error.

Copy link
Member Author

@palfrey palfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, those are good catches. Made all those changes now.

Reviewable status: 0 of 1 LGTMs obtained, and 0 of 9 files reviewed, and pending CI: macos-15, ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, integration-tests, ubuntu-24.04 / stable, windows-2022 / stable, pre-commit-checks, Analyze (javascript-typescript), Analyze (python), Coverage, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Web Platform Deployment / macos-15, Web Platform Deployment / ubuntu-24.04, Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Installation / macos-15, Installation / ubuntu-24.04, buildstream, mongo, asan / ubuntu-24.04, Local / bazel / ubuntu-24.04, vale

@palfrey palfrey force-pushed the remove-with-bad-permissions branch from 11420c1 to d4012b2 Compare October 16, 2025 10:55
@palfrey palfrey merged commit 5e487f3 into TraceMachina:main Oct 16, 2025
30 of 31 checks passed
@palfrey palfrey deleted the remove-with-bad-permissions branch October 16, 2025 12:37
MarcusSorealheis pushed a commit to MarcusSorealheis/nativelink that referenced this pull request Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants