From bc8d8e455720d842c97a6313b0b892360ac47dce Mon Sep 17 00:00:00 2001 From: Jesse Wright Date: Sat, 4 Jul 2026 17:11:59 +0000 Subject: [PATCH 1/3] fix: update stored cache entry on synchronous 304 revalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 304 validation response on the synchronous revalidation path was swallowed by CacheRevalidationHandler before the CacheHandler could see it, so the stored entry was never updated per RFC 9111 §4.3.4: headers carried by the 304 (e.g. a new Cache-Control) were discarded and the entry's freshness was never recomputed, causing a revalidation request on every single subsequent use, forever. The served response also kept the obsolete `Warning: 110` header even though it had just been successfully validated. - CacheHandler now handles 304s before the cacheability gates and recomputes freshness from the merged (stored + 304) headers, so bare 304s also refresh the entry; the stored Vary and ETag are preserved. - CacheRevalidationHandler forwards 304s to a dedicated cache-update CacheHandler and passes the validation response's headers to the callback, so the interceptor serves the freshened headers with age 0 and no stale warning. - The 304 body-reuse path no longer byte-iterates Buffer bodies returned by SqliteCacheStore (Buffer.prototype.values() yields single bytes). Un-skips four 304-etag-update-response-* conformance tests; 304-etag-update-response-Cache-Control remains skipped as it needs an entry-retention floor (the max-age=1 entry is deleted before the test's 3s pause; see #4624). Fixes #5505 Co-Authored-By: Claude Fable 5 --- lib/handler/cache-handler.js | 296 +++++++++++++------- lib/handler/cache-revalidation-handler.js | 47 +++- lib/interceptor/cache.js | 44 ++- test/cache-interceptor/cache-tests.mjs | 8 +- test/interceptors/cache-revalidate-stale.js | 159 +++++++++++ 5 files changed, 438 insertions(+), 116 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 6e13f8eb17c..492fed5a066 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -121,6 +121,12 @@ class CacheHandler { return downstreamOnHeaders() } + // Not modified, freshen the stored response and re-use its body + // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses + if (statusCode === 304) { + return this.#handle304(controller, resHeaders, downstreamOnHeaders) + } + const cacheControlHeader = resHeaders['cache-control'] const heuristicallyCacheable = resHeaders['last-modified'] && HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) if ( @@ -191,126 +197,206 @@ class CacheHandler { deleteAt } - // Not modified, re-use the cached value - // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-304-not-modified - if (statusCode === 304) { - const handle304 = (cachedValue) => { - if (!cachedValue) { - // Do not create a new cache entry, as a 304 won't have a body - so cannot be cached. - return downstreamOnHeaders() + if (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) { + value.etag = resHeaders.etag + } + + 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 } - // 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 } + // TODO (fix): Should we resume even if was paused downstream? + controller.resume() + }) - downstreamOnHeaders() + downstreamOnHeaders() + } - this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value) + /** + * Handles a 304 Not Modified validation response by updating the stored + * response with the validation response's header fields and recomputing + * its freshness from the merged headers, per RFC 9111 §4.3.4. + * + * https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses + * + * @param {import('../../types/dispatcher.d.ts').default.DispatchController} controller + * @param {import('../../types/header.d.ts').IncomingHttpHeaders} resHeaders + * @param {() => void} downstreamOnHeaders + */ + #handle304 (controller, resHeaders, downstreamOnHeaders) { + const handler = this - if (!this.#writeStream || !cachedValue?.body) { - return - } + const handle304 = (cachedValue) => { + if (!cachedValue) { + // We have nothing stored to freshen. Do not create a new cache entry, + // as a 304 won't have a body - so it cannot be cached. + return downstreamOnHeaders() + } - if (typeof cachedValue.body.values === 'function') { - 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 - } - } - } + // https://www.rfc-editor.org/rfc/rfc9111.html#section-4.3.4-4 + const headers = mergeValidationHeaders(cachedValue.headers, resHeaders) + const cacheControlDirectives = headers['cache-control'] + ? parseCacheControlHeader(headers['cache-control']) + : {} - 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 cachedValue.body.on === 'function') { - // Readable stream body (e.g. from async/remote cache stores) - cachedValue.body - .on('data', (chunk) => { - this.#writeStream.write(chunk) - this.#handler.onResponseData?.(controller, chunk) - }) - .on('end', () => { - this.#writeStream.end() - }) - .on('error', () => { - this.#writeStream = undefined - this.#store.delete(this.#cacheKey) - }) - - this.#writeStream - .on('error', function () { - handler.#writeStream = undefined - handler.#store.delete(handler.#cacheKey) - }) - .on('close', function () { - if (handler.#writeStream === this) { - handler.#writeStream = undefined - } - }) + if (!canCacheResponse(this.#cacheType, cachedValue.statusCode, headers, cacheControlDirectives, this.#cacheKey.headers)) { + // The freshened response is no longer storable, leave the stored + // entry as it is + return downstreamOnHeaders() + } + + const now = Date.now() + const resAge = resHeaders.age ? getAge(resHeaders.age) : undefined + if (resAge && resAge >= MAX_RESPONSE_AGE) { + return downstreamOnHeaders() + } + + const resDate = typeof resHeaders.date === 'string' + ? parseHttpDate(resHeaders.date) + : undefined + + // Recompute the freshness lifetime from the merged headers, not just + // the ones present on the 304 itself + const staleAt = + determineStaleAt(this.#cacheType, now, resAge, headers, resDate, cacheControlDirectives) ?? + this.#cacheByDefault + if (staleAt === undefined || (resAge && resAge > staleAt)) { + return downstreamOnHeaders() + } + + const baseTime = resDate ? resDate.getTime() : now + const absoluteStaleAt = staleAt + baseTime + if (now >= absoluteStaleAt) { + // Still stale after freshening, leave the stored entry as it is + return downstreamOnHeaders() + } + + let varyDirectives + if (this.#cacheKey.headers && headers.vary) { + varyDirectives = parseVaryHeader(headers.vary, this.#cacheKey.headers) + if (!varyDirectives) { + // Parse error + return downstreamOnHeaders() } } + const cachedAt = resAge ? now - resAge : now + /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} */ - const result = this.#store.get(this.#cacheKey) - if (result && typeof result.then === 'function') { - result.then(handle304) - } else { - handle304(result) + const value = { + statusCode: cachedValue.statusCode, + statusMessage: cachedValue.statusMessage, + headers, + vary: varyDirectives, + cacheControlDirectives, + cachedAt, + staleAt: absoluteStaleAt, + deleteAt: determineDeleteAt(baseTime, cachedAt, cacheControlDirectives, absoluteStaleAt) } - } else { + if (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) { value.etag = resHeaders.etag + } else { + value.etag = cachedValue.etag } + downstreamOnHeaders() + this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value) - if (!this.#writeStream) { - return downstreamOnHeaders() + if (!this.#writeStream || !cachedValue.body) { + return } - 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) { + if (typeof cachedValue.body.on === 'function') { + // Readable stream body (e.g. from async/remote cache stores) + cachedValue.body + .on('data', (chunk) => { + this.#writeStream.write(chunk) + this.#handler.onResponseData?.(controller, chunk) + }) + .on('end', () => { + this.#writeStream.end() + }) + .on('error', () => { + this.#writeStream = undefined + this.#store.delete(this.#cacheKey) + }) + + this.#writeStream + .on('error', function () { handler.#writeStream = undefined + handler.#store.delete(handler.#cacheKey) + }) + .on('close', function () { + if (handler.#writeStream === this) { + handler.#writeStream = undefined + } + }) + } else { + // Iterable of chunks (e.g. MemoryCacheStore) or a single contiguous + // body (e.g. SqliteCacheStore returns a Buffer, whose values() + // would iterate single bytes) + const bodyIterator = typeof cachedValue.body.values === 'function' && !ArrayBuffer.isView(cachedValue.body) + ? cachedValue.body.values() + : [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 + } + }) - downstreamOnHeaders() + streamCachedBody() + } + } + + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} + */ + const result = this.#store.get(this.#cacheKey) + if (result && typeof result.then === 'function') { + result.then(handle304) + } else { + handle304(result) } } @@ -525,6 +611,27 @@ function determineDeleteAt (baseTime, cachedAt, cacheControlDirectives, staleAt) return Math.max(staleAt, staleWhileRevalidate, staleIfError, immutable) } +/** + * Updates stored response headers with the ones of a validation response, + * per RFC 9111 §4.3.4. Content-Length is not carried over since it + * describes the validation response, not the stored body. + * + * https://www.rfc-editor.org/rfc/rfc9111.html#section-4.3.4-4 + * + * @param {Record} cachedHeaders headers of the stored response + * @param {import('../../types/header.d.ts').IncomingHttpHeaders} resHeaders headers of the validation response + * @returns {Record} + */ +function mergeValidationHeaders (cachedHeaders, resHeaders) { + const cacheControlDirectives = resHeaders['cache-control'] + ? parseCacheControlHeader(resHeaders['cache-control']) + : {} + const validationHeaders = { ...stripNecessaryHeaders(resHeaders, cacheControlDirectives) } + delete validationHeaders['content-length'] + + return { ...cachedHeaders, ...validationHeaders } +} + /** * Strips headers required to be removed in cached responses * @param {import('../../types/header.d.ts').IncomingHttpHeaders} resHeaders @@ -584,3 +691,4 @@ function isValidDate (date) { } module.exports = CacheHandler +module.exports.mergeValidationHeaders = mergeValidationHeaders diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 393d16d52c6..4127f114ce3 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -19,7 +19,13 @@ class CacheRevalidationHandler { #successful = false /** - * @type {((boolean, any) => void) | null} + * Whether we're forwarding a 304 response to the cache update handler + * @type {boolean} + */ + #updatingCache = false + + /** + * @type {((success: boolean, context: any, statusCode?: number, headers?: Record) => void) | null} */ #callback @@ -28,6 +34,13 @@ class CacheRevalidationHandler { */ #handler + /** + * Handler that updates the stored response with a 304's headers + * (https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses) + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandler | null} + */ + #cacheUpdateHandler + #context /** @@ -36,11 +49,12 @@ class CacheRevalidationHandler { #allowErrorStatusCodes /** - * @param {(boolean) => void} callback Function to call if the cached value is valid + * @param {(success: boolean, context: any, statusCode?: number, headers?: Record) => void} callback Function to call if the cached value is valid * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler * @param {boolean} allowErrorStatusCodes + * @param {import('../../types/dispatcher.d.ts').default.DispatchHandler} [cacheUpdateHandler] */ - constructor (callback, handler, allowErrorStatusCodes) { + constructor (callback, handler, allowErrorStatusCodes, cacheUpdateHandler = null) { if (typeof callback !== 'function') { throw new TypeError('callback must be a function') } @@ -48,10 +62,12 @@ class CacheRevalidationHandler { this.#callback = callback this.#handler = handler this.#allowErrorStatusCodes = allowErrorStatusCodes + this.#cacheUpdateHandler = cacheUpdateHandler } onRequestStart (_, context) { this.#successful = false + this.#updatingCache = false this.#context = context } @@ -71,10 +87,24 @@ 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) + this.#callback(this.#successful, this.#context, statusCode, headers) this.#callback = null if (this.#successful) { + if (statusCode === 304 && this.#cacheUpdateHandler) { + // Let the cache update handler freshen the stored response with the + // headers of the validation response + // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses + this.#updatingCache = true + this.#cacheUpdateHandler.onRequestStart?.(controller, this.#context) + this.#cacheUpdateHandler.onResponseStart?.( + controller, + statusCode, + headers, + statusMessage + ) + } + return true } @@ -89,6 +119,9 @@ class CacheRevalidationHandler { onResponseData (controller, chunk) { if (this.#successful) { + if (this.#updatingCache) { + this.#cacheUpdateHandler.onResponseData?.(controller, chunk) + } return } @@ -97,6 +130,9 @@ class CacheRevalidationHandler { onResponseEnd (controller, trailers) { if (this.#successful) { + if (this.#updatingCache) { + this.#cacheUpdateHandler.onResponseEnd?.(controller, trailers) + } return } @@ -105,6 +141,9 @@ class CacheRevalidationHandler { onResponseError (controller, err) { if (this.#successful) { + if (this.#updatingCache) { + this.#cacheUpdateHandler.onResponseError?.(controller, err) + } return } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index fe56baa82c9..98512e11b7a 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -28,6 +28,19 @@ function assertCacheOrigins (origins, name) { const nop = () => {} +/** + * Silent handler for responses that only update the cache + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandler} + */ +const noopHandler = { + onRequestStart () {}, + onRequestUpgrade () {}, + onResponseStart () {}, + onResponseData () {}, + onResponseEnd () {}, + onResponseError () {} +} + /** * @typedef {(options: import('../../types/dispatcher.d.ts').default.DispatchOptions, handler: import('../../types/dispatcher.d.ts').default.DispatchHandler) => void} DispatchFn */ @@ -328,15 +341,7 @@ function handleResult ( ...opts, headers }, - new CacheHandler(globalOpts, cacheKey, { - // Silent handler that just updates the cache - onRequestStart () {}, - onRequestUpgrade () {}, - onResponseStart () {}, - onResponseData () {}, - onResponseEnd () {}, - onResponseError () {} - }) + new CacheHandler(globalOpts, cacheKey, noopHandler) ) }) @@ -373,16 +378,29 @@ function handleResult ( headers }, new CacheRevalidationHandler( - (success, context) => { + (success, context, statusCode, resHeaders) => { if (success) { - // TODO: successful revalidation should be considered fresh (not give stale warning). - sendCachedValue(handler, opts, result, age, context, stale) + if (statusCode === 304) { + // The response was successfully validated, serve it freshened + // with the validation response's headers (RFC 9111 §4.3.4). + // It is not stale anymore and its age resets. + const headers = resHeaders + ? CacheHandler.mergeValidationHeaders(result.headers, resHeaders) + : result.headers + sendCachedValue(handler, opts, { ...result, headers }, 0, context, false) + } else { + // stale-if-error, the response is still stale + sendCachedValue(handler, opts, result, age, context, stale) + } } else if (util.isStream(result.body)) { result.body.on('error', nop).destroy() } }, new CacheHandler(globalOpts, cacheKey, handler), - withinStaleIfErrorThreshold + withinStaleIfErrorThreshold, + // Updates the stored response with the 304's headers while the + // cached value is served to the handler + new CacheHandler(globalOpts, cacheKey, noopHandler) ) ) } diff --git a/test/cache-interceptor/cache-tests.mjs b/test/cache-interceptor/cache-tests.mjs index e83c95c5a13..5defb9c7cc2 100644 --- a/test/cache-interceptor/cache-tests.mjs +++ b/test/cache-interceptor/cache-tests.mjs @@ -73,12 +73,10 @@ const BASE_TEST_ENVIRONMENT = { 'cc-resp-no-cache', 'cc-resp-no-cache-case-insensitive', - // We're not caching 304s currently + // Needs an entry-retention floor: the entry (max-age=1) is deleted at + // ~2x its freshness lifetime, before the test's 3s pause elapses, so + // there is nothing left to revalidate (see nodejs/undici#4624) '304-etag-update-response-Cache-Control', - '304-etag-update-response-Content-Foo', - '304-etag-update-response-Test-Header', - '304-etag-update-response-X-Content-Foo', - '304-etag-update-response-X-Test-Header', // We just trim whatever's in the decimal place off (i.e. 7200.0 -> 7200) 'age-parse-float', diff --git a/test/interceptors/cache-revalidate-stale.js b/test/interceptors/cache-revalidate-stale.js index 5abc58898a6..d8a1ff6f4a9 100644 --- a/test/interceptors/cache-revalidate-stale.js +++ b/test/interceptors/cache-revalidate-stale.js @@ -69,6 +69,165 @@ test('revalidates the request when the response is stale', async () => { } }) +test('304 revalidation response updates the stored entry (RFC 9111 §4.3.4)', async () => { + const clock = FakeTimers.install({ + now: 1000 + }) + after(() => clock.uninstall()) + + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.sendDate = false + res.setHeader('Date', new Date(clock.now).toUTCString()) + + if (req.headers['if-none-match'] === '"asd"') { + revalidationRequests++ + // Extend the response's freshness & update a header (RFC 9111 §4.3.4) + res.statusCode = 304 + res.setHeader('Cache-Control', 'public, max-age=60') + res.setHeader('X-Test-Header', 'updated') + res.end() + } else { + requestsToOrigin++ + res.setHeader('Cache-Control', 'public, max-age=1') + res.setHeader('ETag', '"asd"') + res.setHeader('X-Test-Header', 'original') + res.end('hello world') + } + }) + + 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, populates the cache + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 0) + } + + clock.tick(1500) + + // Response is stale, revalidation gets a 304 carrying new headers. The + // served response was just validated: no stale warning, updated headers + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(res.statusCode, 200) + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + strictEqual(res.headers.warning, undefined) + strictEqual(res.headers['x-test-header'], 'updated') + strictEqual(res.headers['cache-control'], 'public, max-age=60') + } + + // Well past the original freshness lifetime but within the extended one, + // the updated entry must be served from cache without revalidating again + clock.tick(30000) + + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(res.statusCode, 200) + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + strictEqual(res.headers.warning, undefined) + strictEqual(res.headers['x-test-header'], 'updated') + } +}) + +test('bare 304 revalidation response refreshes the stored entry', async () => { + const clock = FakeTimers.install({ + now: 1000 + }) + after(() => clock.uninstall()) + + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.sendDate = false + res.setHeader('Date', new Date(clock.now).toUTCString()) + + if (req.headers['if-none-match'] === '"asd"') { + revalidationRequests++ + // 304 without any headers to merge, freshness is recomputed from the + // stored headers + res.statusCode = 304 + res.end() + } else { + requestsToOrigin++ + res.setHeader('Cache-Control', 'public, max-age=1') + res.setHeader('ETag', '"asd"') + res.end('hello world') + } + }) + + 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, populates the cache + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 0) + } + + clock.tick(1500) + + // Response is stale, revalidation gets a bare 304 + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + strictEqual(res.headers.warning, undefined) + } + + // The entry got a new freshness window from the stored max-age, this is + // served from cache instead of revalidating on every request + clock.tick(400) + + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + } + + // Stale again, revalidates again + clock.tick(1500) + + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 2) + } +}) + describe('revalidates the request, handles 304s during stale-while-revalidate', async () => { function isStale (res) { return res.headers.warning === '110 - "response is stale"' From 9877ea16ad9ceca01bf77c65bd05d32883723fac Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 19:52:57 +0000 Subject: [PATCH 2/3] chore: re-trigger CI (pre-existing flaky SWR test on Windows, also fails on main) 'stale-while-revalidate updates cache after background revalidation (receiving new data)' failed on Windows/Node 26 with revalidationRequests 0 == 1. The identical failure (same test, same assertion at test/interceptors/cache.js:1604) occurred on main's own CI on Windows/Node 22 (run 28649055741, 2026-07-03) without this change. The test passes 10/10 locally on this branch and on main, the sibling SWR test with the same flow passed in the failing job, and Windows/Node 24 passed on this same commit. Co-Authored-By: Claude Fable 5 From 24642c2c729960b51e90d279c4b7d1bbd6a45f65 Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:09:25 +0000 Subject: [PATCH 3/3] docs: shorten cache interceptor comments Collapse the multi-line prose comments introduced with the 304 freshening change to single-line "why" comments, matching the terse style of the surrounding cache interceptor. Addresses review feedback on comment verbosity; no logic change. Co-Authored-By: Claude Fable 5 --- lib/handler/cache-handler.js | 14 +++++--------- lib/handler/cache-revalidation-handler.js | 5 ++--- lib/interceptor/cache.js | 8 +++----- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 492fed5a066..12e4fb9b40d 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -245,8 +245,7 @@ class CacheHandler { const handle304 = (cachedValue) => { if (!cachedValue) { - // We have nothing stored to freshen. Do not create a new cache entry, - // as a 304 won't have a body - so it cannot be cached. + // Do not create a new cache entry, as a 304 won't have a body - so cannot be cached. return downstreamOnHeaders() } @@ -257,8 +256,7 @@ class CacheHandler { : {} if (!canCacheResponse(this.#cacheType, cachedValue.statusCode, headers, cacheControlDirectives, this.#cacheKey.headers)) { - // The freshened response is no longer storable, leave the stored - // entry as it is + // The freshened response is no longer storable, leave the stored entry as-is return downstreamOnHeaders() } @@ -272,8 +270,7 @@ class CacheHandler { ? parseHttpDate(resHeaders.date) : undefined - // Recompute the freshness lifetime from the merged headers, not just - // the ones present on the 304 itself + // Recompute freshness from the merged headers, not just those on the 304 const staleAt = determineStaleAt(this.#cacheType, now, resAge, headers, resDate, cacheControlDirectives) ?? this.#cacheByDefault @@ -353,9 +350,8 @@ class CacheHandler { } }) } else { - // Iterable of chunks (e.g. MemoryCacheStore) or a single contiguous - // body (e.g. SqliteCacheStore returns a Buffer, whose values() - // would iterate single bytes) + // Iterable of chunks (e.g. MemoryCacheStore), or a single Buffer + // (e.g. SqliteCacheStore) whose values() would iterate single bytes const bodyIterator = typeof cachedValue.body.values === 'function' && !ArrayBuffer.isView(cachedValue.body) ? cachedValue.body.values() : [cachedValue.body].values() diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 4127f114ce3..d8958382bc0 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -92,9 +92,8 @@ class CacheRevalidationHandler { if (this.#successful) { if (statusCode === 304 && this.#cacheUpdateHandler) { - // Let the cache update handler freshen the stored response with the - // headers of the validation response - // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses + // Let the cache update handler freshen the stored response with the 304's headers + // https://www.rfc-editor.org/rfc/rfc9111.html#name-freshening-stored-responses this.#updatingCache = true this.#cacheUpdateHandler.onRequestStart?.(controller, this.#context) this.#cacheUpdateHandler.onResponseStart?.( diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 98512e11b7a..2d07a0302f7 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -381,9 +381,8 @@ function handleResult ( (success, context, statusCode, resHeaders) => { if (success) { if (statusCode === 304) { - // The response was successfully validated, serve it freshened - // with the validation response's headers (RFC 9111 §4.3.4). - // It is not stale anymore and its age resets. + // Validated: serve it freshened with the 304's headers, no longer + // stale and with age reset (RFC 9111 §4.3.4) const headers = resHeaders ? CacheHandler.mergeValidationHeaders(result.headers, resHeaders) : result.headers @@ -398,8 +397,7 @@ function handleResult ( }, new CacheHandler(globalOpts, cacheKey, handler), withinStaleIfErrorThreshold, - // Updates the stored response with the 304's headers while the - // cached value is served to the handler + // Updates the stored response with the 304's headers as the cached value is served new CacheHandler(globalOpts, cacheKey, noopHandler) ) )