Repository navigation
fix(cache): reclaim expired entries in set() - #623
Conversation
Todo Tree SummaryNo TODOs found! Last updated Nov 15, 2026, 10:29 UTC · todo-tree |
|
The changes in this PR will be included in the next version bump.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
[Medium impact] The change affects cache behavior for consumers of the cache package. Fix the sweep deleting refreshed values before merging.
|
|
@greptileai review |
| for (const [key, entry] of expired) { | ||
| dropExpired(key, entry); | ||
| } |
There was a problem hiding this comment.
dropAllExpired() can delete a fresh value written by onEvict. With two expired entries, a and b, suppose the callback for a refreshes b with a longer TTL. The second loop still calls dropExpired() with the old entry for b, deleting the replacement and reporting the old value to onEvict.
Before dropping each saved entry, check that store.get(key) still equals that entry.
| for (const [key, entry] of expired) { | |
| dropExpired(key, entry); | |
| } | |
| for (const [key, entry] of expired) { | |
| if (store.get(key) === entry) { | |
| dropExpired(key, entry); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cache/src/cache.ts
Line: 88-90
Comment:
**Refreshed values get deleted**
`dropAllExpired()` can delete a fresh value written by `onEvict`. With two expired entries, `a` and `b`, suppose the callback for `a` refreshes `b` with a longer TTL. The second loop still calls `dropExpired()` with the old entry for `b`, deleting the replacement and reporting the old value to `onEvict`.
Before dropping each saved entry, check that `store.get(key)` still equals that entry.
```suggestion
for (const [key, entry] of expired) {
if (store.get(key) === entry) {
dropExpired(key, entry);
}
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Closes #622
Problem
set()decided whether to evict by counting only live entries, but it never removed the expired ones. As a result:capacity: 2, after two entries expired unread, the cache could end up holding 4 live entries;onEvictnever fired.Fix
When a new key is inserted into a full cache,
set()first drops all expired entries (firingonEvict), and evicts a live entry only if the cache is still full. The store now never holds more thancapacityentries.Changes
packages/cache/src/cache.ts:dropAllExpired()+evictOne()helpers used byset()docs/cache/ttl.mdanddocs/cache/on-evict.mdupdated@zap-studio/cache1.0.0 → 1.0.1🤖 Generated with Claude Code