fix(cache): freshen the stored entry when a 304 extends its lifetime

lib/handler/cache-revalidation-handler.js

@@ -19,7 +19,16 @@ class CacheRevalidationHandler {
   #successful = false
 
   /**
-   * @type {((success: boolean, context?: any, statusCode?: number, headers?: import('../../types/header.d.ts').IncomingHttpHeaders) => void) | null}
+   * Set when the validation response is being forwarded to the wrapped handler
+   * only so the stored entry can be updated, i.e. the client is being served the
+   * cached value elsewhere and must not see anything the wrapped handler emits.
+   *
+   * @type {boolean}
+   */
+  #storeUpdateOnly = false
+
+  /**
+   * @type {((success: boolean, context?: any, statusCode?: number, headers?: import('../../types/header.d.ts').IncomingHttpHeaders) => boolean | void) | null}
    */
   #callback
 
@@ -36,7 +45,7 @@ class CacheRevalidationHandler {
   #allowErrorStatusCodes
 
   /**
-   * @param {(success: boolean, context?: any, statusCode?: number, headers?: import('../../types/header.d.ts').IncomingHttpHeaders) => void} callback Function to call if the cached value is valid
+   * @param {(success: boolean, context?: any, statusCode?: number, headers?: import('../../types/header.d.ts').IncomingHttpHeaders) => boolean | void} callback Function to call if the cached value is valid. Returning true asks for a successful validation response to still be forwarded to the wrapped handler, so it can update the stored entry.
    * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
    * @param {boolean} allowErrorStatusCodes
    */
@@ -52,6 +61,7 @@ class CacheRevalidationHandler {
 
   onRequestStart (_, context) {
     this.#successful = false
+    this.#storeUpdateOnly = false
     this.#context = context
   }
 
@@ -71,13 +81,15 @@ class CacheRevalidationHandler {
     // https://datatracker.ietf.org/doc/html/rfc5861#section-4
     this.#successful = statusCode === 304 ||
       (this.#allowErrorStatusCodes && statusCode >= 500 && statusCode <= 504)
-    this.#callback(this.#successful, this.#context, statusCode, headers)
+    const forwardForStoreUpdate = this.#callback(this.#successful, this.#context, statusCode, headers) === true
     this.#callback = null
 
-    if (this.#successful) {
+    if (this.#successful && !forwardForStoreUpdate) {
       return true
     }
 
+    this.#storeUpdateOnly = this.#successful
+
     this.#handler.onRequestStart?.(controller, this.#context)
     this.#handler.onResponseStart?.(
       controller,
@@ -88,23 +100,23 @@ class CacheRevalidationHandler {
   }
 
   onResponseData (controller, chunk) {
-    if (this.#successful) {
+    if (this.#successful && !this.#storeUpdateOnly) {
       return
     }
 
     return this.#handler.onResponseData?.(controller, chunk)
   }
 
   onResponseEnd (controller, trailers) {
-    if (this.#successful) {
+    if (this.#successful && !this.#storeUpdateOnly) {
       return
     }
 
     this.#handler.onResponseEnd?.(controller, trailers)
   }
 
   onResponseError (controller, err) {
-    if (this.#successful) {
+    if (this.#successful && !this.#storeUpdateOnly) {
       return
     }
 

lib/interceptor/cache.js

@@ -135,6 +135,31 @@ function revalidationResponseUpdatesCacheControl (headers) {
   return headers['cache-control'] !== undefined
 }
 
+/**
+ * Does the validation response hand the stored entry a new freshness lifetime?
+ * Only then is it worth updating the stored response in place: a 304 that says
+ * no-cache, or that carries no lifetime at all, leaves the entry needing
+ * revalidation on every reuse anyway.
+ *
+ * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses
+ *
+ * @param {import('../../types/header.d.ts').IncomingHttpHeaders} headers
+ * @returns {boolean}
+ */
+function revalidationResponseRefreshesFreshness (headers) {
+  const cacheControl = headers['cache-control']
+  if (!cacheControl) {
+    return false
+  }
+
+  const directives = parseCacheControlHeader(cacheControl)
+  if (directives['no-cache'] !== undefined || directives['no-store'] === true) {
+    return false
+  }
+
+  return directives['max-age'] !== undefined || directives['s-maxage'] !== undefined
+}
+
 function deleteCachedValue (store, cacheKey) {
   try {
     store.delete(cacheKey)?.catch?.(nop)
@@ -450,6 +475,32 @@ function handleResult (
 
     const headers = makeRevalidationHeaders(opts, result)
 
+    // A 304 that freshens the stored entry is forwarded to the CacheHandler so the
+    // entry is updated in place. The client is served the cached value from here
+    // instead, so from that point on nothing the CacheHandler emits downstream may
+    // reach the client.
+    let storeUpdateOnly = false
+    const revalidationTarget = {
+      onRequestStart: (...args) => {
+        if (!storeUpdateOnly) return handler.onRequestStart?.(...args)
+      },
+      onRequestUpgrade: (...args) => {
+        if (!storeUpdateOnly) return handler.onRequestUpgrade?.(...args)
+      },
+      onResponseStart: (...args) => {
+        if (!storeUpdateOnly) return handler.onResponseStart?.(...args)
+      },
+      onResponseData: (...args) => {
+        if (!storeUpdateOnly) return handler.onResponseData?.(...args)
+      },
+      onResponseEnd: (...args) => {
+        if (!storeUpdateOnly) return handler.onResponseEnd?.(...args)
+      },
+      onResponseError: (...args) => {
+        if (!storeUpdateOnly) return handler.onResponseError?.(...args)
+      }
+    }
+
     // We need to revalidate the response
     return dispatch(
       {
@@ -470,6 +521,16 @@ function handleResult (
               }
 
               if (revalidationResponseUpdatesCacheControl(headers)) {
+                if (revalidationResponseRefreshesFreshness(headers)) {
+                  // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses
+                  // Serve the cached value now and let the CacheHandler merge the
+                  // validation response into the stored entry, which is what gives
+                  // the entry its new freshness lifetime.
+                  storeUpdateOnly = true
+                  sendCachedValue(handler, opts, result, age, context, stale)
+                  return true
+                }
+
                 deleteCachedValue(globalOpts.store, cacheKey)
               }
             }
@@ -480,7 +541,7 @@ function handleResult (
             result.body.on('error', nop).destroy()
           }
         },
-        new CacheHandler(globalOpts, cacheKey, handler),
+        new CacheHandler(globalOpts, cacheKey, revalidationTarget),
         withinStaleIfErrorThreshold
       )
     )

test/interceptors/cache.js

@@ -1679,6 +1679,74 @@ describe('Cache Interceptor', () => {
     }
   })
 
+  test('304 synchronous revalidation with a new max-age freshens the stored entry', async () => {
+    // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses
+    // The origin answers the conditional request with a longer max-age, which is
+    // how a CDN extends a cached response without resending it. The stored entry
+    // has to pick that lifetime up, otherwise every later request revalidates
+    // again and the 304 buys nothing.
+    const clock = FakeTimers.install({
+      toFake: ['Date']
+    })
+
+    let requestsToOrigin = 0
+    let conditionalRequests = 0
+    const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
+      requestsToOrigin++
+      res.setHeader('date', new Date().toUTCString())
+      res.setHeader('etag', '"cached"')
+
+      if (req.headers['if-none-match']) {
+        conditionalRequests++
+        res.statusCode = 304
+        res.setHeader('cache-control', 'public, max-age=60')
+        res.end()
+        return
+      }
+
+      res.setHeader('cache-control', 'public, max-age=1')
+      res.end('cached')
+    }).listen(0)
+
+    const client = new Client(`http://localhost:${server.address().port}`)
+      .compose(interceptors.cache())
+
+    after(async () => {
+      server.close()
+      await client.close()
+      clock.uninstall()
+    })
+
+    await once(server, 'listening')
+
+    const request = {
+      origin: 'localhost',
+      method: 'GET',
+      path: '/'
+    }
+
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 1)
+      strictEqual(await res.body.text(), 'cached')
+    }
+
+    clock.tick(1500)
+
+    {
+      const res = await client.request(request)
+      equal(conditionalRequests, 1)
+      strictEqual(await res.body.text(), 'cached')
+    }
+
+    {
+      const res = await client.request(request)
+      equal(requestsToOrigin, 2, 'the max-age carried by the 304 must freshen the stored entry')
+      strictEqual(res.headers.warning, undefined)
+      strictEqual(await res.body.text(), 'cached')
+    }
+  })
+
   test('304 stale-while-revalidate metadata that forbids reuse evicts the cached response', async () => {
     for (const testCase of [
       { name: 'no-store', headers: { 'cache-control': 'no-store' } },