Skip to content

Commit c83e67d

Browse files
Jesse Wrightclaude
andcommitted
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 <noreply@anthropic.com>
1 parent cb4c2f1 commit c83e67d

2 files changed

Lines changed: 211 additions & 5 deletions

File tree

lib/handler/cache-handler.js

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ const NOT_UNDERSTOOD_STATUS_CODES = [
2626

2727
const MAX_RESPONSE_AGE = 2147483647000
2828

29+
// How long revalidation-only entries (zero freshness lifetime but a validator
30+
// present, e.g. `cache-control: no-cache` or `max-age=0` with an etag) are
31+
// retained so there is something to revalidate against. Every successful
32+
// revalidation re-stores the entry, sliding this window.
33+
const REVALIDATION_ONLY_RETENTION = 86400000 // 24 hours
34+
2935
/**
3036
* @typedef {import('../../types/dispatcher.d.ts').default.DispatchHandler} DispatchHandler
3137
*
@@ -150,16 +156,26 @@ class CacheHandler {
150156
? parseHttpDate(resHeaders.date)
151157
: undefined
152158

159+
const hasValidator =
160+
(typeof resHeaders.etag === 'string' && isEtagUsable(resHeaders.etag)) ||
161+
typeof resHeaders['last-modified'] === 'string'
162+
153163
const staleAt =
154-
determineStaleAt(this.#cacheType, now, resAge, resHeaders, resDate, cacheControlDirectives) ??
164+
determineStaleAt(this.#cacheType, now, resAge, resHeaders, resDate, cacheControlDirectives, hasValidator) ??
155165
this.#cacheByDefault
156166
if (staleAt === undefined || (resAge && resAge > staleAt)) {
157167
return downstreamOnHeaders()
158168
}
159169

160170
const baseTime = resDate ? resDate.getTime() : now
161171
const absoluteStaleAt = staleAt + baseTime
162-
if (now >= absoluteStaleAt) {
172+
// Responses with a zero freshness lifetime but a validator (e.g.
173+
// `cache-control: no-cache` or `max-age=0` with an etag) are stale from
174+
// the start, but they're still storable - each reuse is preceded by a
175+
// revalidation request.
176+
// https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4
177+
const revalidationOnly = staleAt === 0 && hasValidator
178+
if (now >= absoluteStaleAt && !revalidationOnly) {
163179
// Response is already stale
164180
return downstreamOnHeaders()
165181
}
@@ -422,23 +438,37 @@ function getAge (ageHeader) {
422438
* @param {import('../../types/header.d.ts').IncomingHttpHeaders} resHeaders
423439
* @param {Date | undefined} responseDate
424440
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
441+
* @param {boolean} hasValidator whether the response has a validator (etag or
442+
* last-modified) that revalidation requests can be made with
425443
*
426444
* @returns {number | undefined} time that the value is stale at in seconds or undefined if it shouldn't be cached
427445
*/
428-
function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheControlDirectives) {
446+
function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheControlDirectives, hasValidator) {
429447
if (cacheType === 'shared') {
430448
// Prioritize s-maxage since we're a shared cache
431449
// s-maxage > max-age > Expire
432450
// https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.10-3
433451
const sMaxAge = cacheControlDirectives['s-maxage']
434452
if (sMaxAge !== undefined) {
435-
return sMaxAge > 0 ? sMaxAge * 1000 : undefined
453+
if (sMaxAge > 0) {
454+
return sMaxAge * 1000
455+
}
456+
457+
// An immediately stale response can still be stored if we can
458+
// revalidate it before reuse
459+
return sMaxAge === 0 && hasValidator ? 0 : undefined
436460
}
437461
}
438462

439463
const maxAge = cacheControlDirectives['max-age']
440464
if (maxAge !== undefined) {
441-
return maxAge > 0 ? maxAge * 1000 : undefined
465+
if (maxAge > 0) {
466+
return maxAge * 1000
467+
}
468+
469+
// An immediately stale response can still be stored if we can revalidate
470+
// it before reuse
471+
return maxAge === 0 && hasValidator ? 0 : undefined
442472
}
443473

444474
if (typeof resHeaders.expires === 'string') {
@@ -482,6 +512,13 @@ function determineStaleAt (cacheType, now, age, resHeaders, responseDate, cacheC
482512
return 31536000000
483513
}
484514

515+
if (cacheControlDirectives['no-cache'] === true && hasValidator) {
516+
// The response has no freshness source, but, since it has a validator,
517+
// it can still be stored and revalidated before each reuse
518+
// https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.4
519+
return 0
520+
}
521+
485522
return undefined
486523
}
487524

@@ -518,6 +555,14 @@ function determineDeleteAt (baseTime, cachedAt, cacheControlDirectives, staleAt)
518555
// revalidated.
519556
if (staleWhileRevalidate === -Infinity && staleIfError === -Infinity && immutable === -Infinity) {
520557
const freshnessLifetime = staleAt - baseTime
558+
if (freshnessLifetime <= 0) {
559+
// Revalidation-only entry (zero freshness lifetime, stored solely so
560+
// reuses can be revalidated with a conditional request): there's no
561+
// freshness lifetime to base the buffer on, so retain it for a bounded
562+
// window instead. Every successful revalidation re-stores the entry,
563+
// sliding the window.
564+
return cachedAt + REVALIDATION_ONLY_RETENTION
565+
}
521566
const datePrecisionPadding = Math.min(Math.max(cachedAt - baseTime, 0), 1000)
522567
return staleAt + freshnessLifetime + datePrecisionPadding
523568
}

test/interceptors/cache.js

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,167 @@ describe('Cache Interceptor', () => {
170170
strictEqual(requestCount, 2)
171171
})
172172

173+
test('stores response with no-cache directive and etag, revalidates it on reuse', async () => {
174+
let requestsToOrigin = 0
175+
let revalidationRequests = 0
176+
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
177+
if (req.headers['if-none-match'] === '"asd123"') {
178+
revalidationRequests++
179+
res.statusCode = 304
180+
res.end()
181+
} else {
182+
requestsToOrigin++
183+
res.setHeader('cache-control', 'no-cache')
184+
res.setHeader('etag', '"asd123"')
185+
res.end('asd')
186+
}
187+
}).listen(0)
188+
189+
const client = new Client(`http://localhost:${server.address().port}`)
190+
.compose(interceptors.cache())
191+
192+
after(async () => {
193+
server.close()
194+
await client.close()
195+
})
196+
197+
await once(server, 'listening')
198+
199+
/**
200+
* @type {import('../../types/dispatcher').default.RequestOptions}
201+
*/
202+
const request = {
203+
origin: 'localhost',
204+
method: 'GET',
205+
path: '/'
206+
}
207+
208+
// Send initial request. This should reach the origin
209+
{
210+
const res = await client.request(request)
211+
strictEqual(await res.body.text(), 'asd')
212+
strictEqual(requestsToOrigin, 1)
213+
strictEqual(revalidationRequests, 0)
214+
}
215+
216+
// Send second request. The response was stored, so this should be a
217+
// revalidation request answered with a 304 and served from the cache
218+
{
219+
const res = await client.request(request)
220+
strictEqual(await res.body.text(), 'asd')
221+
strictEqual(requestsToOrigin, 1)
222+
strictEqual(revalidationRequests, 1)
223+
}
224+
})
225+
226+
test('stores response with max-age=0 and etag, revalidates it on reuse', async () => {
227+
let requestsToOrigin = 0
228+
let revalidationRequests = 0
229+
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
230+
if (req.headers['if-none-match'] === '"asd123"') {
231+
revalidationRequests++
232+
res.statusCode = 304
233+
res.end()
234+
} else {
235+
requestsToOrigin++
236+
res.setHeader('cache-control', 'max-age=0')
237+
res.setHeader('etag', '"asd123"')
238+
res.end('asd')
239+
}
240+
}).listen(0)
241+
242+
const client = new Client(`http://localhost:${server.address().port}`)
243+
.compose(interceptors.cache())
244+
245+
after(async () => {
246+
server.close()
247+
await client.close()
248+
})
249+
250+
await once(server, 'listening')
251+
252+
/**
253+
* @type {import('../../types/dispatcher').default.RequestOptions}
254+
*/
255+
const request = {
256+
origin: 'localhost',
257+
method: 'GET',
258+
path: '/'
259+
}
260+
261+
// Send initial request. This should reach the origin
262+
{
263+
const res = await client.request(request)
264+
strictEqual(await res.body.text(), 'asd')
265+
strictEqual(requestsToOrigin, 1)
266+
strictEqual(revalidationRequests, 0)
267+
}
268+
269+
// Send second request. The response was stored but is already stale, so
270+
// this should be a revalidation request answered with a 304 and served
271+
// from the cache
272+
{
273+
const res = await client.request(request)
274+
strictEqual(await res.body.text(), 'asd')
275+
strictEqual(requestsToOrigin, 1)
276+
strictEqual(revalidationRequests, 1)
277+
}
278+
})
279+
280+
test('stores response with no-cache directive, etag and last-modified, revalidates it on reuse', async () => {
281+
let requestsToOrigin = 0
282+
let revalidationRequests = 0
283+
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
284+
if (req.headers['if-none-match'] === '"asd123"') {
285+
revalidationRequests++
286+
res.statusCode = 304
287+
res.end()
288+
} else {
289+
requestsToOrigin++
290+
res.setHeader('cache-control', 'no-cache')
291+
res.setHeader('etag', '"asd123"')
292+
res.setHeader('last-modified', new Date(Date.now() - 60000).toUTCString())
293+
res.end('asd')
294+
}
295+
}).listen(0)
296+
297+
const client = new Client(`http://localhost:${server.address().port}`)
298+
.compose(interceptors.cache())
299+
300+
after(async () => {
301+
server.close()
302+
await client.close()
303+
})
304+
305+
await once(server, 'listening')
306+
307+
/**
308+
* @type {import('../../types/dispatcher').default.RequestOptions}
309+
*/
310+
const request = {
311+
origin: 'localhost',
312+
method: 'GET',
313+
path: '/'
314+
}
315+
316+
// Send initial request. This should reach the origin
317+
{
318+
const res = await client.request(request)
319+
strictEqual(await res.body.text(), 'asd')
320+
strictEqual(requestsToOrigin, 1)
321+
strictEqual(revalidationRequests, 0)
322+
}
323+
324+
// Send second request. The response was stored, so this should be a
325+
// revalidation request answered with a 304 and served from the cache
326+
{
327+
const res = await client.request(request)
328+
strictEqual(await res.body.text(), 'asd')
329+
strictEqual(requestsToOrigin, 1)
330+
strictEqual(revalidationRequests, 1)
331+
}
332+
})
333+
173334
test('expires caching', async () => {
174335
const clock = FakeTimers.install({
175336
toFake: ['Date']

0 commit comments

Comments
 (0)