fix: respect response status code for caching in `adapter-cloudflare` #9768

psytrx · 2023-04-25T16:14:32Z

This aims to solve #9721 .

Currently, adapter-cloudflare does not respect the status code of internal requests for caching.

This is behavior is different to how adapter-cloudflare-workers and adapter-node behave.

This PR extends the cache-check to behave similarly.

This will also help with out-of-sync cache misses during deployments (outdated edge nodes receive a request from the freshly deployed app version, respond with 404s, DNS caches the bad response). This affects thousands of my users on each deployment. Rapid build, test and deploy cycles is one of the reasons I chose SvelteKit.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

changeset-bot · 2023-04-25T16:14:36Z

⚠️ No Changeset found

Latest commit: 05e9488

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

psytrx · 2023-04-25T16:28:33Z

As asked in #9721, is this as straightforward as it seems? What do I miss?

Also, is this a minor bump? It should gracefully fall back to responses from the origin, but changing cache behavior can have a significant impact on some.

Some less relevant thoughts on worker.js:

I found it a bit confusing that pragma and res are being re-used for different requests. Same for is_static_asset. I enjoy consts, which would allow for some cleanup/extraction and an easier to grasp fetch handler. So rather a general question: Are refactors welcome as PRs?

lukeed · 2023-04-26T00:38:31Z

packages/adapter-cloudflare/src/worker.js

-		return pragma && res.ok ? Cache.save(req, res, context) : res;
+
+		// only save good responses to Cache
+		return should_cache(res) ? Cache.save(req, res, context) : res;


This should just be a statusCode check, leaving Cache.save to employ the rest of the "is this cacheable" logic

Thanks! You're right, Cache.save already looks into the cache-control (and also the vary) header, which makes should_cache redundant. I'll revert to the simple is_error check as seen in adapter-cloudflare-workers.

This means I can also remove the pragma && res.ok condition, correct? Because the response will not be ok if status > 399, and Worktop makes sure it respects the headers.

@lukeed I have looked into Cache.save and found out that it didn't handle the case when cache-control is empty and let it cache by default. So every dynamic content without cc will cache by default in case of performance it's good but I'm curious about security as I describe in this issue lukeed/worktop#176.

Can confirm. The check for a cache-control header got lost in this PR.

I'll create an issue and propose a fix.

lukeed · 2023-04-27T07:41:53Z

packages/adapter-cloudflare/src/worker.js

-		return pragma && res.ok ? Cache.save(req, res, context) : res;
+		// write to `Cache` only if response is not an error,
+		// let `Cache.save` handle the Cache-Control and Vary headers
+		return !is_error(res.status) ? Cache.save(req, res, context) : res;


Suggested change

return !is_error(res.status) ? Cache.save(req, res, context) : res;

return res.status >= 400 ? res : Cache.save(req, res, context);

And remove the other lines. I can’t do multi line on phone

Thanks! Done. Remove or revert the comment as well?

eltigerchino · 2023-05-02T14:29:38Z

@fjdumont thanks for the PR! Are you happy with the current state of the PR before I merge?

psytrx · 2023-05-02T16:25:32Z

Do we need a changeset? Is changing cache behavior considered backwards compatible?

Otherwise, it looks good to me, thanks for asking.

There's some bits and bobs I'd like to improve in the worker.js, but I'll consider opening a new PR to get this one merged (see my comment above).

…9768)" This reverts commit 696ab68.

eltigerchino · 2023-05-02T16:27:23Z

I'm an idiot and forgot to make sure there was a changeset before merging.

Do we need a changeset? Is changing cache behavior considered backwards compatible?

I think so, just as a patch.

psytrx added 4 commits April 25, 2023 17:41

don't cache responses with status > 399

0797537

extract should_cache

2bb2863

improve branch readability

d02663f

simplify status code test

ebec5a9

vercel bot deployed to Preview – kit April 25, 2023 16:15 View deployment

psytrx mentioned this pull request Apr 25, 2023

Respect HTTP status for asset caching in adapter-cloudflare #9721

Closed

lukeed reviewed Apr 26, 2023

View reviewed changes

let worktop handle cache headers

5511988

vercel bot deployed to Preview – kit April 26, 2023 06:17 View deployment

benmccann added the pkg:adapter-cloudflare label Apr 26, 2023

lukeed reviewed Apr 27, 2023

View reviewed changes

remove is_error, inline condition

05e9488

vercel bot deployed to Preview – kit May 1, 2023 10:12 View deployment

lukeed approved these changes May 2, 2023

View reviewed changes

vercel bot deployed to Preview – kit-svelte-dev May 2, 2023 14:20 View deployment

eltigerchino merged commit 696ab68 into sveltejs:master May 2, 2023

eltigerchino added a commit that referenced this pull request May 2, 2023

Revert "adapter-cloudflare: respect response status code for caching (#…

9a6b491

…9768)" This reverts commit 696ab68.

eltigerchino mentioned this pull request May 2, 2023

Revert "adapter-cloudflare: respect response status code for caching" #9818

Closed

eltigerchino changed the title ~~adapter-cloudflare: respect response status code for caching~~ fix: respect response status code for caching in adapter-cloudflare May 2, 2023

eltigerchino mentioned this pull request May 2, 2023

changeset for #9768 #9820

Merged

5 tasks

psytrx mentioned this pull request May 9, 2023

defensively handle omitted cache-control header in adapter-cloudflare #9884

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: respect response status code for caching in `adapter-cloudflare` #9768

fix: respect response status code for caching in `adapter-cloudflare` #9768

psytrx commented Apr 25, 2023 •

edited

Loading

changeset-bot bot commented Apr 25, 2023 •

edited

Loading

psytrx commented Apr 25, 2023 •

edited

Loading

lukeed Apr 26, 2023

psytrx Apr 26, 2023 •

edited

Loading

psytrx Apr 26, 2023

lukeed Apr 26, 2023

0x221A May 9, 2023

psytrx May 9, 2023

lukeed Apr 27, 2023

lukeed Apr 27, 2023

psytrx May 1, 2023

eltigerchino commented May 2, 2023

psytrx commented May 2, 2023 •

edited

Loading

eltigerchino commented May 2, 2023 •

edited

Loading

	return !is_error(res.status) ? Cache.save(req, res, context) : res;
	return res.status >= 400 ? res : Cache.save(req, res, context);

fix: respect response status code for caching in adapter-cloudflare #9768

fix: respect response status code for caching in adapter-cloudflare #9768

Conversation

psytrx commented Apr 25, 2023 • edited Loading

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

changeset-bot bot commented Apr 25, 2023 • edited Loading

⚠️ No Changeset found

psytrx commented Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

psytrx Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eltigerchino commented May 2, 2023

psytrx commented May 2, 2023 • edited Loading

eltigerchino commented May 2, 2023 • edited Loading

fix: respect response status code for caching in `adapter-cloudflare` #9768

fix: respect response status code for caching in `adapter-cloudflare` #9768

psytrx commented Apr 25, 2023 •

edited

Loading

changeset-bot bot commented Apr 25, 2023 •

edited

Loading

psytrx commented Apr 25, 2023 •

edited

Loading

psytrx Apr 26, 2023 •

edited

Loading

psytrx commented May 2, 2023 •

edited

Loading

eltigerchino commented May 2, 2023 •

edited

Loading