Skip to content

worker: make directory-cache entries already-writable#2357

Open
erneestoc wants to merge 3 commits into
TraceMachina:mainfrom
erneestoc:ec/pr2243-cache-writable-dirs
Open

worker: make directory-cache entries already-writable#2357
erneestoc wants to merge 3 commits into
TraceMachina:mainfrom
erneestoc:ec/pr2243-cache-writable-dirs

Conversation

@erneestoc
Copy link
Copy Markdown
Contributor

@erneestoc erneestoc commented May 22, 2026

Problem

DirectoryCache locks each cache entry down with set_readonly_recursive after
construction. That helper previously made the entire entry tree mode 0o555
directories included — so every materialization had to follow up with a separate
set_dir_writable_recursive recursive chmod walk in prepare_action_inputs to
re-add write permission to directories (Bazel actions declare outputs at paths
nested inside input subdirectories).

That per-materialization post-walk is redundant work.

Fix

Directories are not hardlink-shared between cache entries — only file content
inodes are — so directory mode can safely be set once, at the cache entry,
instead of on every materialization.

set_readonly_recursive now makes only files read-only (0o555) and leaves
directories writable (0o755). Both materialization paths then produce a
directly-usable tree:

  • macOS clonefile(2) copies the source's modes verbatim → writable dirs,
    read-only files.
  • The Linux per-file hardlink walk creates fresh (writable) directories and
    hardlinks files (which keep the source inode's read-only mode).

Files stay read-only on both paths, so the hermeticity contract and the
CAS-hardlink shared-inode invariant (#2347) are preserved. The now-redundant
set_dir_writable_recursive call is removed from prepare_action_inputs;
set_dir_writable_recursive itself is unchanged and still used by the cache
eviction cleanup path.

Testing

  • fs_util: test_set_readonly_recursive now also asserts directories stay
    writable; the macOS clonefile tests assert cloned subdirs are writable and
    that a nested output file can be created with no set_dir_writable_recursive
    walk.
  • directory_cache: new test_materialized_tree_dirs_writable_files_readonly
    asserts that after get_or_create on both the fresh-materialize and
    cache-hit paths, every directory is writable and every file read-only.
  • bazel test //nativelink-worker/... //nativelink-util/... — 27/27 pass;
    clippy + rustfmt aspects clean.

Note

Companion to #2358 (util: make permission walks symlink-safe). Both PRs touch
nativelink-util/src/fs_util.rs (hardlink_directory_tree_recursive);
whichever lands second needs a trivial rebase.

🤖 Generated with Claude Code


This change is Reviewable

`DirectoryCache` locks each cache entry down with `set_readonly_recursive`
after construction. Previously that helper made the entire entry tree mode
0o555 — directories included — so every materialization had to follow up
with a separate `set_dir_writable_recursive` recursive chmod walk in
`prepare_action_inputs` to re-add write permission to directories (Bazel
actions declare outputs at paths nested inside input subdirectories).

That post-walk is redundant work. Directories are not hardlink-shared
between cache entries — only file content inodes are — so directory mode
can safely be made writable once, at the cache entry, instead of on every
materialization.

`set_readonly_recursive` now locks a tree down as a cache entry by making
only FILES read-only (0o555) and leaving DIRECTORIES writable (0o755).
Both materialization paths then produce a directly-usable tree:

- macOS `clonefile(2)` copies the source's modes verbatim, so the clone's
  directories are writable and its files read-only.
- The Linux per-file hardlink walk creates fresh directories (writable)
  and hardlinks files (which keep the source inode's read-only mode).

Files stay read-only on both paths, so the hermeticity contract and the
CAS-hardlink shared-inode invariant (PR TraceMachina#2347) are preserved. With the
materialized tree already correct, the `set_dir_writable_recursive` call
is removed from `prepare_action_inputs`. `set_dir_writable_recursive`
itself is unchanged and still used by the cache eviction cleanup path.

Tests:
- fs_util: `test_set_readonly_recursive` now also asserts directories stay
  writable; the macOS clonefile tests assert cloned subdirs are writable
  and that a nested output can be created with no `set_dir_writable_recursive`
  walk; `test_set_dir_writable_recursive_walks_nested_dirs` keeps covering
  the eviction-cleanup helper.
- directory_cache: new `test_materialized_tree_dirs_writable_files_readonly`
  builds a nested tree and asserts that, after `get_or_create` on both the
  fresh-materialize and cache-hit paths, every directory is writable and
  every file is read-only, with no separate chmod walk.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@erneestoc erneestoc force-pushed the ec/pr2243-cache-writable-dirs branch from 8ad0ce7 to 2bc32f9 Compare May 22, 2026 03:16
@MarcusSorealheis
Copy link
Copy Markdown
Collaborator

@erneestoc I need time to test it. Also, could you increment the versions of all the top-level Cargo.toml's in line with the instructions in CONTRIBUTING.md#creating-releases. We are basically going to release patch versions a lot more frequently to tighten the feedback loop.

@MarcusSorealheis
Copy link
Copy Markdown
Collaborator

Also, for the past two versions Claude Code has been able to follow the instructions quite well so you could hand it off there.

Bump the workspace version 1.3.0 -> 1.3.1 in MODULE.bazel, the root Cargo.toml, and every nativelink-* crate manifest, per CONTRIBUTING.md step 1 (Creating releases). Cargo.lock refreshed for the 13 workspace members.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@erneestoc
Copy link
Copy Markdown
Contributor Author

@MarcusSorealheis bumped the workspace version 1.3.01.3.1 in MODULE.bazel, the root Cargo.toml, and every nativelink-* crate manifest, per CONTRIBUTING.md#creating-releases step 1; Cargo.lock refreshed for the 13 workspace members. The branch had already picked up the 1.3.0 release via the main merge, so this is the patch bump on top. Bazel build is green.

@MarcusSorealheis
Copy link
Copy Markdown
Collaborator

this one was merged by @2362, or will be.

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.

2 participants