cache: 304 revalidation response never updates the stored entry on the synchronous path (RFC 9111 §4.3.4) — entry stays stale forever
Nobody has claimed this yet.
- Dominant language
- JavaScript
- Stars
- 7.7k
- Forks
- 880
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 68
Description
Bug Description
#4617 (fixing #4596) taught CacheHandler to merge 304 responses into the stored entry — but
that code path is only reachable where CacheHandler sees the response directly (the
stale-while-revalidate background refresh). On the synchronous revalidation path,
CacheRevalidationHandler short-circuits on success (statusCode === 304 →
callback(true) → return true, lib/handler/cache-revalidation-handler.js:72-88) and never
forwards anything to the wrapped CacheHandler, so:
- Headers carried by the 304 (notably a new
Cache-Control/Expires/Date) are discarded.
RFC 9111 §4.3.4: the cache MUST update the stored response with the 304's header fields and
recompute freshness. An origin extending freshness via304 + Cache-Control: max-age=60
(standard Varnish/CDN behavior) gets zero benefit: undici revalidates again on the very next
request, every time, forever. - Even a bare 304 leaves the entry stale — repeated per-request conditional traffic instead of
a fresh window. - The revalidated response is served with
Warning: 110 - "response is stale"although it has
just been successfully validated (acknowledged TODO at lib/interceptor/cache.js:378; the
Warning header is also obsolete per RFC 9111 §5.5).
The five 304-etag-update-response-* cache-tests are currently in the conformance skip list
with the comment "We're not caching 304s currently" (test/cache-interceptor/cache-tests.mjs:76-81);
fixing this would un-skip them.
Reproducible By
const { Agent, interceptors, cacheStores, request } = require('undici')
const http = require('node:http')
const { setTimeout: sleep } = require('node:timers/promises')
let hits = 0
const server = http.createServer((req, res) => {
hits++
if (req.headers['if-none-match'] === '"v1"') {
// Extend freshness for a minute — per §4.3.4 the client must apply this
res.writeHead(304, { 'cache-control': 'max-age=60', etag: '"v1"' })
return res.end()
}
res.writeHead(200, { 'cache-control': 'max-age=1', etag: '"v1"' })
res.end('hello')
}).listen(0, async () => {
const origin = `http://localhost:${server.address().port}`
const d = new Agent().compose(interceptors.cache({ store: new cacheStores.MemoryCacheStore() }))
const get = () => request(origin, { dispatcher: d }).then(r => r.body.text())
await get() // hits=1 (stored, fresh 1s)
await sleep(1400)
await get() // hits=2 (revalidate -> 304 max-age=60)
await get() // expected: served fresh from updated entry (hits stays 2)
console.log(hits) // observed: 3 — and keeps growing on every subsequent request
server.close()
})
Observed on undici 8.7.0 (and current main), Node 22.
Expected Behavior
On a 304 validation response the stored entry must be updated per RFC 9111 §4.3.4 (merge 304
headers minus Content-Length, recompute staleAt/deleteAt) — the merge logic from #4617 in
CacheHandler's 304 branch is the right home; the synchronous path needs to route the 304
through it (or the interceptor's revalidation callback needs to write the updated value back to
the store). The Warning: 110 on successfully validated responses should be dropped in the same
change (TODO at interceptor/cache.js:378).
Note: CacheHandler.onResponseStart's cacheability pre-check (cache-handler.js:124-135) returns
early for 304s that carry no cache-control/expires/last-modified, so bare 304s bypass the merge
branch even on the background path — worth folding into the same fix (a 304's freshness should be
recomputed from the merged headers, not gated on the 304's own headers).
Found during an agent-assisted HTTP-caching review for @jeswr; every claim reproduced on undici 8.6.0 (repo) and 8.7.0 (npm) on Node 22.23.1. Fix PR to follow.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with lib/handler/cache-revalidation-handler.js:72-88 and trace how synchronous 304 responses reach lib/handler/cache-handler.js, then inspect the 304 handling and pre-check in lib/handler/cache-handler.js:124-135. Review the Warning handling around lib/interceptor/cache.js:378. Unskip the five 304-etag-update-response-* cases in test/cache-interceptor/cache-tests.mjs and verify that 304 headers refresh the entry without repeated requests or a stale Warning header.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100