miniflare icon indicating copy to clipboard operation
miniflare copied to clipboard

miniflare caching implementation differs from actual

Open haus opened this issue 3 years ago • 4 comments

When looking into leveraging miniflare for our worker script, we noticed that the caching implemented within miniflare is different from the cache api in workers. Because miniflare leverages "http-cache-semantics" (which cloudworker also uses for its cache implementation), 429s are explicitly not cacheable. This stems from the storable call here, which is defined here. The check against understoodStatuses here and defined here will decide that a 429 is not storable and not cacheable, when the actual worker does allow caching of 429s.

Since http-cache-semantics is adhering to the RFC (and based on https://github.com/kornelski/http-cache-semantics/issues/31 they aren't very open to changing which status codes are cacheable), it seems like if miniflare is going to use http-cache-semantics it might need to override the storable method?

haus avatar Nov 02 '21 19:11 haus

Hey! 👋 Thanks for raising this. What is the correct behaviour here? Would it be ok for Miniflare to treat 429s as 200s? Would be pretty straightforward to rewrite a 429 status to something else here, before passing it to http-cache-semantics: https://github.com/cloudflare/miniflare/blob/b265c37918e230f121cbb38da2395400eb9d9380/packages/cache/src/cache.ts#L111

mrbbot avatar Nov 03 '21 12:11 mrbbot

Rewriting it on the way in would certainly make it cacheable, but I'm not sure how to go about ensuring that the response when matched/retrieved from the cache was a 429 again (or maybe that would just work?).

haus avatar Nov 03 '21 15:11 haus

This would only be changing the status passed to http-cache-semantics. The actual stored Response would have the correct status.

mrbbot avatar Nov 03 '21 18:11 mrbbot

Sweet, then it sounds like that should do the right thing.

haus avatar Nov 03 '21 18:11 haus