Skip to content

Harden filesystem publication and noop-wrapped reads - #2716

Open
michaeljohnston-nunkiharmonia wants to merge 3 commits into
TraceMachina:mainfrom
Nunki-Harmonia:fix/filesystem-store-publication-upstream
Open

Harden filesystem publication and noop-wrapped reads#2716
michaeljohnston-nunkiharmonia wants to merge 3 commits into
TraceMachina:mainfrom
Nunki-Harmonia:fix/filesystem-store-publication-upstream

Conversation

@michaeljohnston-nunkiharmonia

@michaeljohnston-nunkiharmonia michaeljohnston-nunkiharmonia commented Aug 24, 2026

Copy link
Copy Markdown

What and why

A production Buck2 workload exposed two independent cache-store correctness
failures. Concurrent FilesystemStore uploads of the same key could race the
eviction-map update and canonical rename, leaving the map populated after the
canonical file disappeared. Separately, a read-only FastSlowStore with a noop
slow side could reject a valid blob held only by the fast tier of a nested
FastSlowStore, because the outer store gated the read on the inner store's
durable-only has() result.

This change serializes same-key filesystem publication through a bounded set of
keyed lock shards and lets a new upload repair a stale map entry only when its
canonical file is NotFound; other I/O errors remain errors. It also delegates
reads directly to the fast store when the slow store advertises
NoopDownloads, since that configuration has no meaningful fallback.

There is no upstream issue for this change; it was prompted by the production
failures above.

How was this verified?

The added filesystem regressions cover deterministic repair of a missing
canonical entry and 1,024 synchronized same-digest uploads, followed by a read
of the complete canonical blob. The composed-store regression constructs the
production wrapper shape and reads a blob that exists only in the nested fast
tier. Before the respective fixes, the repair/publication case could lose the
canonical path and the wrapper test returned Not found in noop store.

On x86_64-unknown-linux-gnu with nightly 2026-03-24:

  • cargo test -p nativelink-store --test fast_slow_store_test --test filesystem_store_test
    passed 28 and 39 tests.
  • cargo clippy -p nativelink-store --all-targets -- -D warnings passed.
  • NativeLink's nightly rustfmt version and .rustfmt.toml passed for the
    changed Rust files.

The coverage lane's target configuration was also exercised directly with
cargo test --target x86_64-unknown-linux-musl --release --locked -p nativelink-store --test filesystem_store_test; all 39 tests passed. This host
does not have Nix or Bazel installed, so the repository-wide Nix/Bazel paths
remain delegated to CI.

Equivalent fixes backported to NativeLink 1.6.5 were deployed to the affected
cache. A fresh 2,017-action remote Rust composition, the selected build/tests,
and the final read-only selected-closure probe all passed; the latter had failed
before the composed-store change.

Risk

Both changes are on cache hot paths. The filesystem lock set has 256 shards, so
same-key publication is ordered and unrelated keys that collide on a shard can
briefly serialize; the set is bounded and adds no per-key lifetime state. The
noop-wrapper branch relies on NoopDownloads accurately describing a store
that cannot serve a fallback read; a fast-store error is propagated directly.

There are no config-default, wire-format, public-API, or data-migration changes,
and the patch does not delete cache data. If the publication logic is wrong,
writers or subsequent readers notice first through an upload error or a missing
CAS blob. If the wrapper routing is wrong, read-only cache clients notice first
through a failed CAS read. The production cache and CI cache-route probes are
the first operational detectors.


This change is Reviewable

@vercel

vercel Bot commented Aug 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nativelink Ready Ready Preview Aug 25, 2026 4:22pm
nativelink-aidm Ready Ready Preview Aug 25, 2026 4:22pm

Request Review

@github-actions

This comment has been minimized.

@CLAassistant

CLAassistant commented Aug 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

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