Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

fix: expire unused assets on site upload #2221

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Conversation

caass
Copy link
Contributor

@caass caass commented Mar 16, 2022

backport of cloudflare/workers-sdk#587

currently, we just delete unused assets when uploading a new version
of a workers-sites project. this can cause problems when a user
accessing an old site gets a 404 when going to a page when the new
assets haven't been pushed out to the edge yet, but the old assets have
already been deleted.

this commit addresses this by marking old assets for expiration
in five minutes, rather than deleting them outright. this should give
the new worker enough time to propogate to the edge before the old
assets are taken down.

currently, we just delete unused assets when uploading a new version
of a workers-sites project. this can cause problems when a user
accessing an old site gets a 404 when going to a page when the new
assets haven't been pushed out to the edge yet, but the old assets have
already been deleted.

this commit addresses this by marking old assets for expiration
in five minutes, rather than deleting them outright. this should give
the new worker enough time to propogate to the edge before the old
assets are taken down.
@caass caass marked this pull request as ready for review March 17, 2022 14:27
@caass caass requested a review from a team as a code owner March 17, 2022 14:27
@jgentes jgentes requested a review from jspspike March 17, 2022 15:29
Copy link
Contributor

@jspspike jspspike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and seems to work as intended.

@jgentes This change might make publish take a lot longer. Maybe for the future we should make a bulk endpoint to get a set of values with a set of keys so each key doesn't have a request.

// isn't this very slow? yes. can we do this in bulk? unfortunately as of now
// there isn't a KV endpoint for "bulk read".
// https://api.cloudflare.com/#workers-kv-namespace-properties
let five_minutes_from_now = chrono::Utc::now().timestamp() + (60 * 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe use the duration struct in chrono for 5 minutes

@caass caass merged commit 3bb3681 into master Mar 17, 2022
@delete-merged-branch delete-merged-branch bot deleted the expire-unused-sites-assets branch March 17, 2022 18:54
@threepointone
Copy link
Contributor

Looks like this is breaking people's deploys #2224 We should fix this asap. I'll work on a solution for w2 myself.

@threepointone
Copy link
Contributor

threepointone commented Mar 20, 2022

I posted cloudflare/workers-sdk#649 for w2

@caass caass mentioned this pull request Mar 22, 2022
caass pushed a commit that referenced this pull request Mar 22, 2022
caass pushed a commit that referenced this pull request Mar 22, 2022
* Revert "hotfix for expiring unused sites assets (#2226)"

This reverts commit 1eadbcb.

* Revert "fix: expire unused assets on site upload (#2221)"

This reverts commit 3bb3681.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants