| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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 nodejs#4624, relates to discussion nodejs#4620.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #5515 +/- ##
=======================================
Coverage 93.44% 93.44%
=======================================
Files 110 110
Lines 37328 37363 +35
=======================================
+ Hits 34881 34914 +33
- Misses 2447 2449 +2 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
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 <noreply@anthropic.com>
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This relates to...
Discussion #4620 and PR #4624 by @steunix (this PR supersedes #4624 — full credit to @steunix for identifying the problem and taking the first pass at it; see "Relationship to #4624" below for why a fresh approach was needed).
Rationale
Responses with a validator (ETag or Last-Modified) but zero freshness lifetime — Cache-Control: no-cache or max-age=0 with no other freshness source — are never stored by the cache interceptor: determineStaleAt() returns undefined, so no entry is created, no If-None-Match is ever sent, and every request gets a full 200 from the origin. ETag + no-cache is the canonical "cache, but always validate" pattern for API servers, and RFC 9111 §3 / §5.2.2.4 explicitly allows storing these responses as long as each reuse is revalidated — undici currently gets zero 304 benefit from them.
Repro against main (Node 22), origin serving cache-control: no-cache + etag "..." (no Last-Modified):
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 now covered by a regression test.
Changes
Store such responses with immediate-stale semantics (three coordinated pieces in lib/handler/cache-handler.js):
The read side needs no changes: stored unqualified-no-cache entries are already forced through revalidation on every use by needsRevalidation(), and max-age=0 entries are always stale, so isStale() triggers the same conditional-request flow.
Relationship to #4624
#4624 identified exactly this problem and made determineStaleAt() return 0 for no-cache/max-age=0. Testing that diff against main showed two issues (which is why this is a fresh PR rather than a review comment):
Features
N/A
Bug Fixes
Breaking Changes and Deprecations
N/A
Tests
Sibling PRs from the same RFC 9111 review (each self-contained against main): #5510 (fix/cache-request-max-age-zero, the request-side max-age=0 falsy check) and #5512 (fix/cache-if-modified-since-value, validator sent on revalidation) touch the adjacent request-directive/revalidation logic; also #5511, #5513, #5514.
This change is agent-assisted (Claude Fable 5) working with @jeswr.
Status