Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Miniflare] caching implementation differs from actual #4390

Open
haus opened this issue Nov 2, 2021 · 6 comments
Open

[Miniflare] caching implementation differs from actual #4390

haus opened this issue Nov 2, 2021 · 6 comments
Labels
miniflare Relating to Miniflare quick win Potentially easy/straightforward issue to tackle

Comments

@haus
Copy link

haus commented Nov 2, 2021

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 kornelski/http-cache-semantics#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?

@mrbbot
Copy link
Contributor

mrbbot commented Nov 3, 2021

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

@haus
Copy link
Author

haus commented Nov 3, 2021

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?).

@mrbbot
Copy link
Contributor

mrbbot commented Nov 3, 2021

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

@haus
Copy link
Author

haus commented Nov 3, 2021

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

@aeneasr
Copy link

aeneasr commented Dec 28, 2022

We also just hit this when writing tests. Cloudflare is able to cache 401 responses, while miniflare will refuse to cache those calls, which makes it not possible to test this use case. Is there a workaround available?

@mrbbot
Copy link
Contributor

mrbbot commented Nov 7, 2023

Hey! 👋 Thanks for raising this issue. I'm going to transfer this to workers-sdk as that's the new home for Miniflare 3, and that will make it easier for us to track this. 👍

@mrbbot mrbbot changed the title miniflare caching implementation differs from actual [Miniflare] caching implementation differs from actual Nov 7, 2023
@mrbbot mrbbot transferred this issue from cloudflare/miniflare Nov 7, 2023
@mrbbot mrbbot added quick win Potentially easy/straightforward issue to tackle miniflare Relating to Miniflare labels Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miniflare Relating to Miniflare quick win Potentially easy/straightforward issue to tackle
Projects
Status: Backlog
Development

No branches or pull requests

3 participants