Conversation
After a directory construction fails, another caller may already be waiting on its lock. Removing that lock from the map lets a new caller create a different lock and construct the same cache path concurrently with the waiting retry. Keep the lock mapped while other callers hold references to it. Remove it only when the map and the finishing caller are its sole owners, with the map locked to prevent new callers from racing the reference check. Add a regression using the existing get_or_create API that verifies a new request waits for the retry's lock and cleanup removes the lock after the final caller finishes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dumbmoron
changed the title
Chromium builds for macOS use the same Mac SDK across thousands of compile jobs. The directory cache already shares the prepared SDK, but each job still recreates its directories and links, then deletes them afterward. Add experimental_readonly_input_mounts so Linux workers can mount selected cached directories read-only into each job instead. This avoids repeated filesystem work for large, unchanged inputs such as SDKs. The option is disabled by default. Mounted directories stay in the cache until job cleanup finishes, and incompatible jobs use normal input setup.
|
Ok ok, this one is a bit more involved. Give me a second to review |
erneestoc
left a comment
erneestoc
left a comment
There was a problem hiding this comment.
Thanks for this, the mechanics are carefully done. Notes below, most important first. Since #2750 merged on Sep 18 this needs a rebase; it still carries that commit.
What looks right
- Inputs are bound after the namespace goes private and before the tmpfs masks the root, and the self-bind uses
MS_RECso the submounts survive the mask. The test that puts the source under the masked root is a good one. - Absolute paths are resolved in the parent because std runs
pre_execafterchdir. - The remount carries
noexec/atime flags forward fromstatvfs, which a user namespace requires for locked mounts. - A failed mount aborts the spawn instead of running against an empty placeholder and caching a wrong result.
- The cache pin outlives cleanup via the background drop task; the abandoned-action case is covered.
- Eligibility is conservative: overlapping outputs, cwd inside a mount,
..escaping the root, and persistent workers all fall back. - Replacing the
unwrap()inperform_remountwith?and not tombstoning a tree another action is mid-copy from are both improvements on their own.
1. The "mounts disabled" baseline does more work per action than the traditional path. I have not measured this, so please treat it as a request for a number rather than a claim about the magnitude. From directory_cache.rs: with experimental_subtree_caching on and a unique root per compile, every action is a root miss. construct_entry builds the root into a temp tree, during which create_subdirectory hits the SDK subtree and runs hardlink_directory_tree from the SDK entry into the temp root (walk 1). After the rename, construct_and_materialize runs hardlink_directory_tree again from the root entry into the workspace (walk 2). The root entry is then never reused and is eventually evicted, which is a third walk to delete it. The traditional path (directory_cache off) links each file once. So the 256 s / 72 s row is measuring the root cache's known cost on unique roots, not just the cost mounts remove. Could you add a row with directory_cache disabled, and a second sample per cell? The mount numbers may well still win, but that is the comparison an operator deciding whether to enable this needs.
2. When mounts apply, the directory cache is bypassed for the rest of the input tree. In inner_prepare, the mount branch calls download_to_directory_with_mounts directly and never reaches prepare_action_inputs_with_lease, so neither root nor subtree caching is used for the remaining inputs. For Chromium compiles that is probably fine, but any workload whose other inputs were getting cache hits regresses on those, and nothing documents that the two features are exclusive per action. Worth a sentence in the config doc, and possibly a follow-up that lets create_subdirectory leave a mount point for a selected path instead.
3. "Read-only" cannot hold against the action itself. The bind is created inside the action's own user namespace, so MNT_LOCK_READONLY never applies to it. The action runs as mapped root with CAP_SYS_ADMIN in that namespace and can mount -o remount,bind,rw the target and mutate the shared cache tree. Every later action on that worker using that digest then gets a corrupted input with a valid-looking action cache entry. The traditional path already exposes CAS blobs through hardlinks, so this broadens an existing exposure rather than creating a new class, but the doc string's "not a security boundary" undersells it: a single misbehaving action can silently poison results for every other action on the worker. Suggest saying that plainly, and as a follow-up consider dropping CAP_SYS_ADMIN from the bounding set before exec when mounts are active.
4. No fallback when prepare_for_mount fails. An error there propagates out of collect_download_links and fails the action. The cache path in prepare_action_inputs_with_lease warns and falls back to traditional download in the same situation. One retry without mounts would match existing behaviour.
5. Paths whose digest varies per action regress. A selected path that is not digest-stable gets a fresh cache construction per action, pinned until cleanup, which is strictly more work than the traditional path and thrashes the cache. The config doc should say that only digest-stable trees belong in the list.
6. No observability. There are no counters for mount hits, eligibility fallbacks, or prepare failures; in_use_entries is the only signal. An operator cannot tell whether the option is doing anything or how often it falls back.
7. Pinned entries ignore the eviction budget with no ceiling. evict_lru skips pinned entries, so several distinct pinned trees in flight can exceed max_size_bytes by their full size. Documented, but a pinned-bytes cap that falls back to traditional materialization would be safer than relying on disk headroom.
8. The namespace tests can pass without running. Both new tests eprintln! a skip message and return Ok when namespaces are unavailable; that output is captured, so the log shows ok either way. The Cargo job shows all five new tests as ok, but I can't tell from the log whether the two namespace tests executed. Also, no macos-26 job ran on this PR, so the non-Linux build is unverified by CI.
3 participants
Based on top of #2750, will need a rebase eventually.
What and why
Thousands of Chromium compile jobs use the same macOS SDK. NativeLink already caches a prepared copy of the SDK, but each job still creates its own directories and links, then deletes them afterward.
This PR adds
experimental_readonly_input_mounts, which Linux workers can use to share selected cached directories through read-only mounts, avoiding that repeated setup and cleanup.How was this verified?
Ran the same 128 real Chromium compile jobs for a macOS build, submitted from a Mac to four Linux workers running NativeLink v1.6.7 with this patch applied.
Before each cold run, the prepared directories were cleared but the downloaded files were kept. The warm run repeated the jobs without clearing anything. Both configurations used the same cache limits and concurrency, with prefetch enabled. All 512 compilations ran remotely, without action-cache hits, local fallback, or retries.
All four runs produced identical object files. Each configuration was measured once cold and once warm; these are batch timings, not full-build results.
Risk
Low when disabled; moderate when enabled. This option is intended for trusted builds whose jobs do not modify the selected inputs. The main risks are deleting a shared directory while a job still uses it, or keeping too many directories and running out of disk space. Tests cover keeping directories protected until job cleanup finishes.
Jobs with overlapping outputs or unsupported settings use normal input setup, but the overlap checks do not follow symlinks.
This change is