undici icon indicating copy to clipboard operation
undici copied to clipboard

Cache cacheable response with an error status code

Open fredericDelaporte opened this issue 5 months ago • 1 comments

This would solve...

#3231 introduced HTTP caching (great job!), but in my tests it does not cache cacheable responses with an error status code (specifically, 404 in my case, but all cacheable responses should be cached, included 204, 400, ...), and responses with error status codes are cacheable according to RFC 9111.

The implementation should look like...

If there is actually a restriction on cacheable status codes in Undici going beyond what the RFC mandates:

  • maybe realign with the spec,
  • or introduce an option in the cache interceptor to relax this constraint.

I have also considered...

I have checked my test request gets a proper cache-control header in its 404 response (public,max-age=60 in my case), and repeatedly calling it from the node server does always actually call the external API. Doing the same test with a request, replied with a 200 and the same cache directive, does not always actually call the external API. Doing the same tests with these requests from a browser does actually cache both cases, 404 and 200 response cases.

Additional context

This is a nice to have in my opinion, because most gains are already there with the caching of 200 responses, which are usually much more common than other responses. Again, great job with this feature which I really appreciate, and many thanks for it.

These tests were done with Undici 7.10.0 on a Node 22.15.1, just overriding the global dispatcher, still using the default fetch (through ofetch actually, used through Nuxt useFetch). Caching was activated along with HTTP/2 in a Nitro plugin:

import { setGlobalDispatcher, Agent, interceptors } from 'undici'

export default defineNitroPlugin(() => {
  setGlobalDispatcher((new Agent({
    allowH2: true
  })).compose(
    interceptors.cache()
  ))
})

fredericDelaporte avatar Jun 26 '25 08:06 fredericDelaporte

I'm aligned with the proposal, but have my concerns about going straight ahead into cashing non-2xx responses as it might be a breaking change for current usage.

I'd advice the exploration of extending the interceptors option, and make the caching of final responses the new default for further major

metcoder95 avatar Jun 26 '25 08:06 metcoder95

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

mcollina avatar Jul 02 '25 15:07 mcollina

Sorry for the answer delay. Thanks for having done this. (Maybe I would have had a look on next Sunday, but that is not a certainty.)

fredericDelaporte avatar Jul 04 '25 18:07 fredericDelaporte

I realized I was wrong stating it should be changed, "realign with the RFC", because the RFC does not mandate caching in any case (only write "may cache").

fredericDelaporte avatar Jul 04 '25 19:07 fredericDelaporte