Repository navigation
Conversation
b7r6
force-pushed
the
fix/existence-cache-pause
branch
3 times, most recently
from
October 8, 2026 16:32
2c2bbb0 to
3b9d756
Compare
b7r6
force-pushed
the
fix/existence-cache-pause
branch
4 times, most recently
from
October 15, 2026 12:03
1e44244 to
afc24da
Compare
…ict resurrection
The ExistenceCacheStore pauses inner-store remove callbacks while it writes
a blob, so that an eviction fired inline during the write is applied only
after the key is inserted into the existence cache. Two defects made this
mechanism racy — both rooted in the pause being a single, non-refcounted
Option<Vec<StoreKey>> slot shared by all concurrent operations:
1. Non-refcounted pause. Concurrent update() calls all shared one pause
slot. The first update to finish unconditionally take()s it, ending the
pause window for every other in-flight update. A remove callback
(eviction under cache pressure) delivered during a still-running
update's inner write then ran immediately as a no-op — the key was not
yet in the cache — after which that update inserted a stale "exists"
entry for a blob the inner store had already dropped. The cache then
permanently claimed existence, and later re-uploads were drained and
silently discarded.
2. Insert-after-evict ordering in get_part. get_part inserted into the
existence cache without pausing removals at all. An inner store that
evicts a key while a read of it is still in flight (e.g. a filesystem
store streaming from an open file handle) fired the removal before the
insert, so get_part resurrected a zombie exists-entry the inner store
no longer held.
Fix: replace the Option slot with a refcounted PauseState { pause_count,
pending }. Each operation calls pause_remove_callbacks() on entry and
resume_remove_callbacks() after its cache insert; queued removals drain
only when the LAST concurrent holder releases (count -> 0), always after
every holder has inserted. Because the insert happens before the release,
a key evicted during its own operation is removed after it is inserted and
cannot be left resurrected. get_part now uses the same pause guard around
its read + insert.
Includes three reproductions (fail unfixed, pass fixed):
- concurrent_update_pause_refcount_race_test: first finisher steals the
shared pause, losing an eviction during a concurrent update.
- concurrent_update_unpause_race_stale_existence_test: stolen pause window
leaves a stale exists entry for an oversized/evicted blob.
- get_part_races_inner_eviction_and_resurrects_zombie_entry: get_part
resurrects an entry evicted mid-read.
Provenance: adversarial review fleet.
Co-Authored-By: Claude Fable 5 <[email protected]>
…an't wedge invalidation (N6) Co-Authored-By: Claude Opus 4.8 <[email protected]>
…y presence) The regression test short-circuited: BlockingUpdateStore reported a single global `present` flag, so update(B)'s inner_has_with_results saw B as present and returned early, never reaching the parking inner update. Track presence per-key (HashSet) so A is present (populates its existence entry) while B is absent (update proceeds to the parking await). Elide the needless explicit lifetime on PauseGuard, replace let-underscore drop with drop(), and use usize::try_from for the reader length. Co-Authored-By: Claude Opus 4.8 <[email protected]>
… (C20/N6 drop residual) The RAII PauseGuard's Drop decremented the pause count (fixing N6's permanently-wedged pause) but discarded the pending-removal vector that `release()` returns when the last holder goes away. On the evict-before-cancel ordering — evict cached A while upload B holds the sole pause, then cancel B — A's queued invalidation was dropped on the floor and the existence cache kept reporting A forever even though the inner store had evicted it. Drop now drains those queued removals. Since Drop cannot await and the removal is async, it spawns a detached background task (via a weak self-reference) to run drain_pending_removals. New regression `cancelled_update_drains_removals_queued_before_cancel` uses the evict-*before*-cancel ordering and fails on the prior Drop (removals discarded); the existing N6 test `cancelled_update_does_not_leave_callbacks_paused` (evict-after-cancel) is kept. Also closes the C13 has-path residual: inner_has_with_results now pauses remove callbacks around the inner `has` + the positive-result insert, so an eviction delivered mid-`has` lands after the insert instead of being a no-op before it. Co-Authored-By: Claude Opus 4.8 <[email protected]>
b7r6
force-pushed
the
fix/existence-cache-pause
branch
from
October 21, 2026 17:35
afc24da to
a74c1c2
Compare
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
Lane A stability fix from the findings catalog (see
nativelink-findings-2026-10-05.csvon the
meta/findings-catalog-2026-10-05branch), rebased onto OSS main@bf4b3aa855. This fork PR exists to validate the change on CI that is
representative of upstream PR lanes (public read-only cache, same endpoints)
before any upstream submission.
How was this verified?
The branch carries its own regression tests (fails-without where the fix
class allows it — see the branch's test commits). This PR's purpose is the
full representative CI run; green here rehearses the exact checks an
upstream PR would face.
Risk
Fork-internal validation PR; nothing lands anywhere by merging this. The
underlying fix's risk assessment happens at upstream-submission time.
AI assistance
Agent-assisted pipeline (Claude) for rebase and PR mechanics under my
direction; the fix itself carries verification in its tests.