gsadmin/rustfs
1
0
Fork 0
mirror of https://github.com/rustfs/rustfs.git synced 2026-08-17 18:27:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(object-data-cache): close identity-index races in fill/invalidate (#4657)

Browse Source 
fix(object-data-cache): close identity-index races in fill/invalidate (backlog#1117/#1118/#1125/#1128/#1136)

The object body cache's fill-vs-invalidation contract (index.insert before
cache.insert, then re-check index membership and undo the fill on mismatch)
had several races where the identity index and the cache could drift, either
poisoning the race metric, breaking invalidation silently, or letting a
principal evict a hot body on demand.

ODC-12 (backlog#1117) — generation-aware index removal.
The eviction listener removed (identity, key) by key equality with no way to
tell an evicted old entry from a freshly refilled one under the same key.
Verified against moka 0.12.15: an insert over a TTL/TTI-expired but not yet
purged entry runs do_insert_with_hash inline during insert().await, and
do_post_update_steps awaits notify_upsert INLINE with cause Expired
(was_evicted() == true), so the listener deleted the index key the refill just
registered and the post-insert recheck then self-destructed the fresh entry
(SkippedInvalidationRace). A deferred Size notification for an old generation
had the same effect on a cleanly refilled key.
Chose Option A (generation token) over Option B (re-insert after
cache.contains_key): B removes the key first and only reconciles afterward,
which reopens a narrow window where a concurrent invalidate_object misses the
key while it is transiently absent from the index. A never removes a key whose
generation does not match, so the decision is atomic under the shard lock. The
token is the cache entry's Arc pointer captured at fill time (readable from the
value moka hands the listener), so no change to the entry type is needed.

