Skip to content

Fix CAS upload, eviction and cleanup races, and cut evicting map lock contention - #2786

Open
MarcusSorealheis wants to merge 12 commits into
TraceMachina:mainfrom
MarcusSorealheis:remove-races-clean-queues-improve-performance
Open

Fix CAS upload, eviction and cleanup races, and cut evicting map lock contention#2786
MarcusSorealheis wants to merge 12 commits into
TraceMachina:mainfrom
MarcusSorealheis:remove-races-clean-queues-improve-performance

Conversation

@MarcusSorealheis

@MarcusSorealheis MarcusSorealheis commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What and why

This PR fixes the CAS races behind the recent reports of truncated blobs, "likely evicted" hardlink failures and stale existence answers. It also removes most of the EvictingMap lock traffic that #2609 profiled on many-core workers. Each commit stands alone.

Races

  • Short uploads were stored as the whole blob.
    • Both ByteStream write paths accepted finish_write without checking the byte count. The wrapper's "Did not send enough data" check only runs on a poll that never comes after finish_write, and FilesystemStore::update ignored ExactSize.
    • So a client that sent fewer bytes than the digest size got OK. FindMissingBlobs then reported the blob present, and every later read returned the truncated file.
    • The same thing happened when a slow tier ended a read early with a clean EOF: the fast filesystem tier kept the truncated copy.
    • All three places now reject a length mismatch. FastSlowStore only treats the slow store's has() size as exact when it equals the digest size. A CompressionStore reports its encoded size and a GrpcStore for the AC reports u64::MAX, so neither is treated as exact.
    • Addresses CAS uploads succeed but stored files are incomplete due to async rename race condition #2242. The issue's theory (an unawaited rename) doesn't hold on main; this is the gap that produces the same symptom.
  • The existence cache could report a blob that was gone.
    • A single store-wide pause flag let one update replay another update's queued eviction callbacks before that update had cached its key.
    • Updates now register their key individually, and an eviction that lands mid-update keeps the key out of the cache.
    • The "Failed to delete key from cache on callback" line drops to trace, because a miss there is the normal case. Fixes Many "Failed to delete key from cache on callback" messages #2009.
  • A stale eviction could delete a freshly rebuilt executable variant.
    • The .exec/ variant was named by digest alone and deleted by an eviction callback that runs after the map lock is released.
    • So an eviction of generation 1 could delete the variant just rebuilt for generation 2, and the worker's batched hardlink then failed with "Could not make hardlink … likely evicted".
    • Variants are now named {digest}-{generation} and deleted from that generation's unref, under the entry lock that publishing also takes.
    • The worker resolves any link whose source vanished once more before failing the batch.
  • Workspace cleanup could change the modes of shared CAS inodes.

