From 170cb18a2d3124d5163e0e00d8648399e2c525a9 Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Thu, 17 Sep 2026 19:59:56 +0530 Subject: [PATCH 1/2] fix(cache): re-check ownership after the disk write, not only before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership check ran before `store.put().await`, so a takeover landing during the write left a rewrite still reaching the record swap — where it replaced the current owner's record and deleted the object that record named, leaving that owner's index entry pointing at nothing. The comment there claimed a stale writer never reaches it, which was the wrong assumption rather than a wrong line. Ask the index again after the write. The index, not the record: the record may still name a previous owner this writer is legitimately superseding, while the index names whoever the bytes can be read by. A rewrite that is no longer that owner removes the object it just wrote and hands back its reservation instead. This narrows the window rather than closing it — the index read and the record swap are still not atomic, as nothing else in the cache is either (see the note above `supersede_disk_copy`). Closing it needs the two under one lock, which is a larger change than this fix. --- src/core/src/cache/core.rs | 54 ++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/src/core/src/cache/core.rs b/src/core/src/cache/core.rs index b6bc1c08..c7f325c1 100644 --- a/src/core/src/cache/core.rs +++ b/src/core/src/cache/core.rs @@ -1138,6 +1138,7 @@ impl LiquidCache { self.budget.release_disk(len); return Ok(()); } + let is_rewrite = matches!(identity, WriteIdentity::Rewrite(_)); let identity = identity.value(); self.store .put(entry_id_to_key(&entry_id, identity), bytes.to_vec()) @@ -1153,20 +1154,52 @@ impl LiquidCache { }, CacheEntry::DiskLiquid { .. } | CacheEntry::MemoryLiquid(_) => DiskKind::Liquid, }; - let previous = self.disk_copies.lock().unwrap().insert( - entry_id, - DiskCopy { - identity, - kind, - bytes: len, - }, - ); + // The ownership check above happened before the await, so a takeover + // can have landed while the bytes were being written. Re-check while + // swapping the record: a rewrite that is no longer the owner must + // leave the current owner's record and object alone, and take its own + // orphan with it. A caller taking the key may still supersede, which + // is what it is for. + // The check above ran before the await, so a takeover can have landed + // while the bytes were being written. Ask the index again, not the + // record: the record may still name a previous owner this writer is + // legitimately superseding, whereas the index names whoever the bytes + // can actually be read by. A rewrite that is no longer that owner is + // stale and must leave the current one's record and object alone. + let still_owner = !is_rewrite + || self + .index + .get_with_identity(&entry_id) + .is_none_or(|(current, _)| current == identity); + let previous = if still_owner { + Some(self.disk_copies.lock().unwrap().insert( + entry_id, + DiskCopy { + identity, + kind, + bytes: len, + }, + )) + } else { + None + }; + let Some(previous) = previous else { + // Stale: someone else owns the record now. Remove the object this + // write just made — nothing names it — and hand back its + // reservation. + self.store + .remove(&entry_id_to_key(&entry_id, identity)) + .await + .expect("disk remove failed"); + self.budget.release_disk(len); + return Ok(()); + }; if let Some(previous) = previous { // Same owner: the put replaced that object, so its reservation // goes with it and there is nothing left to delete. // - // Different owner: this is the current owner superseding one that - // has let the key go (a stale writer never reaches here). The key + // Different owner: this is a caller taking the key from one that + // has let it go — a stale rewrite was turned away above. The key // carries the identity, so the put landed somewhere else and the // previous object is still there — with no record naming it and // nothing that would ever reach it. Releasing its reservation @@ -1725,7 +1758,6 @@ mod tests { ); assert_eq!(store.budget.disk_usage_bytes(), 0); } - /// A dropped write must hand back the disk it reserved. Nothing records /// those bytes — no `DiskCopy` names them — so no later path would ever /// release them, and repeated takeovers during squeezes would walk the From 70c4c9453a3f544a3e7e342fcad44cb8ee9a9da7 Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Thu, 17 Sep 2026 22:19:48 +0530 Subject: [PATCH 2/2] style(cache): tidy the stale-rewrite guard Drop the duplicated comment, early-return instead of nesting the record swap in an Option, and note that the removal on this path holds no claim on the key. --- src/core/src/cache/core.rs | 47 ++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/core/src/cache/core.rs b/src/core/src/cache/core.rs index c7f325c1..6e0ae3f6 100644 --- a/src/core/src/cache/core.rs +++ b/src/core/src/cache/core.rs @@ -1154,46 +1154,42 @@ impl LiquidCache { }, CacheEntry::DiskLiquid { .. } | CacheEntry::MemoryLiquid(_) => DiskKind::Liquid, }; - // The ownership check above happened before the await, so a takeover - // can have landed while the bytes were being written. Re-check while - // swapping the record: a rewrite that is no longer the owner must - // leave the current owner's record and object alone, and take its own - // orphan with it. A caller taking the key may still supersede, which - // is what it is for. // The check above ran before the await, so a takeover can have landed // while the bytes were being written. Ask the index again, not the // record: the record may still name a previous owner this writer is // legitimately superseding, whereas the index names whoever the bytes // can actually be read by. A rewrite that is no longer that owner is // stale and must leave the current one's record and object alone. - let still_owner = !is_rewrite - || self + // + // The index read and the swap below are two steps, so this narrows the + // window rather than closing it — as with every other check-then-act + // pair in this file. + if is_rewrite + && self .index .get_with_identity(&entry_id) - .is_none_or(|(current, _)| current == identity); - let previous = if still_owner { - Some(self.disk_copies.lock().unwrap().insert( - entry_id, - DiskCopy { - identity, - kind, - bytes: len, - }, - )) - } else { - None - }; - let Some(previous) = previous else { - // Stale: someone else owns the record now. Remove the object this + .is_some_and(|(current, _)| current != identity) + { + // Stale: another identity owns the key now. Remove the object this // write just made — nothing names it — and hand back its - // reservation. + // reservation. The remove holds no claim on the key, so a second + // caller may race it; `t4::Store::remove` reports a missing key as + // `Ok(false)` rather than an error, so that is harmless. self.store .remove(&entry_id_to_key(&entry_id, identity)) .await .expect("disk remove failed"); self.budget.release_disk(len); return Ok(()); - }; + } + let previous = self.disk_copies.lock().unwrap().insert( + entry_id, + DiskCopy { + identity, + kind, + bytes: len, + }, + ); if let Some(previous) = previous { // Same owner: the put replaced that object, so its reservation // goes with it and there is nothing left to delete. @@ -1758,6 +1754,7 @@ mod tests { ); assert_eq!(store.budget.disk_usage_bytes(), 0); } + /// A dropped write must hand back the disk it reserved. Nothing records /// those bytes — no `DiskCopy` names them — so no later path would ever /// release them, and repeated takeovers during squeezes would walk the