From f9cc857ae6023a4806503da5bf016216a2194b29 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Wed, 30 Sep 2026 00:53:31 -0400 Subject: [PATCH] fix(evicting_map): stop has() batch from cascading-evicting every queried blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sizes_for_keys` snapshotted `lru.len()` once for the whole batch and reused that stale length in `should_evict` for every key in the loop. When the map sits one entry over `max_count` — e.g. an action-input lease (or a TTL-pending entry) pins it at `max_count + 1` — the first unleased queried key is popped, bringing the real length back to `max_count`. But iterations 2..N still saw the stale `lru_len == max_count + 1`, so `is_over_count` stayed true and every remaining queried, unleased entry was popped too: remove-callbacks queued, `FileEntryImpl` unref deleting the backing files. The batch then reports `None` for all N digests, the client treats them as absent and re-uploads, and a plain existence check (`has()` → FilesystemStore → `sizes_for_keys`) has destroyed every resident blob it was only asked about. Fix: keep a mutable working count and decrement it as entries are reaped, so the over-count predicate is re-evaluated against the current length each iteration. Eviction now stops the moment the map is back at/under the limit — a size/existence query evicts at most the genuine overage and never cascades. Adds a white-box regression test that pins the map one over `max_count` with a leased entry (bypassing the insert-time eviction pass the way a held lease does) and asserts a batch `sizes_for_keys` over the resident keys leaves all but the single-entry overage present. It fails on the stale-len code (all N reported missing) and passes with the fix. Provenance: adversarial review fleet, highest-severity finding. Co-Authored-By: Claude Fable 5 --- nativelink-util/src/evicting_map.rs | 105 +++++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 1 deletion(-) diff --git a/nativelink-util/src/evicting_map.rs b/nativelink-util/src/evicting_map.rs index a94bfd534a8..8076f9241a0 100644 --- a/nativelink-util/src/evicting_map.rs +++ b/nativelink-util/src/evicting_map.rs @@ -634,7 +634,13 @@ where let (removal_futures, data_to_unref) = { let mut state = self.state.lock(); - let lru_len = state.lru.len(); + // Working copy of the resident count, decremented as entries are + // reaped below. Evaluating `should_evict` against a length + // snapshotted once for the whole batch would keep reporting + // over-count for every remaining key after the first reap brings + // the map back to its limit, cascading into evicting every + // queried entry — reachable from a plain `has()` batch. + let mut lru_len = state.lru.len(); let mut data_to_unref = Vec::new(); let mut removal_futures = Vec::new(); for (key, result) in keys.into_iter().zip(results.iter_mut()) { @@ -656,6 +662,7 @@ where if !is_leased && self.should_evict(lru_len, entry, 0, u64::MAX) { *result = None; if let Some((key, eviction_item)) = state.lru.pop_entry(key.borrow()) { + lru_len -= 1; info!(?key, "Item expired, evicting"); let (data, futures) = state.remove(key.borrow(), &eviction_item, false); @@ -1042,3 +1049,99 @@ where self.state.lock().add_remove_callback(callback); } } + +#[cfg(test)] +mod tests { + use nativelink_config::stores::EvictionPolicy; + use nativelink_macro::nativelink_test; + use pretty_assertions::assert_eq; + + use super::{EvictingMap, EvictionItem, LenEntry}; + use crate::instant_wrapper::MockInstantWrapped; + + #[derive(Clone, Debug, PartialEq, Eq)] + struct TestEntry(u64); + + impl LenEntry for TestEntry { + fn len(&self) -> u64 { + self.0 + } + fn is_empty(&self) -> bool { + self.0 == 0 + } + } + + /// A batch existence check (`has()` → `sizes_for_keys`) over resident + /// keys, run while the map is pinned one entry over `max_count` by a + /// leased entry, must evict at most the genuine overage — never every + /// queried key. + /// + /// The bug: `sizes_for_keys` snapshotted `lru.len()` once for the whole + /// batch and re-used that stale value in `should_evict` for every key. + /// After the first queried entry was reaped (bringing the map back to + /// `max_count`), every remaining queried key still saw the stale + /// over-count and was popped too. A plain `has()` batch would then + /// destroy all N resident blobs and report them all missing. + /// + /// White-box: the pinning entry is planted with a raw leased `State::put` + /// (no eviction pass), modeling the window in which an action-input lease + /// holds the map over its count limit while unleased blobs stay resident. + #[nativelink_test("crate")] + async fn size_check_batch_evicts_at_most_the_overage() { + let map = EvictingMap::::new( + &EvictionPolicy { + max_bytes: 0, + evict_bytes: 0, + max_seconds: 0, + max_count: 3, + }, + MockInstantWrapped::default(), + ); + + map.insert("k1".to_string(), TestEntry(10)).await; + map.insert("k2".to_string(), TestEntry(20)).await; + map.insert("k3".to_string(), TestEntry(30)).await; + + // Pin the map one over `max_count` with a leased entry, bypassing the + // insert-time eviction pass exactly as an already-held lease does. + { + let mut state = map.state.lock(); + state.lease("pinned".to_string()); + let replaced = state.put( + &"pinned".to_string(), + EvictionItem { + seconds_since_anchor: 0, + data: TestEntry(1), + }, + ); + assert!(replaced.is_none()); + state.sum_store_size += 1; + } + assert_eq!(map.len_for_test(), 4); + + let keys = ["k1".to_string(), "k2".to_string(), "k3".to_string()]; + let mut results = [None, None, None]; + map.sizes_for_keys(keys.iter(), &mut results[..], false) + .await; + + // The overage is a single entry, so at most the first queried key may + // be reaped. The rest of the batch must stay resident and report size. + assert_eq!( + results[1], + Some(20), + "k2 must survive a batch size check: {results:?}" + ); + assert_eq!( + results[2], + Some(30), + "k3 must survive a batch size check: {results:?}" + ); + assert_eq!( + map.len_for_test(), + 3, + "only the genuine overage may be evicted by a size query" + ); + assert_eq!(map.size_for_key(&"k2".to_string()).await, Some(20)); + assert_eq!(map.size_for_key(&"k3".to_string()).await, Some(30)); + } +}