cache: fix excessive caching and some other lack of caching #4335

fredericDelaporte · 2025-07-14T11:38:11Z

307 responses must not be cached if they do not have an explicit caching directive.

Final responses with an explicit caching directive can be cached even if their status code is not heuristically cacheable status codes.

206 and 304 semantics are not handled by the cache and as such must not be cached.

This relates to...

fix #4334.

Changes

Cease caching 307 responses without caching directive.
Cache responses with explicit caching directive regardless of their status being heuristically cacheable.
Make sure 206 and 304 responses are not cached.

Features

N/A

Bug Fixes

307 Temporary Redirect without explicit caching directive should not be cached.
206 Partial Content can be cached since cache: allow caching heuristically cacheable error status codes #4318 but must not, because the cached response may be yielded to requests with different ranges.

Breaking Changes and Deprecations

N/A

Status

I have read and agreed to the Developer's Certificate of Origin
Tested
[S] Benchmarked (optional)
[S] Documented
Review ready
In review
Merge ready

307 responses must not be cached if they do not have an explicit caching directive. Final responses with an explicit caching directive can be cached even if their status code is not heuristically cacheable status codes. fix part of nodejs#4334

fredericDelaporte · 2025-07-14T11:43:07Z

lib/handler/cache-handler.js

-  // Allow caching for status codes 200 and 307 (original behavior)
-  // Also allow caching for other status codes that are heuristically cacheable
-  // when they have explicit cache directives
-  if (statusCode !== 200 && statusCode !== 307 && !HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode)) {


Having a heuristically cacheable status code matters only for responses without an explicit caching directive. It is not a requirement for responses with an explicit caching directive, see Storing Responses in Caches.

This previous code also allows to cache 307 responses without an explicit caching directive if the cacheByDefault option is used. But 307 is not heuristically cacheable, and so, it should not be cached if its response lacks explicit caching directive.

fredericDelaporte · 2025-07-14T11:44:25Z

test/interceptors/cache.js

@@ -1453,7 +1456,7 @@ describe('Cache Interceptor', () => {
        res.end(body)
      }).listen(0)

-      const client = new Client(`http://localhost:${server.address().port}`)
+      const client = new Client(`http://localhost:${server.address().port}`, { maxRedirections: 0 })


Since the test includes redirect responses (furthermore, invalid ones), explicitly disable following redirections.

Max redirections is not supported anymore, you'd require to use the interceptor instead.

If I understand it well, it means that redirection are not followed by default, an interceptor has to be added for that. Fine for the test then.

I have removed this option.

fredericDelaporte · 2025-07-14T11:46:02Z

test/interceptors/cache.js

@@ -1489,47 +1492,55 @@ describe('Cache Interceptor', () => {
    })
  }

