fix: 304 not modified reply upon revalidation did not update cache. (#4617)

Author: daan944

lib/handler/cache-handler.js

@@ -17,11 +17,11 @@ const HEURISTICALLY_CACHEABLE_STATUS_CODES = [
 
 // Status codes which semantic is not handled by the cache
 // https://datatracker.ietf.org/doc/html/rfc9111#section-3
-// This list should not grow beyond 206 and 304 unless the RFC is updated
+// This list should not grow beyond 206 unless the RFC is updated
 // by a newer one including more. Please introduce another list if
 // implementing caching of responses with the 'must-understand' directive.
 const NOT_UNDERSTOOD_STATUS_CODES = [
-  206, 304
+  206
 ]
 
 const MAX_RESPONSE_AGE = 2147483647000
@@ -104,6 +104,7 @@ class CacheHandler {
         resHeaders,
         statusMessage
       )
+    const handler = this
 
     if (
       !util.safeHTTPMethods.includes(this.#cacheKey.method) &&
@@ -189,36 +190,92 @@ class CacheHandler {
       deleteAt
     }
 
-    if (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) {
-      value.etag = resHeaders.etag
-    }
+    // Not modified, re-use the cached value
+    // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-304-not-modified
+    if (statusCode === 304) {
+      /**
+       * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue}
+       */
+      const cachedValue = this.#store.get(this.#cacheKey)
+      if (!cachedValue) {
+        // Do not create a new cache entry, as a 304 won't have a body - so cannot be cached.
+        return downstreamOnHeaders()
+      }
 
-    this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value)
-    if (!this.#writeStream) {
-      return downstreamOnHeaders()
-    }
+      // Re-use the cached value: statuscode, statusmessage, headers and body
+      value.statusCode = cachedValue.statusCode
+      value.statusMessage = cachedValue.statusMessage
+      value.etag = cachedValue.etag
+      value.headers = { ...cachedValue.headers, ...strippedHeaders }
 
-    const handler = this
-    this.#writeStream
-      .on('drain', () => controller.resume())
-      .on('error', function () {
-        // TODO (fix): Make error somehow observable?
-        handler.#writeStream = undefined
-
-        // Delete the value in case the cache store is holding onto state from
-        //  the call to createWriteStream
-        handler.#store.delete(handler.#cacheKey)
-      })
-      .on('close', function () {
-        if (handler.#writeStream === this) {
-          handler.#writeStream = undefined
+      downstreamOnHeaders()
+
+      this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value)
+
+      if (!this.#writeStream || !cachedValue?.body) {
+        return
+      }
+
+      const bodyIterator = cachedValue.body.values()
+
+      const streamCachedBody = () => {
+        for (const chunk of bodyIterator) {
+          const full = this.#writeStream.write(chunk) === false
+          this.#handler.onResponseData?.(controller, chunk)
+          // when stream is full stop writing until we get a 'drain' event
+          if (full) {
+            break
+          }
         }
+      }
 
-        // TODO (fix): Should we resume even if was paused downstream?
-        controller.resume()
-      })
+      this.#writeStream
+        .on('error', function () {
+          handler.#writeStream = undefined
+          handler.#store.delete(handler.#cacheKey)
+        })
+        .on('drain', () => {
+          streamCachedBody()
+        })
+        .on('close', function () {
+          if (handler.#writeStream === this) {
+            handler.#writeStream = undefined
+          }
+        })
+
+      streamCachedBody()
+    } else {
+      if (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) {
+        value.etag = resHeaders.etag
+      }
 
-    return downstreamOnHeaders()
+      this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value)
+
+      if (!this.#writeStream) {
+        return downstreamOnHeaders()
+      }
+
+      this.#writeStream
+        .on('drain', () => controller.resume())
+        .on('error', function () {
+          // TODO (fix): Make error somehow observable?
+          handler.#writeStream = undefined
+
+          // Delete the value in case the cache store is holding onto state from
+          //  the call to createWriteStream
+          handler.#store.delete(handler.#cacheKey)
+        })
+        .on('close', function () {
+          if (handler.#writeStream === this) {
+            handler.#writeStream = undefined
+          }
+
+          // TODO (fix): Should we resume even if was paused downstream?
+          controller.resume()
+        })
+
+      downstreamOnHeaders()
+    }
   }
 
   onResponseData (controller, chunk) {

lib/interceptor/cache.js

@@ -18,19 +18,34 @@ const nop = () => {}
 /**
  * @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
  * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives | undefined} cacheControlDirectives
+ * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} opts
  * @returns {boolean}
  */
-function needsRevalidation (result, cacheControlDirectives) {
+function needsRevalidation (result, cacheControlDirectives, { headers = {} }) {
+  // Always revalidate requests with the no-cache request directive.
   if (cacheControlDirectives?.['no-cache']) {
-    // Always revalidate requests with the no-cache request directive
     return true
   }
 
+  // Always revalidate requests with unqualified no-cache response directive.
   if (result.cacheControlDirectives?.['no-cache'] && !Array.isArray(result.cacheControlDirectives['no-cache'])) {
-    // Always revalidate requests with unqualified no-cache response directive
     return true
   }
 
+  // Always revalidate requests with conditional headers.
+  if (headers['if-modified-since'] || headers['if-none-match']) {
+    return true
+  }
+
+  return false
+}
+
+/**
+ * @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
+ * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives | undefined} cacheControlDirectives
+ * @returns {boolean}
+ */
+function isStale (result, cacheControlDirectives) {
   const now = Date.now()
   if (now > result.staleAt) {
     // Response is stale
@@ -241,17 +256,20 @@ function handleResult (
     return dispatch(opts, handler)
   }
 
+  const stale = isStale(result, reqCacheControl)
+  const revalidate = needsRevalidation(result, reqCacheControl, opts)
+
   // Check if the response is stale
-  if (needsRevalidation(result, reqCacheControl)) {
+  if (stale || revalidate) {
     if (util.isStream(opts.body) && util.bodyLength(opts.body) !== 0) {
       // If body is a stream we can't revalidate...
       // TODO (fix): This could be less strict...
       return dispatch(opts, new CacheHandler(globalOpts, cacheKey, handler))
     }
 
     // RFC 5861: If we're within stale-while-revalidate window, serve stale immediately
-    // and revalidate in background
-    if (withinStaleWhileRevalidateWindow(result)) {
+    // and revalidate in background, unless immediate revalidation is necessary
+    if (!revalidate && withinStaleWhileRevalidateWindow(result)) {
       // Serve stale response immediately
       sendCachedValue(handler, opts, result, age, null, true)
 
@@ -325,7 +343,8 @@ function handleResult (
       new CacheRevalidationHandler(
         (success, context) => {
           if (success) {
-            sendCachedValue(handler, opts, result, age, context, true)
+            // TODO: successful revalidation should be considered fresh (not give stale warning).
+            sendCachedValue(handler, opts, result, age, context, stale)
           } else if (util.isStream(result.body)) {
             result.body.on('error', nop).destroy()
           }

test/interceptors/cache-revalidate-stale.js

@@ -1,11 +1,12 @@
 'use strict'
 
-const { test, after } = require('node:test')
+const { test, after, describe } = require('node:test')
 const { strictEqual } = require('node:assert')
 const { createServer } = require('node:http')
 const { once } = require('node:events')
 const { request, Client, interceptors } = require('../../index')
 const FakeTimers = require('@sinonjs/fake-timers')
+const { setTimeout } = require('node:timers/promises')
 
 test('revalidates the request when the response is stale', async () => {
   const clock = FakeTimers.install({
@@ -67,3 +68,92 @@ test('revalidates the request when the response is stale', async () => {
     strictEqual(await res.body.text(), 'hello world 2')
   }
 })
+
+describe('revalidates the request, handles 304s during stale-while-revalidate', async () => {
+  function isStale (res) {
+    return res.headers.warning === '110 - "response is stale"'
+  }
+
+  async function revalidateTest (useEtag = false) {
+    const clock = FakeTimers.install({
+      now: 1
+    })
+    after(() => clock.uninstall())
+
+    let count200 = 0
+    let count304 = 0
+
+    const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
+      res.sendDate = false
+      res.setHeader('Date', new Date(clock.now).toUTCString())
+      res.setHeader('Cache-Control', 'public, max-age=10, stale-while-revalidate=3600')
+      if (useEtag) {
+        res.setHeader('ETag', '"xxx"')
+      }
+
+      // revalidation response.
+      if (req.headers['if-none-match'] || req.headers['if-modified-since']) {
+        count304++
+        res.statusCode = 304
+        res.end()
+      } else {
+        res.end('hello world ' + count200++)
+      }
+    })
+
+    server.listen(0)
+    await once(server, 'listening')
+
+    const dispatcher = new Client(`http://localhost:${server.address().port}`)
+      .compose(interceptors.cache())
+
+    after(async () => {
+      server.close()
+      await dispatcher.close()
+    })
+
+    const url = `http://localhost:${server.address().port}`
+
+    // initial request, cache the response
+    {
+      const res = await request(url, { dispatcher })
+      strictEqual(await res.body.text(), 'hello world 0')
+      strictEqual(isStale(res), false)
+      strictEqual(res.statusCode, 200)
+    }
+
+    // wait nearly a second, still fresh
+    {
+      clock.tick(900)
+      const res = await request(url, { dispatcher })
+      strictEqual(await res.body.text(), 'hello world 0')
+      strictEqual(isStale(res), false)
+      strictEqual(res.statusCode, 200)
+    }
+
+    // within stale-while-revalidate window, still return stale cached response, revalidate in background
+    {
+      clock.tick(12000)
+      const res = await request(url, { dispatcher })
+      strictEqual(await res.body.text(), 'hello world 0')
+      strictEqual(isStale(res), true)
+      strictEqual(res.statusCode, 200)
+      await setTimeout(100) // wait for revalidation to be complete.
+    }
+
+    // should get revalidated content, not stale.
+    {
+      clock.tick(10)
+      const res = await request(url, { dispatcher })
+      strictEqual(await res.body.text(), 'hello world 0')
+      strictEqual(isStale(res), false)
+      strictEqual(res.statusCode, 200)
+    }
+
+    strictEqual(count200, 1)
+    strictEqual(count304, 1)
+  }
+
+  test('using if-none-match', async () => await revalidateTest(true))
+  test('using if-modified-since', async () => await revalidateTest(false))
+})