From 59020d130ced4939b0f2d2118b02e00a3b745641 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 00:37:42 +0000 Subject: [PATCH] fix(cache): don't FIFO-evict strongly-held coordination locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PortableCache::insert_new evicted the capacity-FIFO head unconditionally, with no strong_count guard — unlike the sibling reclaim_init_lock / run_pending_tasks paths. session_locks is a capacity-only (10k, no TTL) Cache> whose values are handed out as clones held across a decrypt/encrypt. Evicting an address that is actively held lets the next session_lock_for miss and mint a second mutex, so two writers enter the same Signal ratchet concurrently -> counter/nonce reuse or SessionError. The FIFO victim is insertion-order, so a long-lived, actively-used address is the first to go. Add an opt-in eviction guard: insert_new skips capacity-eviction of entries the guard rejects, and overflows (bounded, transient) if none are evictable rather than dropping a live lock. Wire session_locks with `|lock| Arc::strong_count(lock) == 1`. Default (no guard) stays plain FIFO. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01L1geaAZffSxDhP7dpNrbbt --- src/client/lifecycle.rs | 5 ++ src/portable_cache.rs | 137 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 136 insertions(+), 6 deletions(-) diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index 4516585d5..4c041897f 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -159,6 +159,11 @@ impl Client { // while tasks hold references would silently break serialisation. session_locks: Cache::builder() .max_capacity(cache_config.session_locks_capacity.max(1)) + // Never FIFO-evict a lock a task still holds: strong_count > 1 + // means an in-flight decrypt/encrypt owns a clone, and dropping it + // would let a concurrent access mint a second mutex for the same + // session and race the ratchet. + .evict_guard(|lock: &Arc>| Arc::strong_count(lock) == 1) .build(), chat_lanes: Cache::builder() .max_capacity(cache_config.chat_lanes_capacity.max(1)) diff --git a/src/portable_cache.rs b/src/portable_cache.rs index cfed42db2..4f57b05af 100644 --- a/src/portable_cache.rs +++ b/src/portable_cache.rs @@ -26,6 +26,10 @@ struct CacheEntry { seq: u64, } +/// Predicate deciding whether a cache entry may be capacity-evicted; see +/// [`PortableCacheBuilder::evict_guard`]. +type EvictGuard = Arc bool + Send + Sync>; + /// Portable, runtime-agnostic in-process cache. /// /// - Max capacity with FIFO eviction @@ -38,6 +42,12 @@ pub struct PortableCache { max_capacity: Option, ttl: Option, tti: Option, + /// Returns `true` when an entry is safe to capacity-evict. Lets a + /// coordination-lock cache refuse to evict a value that is still referenced + /// elsewhere (e.g. an `Arc` a task holds mid-critical-section), which + /// would otherwise mint a second serializer for the same key. `None` = plain + /// FIFO (any entry evictable). + evict_guard: Option>, } struct CacheInner { @@ -71,12 +81,34 @@ where /// Insert a brand-new entry (the caller has already confirmed the key is /// absent), evicting the oldest entries first if at capacity. Assigns and /// records the FIFO sequence. - fn insert_new(&mut self, key: K, value: V, now: Instant, max_capacity: Option) { + fn insert_new( + &mut self, + key: K, + value: V, + now: Instant, + max_capacity: Option, + evict_guard: Option<&(dyn Fn(&V) -> bool + Send + Sync)>, + ) { if let Some(cap) = max_capacity { while self.map.len() as u64 >= cap { - match self.order.pop_first() { - Some((_, oldest_key)) => { - self.map.remove(&oldest_key); + // Pick the oldest entry the guard allows evicting. Without a guard + // that is simply the FIFO head; with one, skip still-referenced + // entries (a live coordination lock) so we never drop and re-mint a + // second serializer for the same key. If nothing is evictable the + // map is left to exceed capacity — a bounded, transient overshoot + // that self-corrects once the held entries are released. + let victim = match evict_guard { + None => self.order.iter().next().map(|(&seq, k)| (seq, k.clone())), + Some(guard) => self + .order + .iter() + .find(|(_, k)| self.map.get(*k).is_none_or(|e| guard(&e.value))) + .map(|(&seq, k)| (seq, k.clone())), + }; + match victim { + Some((seq, victim_key)) => { + self.order.remove(&seq); + self.map.remove(&victim_key); } None => break, } @@ -104,6 +136,7 @@ pub struct PortableCacheBuilder { max_capacity: Option, ttl: Option, tti: Option, + evict_guard: Option>, _marker: std::marker::PhantomData, } @@ -117,6 +150,7 @@ where max_capacity: None, ttl: None, tti: None, + evict_guard: None, _marker: std::marker::PhantomData, } } @@ -136,6 +170,15 @@ where self } + /// Refuse to capacity-evict entries for which `f` returns `false`. Use it for + /// coordination-lock caches (e.g. `|lock| Arc::strong_count(lock) == 1`) so a + /// live lock/lane held by an in-flight task is never dropped and re-minted as + /// a second serializer for the same key. + pub fn evict_guard(mut self, f: impl Fn(&V) -> bool + Send + Sync + 'static) -> Self { + self.evict_guard = Some(Arc::new(f)); + self + } + pub fn build(self) -> PortableCache { PortableCache { inner: Arc::new(RwLock::new(CacheInner::new())), @@ -143,6 +186,7 @@ where max_capacity: self.max_capacity, ttl: self.ttl, tti: self.tti, + evict_guard: self.evict_guard, } } } @@ -232,7 +276,13 @@ where return; } - guard.insert_new(key, value, now, self.max_capacity); + guard.insert_new( + key, + value, + now, + self.max_capacity, + self.evict_guard.as_deref(), + ); } /// Insert and return a clone of the value in one write lock. @@ -253,7 +303,13 @@ where } let ret = value.clone(); - guard.insert_new(key, value, now, self.max_capacity); + guard.insert_new( + key, + value, + now, + self.max_capacity, + self.evict_guard.as_deref(), + ); ret } @@ -486,6 +542,7 @@ impl Clone for PortableCache { max_capacity: self.max_capacity, ttl: self.ttl, tti: self.tti, + evict_guard: self.evict_guard.clone(), } } } @@ -539,6 +596,74 @@ mod tests { assert_eq!(cache.get("d").await, Some(4)); } + // F6: a coordination-lock cache must not FIFO-evict a lock a task still holds + // (strong_count > 1), or a concurrent access mints a second mutex and races + // the ratchet. + #[tokio::test] + async fn test_evict_guard_skips_strongly_held_entries() { + let cache: PortableCache>> = PortableCache::builder() + .max_capacity(2) + .evict_guard(|lock: &Arc>| Arc::strong_count(lock) == 1) + .build(); + + // "a" is held by a task (external clone) -> strong_count 2. + let held = Arc::new(AsyncMutex::new(())); + cache.insert("a".into(), held.clone()).await; + // "b" is unheld (cache is sole owner) -> strong_count 1. + cache + .insert("b".into(), Arc::new(AsyncMutex::new(()))) + .await; + + // At capacity: inserting "c" must skip the held head "a" and evict "b". + cache + .insert("c".into(), Arc::new(AsyncMutex::new(()))) + .await; + + assert!( + cache.get("a").await.is_some(), + "held lock must survive eviction" + ); + assert!( + cache.get("b").await.is_none(), + "unheld older lock is evicted instead" + ); + assert!(cache.get("c").await.is_some()); + drop(held); + } + + #[tokio::test] + async fn test_evict_guard_overflows_when_all_held_then_recovers() { + let cache: PortableCache>> = PortableCache::builder() + .max_capacity(2) + .evict_guard(|lock: &Arc>| Arc::strong_count(lock) == 1) + .build(); + + let a = Arc::new(AsyncMutex::new(())); + let b = Arc::new(AsyncMutex::new(())); + let c = Arc::new(AsyncMutex::new(())); + cache.insert("a".into(), a.clone()).await; + cache.insert("b".into(), b.clone()).await; + // Both held: nothing evictable, so the cache overflows rather than drop a + // live lock — a bounded, transient overshoot. + cache.insert("c".into(), c.clone()).await; + assert_eq!( + cache.entry_count(), + 3, + "held locks force a bounded overflow" + ); + + // Once released, a later insert evicts back down to capacity. + drop((a, b, c)); + cache + .insert("d".into(), Arc::new(AsyncMutex::new(()))) + .await; + assert_eq!( + cache.entry_count(), + 2, + "overflow self-corrects after release" + ); + } + #[tokio::test] async fn test_remove_then_eviction_preserves_fifo_order() { // A removed key must leave the FIFO `order` consistent: eviction must skip