fix(memo): bound the cache with per-entry TTL expiry and oldest-first eviction - #36
fix(memo): bound the cache with per-entry TTL expiry and oldest-first eviction#36Ayush7614 wants to merge 2 commits into
Conversation
… eviction - the old sweep only deleted expired entries, so a burst of fresh request-controlled keys (candle buckets, per-limit list keys) grew the map without limit inside one TTL window - expiry now uses each entry's own TTL; overflow evicts expired first, then oldest; fresh hits refresh recency so hot keys survive cold bursts - new memo.test.ts: hits, rejection retry, in-flight sharing, cap, LRU, TTL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe memo cache now has a 200-entry limit, per-entry TTLs, injectable time, recency-based eviction, guarded rejection cleanup, test helpers, and coverage for these behaviors. ChangesMemo cache behavior
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The bounded cache behavior is covered without an identified current-head defect, so no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Bounding the cache and using each entry's own TTL are worthwhile changes. There is one race in the new eviction path that needs fixing before merge: an evicted request can reject later and delete a successful replacement under the same key.
The five submitted tests passed. I reproduced the race against this head at a constant clock, entirely within the TTL: start a pending key, insert 200 other keys to evict it, successfully reload it, then reject the original request. The next lookup calls the backend a third time instead of using the fresh replacement.
Requesting changes for the ownership check described inline. I also left a small, non-blocking TTL-boundary correction that fits this same change.
| // written is newest, so this never evicts the caller's own entry. | ||
| for (const k of store.keys()) { | ||
| if (store.size <= MEMO_MAX_KEYS) break; | ||
| store.delete(k); |
There was a problem hiding this comment.
This can evict an in-flight entry while its rejection handler at lines 37-38 still owns an unconditional store.delete(key). If the key is reloaded before the old request fails, that old failure deletes the fresh replacement and causes unnecessary backend work. Please make rejection cleanup conditional on the cached promise/entry still being the one that failed. Add an eviction -> successful replacement -> old rejection test; the issue reproduces without advancing time.
There was a problem hiding this comment.
@Vasanthdev2004 Thanks — fixed to conditional delete if (store.get(key)?.value === value) store.delete(key) so the old rejection no longer deletes the fresh replacement. Verified with the constant-clock reproduction you described (pending → 200 inserts → reload → old reject → next lookup hits fresh).
| // Expired entries first (each judged by its own TTL)… | ||
| for (const [k, v] of store) { | ||
| if (store.size <= MEMO_MAX_KEYS) break; | ||
| if (t - v.at > v.ttlMs) store.delete(k); |
There was a problem hiding this comment.
Small non-blocking boundary fix: cache hits require elapsed time to be less than the TTL, so an entry is already expired when elapsed time equals its TTL. This pass uses > and can leave that expired entry in place while the fallback evicts a still-fresh older entry. Use >= here to make the two checks agree.
There was a problem hiding this comment.
@Vasanthdev2004 Thanks, fixed to >= ttl so expiry sweep agrees with the hit check (< ttl).
…; fix TTL boundary - Make fn().catch cleanup conditional: only delete when cached value is still the failing promise, so an evicted in-flight request that later rejects does not delete a successful replacement under the same key (reproduces at constant clock: pending -> 200 inserts evict -> reload -> old reject). - Fix expiry check to >= so entries at exactly TTL are considered expired, matching the hit test (<) and avoiding evicting fresh entries while leaving expired ones.
|
@Vasanthdev2004 Thanks for the review! Fixed both points from your feedback:
Pushed to |
2 participants
The memo sweep only deleted expired entries, so a burst of fresh request-controlled keys (candle buckets, per-limit list keys) grew the map without limit inside one TTL window. Expiry now uses each entry's own TTL; overflow evicts expired first, then oldest, and fresh hits refresh recency so hot keys survive cold bursts. New memo.test.ts covers hits, rejection retry, in-flight sharing, the cap, LRU survival and per-entry TTLs (5 tests). Verified: full app suite 338/338, lint, typecheck, build.
Summary by CodeRabbit
Bug Fixes
Performance