fix(cache): evict least recently used entries in MemoryCacheStore

docs/docs/api/CacheStore.md

@@ -42,7 +42,8 @@ added: v7.0.0
 Stores cached responses in memory. The store enforces upper bounds on the total
 number of responses, the total size of all responses, and the size of any single
 response. When a limit is exceeded, the store evicts approximately half of its
-entries and emits a [`'maxSizeExceeded'`](#event-maxsizeexceeded) event.
+entries, least recently used first, and emits a
+[`'maxSizeExceeded'`](#event-maxsizeexceeded) event.
 
 ```mjs
 import { interceptors, cacheStores, Agent, setGlobalDispatcher } from 'undici'

lib/cache/memory-cache-store.js

@@ -100,6 +100,11 @@ class MemoryCacheStore extends EventEmitter {
 
     const entry = entries ? findEntry(key, entries, now) : null
 
+    if (entry != null) {
+      this.#entries.delete(topLevelKey)
+      this.#entries.set(topLevelKey, entries)
+    }
+
     return entry == null
       ? undefined
       : {
@@ -148,11 +153,13 @@ class MemoryCacheStore extends EventEmitter {
       },
       final (callback) {
         let entries = store.#entries.get(topLevelKey)
-        if (!entries) {
+        if (entries) {
+          store.#entries.delete(topLevelKey)
+        } else {
           entries = []
-          store.#entries.set(topLevelKey, entries)
         }
-        const previousEntry = findEntry(key, entries, Date.now())
+        store.#entries.set(topLevelKey, entries)
+        const previousEntry = findEntry(key, entries, -Infinity)
         if (previousEntry) {
           const index = entries.indexOf(previousEntry)
           entries.splice(index, 1, entry)
@@ -177,16 +184,7 @@ class MemoryCacheStore extends EventEmitter {
             store.#hasEmittedMaxSizeEvent = true
           }
 
-          // Perform eviction
-          for (const [key, entries] of store.#entries) {
-            for (const entry of entries.splice(0, Math.ceil(entries.length / 2))) {
-              store.#size -= entry.size
-              store.#count -= 1
-            }
-            if (entries.length === 0) {
-              store.#entries.delete(key)
-            }
-          }
+          store.#evict(entry)
 
           // Reset the event flag after eviction
           if (store.#size < store.#maxSize && store.#count < store.#maxCount) {
@@ -199,6 +197,43 @@ class MemoryCacheStore extends EventEmitter {
     })
   }
 
+  #evict (keep) {
+    const targetSize = this.#maxSize / 2
+    const targetCount = this.#maxCount / 2
+
+    for (const [key, entries] of this.#entries) {
+      for (let i = 0; i < entries.length && (this.#size > targetSize || this.#count > targetCount);) {
+        const entry = entries[i]
+        if (entry === keep) {
+          i++
+          continue
+        }
+        entries.splice(i, 1)
+        this.#size -= entry.size
+        this.#count -= 1
+      }
+
+      if (entries.length === 0) {
+        this.#entries.delete(key)
+      }
+
+      if (this.#size <= targetSize && this.#count <= targetCount) {
+        return
+      }
+    }
+
+    if (this.#size > this.#maxSize || this.#count > this.#maxCount) {
+      const topLevelKey = `${keep.origin}:${keep.path}`
+      const entries = this.#entries.get(topLevelKey)
+      entries.splice(entries.indexOf(keep), 1)
+      this.#size -= keep.size
+      this.#count -= 1
+      if (entries.length === 0) {
+        this.#entries.delete(topLevelKey)
+      }
+    }
+  }
+
   /**
    * @param {CacheKey} key
    */

test/cache-interceptor/memory-cache-store-tests.js

@@ -1,7 +1,7 @@
 'use strict'
 
 const { test } = require('node:test')
-const { equal } = require('node:assert')
+const { equal, deepStrictEqual } = require('node:assert')
 const MemoryCacheStore = require('../../lib/cache/memory-cache-store')
 const { cacheStoreTests } = require('./cache-store-test-utils.js')
 
@@ -204,3 +204,99 @@ test('emits maxSizeExceeded event when limits exceeded', async () => {
   equal(typeof eventPayload.count, 'number', 'Payload should have count')
   equal(typeof eventPayload.maxCount, 'number', 'Payload should have maxCount')
 })
+
+function writeEntry (store, path, { body = 'x', ttl = 60000, vary } = {}) {
+  const now = Date.now()
+  const writeStream = store.createWriteStream(
+    { origin: 'test', path, method: 'GET', headers: vary },
+    {
+      statusCode: 200,
+      statusMessage: 'OK',
+      headers: {},
+      vary,
+      cachedAt: now,
+      staleAt: now + ttl,
+      deleteAt: now + ttl
+    }
+  )
+  writeStream.end(body)
+}
+
+function hasEntry (store, path, headers = {}) {
+  return store.get({ origin: 'test', path, method: 'GET', headers }) !== undefined
+}
+
+test('eviction removes the oldest entries and keeps the newest', () => {
+  const store = new MemoryCacheStore({ maxCount: 10 })
+
+  for (let i = 0; i < 11; i++) {
+    writeEntry(store, `/${i}`)
+  }
+
+  const kept = []
+  for (let i = 0; i < 11; i++) {
+    if (hasEntry(store, `/${i}`)) kept.push(i)
+  }
+  deepStrictEqual(kept, [6, 7, 8, 9, 10])
+})
+
+test('eviction by maxSize removes the oldest entries and keeps the newest', () => {
+  const store = new MemoryCacheStore({ maxSize: 1000 })
+
+  for (let i = 0; i < 11; i++) {
+    writeEntry(store, `/${i}`, { body: 'x'.repeat(100) })
+  }
+
+  equal(store.size, 500)
+  equal(hasEntry(store, '/0'), false)
+  equal(hasEntry(store, '/10'), true)
+})
+
+test('eviction treats a rewritten entry as the newest', () => {
+  const store = new MemoryCacheStore({ maxCount: 4 })
+
+  for (let i = 0; i < 4; i++) {
+    writeEntry(store, `/${i}`)
+  }
+  writeEntry(store, '/0')
+  writeEntry(store, '/4')
+
+  deepStrictEqual([0, 1, 2, 3, 4].filter((i) => hasEntry(store, `/${i}`)), [0, 4])
+})
+
+test('eviction treats a read entry as the most recently used', () => {
+  const store = new MemoryCacheStore({ maxCount: 4 })
+
+  for (let i = 0; i < 4; i++) {
+    writeEntry(store, `/${i}`)
+  }
+  hasEntry(store, '/0')
+  writeEntry(store, '/4')
+
+  deepStrictEqual([0, 1, 2, 3, 4].filter((i) => hasEntry(store, `/${i}`)), [0, 4])
+})
+
+test('eviction keeps the entry just written', () => {
+  const store = new MemoryCacheStore({ maxSize: 300 })
+
+  writeEntry(store, '/', { body: 'x'.repeat(100), vary: { accept: 'a' } })
+  writeEntry(store, '/', { body: 'x'.repeat(100), vary: { accept: 'b' } })
+  writeEntry(store, '/other', { body: 'x'.repeat(100) })
+  writeEntry(store, '/', { body: 'x'.repeat(250), vary: { accept: 'a' } })
+
+  equal(hasEntry(store, '/', { accept: 'a' }), true)
+  equal(hasEntry(store, '/', { accept: 'b' }), false)
+  equal(hasEntry(store, '/other'), false)
+  equal(store.size, 250)
+})
+
+test('an expired entry is replaced rather than duplicated', async () => {
+  const store = new MemoryCacheStore()
+
+  for (let i = 0; i < 5; i++) {
+    writeEntry(store, '/', { body: 'x'.repeat(100), ttl: 5 })
+    await new Promise((resolve) => setTimeout(resolve, 10))
+  }
+
+  equal(store.size, 100)
+})