-  test('does not cache non-heuristically cacheable error status codes', async () => {


The RFC allows to cache non-heuristically cacheable error responses if they have an explicit caching directive.

So, I change this test to check we do not cache non-heuristically cacheable response not having a caching directive.

finish fixing nodejs#4334

fredericDelaporte · 2025-07-14T13:13:32Z

lib/handler/cache-handler.js

+// https://datatracker.ietf.org/doc/html/rfc9111#section-3
+// This list should not grow beyond 206 and 304 unless the RFC is updated
+// by a newer one including more. Please introduce another list if
+// implementing caching of responses with the 'must-understand' directive.


Currently responses with the must-understand directive may be cached without further checks by the current implementation, if they do not follow the RFC recommendation which states they should also includes a no-store directive (that has to be ignored by caches supporting must-understand).

So, I think the current implementation is good enough for now, about that directive. (The current implementation appears to me as just reading this directive and flagging it, but not doing anything of it beyond that.)

fredericDelaporte · 2025-07-14T13:14:32Z

test/interceptors/cache.js

+    })
+  }
+
+  test('discriminates caching of range requests, or does not cache them', async () => {


Prior the second commit of this PR, this new test was failing.

fredericDelaporte · 2025-07-14T13:14:56Z

test/interceptors/cache.js

+    }
+  })
+
+  test('discriminates caching of conditionnal requests, or does not cache them', async () => {


Prior the second commit of this PR but after the first one, this new test was failing too.

(Prior the first one, it was not, since non heuristically cacheable statuses were not cached, and 304 is not heuristically cacheable.)

fredericDelaporte · 2025-07-14T13:16:03Z

test/interceptors/cache.js

+    }
+
+    // Third request with different since should hit the origin too
+    request.headers['if-modified-since'] = new Date(0).toUTCString()


I have checked this third request case before the fix by moving it as the second request, and it was failing too.

mcollina

lgtm

mcollina · 2025-07-15T13:24:13Z

@fredericDelaporte could you check if there is any docs that needs updating?

fredericDelaporte · 2025-07-15T15:06:53Z

I have put a "skipped" in the "Documented" item because I considered no documentation update was required. (Nothing documented about cache has changed, and the change is bug fixes and a slight improvement. I have also checked no mention of 307 being specifically cached were there.)

Maybe I should have instead ticked it for meaning I have done these checks?

test/interceptors/cache.js

metcoder95

lgtm, but CI seems red for cache

fredericDelaporte · 2025-07-16T18:58:29Z

lib/handler/cache-handler.js

+  }
+  // Responses with neither status codes that are heuristically cacheable, nor explicit caching directives,
+  // are not cacheable.
+  if (!HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) && !cacheControlDirectives && !resHeaders['expires']) {


cacheControlDirectives is never undefined or null but just { } if there is no directive. So, that is a bug here.

But I have not seen yet why the tests still succeed on my setup.

Edit: On my setup the test response has a date which causes the cacheByDefault directive to be ignored for determining the freshness.

fredericDelaporte · 2025-07-16T19:52:43Z

lib/handler/cache-handler.js

+  // Responses with neither status codes that are heuristically cacheable, nor "explicit enough" caching
+  // directives, are not cacheable. "Explicit enough": see https://www.rfc-editor.org/rfc/rfc9111.html#section-3
+  if (!HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) && !resHeaders['expires'] &&
+    !cacheControlDirectives.public &&
+    cacheControlDirectives['max-age'] === undefined &&
+    // RFC 9111: a private response directive, if the cache is not shared
+    !(cacheControlDirectives.private && cacheType === 'private') &&
+    !(cacheControlDirectives['s-maxage'] !== undefined && cacheType === 'shared')
+  ) {


Simply testing there is a cache-control directive is not enough. It is approximate, and thinking about it again, it may allow to cache cases that should not.

But the complete test is not straightforward.

Especially I thought writing !(cacheControlDirectives.private === true && cacheType === 'private') instead of !(cacheControlDirectives.private && cacheType === 'private'), because a qualified private directive allow caching by shared caches if they strip the headers listed by private.

But the RFC wording, "a private response directive, if the cache is not shared", can be understood as private directive, qualified or not, should allow caching (in the absence of the other alternatives) only for private caches.

fredericDelaporte · 2025-07-16T20:08:16Z

lib/handler/cache-handler.js

+  if (cacheByDefault) {
+    // When cacheByDefault is truthy, the cache interceptor does no more mandate an explicit expiry prior to
+    // reaching here. We should then check the full condition allowing to consider caching responses.
+    // Responses with neither status codes that are heuristically cacheable, nor "explicit enough" caching
+    // directives, are not cacheable. "Explicit enough": see https://www.rfc-editor.org/rfc/rfc9111.html#section-3
+    if (!HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) && !resHeaders['expires'] &&
+      !cacheControlDirectives.public &&
+      cacheControlDirectives['max-age'] === undefined &&
+      // RFC 9111: a private response directive, if the cache is not shared
+      !(cacheControlDirectives.private && cacheType === 'private') &&
+      !(cacheControlDirectives['s-maxage'] !== undefined && cacheType === 'shared')
+    ) {
+      return false
+    }
+  }


When I considered the subject, I was not expecting it to turn that way because I was not aware of the cacheByDefault option.

