perf(cache): mutex-guard memory map + periodic cleanup + 24h home TTL (E4.1)#548
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughCacheManager now protects the in-memory cache with a Mutex across all operations (get, put, invalidate, invalidateByPrefix, clearAll, cleanupExpired). An optional appScope parameter enables a background janitor coroutine that periodically calls cleanupExpired(). Cache TTL for HOME_REPOS increased to 24 hours, and the DI wiring passes appScope to CacheManager on construction. ChangesCache Concurrency & Background Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/cache/CacheManager.kt`:
- Around line 55-67: In CacheManager.get(), avoid unconditionally removing an
in-memory or DB entry based on a stale snapshot: when you read the snapshot from
memoryCache under memoryCacheMutex (the local cached / (expiresAt, jsonData)
pair), decode jsonData before mutating any cache or DB, and only remove from
memoryCache if the current memoryCache[key] still exactly equals the snapshot
you read (compare the tuple) to avoid evicting a newer put(); for the DB-backed
branch, when purging malformed or expired rows use a conditional delete tied to
the exact row/version (e.g., include the original row id/version in the WHERE)
rather than a plain delete(key). Ensure these checks are applied in both the
decode-failure and expiry branches so remove(key)/delete(key) is only executed
when the stored value still matches the snapshot.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 253098d0-6ffd-4b68-93f6-99179d4c452e
📒 Files selected for processing (2)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/cache/CacheManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt
First slice of E4 Performance Pass. Pure static-friendly changes; no profiler required.
What & why
1. Thread-safe memory cache
CacheManager.memoryCacheis a plainHashMapmutated from any caller's coroutine — Home, Details, Search, Profile repositories all hit it concurrently from their respective ViewModel scopes. Concurrent structural mutations (putduring another caller's rehash) can corrupt internal state. Addedkotlinx.coroutines.sync.Mutexand wrapped every read/write touching the map.memoryCache,memoryCacheMutex,cacheDao, andjsonare now@PublishedApi internalso the inline reifiedget<T>()/put<T>()keep working from feature-module call sites without the map being effectively public-API.2. Periodic janitor for the in-memory cache
cleanupExpired()was defined but had no callers. Memory map grew unbounded over a long session.CacheManagerconstructor now takes an optionalappScope: CoroutineScope?and launches a 1h janitor loop that runscleanupExpired()off the critical path. Mutex serialises it with foreground reads/writes. Wired incoreModuleKoin viaappScope = get().3.
HOME_REPOSTTL 12h → 24hPer E4 spec recommendation. Home grid is browsed often, tolerates stale data well, longer TTL cuts cold-start network round-trips on the highest-traffic surface.
Test plan
:core:data:compileDebugKotlinAndroid✅:core:data:compileKotlinJvm✅@PublishedApi internalvisibility fix from the first pass).Out of scope (later E4 slices)
Springsweep (survey Refactor: Introduce BrowserHelper and ClipboardHelper #16).Application.onCreate/MainActivity.onCreatework.DetailsViewModel.loadDetailsparallelasync {}.memoryCache— bounded growth is good-enough with periodic cleanup; LRU adds complexity, defer until profile shows it matters.getStalecorrectly; Home / Search don't. Separate PR.Summary by CodeRabbit
New Features
Bug Fixes
Performance