Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…aphore Evicted entries' backing files are unlinked by a fire-and-forget background_spawn! in EncodedFilePath::drop, with no bound. Under an eviction storm (cache wipe, overwrite flood) that grows without limit: queued task frames consume memory and disk/inode reclamation lags behind what the async runtime can schedule. Open FDs are already bounded by a semaphore; deletes were not. Add max_concurrent_deletes. drop is sync so it cannot await a permit; it try_acquire_owned's and, when saturated, deletes synchronously inline -- which is the backpressure (the evicting thread pays the unlink cost directly) and never leaks a file. An inline_deletes metric exposes how often backpressure engages. Bounded by default (1024), unlike max_concurrent_writes: leaving an unbounded delete backlog off by default would move a latent memory/inode blowup from a high-context code review to a late-night config change nobody remembers to make. The default is generous enough that normal eviction never reaches it (so the inline path never fires in ordinary operation); it only engages under a genuine storm. 0 remains the explicit escape hatch for unlimited. The serde field default governs JSON5 configs (production); derive(Default) still yields 0, so existing Rust-constructed tests are unchanged.
b7r6
force-pushed
the
upstream/delete-backpressure
branch
from
October 14, 2026 17:40
29b62fd to
d4352d5
Compare
This branch was successfully deployed
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
1 participant
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.
What and why
Open file descriptors in the filesystem store are bounded by a semaphore, but
deletes are not. When an entry leaves the store, its backing file is
unlinked by a fire-and-forget
background_spawn!inEncodedFilePath::drop.Under an eviction storm — a cache wipe, or a flood of overwrites — those spawns
have no ceiling: queued task frames grow memory, and disk/inode reclamation
lags behind what the async runtime can schedule. It drains when load subsides,
so it's not a permanent leak, but it's an unbounded, load-sensitive balloon.
This adds
max_concurrent_deletes.dropis synchronous, so it can't.awaita permit; it
try_acquire_owned()s instead, and when the permits are exhaustedit deletes synchronously inline — which is the backpressure (the evicting
thread pays the unlink cost directly instead of queueing another task) and never
leaks a file. An
inline_deletesmetric exposes how often backpressure engages.Bounded by default (1024), not unlimited. Leaving the protection off by
default would move an unbounded-memory/inode risk from a high-context review to
a late-night config change nobody remembers to make. The default is generous
enough that normal eviction never reaches it (the inline path stays dormant in
ordinary operation); it engages only under a genuine storm.
0is the explicitescape hatch for unlimited. (The serde field default governs config files;
derive(Default)still yields0, so Rust-constructed tests are unchanged.)How was this verified?
cargo test -p nativelink-store --test filesystem_store_test— all tests pass,including a new
bounded_delete_semaphore_still_removes_evicted_filesthatdrives an eviction through the bounded path and asserts the backing file is
still removed.
Risk
Low-moderate. The default changes from unlimited to 1024, so a deployment whose
normal steady-state exceeds 1024 concurrent in-flight deletes would start
seeing inline (synchronous) deletes — the intended backpressure, not
incorrectness;
0restores the old behavior. Touches one store; no protocolsurface beyond the new optional field.
AI assistance
Claude (Fable 5) wrote the change and test from my direction after tracing the
unbounded-spawn path together; I reviewed every line.