diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 6e13f8eb17c..12e4fb9b40d 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,202 @@ 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) { + // Do not create a new cache entry, as a 304 won't have a body - so 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-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 freshness from the merged headers, not just those on the 304 + 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 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() + + 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 +607,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 +687,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..d8958382bc0 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,23 @@ 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 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?.( + controller, + statusCode, + headers, + statusMessage + ) + } + return true } @@ -89,6 +118,9 @@ class CacheRevalidationHandler { onResponseData (controller, chunk) { if (this.#successful) { + if (this.#updatingCache) { + this.#cacheUpdateHandler.onResponseData?.(controller, chunk) + } return } @@ -97,6 +129,9 @@ class CacheRevalidationHandler { onResponseEnd (controller, trailers) { if (this.#successful) { + if (this.#updatingCache) { + this.#cacheUpdateHandler.onResponseEnd?.(controller, trailers) + } return } @@ -105,6 +140,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..2d07a0302f7 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,27 @@ 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) { + // 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 + 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 as the cached value is served + 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"'