fix: share zig's global cache and retry its LLVM registry race - #10985
Draft
basvandijk wants to merge 1 commit into
Draft
fix: share zig's global cache and retry its LLVM registry race#10985basvandijk wants to merge 1 commit into
basvandijk wants to merge 1 commit into
Conversation
`bazel-test-all-rbe` failed on 13355d0 with error: sub-compilation of compiler_rt failed note: LLVM failed to parse 'x86_64-unknown-linux5.10.0-gnu2.31.0': No available targets are compatible with triple "..." note: failed to link with LLD: UnableToWriteArchive and RBE was disabled again in c7ae711. The triple is a red herring: 5.10.0 is zig 0.15.2's default minimum Linux kernel and it emits that triple on every host. What fails is LLVM's target *lookup*. A cold zig link fans out ~30 sub-compilations (compiler_rt, ubsan_rt, glibc's crt files and .so stubs, libc++) concurrently on zig's thread pool, each registering LLVM targets from its own thread -- via initializeLLVMTarget() in src/codegen/llvm.zig and, for the C pieces, llvm::InitializeAllTargets() in src/zig_clang_cc1_main.cpp -- and LLVM documents TargetRegistry as not thread-safe. A lookup can then miss the target that was just registered. It is rare, roughly one in a few hundred cold links, and both call sites are still unguarded on zig master. Non-RBE CI barely noticed because it does about one cold link per job. RBE did: our zig cache was sharded per executor slot, so a full invocation started out cold in up to 240 shards. Stop paying for that. The shared sub-compilations live in zig's *global* cache, which is content addressed and records no paths, so keep it shared and shard only the local cache, which holds nothing anyone else reuses. To keep many actions from building the shared entries at the same time -- concurrent cold writers being what corrupts the cache (uber/ hermetic_cc_toolchain#224) and what crashed zig's linker child with SIGABRT -- the first link for a given tool/flags/target/toolchain takes an exclusive lock, warms the cache and leaves a marker; everyone else finds the marker and runs concurrently. Compilations sit it out: they build nothing in the global cache, and letting one record a marker would leave the first links to run cold together. The registry race is internal to one zig process, so process-level locking does not fix it. Pinning the warm-up to a single CPU would (zig sizes its thread pool from the affinity mask) but costs too much: a cold C++ link takes ~67s pinned versus ~8s unpinned, since the libc++ build parallelizes ~7x, and arriving actions wait that out. Instead the wrapper captures zig's stderr and re-runs the command once when a failure carries the race's message. Only the last attempt's stderr is forwarded, so a link that survived on retry stays quiet -- rustc's linker_messages lint reports anything the linker printed and would turn the survived flake into a failure under -D warnings. Measured with the wrapper bazel builds, 8 concurrent links on a cold shared cache: 8.3s wall, one process warming (62s CPU) and seven blocked then warm (0.03s CPU each), one marker, 116 global entries. 16 concurrent warm links: 0.12s. Compile-only on a cold cache: no marker, no entries. Genuine compile and link errors are still reported, warnings on successful compiles are still forwarded, and -E still streams to stdout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the hermetic Zig C/C++ toolchain wrapper patch to reduce flakiness and overhead in RBE by (1) sharing Zig’s global cache across executor slots while still sharding the local cache, and (2) adding a warm-up lock/marker plus a targeted retry for a known transient LLVM target-registry race.
Changes:
- Keep
ZIG_GLOBAL_CACHE_DIRshared while sharding onlyZIG_LOCAL_CACHE_DIRby config/slot-derived suffix. - Serialize the first cold link per tool/flags/target/toolchain via an
flock+ marker file to avoid concurrent global-cache writers. - Capture Zig stderr to detect the transient LLVM registry race and retry once, forwarding only the final attempt’s diagnostics; adds unit tests for the new behavior.
Suppressed comments (2)
bazel/hermetic_cc_toolchain_cache_dir.patch:442
- This test uses a
GeneralPurposeAllocatorwithout callingdeinit(), which can hide leaks from theArrayListUnmanagedallocations below. Consider deinitializing the GPA in adeferto avoid silent leaks.
+ var gpa = std.heap.GeneralPurposeAllocator(.{}){};
+ const allocator = gpa.allocator();
bazel/hermetic_cc_toolchain_cache_dir.patch:519
- This test also creates a
GeneralPurposeAllocatorbut never callsdeinit(). Adding adefer _ = gpa.deinit();makes allocator lifetime explicit and prevents silent leaks.
+ var gpa = std.heap.GeneralPurposeAllocator(.{}){};
+ const allocator = gpa.allocator();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+208
to
+212
| + // child dies with SIGABRT). Shard per slot instead: a slot runs one | ||
| + // action at a time, so a shard never has more than one writer, while | ||
| + // successive actions on that slot still reuse the expensive | ||
| + // sub-compilations (compiler_rt et al.) already in it. Invocations | ||
| + // from subdirectories of one action (e.g. foreign_cc configure |
Comment on lines
+415
to
+417
| + var gpa = std.heap.GeneralPurposeAllocator(.{}){}; | ||
| + const allocator = gpa.allocator(); | ||
| + |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #10981 so
bazel-test-all-rbeactually runs on this change.Why RBE was failing
bazel-test-all-rbefailed on 13355d0 (run) withand was disabled again in c7ae711. The triple is a red herring: 5.10.0 is zig 0.15.2's default minimum Linux kernel, and it emits that triple on every host (
zig build-obj --show-builtinconfirms it). What fails is LLVM's target lookup.A cold zig link fans out ~30 sub-compilations — compiler_rt, ubsan_rt, glibc's crt files and .so stubs, libc++ — concurrently on zig's thread pool. Each registers LLVM targets from its own thread:
initializeLLVMTarget()insrc/codegen/llvm.zig:982for the Zig pieces,llvm::InitializeAllTargets()insrc/zig_clang_cc1_main.cpp:225for the C ones. LLVM documentsTargetRegistryas not thread-safe — registration must not happen while another thread reads the registry — so a lookup can miss the target that was just registered. It happens roughly once in a few hundred cold links, and both call sites are still unguarded on zig master, so a zig bump would not fix it.Non-RBE CI barely noticed: it does about one cold link per job. RBE did, because our zig cache was sharded per executor slot, so an invocation started cold in up to 240 shards — about 240 chances per build.
What this changes
The expensive sub-compilations live in zig's global cache (measured: 116 entries there vs 1 in the local cache), which is content addressed and records no paths — a cache warmed by one action serves another at a different execroot. So the global cache stays shared and only the local cache is sharded.
To stop many actions from building those entries simultaneously — concurrent cold writers being what corrupts the cache (uber/hermetic_cc_toolchain#224) and what crashed zig's linker child with SIGABRT — the first link for a given tool/flags/target/toolchain takes an exclusive
flock, warms the cache and leaves a marker; everyone else finds the marker and runs concurrently. Compilations sit it out: they build nothing in the global cache, and letting one record a marker would leave the first links to run cold together.The registry race is internal to a single zig process, so process-level locking cannot fix it. Pinning the warm-up to one CPU would (zig sizes its thread pool from the affinity mask) but costs too much — a cold C++ link is ~67s pinned vs ~8s unpinned, because the libc++ build parallelizes ~7×, and arriving actions wait that out. Instead the wrapper captures zig's stderr and re-runs the command once when a failure carries the race's message. Only the last attempt's stderr is forwarded, so a link that survived on retry stays quiet: rustc's
linker_messageslint reports anything the linker printed and would turn the survived flake into a failure under-D warnings(the same traphermetic_cc_toolchain_lto.patchworks around).No prewarming, no worker-image plumbing, and a zig upgrade invalidates the marker through a
statfingerprint of the zig binary.Verification
Against the wrapper Bazel actually builds:
Regression checks on the stderr change (inherit → capture+forward): genuine compile errors and link errors are still reported with the right exit code, warnings on successful compiles are still forwarded, and
-Estill streams to stdout.7 wrapper unit tests pass under the pinned zig 0.15.2, including a real-process test driving a fake tool that fails with the race message once then succeeds, plus a 128 KB stderr case covering the pipe draining. The patch stack applies cleanly onto a fresh v4.3.0 tarball in
MODULE.bazelorder.Follow-up
Report the unguarded LLVM target registration upstream — the retry is a workaround.
🤖 Generated with Claude Code