Skip to content

fix: enforce must-revalidate and proxy-revalidate over max-stale and stale-if-error#5511

Open
jeswr wants to merge 3 commits into
nodejs:mainfrom
jeswr:fix/cache-must-revalidate-max-stale
Open

fix: enforce must-revalidate and proxy-revalidate over max-stale and stale-if-error#5511
jeswr wants to merge 3 commits into
nodejs:mainfrom
jeswr:fix/cache-must-revalidate-max-stale

Conversation

@jeswr

@jeswr jeswr commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

What

The cache interceptor's isStale() granted the request max-stale grace window without consulting the stored response's directives, and handleResult() applied the stale-if-error threshold unconditionally.

RFC 9111 §5.2.2.2: a cache MUST NOT use a stale response with must-revalidate to satisfy any request without successful validation — this overrides the request's max-stale (see the note in §5.2.1.2) and also excludes the response from stale-if-error (RFC 5861 §4). proxy-revalidate has the same semantics for shared caches — the interceptor's default type — and was parsed but never enforced anywhere.

Fixes #5508

Repro

Store 200 Cache-Control: max-age=1, must-revalidate (+ ETag), wait past staleness, then request with Cache-Control: max-stale=600:

  • Before (undici 8.7.0 / current main, Node 22): served straight from cache, zero origin contact.
  • After: conditional request (if-none-match/if-modified-since) sent to the origin; the cached response is only reused after successful validation.

Same for stale-if-error: a stale must-revalidate response was previously served when revalidation returned a 5xx within the stale-if-error window; now the error is propagated.

Changes

  • lib/interceptor/cache.js: new forbidsServingStale(result, cacheType) helper; isStale() skips the max-stale grace window and handleResult() skips the stale-if-error threshold when the stored response has must-revalidate (or proxy-revalidate on a shared cache). proxy-revalidate correctly remains inert for type: 'private' caches.
  • test/interceptors/cache.js: three new tests (must-revalidate vs max-stale, proxy-revalidate shared vs private, must-revalidate excluded from stale-if-error), all failing before the fix and passing after.

Testing

  • New tests fail on main (stale served with 0 revalidation requests / 200 instead of 500) and pass with the fix.
  • Full cache interceptor suite (test/interceptors/cache*.js, test/cache-interceptor/*.js): 124 tests, 124 pass, 0 fail (121 baseline + 3 new).
  • mnot cache-tests conformance runner (node test/cache-interceptor/cache-tests.mjs): identical to main's baseline — 283 total, 220 passed, 0 failed, 58 failed-optional, 5 setup-failed. (The suite has no test for the max-stale + must-revalidate combination, so no skip-list entries could be un-skipped; the skipped stale-close-must-revalidate test depends on the connection-close revalidation path, which is out of scope here.)

Notes

🤖 Generated with Claude Code

…stale-if-error

RFC 9111 §5.2.2.2: a cache MUST NOT use a stale response with
must-revalidate to satisfy any request without successful validation —
this takes precedence over the request's max-stale grace (§5.2.1.2) and
excludes the response from stale-if-error (RFC 5861 §4).
proxy-revalidate (§5.2.2.8) has the same semantics for shared caches
(the interceptor's default type) and was parsed but never enforced.

Before: store `200 Cache-Control: max-age=1, must-revalidate` (+ ETag),
wait past staleness, request with `Cache-Control: max-stale=600` — the
stale response is served straight from cache with zero origin contact.
After: a conditional request is sent to the origin and the cached
response is only reused on successful validation.

isStale() now skips the max-stale grace window and handleResult()
skips the stale-if-error threshold when the stored response carries
must-revalidate (or proxy-revalidate on a shared cache).

Fixes nodejs#5508

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 4, 2026 16:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov-commenter

codecov-commenter commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.44%. Comparing base (cb4c2f1) to head (c391280).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5511   +/-   ##
=======================================
  Coverage   93.44%   93.44%           
=======================================
  Files         110      110           
  Lines       37328    37345   +17     
=======================================
+ Hits        34881    34898   +17     
  Misses       2447     2447           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jeswr and others added 2 commits July 4, 2026 21:16
'Should end h2 zero-length request bodies with headers' failed with
ERR_OSSL_ASN1_ILLEGAL_PADDING from a runtime-generated self-signed cert
(@metcoder95/https-pem). The plain Node 26/ubuntu job passed on the same
commit and the test passes locally on this branch and on main; this PR
only touches lib/interceptor/cache.js.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses review feedback by tightening the forbidsServingStale JSDoc to
match undici's terse comment style; RFC references retained.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jeswr jeswr force-pushed the fix/cache-must-revalidate-max-stale branch from 5d8f3ba to c391280 Compare July 4, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cache: response "must-revalidate" ignored when request has "max-stale" — stale served without validation (RFC 9111 §5.2.2.2)

3 participants