From c83e67d0fe0661410c95cc784fbe61fcb0ff176e Mon Sep 17 00:00:00 2001 From: Jesse Wright Date: Sat, 4 Jul 2026 17:02:46 +0000 Subject: [PATCH 1/2] fix: store revalidation-only responses so etag revalidation can engage Responses with a validator (ETag or Last-Modified) but zero freshness lifetime - `cache-control: no-cache` or `max-age=0` without any other freshness source - were never stored: determineStaleAt() returned undefined, so the conditional-request flow never engaged and every request was answered by a full 200 from the origin. This is the canonical "cache but always validate" pattern for API servers, and RFC 9111 (sections 3 and 5.2.2.4) explicitly allows storing these responses as long as each reuse is revalidated. Repro (undici main, Node 22): origin serving `cache-control: no-cache` + `etag` (no Last-Modified): before: 2 requests -> 2 full 200s, no if-none-match sent after: 2 requests -> 1 full 200 + 1 conditional revalidation answered with a 304, cached body reused Same for `cache-control: max-age=0` + `etag`. The already-working no-cache + Last-Modified case (stored via the heuristic-freshness branch) is unchanged and covered by a new regression test. The fix stores such responses with immediate-stale semantics: - determineStaleAt() returns 0 (instead of undefined) for max-age=0 / s-maxage=0 / unqualified no-cache when the response has a usable validator; - the "response is already stale" rejection in onResponseStart() is relaxed for these revalidation-only entries; - determineDeleteAt() retains them for a bounded 24h window (the usual buffer is proportional to the freshness lifetime, which is zero here); every successful revalidation re-stores the entry, sliding the window. The read side needs no changes: stored no-cache entries are already forced through revalidation by needsRevalidation(), and max-age=0 entries are always stale, so isStale() triggers the same conditional flow. Also makes the previously failing (optional) mnot cache-tests conformance test cc-resp-no-cache-revalidate pass in all environments (220 -> 221 passed, 58 -> 57 failed-optional, 0 required failures). Supersedes #4624, relates to discussion #4620. Co-Authored-By: Claude Fable 5 --- lib/handler/cache-handler.js | 55 ++++++++++-- test/interceptors/cache.js | 161 +++++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+), 5 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 6e13f8eb17c..3f0824bbbb6 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -26,6 +26,12 @@ const NOT_UNDERSTOOD_STATUS_CODES = [ const MAX_RESPONSE_AGE = 2147483647000 +// How long revalidation-only entries (zero freshness lifetime but a validator +// present, e.g. `cache-control: no-cache` or `max-age=0` with an etag) are +// retained so there is something to revalidate against. Every successful +// revalidation re-stores the entry, sliding this window. +const REVALIDATION_ONLY_RETENTION = 86400000 // 24 hours + /** * @typedef {import('../../types/dispatcher.d.ts').default.DispatchHandler} DispatchHandler * @@ -150,8 +156,12 @@ class CacheHandler { ? parseHttpDate(resHeaders.date) : undefined + const hasValidator = + (typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) || + typeof resHeaders['last-modified'] === 'string' + const staleAt = - determineStaleAt(this.#cacheType, now, resAge, resHeaders, resDate, cacheControlDirectives) ?? + determineStaleAt(this.#cacheType, now, resAge, resHeaders, resDate, cacheControlDirectives, hasValidator) ?? this.#cacheByDefault if (staleAt === undefined || (resAge && resAge > staleAt)) { return downstreamOnHeaders() @@ -159,7 +169,13 @@ class CacheHandler { const baseTime = resDate ? resDate.getTime() : now const absoluteStaleAt = staleAt + baseTime - if (now >= absoluteStaleAt) { + // Responses with a zero freshness lifetime but a validator (e.g. + // `cache-control: no-cache` or `max-age=0` with an etag) are stale from + // the start, but they're still storable - each reuse is preceded by a + // revalidation request. + // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4 + const revalidationOnly = staleAt === 0 && hasValidator + if (now >= absoluteStaleAt && !revalidationOnly) { // Response is already stale return downstreamOnHeaders() } @@ -422,23 +438,37 @@ function getAge (ageHeader) { * @param {import('../../types/header.d.ts').IncomingHttpHeaders} resHeaders * @param {Date | undefined} responseDate * @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives + * @param {boolean} hasValidator whether the response has a validator (etag or + * last-modified) that revalidation requests can be made with * * @returns {number | undefined} time that the value is stale at in seconds or undefined if it shouldn't be cached */ -function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheControlDirectives) { +function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheControlDirectives, hasValidator) { if (cacheType === 'shared') { // Prioritize s-maxage since we're a shared cache // s-maxage > max-age > Expire // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.10-3 const sMaxAge = cacheControlDirectives['s-maxage'] if (sMaxAge !== undefined) { - return sMaxAge > 0 ? sMaxAge * 1000 : undefined + if (sMaxAge > 0) { + return sMaxAge * 1000 + } + + // An immediately stale response can still be stored if we can + // revalidate it before reuse + return sMaxAge === 0 && hasValidator ? 0 : undefined } } const maxAge = cacheControlDirectives['max-age'] if (maxAge !== undefined) { - return maxAge > 0 ? maxAge * 1000 : undefined + if (maxAge > 0) { + return maxAge * 1000 + } + + // An immediately stale response can still be stored if we can revalidate + // it before reuse + return maxAge === 0 && hasValidator ? 0 : undefined } if (typeof resHeaders.expires === 'string') { @@ -482,6 +512,13 @@ function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheC return 31536000000 } + if (cacheControlDirectives['no-cache'] === true && hasValidator) { + // The response has no freshness source, but, since it has a validator, + // it can still be stored and revalidated before each reuse + // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4 + return 0 + } + return undefined } @@ -518,6 +555,14 @@ function determineDeleteAt (baseTime, cachedAt, cacheControlDirectives, staleAt) // revalidated. if (staleWhileRevalidate === -Infinity && staleIfError === -Infinity && immutable === -Infinity) { const freshnessLifetime = staleAt - baseTime + if (freshnessLifetime <= 0) { + // Revalidation-only entry (zero freshness lifetime, stored solely so + // reuses can be revalidated with a conditional request): there's no + // freshness lifetime to base the buffer on, so retain it for a bounded + // window instead. Every successful revalidation re-stores the entry, + // sliding the window. + return cachedAt + REVALIDATION_ONLY_RETENTION + } const datePrecisionPadding = Math.min(Math.max(cachedAt - baseTime, 0), 1000) return staleAt + freshnessLifetime + datePrecisionPadding } diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index e31b1e307d0..28891655353 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -170,6 +170,167 @@ describe('Cache Interceptor', () => { strictEqual(requestCount, 2) }) + test('stores response with no-cache directive and etag, revalidates it on reuse', async () => { + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + if (req.headers['if-none-match'] === '"asd123"') { + revalidationRequests++ + res.statusCode = 304 + res.end() + } else { + requestsToOrigin++ + res.setHeader('cache-control', 'no-cache') + res.setHeader('etag', '"asd123"') + res.end('asd') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 0) + } + + // Send second request. The response was stored, so this should be a + // revalidation request answered with a 304 and served from the cache + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + } + }) + + test('stores response with max-age=0 and etag, revalidates it on reuse', async () => { + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + if (req.headers['if-none-match'] === '"asd123"') { + revalidationRequests++ + res.statusCode = 304 + res.end() + } else { + requestsToOrigin++ + res.setHeader('cache-control', 'max-age=0') + res.setHeader('etag', '"asd123"') + res.end('asd') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 0) + } + + // Send second request. The response was stored but is already stale, so + // this should be a revalidation request answered with a 304 and served + // from the cache + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + } + }) + + test('stores response with no-cache directive, etag and last-modified, revalidates it on reuse', async () => { + let requestsToOrigin = 0 + let revalidationRequests = 0 + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + if (req.headers['if-none-match'] === '"asd123"') { + revalidationRequests++ + res.statusCode = 304 + res.end() + } else { + requestsToOrigin++ + res.setHeader('cache-control', 'no-cache') + res.setHeader('etag', '"asd123"') + res.setHeader('last-modified', new Date(Date.now() - 60000).toUTCString()) + res.end('asd') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + + /** + * @type {import('../../types/dispatcher').default.RequestOptions} + */ + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 0) + } + + // Send second request. The response was stored, so this should be a + // revalidation request answered with a 304 and served from the cache + { + const res = await client.request(request) + strictEqual(await res.body.text(), 'asd') + strictEqual(requestsToOrigin, 1) + strictEqual(revalidationRequests, 1) + } + }) + test('expires caching', async () => { const clock = FakeTimers.install({ toFake: ['Date'] From 038c4f6d3d2821c252b988114af8d56d6cb20565 Mon Sep 17 00:00:00 2001 From: Jesse Wright <63333554+jeswr@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:08:18 +0000 Subject: [PATCH 2/2] docs: shorten cache interceptor comments Collapse the multi-sentence comments introduced by this PR to the terse, single-line style used elsewhere in lib/interceptor, keeping the RFC 9111 references. Addresses proactive review feedback on comment terseness. No logic change. Co-Authored-By: Claude Fable 5 --- lib/handler/cache-handler.js | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 3f0824bbbb6..d4bbd86d51b 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -26,10 +26,8 @@ const NOT_UNDERSTOOD_STATUS_CODES = [ const MAX_RESPONSE_AGE = 2147483647000 -// How long revalidation-only entries (zero freshness lifetime but a validator -// present, e.g. `cache-control: no-cache` or `max-age=0` with an etag) are -// retained so there is something to revalidate against. Every successful -// revalidation re-stores the entry, sliding this window. +// Retention for revalidation-only entries (zero freshness lifetime but a +// validator present); each successful revalidation re-stores the entry. const REVALIDATION_ONLY_RETENTION = 86400000 // 24 hours /** @@ -169,10 +167,8 @@ class CacheHandler { const baseTime = resDate ? resDate.getTime() : now const absoluteStaleAt = staleAt + baseTime - // Responses with a zero freshness lifetime but a validator (e.g. - // `cache-control: no-cache` or `max-age=0` with an etag) are stale from - // the start, but they're still storable - each reuse is preceded by a - // revalidation request. + // Zero freshness lifetime but a validator: stale from the start, yet still + // storable since each reuse is preceded by a revalidation request. // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4 const revalidationOnly = staleAt === 0 && hasValidator if (now >= absoluteStaleAt && !revalidationOnly) { @@ -454,8 +450,7 @@ function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheC return sMaxAge * 1000 } - // An immediately stale response can still be stored if we can - // revalidate it before reuse + // Immediately stale, but storable if we can revalidate it before reuse. return sMaxAge === 0 && hasValidator ? 0 : undefined } } @@ -466,8 +461,7 @@ function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheC return maxAge * 1000 } - // An immediately stale response can still be stored if we can revalidate - // it before reuse + // Immediately stale, but storable if we can revalidate it before reuse. return maxAge === 0 && hasValidator ? 0 : undefined } @@ -513,8 +507,7 @@ function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheC } if (cacheControlDirectives['no-cache'] === true && hasValidator) { - // The response has no freshness source, but, since it has a validator, - // it can still be stored and revalidated before each reuse + // No freshness source, but a validator lets us revalidate before reuse. // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4 return 0 } @@ -556,11 +549,8 @@ function determineDeleteAt (baseTime, cachedAt, cacheControlDirectives, staleAt) if (staleWhileRevalidate === -Infinity && staleIfError === -Infinity && immutable === -Infinity) { const freshnessLifetime = staleAt - baseTime if (freshnessLifetime <= 0) { - // Revalidation-only entry (zero freshness lifetime, stored solely so - // reuses can be revalidated with a conditional request): there's no - // freshness lifetime to base the buffer on, so retain it for a bounded - // window instead. Every successful revalidation re-stores the entry, - // sliding the window. + // Revalidation-only entry: no freshness lifetime to size the buffer on, + // so retain it for a bounded window instead. return cachedAt + REVALIDATION_ONLY_RETENTION } const datePrecisionPadding = Math.min(Math.max(cachedAt - baseTime, 0), 1000)