ODC-13 (backlog#1118) — cancellation-safe insert/recheck/undo.
The recheck+undo sat after two awaits inside the S3 GET future; a client
disconnect could cancel the fill at the recheck await after cache.insert made
the entry visible, stranding a stale body with no index entry. The
register/insert/recheck/undo sequence now runs in a spawned task whose
JoinHandle the leader awaits: cancelling the await detaches the task, which
still finishes the recheck and undo. The dropped leader unblocks waiters as
before (SkippedSingleflightClosed), so no waiter is stranded.

ODC-20 (backlog#1125) — do not prune in-flight sibling keys.
prune_missing could remove another in-flight fill's index key (a different key
of the same identity that had registered but not yet published its cache
entry), making that fill's recheck fail and delete its own valid entry. Added a
read-only ObjectDataCacheSingleflight::has_inflight and consult it in the prune
predicate alongside cache.contains_key.

ODC-23 (backlog#1128) — bounded eviction instead of clear-and-reject.
Exceeding identity_keys_max drained the whole key set and rejected the incoming
key, so a principal able to GET identity_keys_max+1 distinct versions could
evict an object's hot body on demand, and identity_keys_max=1 on a versioned
bucket meant a permanent 0% hit rate. The key set now evicts only the oldest
key(s) (the Vec preserves insertion order) and inserts the new key. The insert
result carries evicted_keys so the backend removes exactly those from the cache
and still caches the newest body. The Overflow variant and the now-unused
SkippedIdentityOverflow construction are gone.
NOTE: the audit also asks for identity_keys_max >= 2 validation; that lives in
config.rs (out of scope here) and is deferred to the config batch.

ODC-31 (backlog#1136) — deterministic race coverage.
Added a #[cfg(test)] fill barrier between the index insert and the cache insert
so the fill-vs-invalidation recheck can be driven deterministically, plus a
companion test asserting the identity-budget eviction drops the evicted keys'
cache entries.

New tests:
- index: key_set_bounded_eviction_evicts_oldest_and_inserts_new,
  key_set_removes_only_matching_generation_token,
  key_set_duplicate_refreshes_generation_token
- starshard_index: identity_index_bounded_eviction_evicts_oldest,
  identity_index_stale_eviction_preserves_refreshed_key,
  identity_index_matching_eviction_removes_key
- moka_backend: moka_backend_refill_after_expiry_without_maintenance_inserts,
  moka_backend_concurrent_fills_same_identity_both_insert,
  moka_backend_identity_budget_evicts_oldest_and_caches_newest,
  moka_backend_fill_loses_race_to_invalidation,
  moka_backend_cancelled_fill_still_undoes_lost_race

cargo fmt --all, cargo check/clippy/test -p rustfs-object-data-cache all pass
(44 tests). Verified the refill-after-expiry and concurrent-fill tests fail
without their respective fixes.

Co-authored-by: heihutu <heihutu@gmail.com>
This commit is contained in:
houseme
2026-07-10 16:04:57 +08:00
committed by GitHub
parent 8c76efead2
commit d29e637890
4 changed files with 474 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/object-data-cache/src/starshard_index.rs
+76 -23
View File
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use crate::index::{ObjectDataCacheIndexInsertResult, ObjectDataCacheKeySet};
use crate::index::{ObjectDataCacheIndexInsertResult, ObjectDataCacheKeySet, ObjectDataCacheKeyToken};
use crate::key::{ObjectDataCacheIdentity, ObjectDataCacheKey};
use starshard::{AsyncShardedHashMap, DEFAULT_SHARDS, SnapshotMode};
use std::collections::hash_map::RandomState;
@@ -50,7 +50,12 @@ impl StarshardIdentityIndex {
///
/// Runs the read-modify-write under the shard write lock so concurrent
/// fills for the same identity cannot drop each other's keys.
pub async fn insert(&self, identity: ObjectDataCacheIdentity, key: ObjectDataCacheKey) -> ObjectDataCacheIndexInsertResult {
pub async fn insert(
&self,
identity: ObjectDataCacheIdentity,
key: ObjectDataCacheKey,
token: ObjectDataCacheKeyToken,
) -> ObjectDataCacheIndexInsertResult {
let max_keys = self.max_keys_per_identity;
loop {
let mut outcome = None;
@@ -60,10 +65,12 @@ impl StarshardIdentityIndex {
let _ = self
.by_object
.compute_if_present(&identity, move |mut key_set| {
let result = key_set.insert(key, max_keys);
let keep = !matches!(result, ObjectDataCacheIndexInsertResult::Overflow { .. }) && !key_set.is_empty();
let result = key_set.insert(key, token, max_keys);
// Insert never empties the set (it either dedups or
// bounded-evicts and adds the new key), but keep the
// guard so an already-empty entry is not republished.
*outcome = Some(result);
keep.then_some(key_set)
(!key_set.is_empty()).then_some(key_set)
})
.await;
}
@@ -73,13 +80,10 @@ impl StarshardIdentityIndex {
// Identity not tracked yet: publish a fresh single-key set.
let mut fresh = ObjectDataCacheKeySet::default();
let result = fresh.insert(key.clone(), max_keys);
if !matches!(result, ObjectDataCacheIndexInsertResult::Inserted) {
return result;
}
let result = fresh.insert(key.clone(), token, max_keys);
let final_set = self.by_object.compute_if_absent(identity.clone(), move || fresh).await;
if final_set.contains(&key) {
return ObjectDataCacheIndexInsertResult::Inserted;
return result;
}
// Lost the race to a concurrent insert; retry against the now
// present entry.
@@ -94,15 +98,26 @@ impl StarshardIdentityIndex {
.map_or_else(Vec::new, |set| set.cloned())
}
/// Removes a single key tracked under an identity.
pub async fn remove_key(&self, identity: &ObjectDataCacheIdentity, key: &ObjectDataCacheKey) -> bool {
/// Removes a single evicted key tracked under an identity, but only when the
/// evicted entry's generation token still matches the tracked one.
///
/// This lets the cache eviction listener prune keys of genuinely evicted
/// entries while leaving a key that was refilled under a new generation
/// intact, so a stale (inline `Expired` upsert or deferred `Size`) eviction
/// notification cannot silently break the invalidation contract.
pub async fn remove_evicted_key(
&self,
identity: &ObjectDataCacheIdentity,
key: &ObjectDataCacheKey,
token: ObjectDataCacheKeyToken,
) -> bool {
let mut removed = false;
{
let removed = &mut removed;
let _ = self
.by_object
.compute_if_present(identity, move |mut key_set| {
*removed = key_set.remove_key(key);
*removed = key_set.remove_evicted_key(key, token);
(!key_set.is_empty()).then_some(key_set)
})
.await;
@@ -160,8 +175,8 @@ mod tests {
let key_a = key("v1");
let key_b = key("v2");
let _ = index.insert(identity.clone(), key_a.clone()).await;
let _ = index.insert(identity.clone(), key_b.clone()).await;
let _ = index.insert(identity.clone(), key_a.clone(), 1).await;
let _ = index.insert(identity.clone(), key_b.clone(), 2).await;
let removed = index.remove_identity(&identity).await;
assert_eq!(removed, vec![key_a, key_b]);
@@ -174,8 +189,8 @@ mod tests {
let key_a = key("v1");
let key_b = key("v2");
let _ = index.insert(identity.clone(), key_a.clone()).await;
let _ = index.insert(identity.clone(), key_b.clone()).await;
let _ = index.insert(identity.clone(), key_a.clone(), 1).await;
let _ = index.insert(identity.clone(), key_b.clone(), 2).await;
index.prune_missing(&identity, |candidate| candidate == &key_b).await;
let removed = index.remove_identity(&identity).await;
@@ -191,7 +206,9 @@ mod tests {
for i in 0..32 {
let index = index.clone();
let identity = identity.clone();
handles.push(tokio::spawn(async move { index.insert(identity, key(&format!("v{i}"))).await }));
handles.push(tokio::spawn(
async move { index.insert(identity, key(&format!("v{i}")), i as usize).await },
));
}
for handle in handles {
handle.await.expect("insert task should complete");
@@ -202,20 +219,56 @@ mod tests {
}
#[tokio::test]
async fn identity_index_overflow_clears_identity() {
async fn identity_index_bounded_eviction_evicts_oldest() {
let index = StarshardIdentityIndex::new(1);
let identity = identity();
let key_a = key("v1");
let key_b = key("v2");
let _ = index.insert(identity.clone(), key_a.clone()).await;
let result = index.insert(identity.clone(), key_b).await;
let _ = index.insert(identity.clone(), key_a.clone(), 1).await;
let result = index.insert(identity.clone(), key_b.clone(), 2).await;
let removed = index.remove_identity(&identity).await;
assert!(matches!(
result,
ObjectDataCacheIndexInsertResult::Overflow { cleared_keys } if cleared_keys == vec![key_a]
ObjectDataCacheIndexInsertResult::Inserted { evicted_keys } if evicted_keys == vec![key_a]
));
assert!(removed.is_empty());
// The newest key is retained rather than the identity being cleared.
assert_eq!(removed, vec![key_b]);
}
#[tokio::test]
async fn identity_index_stale_eviction_preserves_refreshed_key() {
// Manifestation of ODC-12(2): a deferred eviction notification for an old
// generation must not break a later invalidate_object for the same key.
let index = StarshardIdentityIndex::new(4);
let identity = identity();
let key_a = key("v1");
// First fill registers token 1; a refill (same key, new body) refreshes
// the token to 2.
let _ = index.insert(identity.clone(), key_a.clone(), 1).await;
let _ = index.insert(identity.clone(), key_a.clone(), 2).await;
// A stale eviction notification for the old generation (token 1) arrives.
let removed = index.remove_evicted_key(&identity, &key_a, 1).await;
assert!(!removed, "stale-generation eviction must not remove the refreshed key");
// A later invalidate_object still finds and removes the key.
let invalidated = index.remove_identity(&identity).await;
assert_eq!(invalidated, vec![key_a]);
}
#[tokio::test]
async fn identity_index_matching_eviction_removes_key() {
let index = StarshardIdentityIndex::new(4);
let identity = identity();
let key_a = key("v1");
let _ = index.insert(identity.clone(), key_a.clone(), 7).await;
let removed = index.remove_evicted_key(&identity, &key_a, 7).await;
assert!(removed, "an eviction carrying the tracked token must remove the key");
assert!(index.remove_identity(&identity).await.is_empty());
}
}