feat: expire unused assets in `[site]` uploads #587

threepointone · 2022-03-13T13:30:10Z

This expires any previously uploaded assets when using a Sites / [site] configuration. Because we currently do a full iteration of a namespace's keys when publishing, for rapidly changing sites this means that uploads get slower and slower. We can't just delete unused assets because it leads to occasional 404s on older publishes while we're publishing. So we expire previous assets while uploading new ones. The implementation/constraints of the kv api means that uploads may become slower, but should hopefully be faster overall. These optimisations also only matter for rapidly changing sites, so common usecases still have the same perf characteristics.

I've started this PR as a discussion point, to discuss tradeoffs of the approach, and to make the constraints apparent before we move ahead with this. // @jgentes @koeninger

Closes #459

changeset-bot · 2022-03-13T13:30:12Z

🦋 Changeset detected

Latest commit: 840c237

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

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

github-actions · 2022-03-13T13:30:54Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/1998676775/npm-package-wrangler-587

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/587/npm-package-wrangler-587

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/1998676775/npm-package-wrangler-587 dev path/to/script.js

threepointone · 2022-03-13T13:31:00Z

packages/wrangler/src/sites.tsx

+    toExpire.map(async (asset) => ({
+      ...asset,
+      // it would be great if we didn't have to do this fetch at all
+      value: await getKeyValue(accountId, namespace, asset.key),


Is there a way to update a value's expiration time without having to fetch and send back the value? This is currently the most expensive part of this strategy.

It would be ideal if the bulk:kv PUT API did not update values if the value property is missing.
Then we could do this whole update (additions and expirations) in one go.

@koeninger thoughts?

@koeninger if this looks fine to you, and there's no additional APIs that can be provided, we'll land this and backport to wrangler 1.x.

There's nothing different about bulk api in terms of efficiency, it's just a wrapper around multiple individual writes. There's not a way to update expiration without a write.

packages/wrangler/src/sites.tsx

threepointone · 2022-03-17T03:13:49Z

Opening this up for review, but we'll wait for @koeninger to have a look before we land it

petebacondarwin

Just a couple of worries about expiration value and how it is tested.
Otherwise looking great!

packages/wrangler/src/sites.tsx

packages/wrangler/src/__tests__/publish.test.ts

packages/wrangler/src/__tests__/helpers/mock-kv.ts

petebacondarwin · 2022-03-17T12:24:25Z

packages/wrangler/src/__tests__/publish.test.ts

+
+    describe("expire assets", () => {
+      // it's a 100 seconds past epoch
+      // everyone's still talking about how great woodstock was


packages/wrangler/src/sites.tsx

petebacondarwin

Smashing!

packages/wrangler/src/__tests__/publish.test.ts

threepointone · 2022-03-17T12:42:57Z

I'll wait for a few more hours for @koeninger to have a look; but if he's not around I'll merge this so we can take it for a spin regardless, and he can drop his thoughts whenever.

This expires any previously uploaded assets when using a Sites / `[site]` configuration. Because we currently do a full iteration of a namespace's keys when publishing, for rapidly changing sites this means that uploads get slower and slower. We can't just delete unused assets because it leads to occasional 404s on older publishes while we're publishing. So we expire previous assets while uploading new ones. The implementation/constraints of the kv api means that uploads may become slower, but should hopefully be faster overall. These optimisations also only matter for rapidly changing sites, so common usecases still have the same perf characteristics.

threepointone · 2022-03-17T13:14:58Z

merging, we can review later. I want to actually try it out over the weekend.

koeninger · 2022-03-17T14:12:16Z

packages/wrangler/src/sites.tsx

+      console.log(`expiring unused ${asset.name}...`);
+      toExpire.push({
+        key: asset.name,
+        value: "", // we'll fill all the values in one go


Is there any way someone is relying on metadata associated with those keys?

* Use Node's root certificates on Windows `workerd`'s `trustBrowserCas` uses `SSL_CTX_set_default_verify_paths()` to enable the system trust store. Unfortunately, this doesn't work on Windows, meaning any HTTPS `fetch()` would fail, with an `unable to get local issuer certificate` error. This change passes the root certificates from Node's bundled CA store to `workerd` as `trustedCertificates` on Windows. Closes #3264 * Read extra trusted certificates from `NODE_EXTRA_CA_CERTS` Wrangler passes the Cloudflare root certificate using the `NODE_EXTRA_CA_CERTS` environment variable. This change loads CA certs from this variable, fixing HTTPS `fetch()`s with WARP enabled. This can also be used for trusting self-signed certificates. Closes #3218

threepointone commented Mar 13, 2022

View reviewed changes

threepointone force-pushed the expire-unused-assets branch from 843628c to 023e97f Compare March 13, 2022 13:31

petebacondarwin reviewed Mar 13, 2022

View reviewed changes

packages/wrangler/src/sites.tsx Outdated Show resolved Hide resolved

threepointone force-pushed the expire-unused-assets branch from 023e97f to 3e8892b Compare March 14, 2022 04:12

threepointone mentioned this pull request Mar 15, 2022

Revert #2096 - [sites] Don't delete unused assets cloudflare/wrangler-legacy#2214

Merged

threepointone force-pushed the expire-unused-assets branch 2 times, most recently from 120acd0 to 46c8133 Compare March 15, 2022 11:27

caass reviewed Mar 16, 2022

View reviewed changes

packages/wrangler/src/sites.tsx Show resolved Hide resolved

caass mentioned this pull request Mar 16, 2022

Add endpoint to read values from KV cloudflare/cloudflare-rs#177

Closed

threepointone commented Mar 16, 2022

View reviewed changes

packages/wrangler/src/sites.tsx Outdated Show resolved Hide resolved

threepointone force-pushed the expire-unused-assets branch from 46c8133 to 7756c7d Compare March 16, 2022 18:03

caass mentioned this pull request Mar 16, 2022

fix: expire unused assets on site upload cloudflare/wrangler-legacy#2221

Merged

threepointone changed the title ~~WIP feat: expire unused assets in [site] uploads~~ feat: expire unused assets in [site] uploads Mar 17, 2022

threepointone marked this pull request as ready for review March 17, 2022 03:13

threepointone force-pushed the expire-unused-assets branch from 7756c7d to 1c43683 Compare March 17, 2022 03:15

petebacondarwin requested changes Mar 17, 2022

View reviewed changes

packages/wrangler/src/sites.tsx Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/publish.test.ts Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/helpers/mock-kv.ts Outdated Show resolved Hide resolved

threepointone force-pushed the expire-unused-assets branch 2 times, most recently from 10c5d78 to 7670bec Compare March 17, 2022 12:19

threepointone requested a review from petebacondarwin March 17, 2022 12:19

petebacondarwin reviewed Mar 17, 2022

View reviewed changes

threepointone commented Mar 17, 2022

View reviewed changes

packages/wrangler/src/sites.tsx Outdated Show resolved Hide resolved

threepointone force-pushed the expire-unused-assets branch from 7670bec to 337944b Compare March 17, 2022 12:29

petebacondarwin approved these changes Mar 17, 2022

View reviewed changes

packages/wrangler/src/__tests__/publish.test.ts Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/publish.test.ts Outdated Show resolved Hide resolved

threepointone force-pushed the expire-unused-assets branch from 337944b to 6b88c08 Compare March 17, 2022 12:40

threepointone force-pushed the expire-unused-assets branch from 6b88c08 to 840c237 Compare March 17, 2022 12:51

threepointone merged commit 49869a3 into main Mar 17, 2022

threepointone deleted the expire-unused-assets branch March 17, 2022 13:15

github-actions bot mentioned this pull request Mar 17, 2022

Version Packages #562

Merged

koeninger reviewed Mar 17, 2022

View reviewed changes

jplhomer mentioned this pull request Mar 18, 2022

Expiration changes hanging deploys cloudflare/wrangler-legacy#2224

Closed

fcomrqz mentioned this pull request Mar 22, 2022

🐛 BUG: bulk put: could not base64 decode value for key #666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expire unused assets in `[site]` uploads #587

feat: expire unused assets in `[site]` uploads #587

threepointone commented Mar 13, 2022 •

edited by caass

Loading

changeset-bot bot commented Mar 13, 2022 •

edited

Loading

github-actions bot commented Mar 13, 2022 •

edited

Loading

threepointone Mar 13, 2022

petebacondarwin Mar 13, 2022

threepointone Mar 13, 2022

threepointone Mar 15, 2022

threepointone Mar 16, 2022

koeninger Mar 17, 2022

threepointone commented Mar 17, 2022

petebacondarwin left a comment

petebacondarwin Mar 17, 2022

petebacondarwin left a comment

threepointone commented Mar 17, 2022

threepointone commented Mar 17, 2022

koeninger Mar 17, 2022

feat: expire unused assets in [site] uploads #587

feat: expire unused assets in [site] uploads #587

Conversation

threepointone commented Mar 13, 2022 • edited by caass Loading

changeset-bot bot commented Mar 13, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 13, 2022 • 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

threepointone commented Mar 17, 2022

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

threepointone commented Mar 17, 2022

threepointone commented Mar 17, 2022

Choose a reason for hiding this comment

feat: expire unused assets in `[site]` uploads #587

feat: expire unused assets in `[site]` uploads #587

threepointone commented Mar 13, 2022 •

edited by caass

Loading

changeset-bot bot commented Mar 13, 2022 •

edited

Loading

github-actions bot commented Mar 13, 2022 •

edited

Loading