Lock contention (#2609)

  • Cache-size metrics no longer retake the map lock after every operation. The deltas are taken inside the section that already holds it.
  • Less work inside the lock: the clock is read once per operation outside it, and eviction and expiry logs are written after unlocking. The stdout layer writes synchronously, so these used to be a write(2) under the lock.
  • Cheaper reads: a get hit re-links one LRU instead of two, since the main LRU's order is never read.
  • EvictingMap::get_many and lease_keys take the lock once per 1024 keys. The worker's plain-file and executable-variant lookups and its per-directory leases use them.
  • One lookup per fast-tier hit: FastSlowStore::get_part reads a filesystem or memory fast tier with a single lookup instead of has() followed by get().

How was this verified?

  • Every new race test fails on main and passes here:
    • a short oneshot write and a short streamed write;
    • a truncated slow-tier read;
    • an ExactSize mismatch on the filesystem store;
    • an eviction mid-update, driven by a gated inner store;
    • the stale-eviction variant replay;
    • hardlinked 0o444 files keeping their mode through cleanup.
  • get_many has tests for hits, misses, duplicates, expired and leased-expired entries, input order and multi-chunk batches. There are also tests for metric totals under concurrent enable and for fast-tier reads not being counted as stale.
  • max_decoding_message_size_test had been sending fewer bytes than its resource name promised and only passed because of the bug. It now uses the real size.
  • cargo test passes for nativelink-store, -service, -util, -worker and -scheduler on macOS. The worker's Linux-only namespace tests did not run.
  • Clippy with -D warnings is clean on the changed targets. The pre-existing lint in nativelink-util/tests/connection_manager_test.rs is unchanged.
  • Microbenchmark: release build, 18-core macOS, 16 tasks, not committed.
    • get throughput went from about 3.4 to 3.9 Mops/s.
    • Resolving a 15k-file input tree while 15 tasks hammer get went from about 85 ms to about 0.8 ms.
  • I haven't reproduced CAS uploads succeed but stored files are incomplete due to async rename race condition #2242 against the reporter's client, so it says "Addresses" rather than "Fixes".

Risk

  • Clients that relied on short writes succeeding now get INVALID_ARGUMENT. That's the intended fix, but it is a visible change: any client or store wrapper that sent a length different from the digest's now fails.
  • Executable variants move to generation-suffixed paths. The .exec/ directory is already wiped at writable startup, so old names don't survive a restart. A read of a variant now also refreshes its CAS entry's LRU position.
  • EvictingMap is on every CAS path. Eviction order, TTL, leases and metric totals are unchanged, and existing tests pin them. The two behavior differences:
    • an entry evicted between the old has() and get() is now an ordinary miss rather than "stale";
    • a malformed digest fails its directory level before that level's earlier siblings are leased.

AI assistance

Claude Code (Claude Opus 5.5) wrote the fixes and tests, and ran the verification above.

🤖 Generated with Claude Code

Its work is being evaluated by a swarm of models.

This change is Reviewable

MarcusSorealheis and others added 10 commits September 22, 2026 20:47
@MarcusSorealheis @claude
Reject CAS writes that end short of the expected size
5a22065 
A ByteStream upload whose `finish_write` arrived before the resource
size had been sent was committed under the full digest. The stream
wrapper only reports "Did not send enough data" when it is polled again
after `finish_write`, and neither the streaming nor the oneshot write
path polls it again, so both sent EOF (or called `update_oneshot`) with
the short data. Nothing below caught it: `FilesystemStore::update`
ignored its `UploadSizeInfo`, and `FastSlowStore` sent EOF to the fast
store whenever the slow store's read ended cleanly, even when a Redis
eviction or a short GCS body cut it off partway. The truncated blob then
stayed in the fast tier and was served as the whole digest.

- ByteStream: reject `finish_write` with InvalidArgument unless the
  bytes received equal the resource size, on both identity write paths.
  Compressed uploads keep their own path and checks, and zero-byte
  digests still succeed.
- FilesystemStore: fail an update whose stream length differs from its
  `ExactSize` before the file is published. The temp file is deleted.
- FastSlowStore: fail a fast-store fill whose slow-store read ends at a
  different length than expected instead of sending EOF, so neither the
  fast store nor the reader keeps the truncated data.

The slow store's `has()` size is not always the length `get()` streams:
`CompressionStore` reports its encoded size and `GrpcStore` for the AC
reports `u64::MAX`. The fill therefore treats that size as exact only
when it equals the digest size, as it does for CAS blobs, and otherwise
passes an unbounded `MaxSize`. Those setups keep working with the new
filesystem check instead of handing the fast store a wrong `ExactSize`.

`max_decoding_message_size_test` sent fewer bytes than its resource
name promised and relied on the old behaviour; it now names the real
size.

Addresses TraceMachina#2242

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Keep keys evicted mid-update out of the existence cache
c343bf2 
`ExistenceCacheStore::update` paused the inner store's remove callbacks
with one store-wide flag while any update was in flight, queued them,
and replayed the queue when an update finished. Concurrent updates
shared that flag, so one update could replay a removal meant for
another:

1. Update B writes key B to the inner store, which evicts it straight
   away. The removal of B is queued.
2. Update A of another key finishes first and replays the queue. B is
   not in the existence cache yet, so the removal does nothing.
3. Update B finishes and adds B to the existence cache.

The cache then reports B present while the inner store does not have
it. FindMissingBlobs tells clients not to upload B, `update()` discards
re-uploads of B, and reads of B fail with NotFound until the cache entry
expires.

Track in-flight updates per key instead. A remove callback flags its key
if an update of it is in flight, and removes the key from the cache at
once as it does for any other key. An update only caches its key if the
key was not flagged, and checks again after inserting in case a callback
raced the insert. Callbacks for other keys are no longer delayed, and no
lock is held across an await.

A callback for a key the cache does not hold is the common case, since
the inner store evicts many keys that were never queried through the
cache, so log it at trace instead of info.

Fixes TraceMachina#2009

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Tie executable variants to the generation they were built from
8a4db03 
Executable variants, the 0o555 copies in `{content_path}.exec` that
executable inputs are hardlinked from, were named by digest alone and
deleted by an eviction-map remove callback. That callback fires on both
eviction and replacement, runs after the map lock is released and does
not take `executable_locks`, so it can run after the variant is rebuilt:

1. Inserting another blob evicts digest D and queues D's callback.
2. An action fetches D again and rebuilds `.exec/d2/D`.
3. The queued callback deletes the rebuilt variant.
4. The action's batched hardlink fails with `NotFound` ("Could not make
   hardlink ... file was likely evicted from cache").

A newer generation replacing D ends the same way.

Name each variant after the generation it was built from,
`{digest}-{generation}` like content files since TraceMachina#2762, and delete it in
`FileEntryImpl::unref`, which retires exactly that generation. The
entry's path lock serializes publishing a variant against retiring its
generation, and a retired generation refuses a late publish, so a
variant built while its digest is evicted is no longer left outside
eviction accounting (TraceMachina#2474). With the callback gone, evicting a blob that
never had a variant no longer costs an unlink.

The batched variant lookup now reads each digest's resident entry, so
using an executable through its variant counts as a use of its CAS
entry for LRU eviction, which it previously did not.

No store change can stop the resolved generation itself from being
evicted before the worker links it, the same window a plain CAS blob has
between resolution and the batched hardlink. The worker now resolves
any link whose source went missing again and retries it in a second
batch before reporting the error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@MarcusSorealheis @claude
Stop workspace cleanup from chmodding shared CAS inodes
62f4421 
`fs::remove_dir_all` falls back to a permission-fixing walk when the
stock removal hits `EACCES`, and that walk set every file it visited to
0o600. Action work directories are removed through it, and their files
are hardlinks of FilesystemStore CAS blobs (0o444) and executable
variants (0o555). An action that leaves a read-only directory behind
therefore made cleanup chmod the shared inodes: the store's blob and
every other action's hardlink of it turned 0o600, and an executable
variant lost its execute bits. This is the TraceMachina#2347 corruption class, which
fixed the directory cache's cleanup but not this one.

Only chmod directories. Unlinking a file needs write and search
permission on the directory holding it and never on the file itself, on
Linux and macOS alike, so making each directory that blocks removal
u+rwx is sufficient. The existing test that removes a 0o400 file from a
0o100 directory still passes on macOS, and the same unlink semantics
were checked on Linux in a container as an unprivileged user.

Add a test that removes a work directory holding hardlinks of a 0o444
file and a 0o555 file inside a 0o555 directory, and checks that both
originals keep their modes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@MarcusSorealheis @claude
Report evicting map cache size metrics without retaking the lock
6eff085 
Part of TraceMachina#2609 (lock-handoff storms on many-core hosts).

Before: once `cache.size`/`cache.entries` reporting was enabled, every
EvictingMap operation released the state mutex and then
`flush_cache_size_metrics` locked it a second time only to `mem::take` the
two pending deltas. That doubled mutex acquisitions per operation, even
for pure reads (`get`, `sizes_for_keys`) that never change a delta, and a
second hand-off per call is exactly what piles threads up in
`lock_slow` under contention.

After: each critical section takes its own pending deltas just before it
releases the lock (two integer swaps, skipped entirely while reporting is
off) and records them to OpenTelemetry once unlocked. Reporting never
reacquires the lock: one acquisition per operation instead of two with
cache metrics on (unchanged with them off). A section that does not take
its deltas still only delays them to the next one, as before.

Enabling now flips a flag under the same lock that folds the existing
totals into the first report, so a writer racing the enable is counted
exactly once. Previously a writer that flushed between the `OnceLock`
set and the enable's lock could be reported twice.

The cache size test now also covers expiries reaped by `get` and
`sizes_for_keys`, and eight concurrent writers racing the enable,
checking the exported totals against what is actually resident.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Keep clock reads and eviction logging out of the evicting map lock
a8ff1f8 
Part of TraceMachina#2609 (lock-handoff storms on many-core hosts).

Before: `should_evict` read the clock itself, so every call happened
inside the store-wide critical section: twice per `get` (the expiry
check and the new timestamp), twice per key in `sizes_for_keys`, and
once per candidate in the eviction loop, plus once more for the
`insert_many` timestamp, which was evaluated after the lock was taken.
For the filesystem store that is a `SystemTime` read each time. Worse,
`info!("Item expired, evicting")`, `debug!("Evicting")`,
`debug!("Evicting old item")` and the all-leased `debug!` were emitted
while the lock was held. The stdout fmt layer writes synchronously, so
an enabled event cost a `write(2)` per entry with every other reader
and writer of the store parked behind it.

After: each operation reads the clock once, before locking, and passes
that time to `should_evict`, `evict_items` and the timestamp writes. The
lock no longer covers any clock read. Log events are collected under the
lock and written after it is released, with the same levels, messages
and fields. Evicted and replaced keys are only kept for that when debug
logging is enabled, so the default info-level path allocates nothing
new. Taking the time before the lock can make it earlier by however
long the lock wait was. For TTL (whole seconds) that is the same as a
call made a moment earlier.

A new test checks that replacement, eviction, expiry and all-leased
events are still logged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Promote an evicting map read in one LRU instead of two
e64010b 
Part of TraceMachina#2609 (lock-handoff storms on many-core hosts).

Before: a promoting read (`get`, `sizes_for_keys` without `peek`, and
the existence checks in `insert_if` and `remove_if`) did three hash
lookups and two list re-links under the store-wide lock:
`touch_evictable` looked the key up in `leases`, then re-linked it in
`evictable_lru`, and `lru.get_mut` re-linked it again in `lru`. A miss
still paid all three lookups.

The recency order of `lru` is never read. Eviction walks
`evictable_lru` alone (`peek_evictable`/`pop_evictable`), and `lru`
serves only keyed lookups, `len`, and the unordered `range` index
rebuild. Its re-link was therefore dead work inside the critical
section.

After: `State::get_resident` looks the key up with `lru.peek_mut` and
re-links only `evictable_lru`. A resident key is in `evictable_lru`
exactly when it is unleased (maintained by `lease`, `release_lease` +
`reinsert_evictable`, `put`/`insert_evictable` and `remove`), so the
same promoting lookup also answers whether the entry is leased. A hit
now costs two lookups and one re-link, and a miss costs one lookup.
Eviction order, lease protection and TTL are unchanged, and a peek
still checks `leases` without promoting.

I left out the optional "skip the re-link if promoted within the last
second" shortcut. With frozen mock clocks, entries touched in the same
second would stop being promoted, which changes eviction order under
pressure: `evictable_lru_tracks_unleased_access` and
`remove_if_tracks_unleased_access` already pin that order. The saving
would be one re-link, since the lookup itself is still needed.

A new test covers a promoting read of a leased, TTL-expired entry. It
must be neither reaped nor made an eviction candidate, and it still
expires normally after release.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Batch worker input lookups and leases on the filesystem store
12e5f2c 
Part of TraceMachina#2609 (lock-handoff storms on many-core hosts).

Before: materializing an input tree took the filesystem store's
store-wide eviction lock once per file. `resolve_plain_files` called
`get_file_entry_for_digest` for each non-executable file, one
`EvictingMap::get` apiece. With `active_input_leases` on,
`collect_download_links` also leased every file and child directory one
at a time, one more acquisition per digest per filesystem tier. A
15k-file tree made about 30k acquisitions on a single tier, all racing
the other actions on the host.

After:
- `EvictingMap::get_many` answers a batch of keys in input order, and
  each key gets exactly what `get` would give it (promote and refresh a
  live entry, reap an expired unleased one and report `None`, keep a
  leased one, only touch keys asked about). It takes the lock once per
  1024 keys, so a huge batch still lets others in between chunks. `get`
  and `get_many` share the locked body (`get_locked`) and the post-unlock
  cleanup (`finish_reaped`). That cleanup now skips building empty
  `FuturesUnordered`s on the common no-reap path.
- `EvictingMap::lease_keys` does the same for leases (same reference
  counting as repeated `lease_key`).
- `FilesystemStore::get_file_entries_for_digests` wraps `get_many` with
  the exact per-digest results of `get_file_entry_for_digest`: same
  entries, same NotFound for misses, and zero digests still rejected
  without touching the map. `FilesystemStore::lease_digests` wraps
  `lease_keys`.
- `resolve_plain_files` resolves the whole tree with one batched lookup
  and keeps the per-file fallback (populate, then a single lookup) for
  misses. `collect_download_links` gathers each level's file and child
  directory digests and leases them in one `ActionInputLease::lease_digests`
  call. That call still holds the action's lease set while leasing,
  still runs before any child future is polled, and still leases only
  digests new to the action.

Lock acquisitions per input tree drop from one per file to one per 1024
files for resolution. With leases on, they drop from one per file or
directory per tier to one per directory level per tier. The only
observable difference is on an error path: a malformed digest in a
`Directory` now fails the action before that level's earlier siblings
are leased, which they previously were. The action fails either way and
releases what it holds.

New tests cover `get_many` (hits, misses, duplicates, expired and
leased-expired entries, untouched neighbours, promotion and age refresh,
batches spanning several lock chunks), `lease_keys` across chunks, and
`get_file_entries_for_digests` against the single lookup.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Read filesystem and memory fast tiers without a separate existence check
c9c1536 
Part of TraceMachina#2609 (lock-handoff storms on many-core hosts).

Before: every `FastSlowStore::get_part` asked the fast store `has()`
first and then called its `get_part()`. When the fast tier is a
`FilesystemStore` (the usual worker and CAS layout) or a `MemoryStore`
(the usual in-memory CAS front), that is two acquisitions of the
store-wide eviction lock per read: `sizes_for_keys` for `has()`, then
`EvictingMap::get` inside `get_part`. A zero digest on a filesystem tier
took the lock twice as well, because its `get_part` calls `has()` again.

Just dropping `has()` would not preserve behaviour. Both stores' `get_part`
report a key they do not hold as `NotFound`, which is exactly how the
fast/slow read recognises a stale filesystem map entry whose file is gone.
Every ordinary miss would then be counted as
`fast_store_stale_map_falls_through`, recorded as a "stale" tier read, and
logged at warn. Other fast stores give no such guarantee: noop stores,
wrappers such as the cache metrics, existence cache or shard stores (which
add work or error context of their own), and remote stores. Their
existence check has to stay.

After: `FilesystemStore::get_part_if_present` and
`MemoryStore::get_part_if_present` hold the old `get_part` bodies, except
that a key missing from the eviction map is `Ok(false)` with nothing
written instead of `NotFound`. Each trait `get_part` now wraps its
method and returns the same `NotFound` as before. When the fast store
itself is one of these two types, `FastSlowStore` calls the method
instead of `has()` + `get_part()`. A fast-tier hit or zero digest now
takes the lock once instead of twice, a miss still takes it once before
the slow path, and the hit, miss and stale classification are
unchanged. Offsets, lengths, zero digests and the stale-file self-heal
run the same code as before. Any other fast store, including one wrapped
in another store, still gets `has()` followed by `get_part()`.

One race is now classified better. An entry evicted between the old
`has()` and `get_part()` used to be reported as a stale map entry. With
a single lookup it is an ordinary miss.

A new test reads through filesystem and memory fast tiers: a fast-only
hit (whole and ranged, never touching the slow tier), a slow-only read
that populates the fast tier, a blob in neither tier (NotFound), and a
zero digest. None of those reads may be logged as a stale entry. The
existing stale-entry recovery test still covers the fall-through, and
the memory-backed fast/slow tests now run through the new path.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@MarcusSorealheis @claude
Look up executable variant generations in one batch
0a18f7f 
materialized_variants took the evicting map lock once per executable
input to learn each digest's resident generation. Use get_many, which
takes it once per 1024 keys, like the plain-file lookups beside it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UyNuCCMJCWA4HBz5zURHMN
@vercel

vercel Bot commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Project Deployment Actions Updated
nativelink Ready Ready Preview Sep 28, 2026 8:43pm UTC
nativelink-aidm Ready Ready Preview Sep 28, 2026 8:43pm UTC

Request Review

MarcusSorealheis
MarcusSorealheis commented Sep 28, 2026
View reviewed changes
Comment thread nativelink-store/src/filesystem_store.rs
}
let variant_owned = variant_path.to_os_string();
let rename_fn = self.rename_fn;
spawn_blocking!("filesystem_store_executable_variant_publish", move || {

@MarcusSorealheis MarcusSorealheis Sep 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This publication is still vulnerable to cancellation. Once the blocking rename closure has started, dropping its JoinHandleDropGuard cannot stop it. Cancelling input staging while awaiting it drops encoded_file_path's write guard before Published is recorded. Eviction can then acquire the lock, change Absent to Retired, and skip deletion; the blocking closure subsequently publishes a variant that no resident generation owns. The same leak is possible if rename finishes but the awaiting task is cancelled before line 1856.

I reproduced the first ordering with a blocking-rename barrier and the same abort-on-drop behavior: retirement completed, then rename left the executable on disk. With generation-specific filenames this can accumulate outside the eviction budget until restart. Please make the rename, lifecycle check and Published transition one cancellation-safe operation that owns the synchronization through completion, and add a test cancelling publication after the blocking closure starts. Any failed cleanup should log the digest/generation and variant path so an operator can identify the leaked file.

This branch was successfully deployed

2 active deployments
Preview – nativelink — d56106a9 Deployed Sep 28, 2026 by vercel[bot]
View deployment
Preview – nativelink-aidm — d56106a9 Deployed Sep 28, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Many "Failed to delete key from cache on callback" messages

1 participant

@MarcusSorealheis