feat(cache): content-based cache keys with SHA-256 signatures (1/2) - #106
Conversation
Implement content-based caching for issue #50: - Add signature field to CacheKey (SHA-256 hex string) - Update CompletionRequest.cache_key() to hash (model + messages + params) - Add CacheStats struct with hit_rate() and utilization() methods - Add LRU eviction to InMemoryCache with max_entries support - Add AtomicU64 counters (hits, misses, evictions) - Implement stats() method in InMemoryCache - Add comprehensive unit tests for hashing, LRU, and stats Breaking change: CacheKey now requires signature field (documented in proposal)
|
Warning Review limit reached
More reviews will be available in 49 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements a semantic response cache using content-based signatures derived from SHA-256 hashing of request semantic fields (model, messages, parameters). The implementation spans domain contract extensions, infrastructure storage with LRU eviction and atomic statistics tracking, dependency injection wiring, and comprehensive design documentation defining the architecture and acceptance criteria. ChangesSemantic Read Cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/infrastructure/cache-memory/src/lib.rs (1)
96-112: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor inefficiency: eviction triggered even when overwriting existing key.
When
set()overwrites an existing key,evict_if_needed()is still called. If the cache is at capacity, this unnecessarily evicts an entry even though no new slot is needed.This is not a correctness bug—the cache still works correctly—but it may cause premature evictions during update-heavy workloads.
♻️ Optional fix to skip eviction on overwrites
async fn set( &self, key: &CacheKey, value: &CompletionResponse, ttl: Duration, ) -> CortexResult<()> { - // Evict oldest entry if at capacity - self.evict_if_needed(); + // Evict oldest entry if at capacity and not overwriting + if !self.store.contains_key(key) { + self.evict_if_needed(); + } self.store.insert(key.clone(), value.clone());🤖 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 `@crates/infrastructure/cache-memory/src/lib.rs` around lines 96 - 112, The set method calls evict_if_needed unconditionally which can evict when overwriting an existing key; change set in cache-memory::lib.rs to check if key already exists in self.store (e.g., via self.store.contains_key(key) or retrieval) and only call evict_if_needed when inserting a new key (not when updating), then proceed to insert/update expiry and last_accessed as before; reference functions/fields: set, evict_if_needed, self.store, self.expiry, self.last_accessed.
🤖 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.
Inline comments:
In `@crates/infrastructure/cache-memory/src/lib.rs`:
- Around line 52-71: Add a short documentation note to evict_if_needed (and/or
the Cache type) stating that LRU eviction is approximate under concurrency:
explain that evict_if_needed finds the oldest key by iterating last_accessed and
may race with other threads (an entry could be accessed between selection and
removal), so strict LRU is not guaranteed without heavier locking; reference the
evict_if_needed method, last_accessed map, and the eviction counter (evictions)
so maintainers know where the approximation occurs and why it's acceptable for
performance.
In `@openspec/changes/read-cache/specs/cache/spec.md`:
- Around line 186-196: The spec currently conflicts with the code's
CacheConfig.max_entries (Option<usize>) and InMemoryCache semantics — update the
spec to clarify which interpretation you choose: either (A) treat 0/None as
"unlimited" by allowing max_entries = 0 (or prefer explicit None) and change the
"Reject zero" scenario to only reject negative values removed and instead
validate only when max_entries.is_some(), or (B) enforce non-zero capacity by
changing the config type to Option<NonZeroUsize> (and adjust tasks/architecture
and InMemoryCache accordingly) and update the scenarios to reject zero; also
remove the impossible "negative" case since usize cannot be negative. Ensure you
reference CacheConfig.max_entries and InMemoryCache in the spec so the behavior
matches the implementation and issue `#50` objectives.
In `@openspec/changes/read-cache/tasks.md`:
- Line 54: Clarify and implement explicit validation in CacheConfig::validate()
(apps/rook/src/config.rs): keep the existing ttl_secs check (reject > 86400) and
add a deterministic rule for max_entries such that if max_entries is Some(n) it
must be n > 0 (reject zero or negative), and if max_entries is None allow it
(unlimited capacity); return an error when the max_entries Some constraint fails
and otherwise pass validation.
---
Outside diff comments:
In `@crates/infrastructure/cache-memory/src/lib.rs`:
- Around line 96-112: The set method calls evict_if_needed unconditionally which
can evict when overwriting an existing key; change set in cache-memory::lib.rs
to check if key already exists in self.store (e.g., via
self.store.contains_key(key) or retrieval) and only call evict_if_needed when
inserting a new key (not when updating), then proceed to insert/update expiry
and last_accessed as before; reference functions/fields: set, evict_if_needed,
self.store, self.expiry, self.last_accessed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d676158b-a563-48b5-8cf4-1ea84df72d6e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
apps/rook/src/di.rscrates/domain/rook-core/Cargo.tomlcrates/domain/rook-core/src/model.rscrates/domain/shared-kernel/src/lib.rscrates/infrastructure/cache-memory/src/lib.rsopenspec/changes/read-cache/design.mdopenspec/changes/read-cache/exploration.mdopenspec/changes/read-cache/proposal.mdopenspec/changes/read-cache/specs/cache/spec.mdopenspec/changes/read-cache/state.yamlopenspec/changes/read-cache/tasks.md
- Add documentation for approximate LRU under concurrency - Fix evict_if_needed to only trigger on new keys (not overwrites) - Add test for no-eviction-on-overwrite behavior - Clarify spec: None=unlimited, Some(0)=rejected (usize cannot be negative) - Update tasks to specify max_entries validation rules
Issue
Closes #50 (Part 1 of 2)
Summary
Implements content-based caching foundation with SHA-256 signatures, LRU eviction, and stats tracking.
CacheKeynow includessignature: Stringfield (documented in proposal)What Changed
Phase 1: Foundation
signature: Stringfield toCacheKey(breaking change)Displaytrait forCacheKey(shows first 8 chars of signature)CacheStatsstruct withhit_rate()andutilization()methodssha2andhexdependencies torook-coreCompletionRequest::cache_key()Phase 2: Infrastructure
DashMap<CacheKey, Instant>for last_accessed timestampsAtomicU64counters (hits, misses, evictions) toInMemoryCacheInMemoryCache::new()constructor to acceptmax_entries: Option<usize>set()when capacity reachedget()to track hits/misses and update last_accessedclear()to reset all statsstats()method returningCacheStatsPhase 8: Unit Tests
Verification
Files Changed
crates/domain/shared-kernel/src/lib.rs— CacheKey signature field + Display + test helpercrates/domain/rook-core/src/model.rs— CacheStats struct + SHA-256 hashing + 11 unit testscrates/infrastructure/cache-memory/src/lib.rs— LRU + stats counters + 12 unit testscrates/domain/rook-core/Cargo.toml— sha2 + hex dependenciesapps/rook/src/di.rs— updated InMemoryCache constructor callTotal: ~657 lines (higher than estimated due to comprehensive tests)
Next Steps (PR 2)
CachePort::statsmethod)max_entriesfield, TTL validation)RouteRequest)SDD Artifacts
Full SDD cycle completed:
Artifacts available in:
openspec/changes/read-cache/