Restore cache tests & fix max-age behavior (#4198)

* Restore cache tests

Signed-off-by: Matteo Collina <[email protected]>

* fixed caching

Signed-off-by: Matteo Collina <[email protected]>

* fixup

Signed-off-by: Matteo Collina <[email protected]>

* fixup

Signed-off-by: Matteo Collina <[email protected]>

---------

Signed-off-by: Matteo Collina <[email protected]>

Author: mcollina

lib/cache/memory-cache-store.js

@@ -76,17 +76,9 @@ class MemoryCacheStore {
     const topLevelKey = `${key.origin}:${key.path}`
 
     const now = Date.now()
-    const entry = this.#entries.get(topLevelKey)?.find((entry) => (
-      entry.deleteAt > now &&
-      entry.method === key.method &&
-      (entry.vary == null || Object.keys(entry.vary).every(headerName => {
-        if (entry.vary[headerName] === null) {
-          return key.headers[headerName] === undefined
-        }
+    const entries = this.#entries.get(topLevelKey)
 
-        return entry.vary[headerName] === key.headers[headerName]
-      }))
-    ))
+    const entry = entries ? findEntry(key, entries, now) : null
 
     return entry == null
       ? undefined
@@ -140,10 +132,17 @@ class MemoryCacheStore {
           entries = []
           store.#entries.set(topLevelKey, entries)
         }
-        entries.push(entry)
+        const previousEntry = findEntry(key, entries, Date.now())
+        if (previousEntry) {
+          const index = entries.indexOf(previousEntry)
+          entries.splice(index, 1, entry)
+          store.#size -= previousEntry.size
+        } else {
+          entries.push(entry)
+          store.#count += 1
+        }
 
         store.#size += entry.size
-        store.#count += 1
 
         if (store.#size > store.#maxSize || store.#count > store.#maxCount) {
           for (const [key, entries] of store.#entries) {
@@ -180,4 +179,18 @@ class MemoryCacheStore {
   }
 }
 
+function findEntry (key, entries, now) {
+  return entries.find((entry) => (
+    entry.deleteAt > now &&
+    entry.method === key.method &&
+    (entry.vary == null || Object.keys(entry.vary).every(headerName => {
+      if (entry.vary[headerName] === null) {
+        return key.headers[headerName] === undefined
+      }
+
+      return entry.vary[headerName] === key.headers[headerName]
+    }))
+  ))
+}
+
 module.exports = MemoryCacheStore

test/cache-interceptor/cache-store-test-utils.js

@@ -164,6 +164,87 @@ function cacheStoreTests (CacheStore) {
       equal(await store.get(key), undefined)
     })
 
+    test('a stale request is overwritten', async () => {
+      const clock = FakeTimers.install({
+        shouldClearNativeTimers: true
+      })
+
+      after(() => clock.uninstall())
+
+      /**
+       * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey}
+       */
+      const key = {
+        origin: 'localhost',
+        path: '/',
+        method: 'GET',
+        headers: {}
+      }
+
+      /**
+       * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue}
+       */
+      const value = {
+        statusCode: 200,
+        statusMessage: '',
+        headers: { foo: 'bar' },
+        cacheControlDirectives: {},
+        cachedAt: Date.now(),
+        staleAt: Date.now() + 1000,
+        // deleteAt is different because stale-while-revalidate, stale-if-error, ...
+        deleteAt: Date.now() + 5000
+      }
+
+      const body = [Buffer.from('asd'), Buffer.from('123')]
+
+      const store = new CacheStore()
+
+      // Sanity check
+      equal(store.get(key), undefined)
+
+      {
+        const writable = store.createWriteStream(key, value)
+        notEqual(writable, undefined)
+        writeBody(writable, body)
+      }
+
+      clock.tick(1500)
+
+      {
+        const result = await store.get(structuredClone(key))
+        notEqual(result, undefined)
+        await compareGetResults(result, value, body)
+      }
+
+      /**
+       * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue}
+       */
+      const value2 = {
+        statusCode: 200,
+        statusMessage: '',
+        headers: { foo: 'baz' },
+        cacheControlDirectives: {},
+        cachedAt: Date.now(),
+        staleAt: Date.now() + 1000,
+        // deleteAt is different because stale-while-revalidate, stale-if-error, ...
+        deleteAt: Date.now() + 5000
+      }
+
+      const body2 = [Buffer.from('foo'), Buffer.from('123')]
+
+      {
+        const writable = store.createWriteStream(key, value2)
+        notEqual(writable, undefined)
+        writeBody(writable, body2)
+      }
+
+      {
+        const result = await store.get(structuredClone(key))
+        notEqual(result, undefined)
+        await compareGetResults(result, value2, body2)
+      }
+    })
+
     test('vary directives used to decide which response to use', async () => {
       /**
        * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey}

test/cache-interceptor/cache-tests-worker.mjs

@@ -50,8 +50,10 @@ if (environment.ignoredTests) {
 const client = new Agent().compose(interceptors.cache(environment.opts))
 setGlobalDispatcher(client)
 
+globalThis.fetch = fetch
+
 // Run the suite
-await runTestSuite(tests, fetch, true, process.env.BASE_URL)
+await runTestSuite(tests, true, process.env.BASE_URL)
 
 let exitCode = 0
 
@@ -207,6 +209,10 @@ function printStats (stats) {
     retried
   } = stats
 
+  if (total < 0) {
+    throw new Error('Total tests cannot be negative')
+  }
+
   console.log(`\n        Total tests: ${total}`)
   console.log(`            ${styleText('gray', 'Skipped')}: ${skipped} (${((skipped / total) * 100).toFixed(1)}%)`)
   console.log(`             ${styleText('green', 'Passed')}: ${passed} (${((passed / total) * 100).toFixed(1)}%)`)

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

@@ -38,6 +38,8 @@ test('SqliteCacheStore works nicely with multiple stores', async (t) => {
   })
 
   t.after(async () => {
+    storeA.close()
+    storeB.close()
     await rm(sqliteLocation)
   })