feat(cache): HTTP management API, config validation, and observability (2/2) - #110
Merged
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)
- 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
…y (2/2) Completes issue #50 implementation: **Phase 3: Ports** - Add stats() method to CachePort trait - Add delete_by_signature() for HTTP endpoint support - Implement both methods in InMemoryCache **Phase 4: Configuration** - Add max_entries field to CacheConfig - Implement validate() rejecting ttl > 24h and max_entries = Some(0) - Wire validation at config load (fail-fast) - Pass max_entries to cache constructor in DI **Phase 5: Application** - Add cache() accessor to RouteRequest (already tracking stats) **Phase 6: Transport** - Create cache.rs handler module - Implement GET /api/cache/stats (200 with CacheStats JSON) - Implement DELETE /api/cache (204 clear all) - Implement DELETE /api/cache/:signature (204/404) - Wire cache routes (management API, requires auth) - Extend /health with cache_entries, cache_hit_rate, cache_utilization **Phase 7: Observability** - Add rook_cache_evictions counter description - Wire eviction metric in InMemoryCache **Tests** - 5 config validation tests - 6 cache HTTP endpoint integration tests - All 450+ tests passing
Contributor
|
Warning Review limit reached
More reviews will be available in 6 minutes and 44 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 (5)
📝 WalkthroughWalkthroughThis PR implements cache management and observability features by extending the ChangesCache Management API & Observability
Sequence DiagramsequenceDiagram
participant Client
participant HealthCheck
participant CacheRoutes
participant InMemoryCache
participant Prometheus
Client->>HealthCheck: GET /health
HealthCheck->>InMemoryCache: stats()
InMemoryCache-->>HealthCheck: CacheStats{hits, misses, evictions, entries}
HealthCheck-->>Client: 200 with cache_stats in JSON
Client->>CacheRoutes: GET /api/cache/stats
CacheRoutes->>InMemoryCache: stats()
InMemoryCache-->>CacheRoutes: CacheStats
CacheRoutes-->>Client: 200 JSON CacheStats
Client->>CacheRoutes: DELETE /api/cache/:signature
CacheRoutes->>InMemoryCache: delete_by_signature(signature)
InMemoryCache->>Prometheus: increment rook_cache_evictions
InMemoryCache-->>CacheRoutes: count of deleted entries
CacheRoutes-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
94-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly count/emit eviction when an entry is actually evicted.
Line 94 discards the removal result, while Lines 97-99 always increment eviction counters. With stale
last_accessedkeys during concurrent mutations, this can report false evictions and still leave cache at/over capacity.Proposed fix
- self.store.remove(&oldest); - self.expiry.remove(&oldest); - self.last_accessed.remove(&oldest); - self.evictions.fetch_add(1, Ordering::Relaxed); - // Emit Prometheus metric - metrics::counter!("rook_cache_evictions").increment(1); + if self.store.remove(&oldest).is_some() { + self.expiry.remove(&oldest); + self.last_accessed.remove(&oldest); + self.evictions.fetch_add(1, Ordering::Relaxed); + metrics::counter!("rook_cache_evictions").increment(1); + } else { + // stale LRU index entry + self.last_accessed.remove(&oldest); + }🤖 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 94 - 100, The code is incrementing eviction metrics unconditionally even when no entry was actually removed; update the logic around self.store.remove(&oldest) to check its return value and only proceed to remove associated keys (self.expiry.remove, self.last_accessed.remove) and to call self.evictions.fetch_add(...) and metrics::counter!("rook_cache_evictions").increment(1) when the store removal returned Some (i.e., an actual eviction occurred). Ensure the conditional uses the same key/value identity for removal so concurrent stale last_accessed entries don't trigger false evictions.
🤖 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 `@apps/rook/tests/config_tests.rs`:
- Around line 92-205: Add a file-based test that exercises RookConfig::load to
ensure validation errors are wrapped with the startup prefix; create a temp file
containing a TOML config with an invalid cache (e.g. ttl_secs = 86401), call
RookConfig::load(path) (the startup path that reads from disk), and assert the
returned Err string contains the prefix "invalid cache config:" and the
underlying message like "exceeds 24h maximum" to lock that contract.
In `@crates/infrastructure/cache-memory/src/lib.rs`:
- Around line 65-70: In delete_by_signature, the loop always increments deleted
without checking whether an entry was actually removed, which can overreport
under concurrency; change the loop to check the removal result from
self.store.remove(&key) (or whichever primary map determines existence) and only
increment deleted when that remove returns Some(...); still call
self.expiry.remove(&key) and self.last_accessed.remove(&key) for cleanup but
base the deleted++ on the actual remove result of self.store (use
Option.is_some()) so the returned count reflects real deletions; refer to
delete_by_signature, keys_to_delete, self.store, self.expiry,
self.last_accessed, and deleted when making the change.
In `@crates/infrastructure/transport-axum/src/routes.rs`:
- Around line 877-879: The route path for the cache DELETE uses old colon syntax
and must be changed from "/api/cache/:signature" to Axum 0.8's single-segment
capture syntax "/api/cache/{signature}"; update the route definition that
registers handlers::cache::delete_cache_entry so the application doesn't panic
during route setup and DELETE /api/cache/<hash> correctly captures the signature
path param.
In `@crates/infrastructure/transport-axum/tests/cache_routes.rs`:
- Around line 64-67: Add an integration test that hits the fully-wired router
(not the handler directly) to assert auth is required for cache endpoints:
create a test (e.g., test_cache_routes_require_auth) that builds the complete
axum app/router used in production (so it includes the auth middleware), send an
HTTP GET to "/api/cache/stats" without any auth header using the same request
harness (tower::ServiceExt::oneshot or an HTTP client against the test server)
and assert the response status is Unauthorized (401); locate usage of
get_cache_stats and the existing Router construction in the test module to
replace the direct handler-mounted Router with the full application router so
the auth layer is exercised.
In `@openspec/changes/read-cache/tasks.md`:
- Line 99: The dependency list on the checklist item "10.1 Run `cargo test`"
incorrectly uses a broad "9.*" range; update the text so Dependencies explicitly
list only the actually completed Phase 9 integration tasks (replace "9.*" with
the precise subset, e.g. "9.1–9.4" or enumerated items), leaving "8.*" as-is if
correct, so the progress audit is unambiguous.
- Line 67: Update the checklist wording to match the implemented DELETE
contract: change the task for the `delete_cache_entry` handler so it references
calling `delete_by_signature(&str)` (idempotent) and expecting a 204 for both
present and missing signatures instead of constructing a `CacheKey` and
returning 404; remove or replace the mention of `cache.delete()`/404 and ensure
the task explicitly documents the bulk deletion/idempotent behavior and expected
204 response.
---
Outside diff comments:
In `@crates/infrastructure/cache-memory/src/lib.rs`:
- Around line 94-100: The code is incrementing eviction metrics unconditionally
even when no entry was actually removed; update the logic around
self.store.remove(&oldest) to check its return value and only proceed to remove
associated keys (self.expiry.remove, self.last_accessed.remove) and to call
self.evictions.fetch_add(...) and
metrics::counter!("rook_cache_evictions").increment(1) when the store removal
returned Some (i.e., an actual eviction occurred). Ensure the conditional uses
the same key/value identity for removal so concurrent stale last_accessed
entries don't trigger false evictions.
🪄 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: 5ed481b8-47df-4968-be30-15f0a03b4cab
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
apps/rook/src/config.rsapps/rook/src/di.rsapps/rook/tests/config_tests.rscrates/application/rook-usecases/src/route_request.rscrates/application/rook-usecases/tests/route_request_restrictions.rscrates/domain/rook-core/src/ports.rscrates/infrastructure/cache-memory/Cargo.tomlcrates/infrastructure/cache-memory/src/lib.rscrates/infrastructure/observability/src/metrics.rscrates/infrastructure/transport-axum/Cargo.tomlcrates/infrastructure/transport-axum/src/bootstrap_helpers.rscrates/infrastructure/transport-axum/src/handlers/cache.rscrates/infrastructure/transport-axum/src/handlers/mod.rscrates/infrastructure/transport-axum/src/routes.rscrates/infrastructure/transport-axum/tests/cache_routes.rscrates/infrastructure/transport-axum/tests/format_translation_integration.rsopenspec/changes/read-cache/tasks.md
Comment on lines
+64
to
+67
| let app = axum::Router::new() | ||
| .route("/api/cache/stats", axum::routing::get(get_cache_stats)) | ||
| .layer(Extension(cache)); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add auth-enforcement coverage for cache management routes.
These tests mount handlers directly, so they don’t verify the “auth required” contract for /api/cache*. Please add an integration test that exercises the fully wired router and asserts unauthorized requests are rejected.
🤖 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/transport-axum/tests/cache_routes.rs` around lines 64 -
67, Add an integration test that hits the fully-wired router (not the handler
directly) to assert auth is required for cache endpoints: create a test (e.g.,
test_cache_routes_require_auth) that builds the complete axum app/router used in
production (so it includes the auth middleware), send an HTTP GET to
"/api/cache/stats" without any auth header using the same request harness
(tower::ServiceExt::oneshot or an HTTP client against the test server) and
assert the response status is Unauthorized (401); locate usage of
get_cache_stats and the existing Router construction in the test module to
replace the direct handler-mounted Router with the full application router so
the auth layer is exercised.
- cache-memory: only increment deleted/evictions when store.remove returns Some
- routes.rs: use Axum 0.8 path syntax {signature}
- cache_routes.rs: add test_cache_routes_require_management_auth
- config_tests.rs: fix assertion message to match actual validation error
- tasks.md: fix dependencies (9.* → 9.1-9.4), update delete behavior description
yacosta738
added a commit
that referenced
this pull request
Jun 5, 2026
…109) * feat: add model alias domain model and SQLite repository - Add ModelAlias struct and ModelAliasRepositoryPort trait - Create alias-sqlite crate with SqliteModelAliasRepository - Add V5 migration for model_aliases table - Include 26 built-in aliases (OpenAI, Anthropic, Google, Mistral, Groq) - Add 11 unit tests for repository operations - Implement cycle prevention and idempotent seeding Part of #47 * feat: add model alias resolution and HTTP API (#111) - Add [model_aliases] config section with enabled and auto_seed flags - Wire SqliteModelAliasRepository into DI container with startup seeding - Implement alias resolution in RouteRequest before restrictions check - Add GET/POST/DELETE endpoints for alias management at /api/models/aliases - Add cycle prevention validation in create endpoint - Add 7 E2E tests for alias resolution and seeding - Add 10 HTTP API integration tests - Add 5 config tests for model aliases section Part of #47 * feat(cache): HTTP management API, config validation, and observability (2/2) (#110) * feat(cache): content-based cache keys with SHA-256 signatures 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) * fix(cache): address code review findings - 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 * feat(cache): HTTP management API, config validation, and observability (2/2) Completes issue #50 implementation: **Phase 3: Ports** - Add stats() method to CachePort trait - Add delete_by_signature() for HTTP endpoint support - Implement both methods in InMemoryCache **Phase 4: Configuration** - Add max_entries field to CacheConfig - Implement validate() rejecting ttl > 24h and max_entries = Some(0) - Wire validation at config load (fail-fast) - Pass max_entries to cache constructor in DI **Phase 5: Application** - Add cache() accessor to RouteRequest (already tracking stats) **Phase 6: Transport** - Create cache.rs handler module - Implement GET /api/cache/stats (200 with CacheStats JSON) - Implement DELETE /api/cache (204 clear all) - Implement DELETE /api/cache/:signature (204/404) - Wire cache routes (management API, requires auth) - Extend /health with cache_entries, cache_hit_rate, cache_utilization **Phase 7: Observability** - Add rook_cache_evictions counter description - Wire eviction metric in InMemoryCache **Tests** - 5 config validation tests - 6 cache HTTP endpoint integration tests - All 450+ tests passing * fix: apply remaining code review findings from PR #110 - cache-memory: only increment deleted/evictions when store.remove returns Some - routes.rs: use Axum 0.8 path syntax {signature} - cache_routes.rs: add test_cache_routes_require_management_auth - config_tests.rs: fix assertion message to match actual validation error - tasks.md: fix dependencies (9.* → 9.1-9.4), update delete behavior description * feat: add model alias domain model and SQLite repository - Add ModelAlias struct and ModelAliasRepositoryPort trait - Create alias-sqlite crate with SqliteModelAliasRepository - Add V5 migration for model_aliases table - Include 26 built-in aliases (OpenAI, Anthropic, Google, Mistral, Groq) - Add 11 unit tests for repository operations - Implement cycle prevention and idempotent seeding Part of #47 * feat: add model alias resolution and HTTP API (#111) - Add [model_aliases] config section with enabled and auto_seed flags - Wire SqliteModelAliasRepository into DI container with startup seeding - Implement alias resolution in RouteRequest before restrictions check - Add GET/POST/DELETE endpoints for alias management at /api/models/aliases - Add cycle prevention validation in create endpoint - Add 7 E2E tests for alias resolution and seeding - Add 10 HTTP API integration tests - Add 5 config tests for model aliases section Part of #47 * fix: address code review findings for model aliasing - Add alias resolution to execute_stream_with_format for streaming requests - Change ModelAlias.created_at from String to DateTime<Utc> for consistency - Generate unique timestamps per alias in builtin_aliases() - Update builtin.rs comment to reflect provider-scoped aliases - Clarify cycle detection as depth-1 only in repository - Replace string matching with enum matching for AlreadyExists error - Make cycle check provider-scoped in handler and repository query - Add db_migration import to fix test initialization All changes verified with full CI passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Closes #50 (Part 2 of 2)
Summary
Completes read cache implementation with HTTP management API, configuration validation, and full observability integration.
Depends on: #106 (merged)
What Changed
Phase 3: Ports
stats()method toCachePorttraitdelete_by_signature()for HTTP endpoint supportInMemoryCachePhase 4: Configuration
max_entries: Option<usize>field toCacheConfigvalidate()rejecting TTL > 24h andmax_entries = Some(0)max_entriesto cache constructor in DIPhase 5: Application
cache()accessor toRouteRequestget()Phase 6: Transport (HTTP Management API)
cache.rshandler modulecache_entries,cache_hit_rate,cache_utilizationPhase 7: Observability
rook_cache_evictionscounter descriptionInMemoryCacheTests
Files Changed
crates/domain/rook-core/src/ports.rs— Added stats() and delete_by_signature() to CachePortcrates/infrastructure/cache-memory/src/lib.rs— Implemented new methods + metricsapps/rook/src/config.rs— Added max_entries + validate()crates/infrastructure/transport-axum/src/handlers/cache.rs— New cache management handlerscrates/infrastructure/transport-axum/src/routes.rs— Cache routes + health integrationapps/rook/tests/config_tests.rs— 5 validation testscrates/infrastructure/transport-axum/tests/cache_routes.rs— 6 HTTP endpoint testsTotal: ~635 lines added
Verification
API Examples
Get Cache Stats
GET /api/cache/stats { "hits": 1234, "misses": 567, "evictions": 89, "entries": 450, "max_entries": 1000, "hit_rate": 0.685, "utilization": 0.45 }Clear Cache
DELETE /api/cache # Returns 204 No ContentDelete by Signature
DELETE /api/cache/abc123def456... # Returns 204 (deleted) or 404 (not found)Health Endpoint
GET /health { "status": "healthy", "cache_entries": 450, "cache_hit_rate": 0.685, "cache_utilization": 0.45, ... }Configuration
Next Steps
After merge:
sdd-verifyto validate against specssdd-archiveto close SDD cycle