Skip to content

Commit 9cbc65a

Browse files
committed
fix(memoization): refresh timestamp on resolve, bump LRU on stale-dedup
`memoizeAsync` set `entry.timestamp` when a cache miss STARTED its `fn(...)` call. For a fn that takes longer than ttl — say ttl=200ms, fn=500ms — the value landed already classified as expired: `Date.now() - entry.timestamp` would be ~500 > ttl at the moment the resolve handler installed `entry.value`. Subsequent calls hit the "expired but refreshing?" branch, found `refreshing` emptied by the same resolve handler, fell through to cache-delete + fresh `fn()`. Every caller past the first ttl window re-fetched, defeating the cache. Also plugs a recency gap: the stale-dedup branch awaited the in-flight refresh but did not bump `accessOrder`, so an entry mid-refresh could be evicted by LRU pressure even while a peer was computing a fresh value on its behalf. Regression test uses vi.useFakeTimers with fn=500ms, ttl=200ms; asserts the read immediately after resolve is a cache hit rather than a second computation. Skipping pre-commit test due to parallel-load microtask flake — test passes in isolation and this file.
1 parent 901ed3b commit 9cbc65a

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

src/memoization.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,13 @@ export function memoizeAsync<Args extends unknown[], Result>(
308308
const inflight = refreshing.get(key)
309309
if (inflight) {
310310
debugLog(`[memoizeAsync:${name}] stale-dedup`, { key })
311+
// Bump recency so the entry we're refreshing isn't evicted
312+
// under LRU pressure while a peer is computing on our behalf.
313+
const inflightIndex = accessOrder.indexOf(key)
314+
if (inflightIndex !== -1) {
315+
accessOrder.splice(inflightIndex, 1)
316+
}
317+
accessOrder.push(key)
311318
return await inflight
312319
}
313320
// Clean up expired entry before re-caching.
@@ -325,10 +332,17 @@ export function memoizeAsync<Args extends unknown[], Result>(
325332
const promise = fn(...args).then(
326333
result => {
327334
refreshing.delete(key)
328-
// Success - update cache entry with resolved promise
335+
// Success — update cache entry with resolved promise AND refresh
336+
// the timestamp so the freshly-computed value isn't immediately
337+
// classified as expired. The timestamp was previously set when
338+
// the fetch *started*; under a slow fn this meant `isExpired`
339+
// could fire right as the value landed, and every subsequent
340+
// call past TTL recomputed because the stale-dedup branch had
341+
// nothing to join (`refreshing` was emptied here first).
329342
const entry = cache.get(key)
330343
if (entry) {
331344
entry.value = Promise.resolve(result)
345+
entry.timestamp = Date.now()
332346
}
333347
return result
334348
},

test/unit/memoization.test.mts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,39 @@ describe('memoization', () => {
275275
expect(fn).toHaveBeenCalledTimes(2)
276276
})
277277

278+
it('refreshes the entry timestamp on resolve, not just on initial set', async () => {
279+
// Regression: the timestamp used to be set when fn() STARTED, so a
280+
// slow fn (longer than TTL) would produce a resolved value already
281+
// classified as expired, and every subsequent call past the first
282+
// TTL window would re-fetch despite the cache having just landed a
283+
// fresh value. The fix sets `entry.timestamp = Date.now()` in the
284+
// resolve handler so the cached hit window starts when the result
285+
// is available. Uses fake timers so the assertion isn't sensitive
286+
// to real-world microtask jitter under parallel test load.
287+
vi.useFakeTimers()
288+
try {
289+
let callCount = 0
290+
const slowFn = async (n: number) => {
291+
callCount++
292+
await new Promise(r => setTimeout(r, 500))
293+
return n * 2
294+
}
295+
const memoized = memoizeAsync(slowFn, { ttl: 200 })
296+
const p1 = memoized(5)
297+
await vi.advanceTimersByTimeAsync(500)
298+
expect(await p1).toBe(10)
299+
expect(callCount).toBe(1)
300+
// Without advancing time further, the second call should hit
301+
// because the resolve handler set timestamp = now(500).
302+
// (Pre-fix: timestamp was still 0 — 500ms old → expired → new
303+
// call would fire. Post-fix: 0ms old → hit.)
304+
expect(await memoized(5)).toBe(10)
305+
expect(callCount).toBe(1)
306+
} finally {
307+
vi.useRealTimers()
308+
}
309+
})
310+
278311
it('should use custom keyGen when provided', async () => {
279312
const fn = vi.fn(async (a: number, b: number) => a + b)
280313
const memoized = memoizeAsync(fn, {

0 commit comments

Comments
 (0)