Do to the code preceding the call to canCacheResponse, which actually does already some cacheability checks, we need that big test only when cacheByDefault is truthy.

See https://github.com/nodejs/undici/blob/4608ef157cc75d5ce3d2802e6949cb865d73146c/lib/handler/cache-handler.js#L115C1-L125C6

Maybe that code should be moved into canCacheResponse instead of staying out of it, allowing to have a better picture of how the cache interceptor checks cacheability.

fredericDelaporte · 2025-07-16T20:50:12Z

The remaining CI failures seem to be a flaky websockets test. (There is a flaky cache test too which has blown on my setup once, name "revalidates the request when the response is stale".)

fredericDelaporte · 2025-07-17T06:53:14Z

lib/handler/cache-handler.js

+    // When cacheByDefault is truthy, the cache interceptor does no more mandate an explicit expiry or
+    // heuristically cacheability prior to reaching here. We should then check the full condition allowing
+    // to consider caching responses.


That is wrong, even when cacheByDefault is not defined or falsy, an explicit expiry is not mandated by the current implementation. (Also see #4338.) This shortcut has to be removed.

This reverts commit 5cf41d4.

mcollina

lgtm

cache: fix heuristic caching

0797def

307 responses must not be cached if they do not have an explicit caching directive. Final responses with an explicit caching directive can be cached even if their status code is not heuristically cacheable status codes. fix part of nodejs#4334

fredericDelaporte commented Jul 14, 2025

View reviewed changes

fix caching of not understood statuses

00d1d45

finish fixing nodejs#4334

fredericDelaporte force-pushed the fixes/heuristic-cache branch from 26fe391 to 00d1d45 Compare July 14, 2025 13:05

fredericDelaporte commented Jul 14, 2025

View reviewed changes

fredericDelaporte marked this pull request as ready for review July 14, 2025 13:16

mcollina approved these changes Jul 15, 2025

View reviewed changes

fredericDelaporte commented Jul 15, 2025

View reviewed changes

test/interceptors/cache.js Outdated Show resolved Hide resolved

Split conditionnal request test in two

1c74975

metcoder95 reviewed Jul 16, 2025

View reviewed changes

fredericDelaporte commented Jul 16, 2025

View reviewed changes

fredericDelaporte added 2 commits July 16, 2025 21:22

Fix the change and the test

a8c858a

Adhere stricly to RFC 9111 cacheable conditions

e6bd1a1

fredericDelaporte commented Jul 16, 2025

View reviewed changes

Do the full cacheability test only for cases requiring it

5cf41d4

fredericDelaporte commented Jul 16, 2025

View reviewed changes

fredericDelaporte added 2 commits July 16, 2025 22:11

Adujst a comment

27c0705

Remove a forgotten hack in test

3764f34

Redo the hack, required for actually testing

0fea79b

fredericDelaporte commented Jul 17, 2025

View reviewed changes

Revert "Do the full cacheability test only for cases requiring it"

3a2c981

This reverts commit 5cf41d4.

fredericDelaporte mentioned this pull request Jul 17, 2025

Fix misleading cacheByDefault documentation #4338

Merged

6 tasks

Remove obsolete config from test

662c81b

mcollina approved these changes Jul 17, 2025

View reviewed changes

Uh oh!

cache: fix excessive caching and some other lack of caching #4335

Are you sure you want to change the base?

cache: fix excessive caching and some other lack of caching #4335

Conversation

fredericDelaporte commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This relates to...

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Uh oh!

fredericDelaporte Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 15, 2025

Uh oh!

fredericDelaporte commented Jul 15, 2025

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredericDelaporte commented Jul 14, 2025 •

edited

Loading

fredericDelaporte Jul 14, 2025 •

edited

Loading

fredericDelaporte Jul 14, 2025 •

edited

Loading

fredericDelaporte Jul 16, 2025 •

edited

Loading

fredericDelaporte Jul 16, 2025 •

edited

Loading

fredericDelaporte Jul 16, 2025 •

edited

Loading