feat: pluggable cache store adapter for custom backends - #381
Conversation
Add a `CacheStore` trait that allows replacing the default in-process moka caches with external backends like Redis, Memcached, or SQLite for shared/distributed cache state. - Define `CacheStore` trait in wacore (get/set/delete/clear/entry_count) - Add `TypedCache<K, V>` wrapper dispatching to moka or custom store with zero overhead on the moka path (no serde, no extra allocs) - Add `CacheStores` struct with per-cache `Option<Arc<dyn CacheStore>>` for granular control (e.g., only group_cache on Redis) - Add `CacheStores::all(store)` convenience for overriding all at once - Add Serialize/Deserialize to GroupInfo, LidPnEntry, LearningSource, AddressingMode for custom store serialization - Wire up group_cache, device_cache, device_registry_cache, lid_pn_cache - Coordination caches (session_locks, message_queues, signal_cache) remain in-process — they hold live Rust objects Fully backward compatible: CacheConfig::default() uses moka everywhere.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a pluggable cache abstraction layer to replace in-process moka caches with a configurable backend system. It adds a CacheStore trait defining standard cache operations (get, set, delete, clear), a TypedCache wrapper supporting both moka and custom backends, a CacheStores configuration struct for per-cache customization, and updates core caches (group, device, lid_pn, device_registry) to use the new abstraction. Serialization support is added to related types for compatibility. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant TC as TypedCache
participant MK as Moka Cache
participant CS as CacheStore Backend
App->>TC: get(key)
alt Moka Path
TC->>MK: get(key)
MK-->>TC: Option<V>
TC-->>App: Option<V>
else Custom Store Path
TC->>CS: get(namespace, key)
CS-->>TC: Option<Vec<u8>>
TC->>TC: deserialize JSON to V
TC-->>App: Option<V>
end
App->>TC: insert(key, value)
alt Moka Path
TC->>MK: insert(key, value)
else Custom Store Path
TC->>TC: serialize V to JSON
TC->>CS: set(namespace, key, bytes, ttl)
CS-->>TC: Result<()>
end
App->>TC: invalidate(key)
alt Moka Path
TC->>MK: invalidate(key)
else Custom Store Path
TC->>CS: delete(namespace, key)
CS-->>TC: Result<()>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bencher Report
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 48s —— View job PR Review: Pluggable Cache Store Adapter
Overall this is a clean, well-structured PR. The Issues1. TOCTOU race in For now this is probably acceptable since the PR description notes it's a cache (best-effort), but worth documenting with a comment. Fix this → 2.
The current approach is functional but confusing — the trait method exists but is dead code. 3. Suggestions4. Repeated match pattern for store initialization impl CacheEntryConfig {
pub(crate) fn build_typed_ttl<K, V>(
&self,
store: Option<Arc<dyn CacheStore>>,
namespace: &'static str,
) -> TypedCache<K, V>
where
K: Hash + Eq + Display + Send + Sync + 'static,
V: Clone + Serialize + DeserializeOwned + Send + Sync + 'static,
{
match store {
Some(s) => TypedCache::from_store(s, namespace, self.timeout),
None => TypedCache::from_moka(self.build_with_ttl()),
}
}
}This would reduce the 3 match blocks to one-liners. Fix this → 5. Unnecessary pub async fn get_current_lid(&self, phone: &str) -> Option<String> {
self.pn_to_entry.get(phone).await.map(|e| e.lid.clone())
}This avoids an allocation per lookup. Fix this → Looks Good
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cache_store.rs (1)
195-199: Custom backend entry counts are currently discarded.
TypedCache::entry_count()always returns0forInner::Custom, even whenCacheStore::entry_countis implemented. This drops useful diagnostics.Suggested API addition (non-breaking)
+ /// Entry count for both backends (async for custom stores). + pub async fn entry_count_async(&self) -> u64 { + match &self.inner { + Inner::Moka(cache) => cache.entry_count(), + Inner::Custom { store, namespace, .. } => { + match store.entry_count(namespace).await { + Ok(count) => count, + Err(e) => { + log::warn!("TypedCache[{namespace}]: entry_count() error: {e}"); + 0 + } + } + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache_store.rs` around lines 195 - 199, TypedCache::entry_count currently discards counts for Inner::Custom by returning 0; change the Inner::Custom match arm to delegate to the custom backend's entry_count implementation instead of hardcoding 0. Locate the enum variant used in self.inner (Inner::Custom { ... }) and extract the stored custom cache/backend instance, then call its CacheStore::entry_count (or equivalent method on the custom backend) and return that value; keep a 0 fallback only if the custom backend truly lacks an entry_count method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cache_store.rs`:
- Around line 165-177: invalidate_all() currently unconditionally calls
tokio::spawn from a synchronous, public method which can panic if no Tokio
runtime exists; change the Inner::Custom branch to attempt to get a current
runtime with tokio::runtime::Handle::try_current() and, if successful, use
tokio::spawn(async move { if let Err(e) = store.clear(ns).await {
log::warn!(...) } }), but if try_current() fails, fall back to spawning a
standard thread (std::thread::spawn) and create a small single-threaded Tokio
runtime inside it
(tokio::runtime::Builder::new_current_thread().enable_all().build()) to
block_on(store.clear(ns)) and log errors similarly; update references to
invalidate_all, Inner::Custom, store.clear, and tokio::spawn in the diff
accordingly.
---
Nitpick comments:
In `@src/cache_store.rs`:
- Around line 195-199: TypedCache::entry_count currently discards counts for
Inner::Custom by returning 0; change the Inner::Custom match arm to delegate to
the custom backend's entry_count implementation instead of hardcoding 0. Locate
the enum variant used in self.inner (Inner::Custom { ... }) and extract the
stored custom cache/backend instance, then call its CacheStore::entry_count (or
equivalent method on the custom backend) and return that value; keep a 0
fallback only if the custom backend truly lacks an entry_count method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26c56938-65c8-4214-a807-dd3c20edc4f7
📒 Files selected for processing (10)
src/cache_config.rssrc/cache_store.rssrc/client.rssrc/lib.rssrc/lid_pn_cache.rswacore/src/client/context.rswacore/src/store/cache.rswacore/src/store/mod.rswacore/src/types/lid_pn.rswacore/src/types/message.rs
- invalidate_all: use Handle::try_current() instead of tokio::spawn
to avoid panic outside Tokio runtime (CodeRabbit)
- Add TypedCache::clear() async method and entry_count_async() that
delegates to CacheStore::entry_count (CodeRabbit + Claude)
- LidPnCache::clear now awaits the actual clear instead of
fire-and-forget invalidate_all (Claude)
- Remove unnecessary .to_owned() allocations in LidPnCache lookups;
pass &str directly via Borrow<Q> bounds (Claude)
- Add build_typed_ttl helper on CacheEntryConfig to deduplicate the
match store { Some => from_store, None => from_moka } pattern
repeated in client.rs (Claude)
- Document TOCTOU race in LidPnCache::add for external backends (Claude)
d8404d5 to
ac1b36b
Compare
1 participant
Summary
CacheStoretrait inwacorethat allows replacing the default in-process moka caches with external backends (Redis, Memcached, SQLite, etc.)TypedCache<K, V>wrapper dispatches to either moka (zero overhead) or a customCacheStorewith serde_json serializationCacheStoresstruct — users can override only specific caches (e.g., onlygroup_cacheanddevice_cacheon Redis) while keeping others in-processsession_locks,message_queues,signal_cache, etc.) always stay in-process since they hold live Rust objectsUsage
Pluggable caches
group_cache"group"device_cache"device"device_registry_cache"device_registry"lid_pn_cache"lid_pn_by_lid"/"lid_pn_by_pn"session_locksMutex)message_queuessignal_cacheNew types
wacore::store::CacheStore— async trait for custom backendsCacheStores— per-cache optional store overridesTypedCache<K, V>— moka-or-custom dispatcher withBorrow<Q>ergonomicsSerde derives added
GroupInfo,LidPnEntry,LearningSource→Serialize + DeserializeAddressingMode→Deserialize(already hadSerialize)Test plan
cargo test -p whatsapp-rust --lib)cargo clippy --all-targetscleanSummary by CodeRabbit
Release Notes