Add in-memory cache infrastructure#98
Conversation
📝 WalkthroughWalkthroughAdds an async, TTL-capable caching layer using moka with an ChangesCaching Layer
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant AppCache as AppCache<K, V>
participant Moka as moka::future::Cache
participant FetchFn as Fetch Function
Note over Client,FetchFn: Cache get_or_try_insert flow (concurrent miss coalescing)
par Request 1
Client->>AppCache: get_or_try_insert(key, fetch_fn)
and Request 2
Client->>AppCache: get_or_try_insert(key, fetch_fn)
end
Note over AppCache: Both requests observe cache miss
AppCache->>Moka: try_get_with(key, fetch_fn)
Note over AppCache,Moka: try_get_with coalesces concurrent fetches for same key
Moka->>FetchFn: invoke fetch_fn() once
FetchFn-->>Moka: Result<V, E>
Moka->>Moka: insert value if Ok
Moka-->>AppCache: Result<V, Arc<E>>
AppCache-->>Client: Result<V, Arc<E>>
AppCache-->>Client: Result<V, Arc<E>>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
How to use the Graphite Merge QueueAdd the label add-to-gt-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cache.rs (2)
66-68: ⚡ Quick winConsider implementing
DefaultforCacheGroup.Since
new()takes no arguments and returnsSelfwith a trivially constructible state, Clippy'snew_without_defaultlint will fire here. The fix is a one-liner.♻️ Proposed fix
+impl Default for CacheGroup { + fn default() -> Self { + Self::new() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache.rs` around lines 66 - 68, Implement Default for CacheGroup and have new() delegate to Default::default(); specifically, add an impl Default for CacheGroup that returns Self { caches: Vec::new() } and change or keep pub(crate) fn new() -> Self to call Self::default() (or replace it entirely). This addresses the clippy new_without_default lint by providing a canonical Default implementation for CacheGroup and ensures new() remains available and trivial.
107-110: ⚡ Quick winPrefer
run_pending_tasks()overyield_now()afterinvalidate_all()in tests.
tokio::task::yield_now()cedes the scheduler for one tick, but does not guarantee that moka's internal write-op queue has been drained. Maintenance tasks are executed lazily when specific bounded-channel conditions are met, so a single yield may not be sufficient under different scheduler configurations or future moka internals.run_pending_tasks().awaitis moka's explicit API for flushing pending maintenance in tests, making the assertions unconditionally reliable. Because the tests are in the same module,cache.0is accessible.♻️ Proposed fix (applies to both affected tests)
- tokio::task::yield_now().await; + cache.0.run_pending_tasks().await;and for the group test:
- tokio::task::yield_now().await; + cache_a.0.run_pending_tasks().await; + cache_b.0.run_pending_tasks().await;Also applies to: 183-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache.rs` around lines 107 - 110, The test uses tokio::task::yield_now().await after calling cache.invalidate_all(), which does not guarantee moka's internal maintenance/write-op queue is flushed; replace the yield with calling the crate's test helper to flush pending maintenance by awaiting run_pending_tasks() via cache.0 (e.g., cache.0.run_pending_tasks().await) so the subsequent assertions on cache.get(&"a").await and cache.get(&"b").await are reliably valid; apply the same replacement in the other affected test where invalidate_all() is followed by yield_now().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cache.rs`:
- Around line 66-68: Implement Default for CacheGroup and have new() delegate to
Default::default(); specifically, add an impl Default for CacheGroup that
returns Self { caches: Vec::new() } and change or keep pub(crate) fn new() ->
Self to call Self::default() (or replace it entirely). This addresses the clippy
new_without_default lint by providing a canonical Default implementation for
CacheGroup and ensures new() remains available and trivial.
- Around line 107-110: The test uses tokio::task::yield_now().await after
calling cache.invalidate_all(), which does not guarantee moka's internal
maintenance/write-op queue is flushed; replace the yield with calling the
crate's test helper to flush pending maintenance by awaiting run_pending_tasks()
via cache.0 (e.g., cache.0.run_pending_tasks().await) so the subsequent
assertions on cache.get(&"a").await and cache.get(&"b").await are reliably
valid; apply the same replacement in the other affected test where
invalidate_all() is followed by yield_now().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba9c0eae-e355-4723-b2c5-6d67d433b529
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/cache.rssrc/main.rs
Merge activity
|
## Motivation PR #54 mixes reusable cache infrastructure with endpoint behavior changes. This PR keeps the first stack item narrow by adding only the in-memory caching primitives on top of `main`. ## Solution - Add `moka` with the async `future` feature. - Add `AppCache<K, V>` as a crate-local wrapper for TTL/capacity bounded in-memory caches. - Add `get_or_try_insert` for async fallible cache population with request coalescing. - Add `CacheGroup` for grouped invalidation across registered caches. - Cover hit, miss, invalidation, error, and concurrent miss behavior in unit tests. ## Checks - [x] `nix develop -c cargo fmt` - [x] `nix develop -c cargo test cache` - [x] `nix develop -c rainix-rs-static` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Included comprehensive unit tests for cache functionality covering insert/get operations, TTL-based invalidation, concurrent miss coalescing, error handling, and group-wide cache invalidation. * **Chores** * Introduced an internal caching layer with configurable capacity, automatic time-based entry expiration, concurrent request optimization, and multi-cache management capabilities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
Bot
force-pushed
the
in-memory-cache-infra
branch
from
753da6a to
27c67e9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/cache.rs (3)
51-59: ⚖️ Poor tradeoff
Invalidatableis implemented on the innerCache, not onAppCache— bypasses the abstraction.
CacheGroupstores clones of the innermoka::future::Cachedirectly, so any logic that might be added toAppCache::invalidate_allin the future (e.g. tracing events, metrics) would be silently bypassed. ImplementingInvalidatableonAppCache<K,V>itself and adjustingregisterto accept something likeArc<AppCache<K,V>>would keep the group operating through the intended abstraction boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.rs` around lines 51 - 59, The current impl of Invalidatable is on Cache<K,V> which lets CacheGroup operate on the inner moka::future::Cache and bypass AppCache logic; change the impl to target AppCache<K,V> (implement Invalidatable for AppCache<K,V> with its invalidate_all calling AppCache::invalidate_all) and update CacheGroup::register (and any call sites) to accept Arc<AppCache<K,V>> instead of clones of the inner cache so CacheGroup stores and calls invalidate_all via the AppCache abstraction, preserving future metrics/tracing hooks.
65-68: 💤 Low value
CacheGroup::new()should implementDefault.
new()on a plain empty struct with no constructor arguments is idiomatic Rust only whenDefaultis also implemented (or derived). Consider adding#[derive(Default)]or a manualDefaultimpl.✏️ Proposed change
+#[derive(Default)] pub(crate) struct CacheGroup { caches: Vec<Arc<dyn Invalidatable>>, } impl CacheGroup { - pub(crate) fn new() -> Self { - Self { caches: Vec::new() } - } + pub(crate) fn new() -> Self { + Self::default() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.rs` around lines 65 - 68, CacheGroup::new() constructs an empty CacheGroup but CacheGroup lacks a Default implementation; add Default for CacheGroup so callers can use CacheGroup::default() and derive/impl it consistently. Modify the CacheGroup definition to either add #[derive(Default)] or implement impl Default for CacheGroup { fn default() -> Self { Self { caches: Vec::new() } } }, and keep or forward CacheGroup::new() to call Self::default() (or remove new() if you prefer the derived default).
42-44: ⚡ Quick winDocument the eventual-consistency of
invalidate_all.
moka::future::Cache::invalidate_allschedules removal rather than evicting entries synchronously. Entries can still be read immediately after this call until the runtime is given a chance to run pending tasks. The tests already account for this withtokio::task::yield_now().await, but callsites in production code may not know they need to yield before expecting a clean state. A doc comment on the method would make this contract explicit.✏️ Suggested doc comment
+ /// Schedules invalidation of all entries. Entries may still be visible + /// immediately after this call; yield to the async runtime + /// (e.g. `tokio::task::yield_now().await`) before expecting an empty cache. pub(crate) fn invalidate_all(&self) { self.0.invalidate_all() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.rs` around lines 42 - 44, Add a doc comment to the pub(crate) fn invalidate_all(&self) method noting that it delegates to moka::future::Cache::invalidate_all which schedules removals (eventual consistency) rather than synchronously evicting entries, so entries may still be readable immediately after the call until the runtime runs pending tasks; advise callers that require a clean state to await the runtime (e.g., tokio::task::yield_now().await) or use a synchronous/blocking invalidate alternative if available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cache.rs`:
- Around line 51-59: The current impl of Invalidatable is on Cache<K,V> which
lets CacheGroup operate on the inner moka::future::Cache and bypass AppCache
logic; change the impl to target AppCache<K,V> (implement Invalidatable for
AppCache<K,V> with its invalidate_all calling AppCache::invalidate_all) and
update CacheGroup::register (and any call sites) to accept Arc<AppCache<K,V>>
instead of clones of the inner cache so CacheGroup stores and calls
invalidate_all via the AppCache abstraction, preserving future metrics/tracing
hooks.
- Around line 65-68: CacheGroup::new() constructs an empty CacheGroup but
CacheGroup lacks a Default implementation; add Default for CacheGroup so callers
can use CacheGroup::default() and derive/impl it consistently. Modify the
CacheGroup definition to either add #[derive(Default)] or implement impl Default
for CacheGroup { fn default() -> Self { Self { caches: Vec::new() } } }, and
keep or forward CacheGroup::new() to call Self::default() (or remove new() if
you prefer the derived default).
- Around line 42-44: Add a doc comment to the pub(crate) fn
invalidate_all(&self) method noting that it delegates to
moka::future::Cache::invalidate_all which schedules removals (eventual
consistency) rather than synchronously evicting entries, so entries may still be
readable immediately after the call until the runtime runs pending tasks; advise
callers that require a clean state to await the runtime (e.g.,
tokio::task::yield_now().await) or use a synchronous/blocking invalidate
alternative if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6185e05f-9d58-4b75-89aa-351696bf9460
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/cache.rssrc/main.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
3 participants
Motivation
PR #54 mixes reusable cache infrastructure with endpoint behavior changes. This PR keeps the first stack item narrow by adding only the in-memory caching primitives on top of
main.Solution
mokawith the asyncfuturefeature.AppCache<K, V>as a crate-local wrapper for TTL/capacity bounded in-memory caches.get_or_try_insertfor async fallible cache population with request coalescing.CacheGroupfor grouped invalidation across registered caches.Checks
nix develop -c cargo fmtnix develop -c cargo test cachenix develop -c rainix-rs-staticSummary by CodeRabbit
Tests
Chores