[wrangler] Add deploy --old-asset-ttl option #3384
Conversation
🦋 Changeset detectedLatest commit: 2b7aa3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5291243880/npm-package-wrangler-3384You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3384/npm-package-wrangler-3384Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5291243880/npm-package-wrangler-3384 dev path/to/script.jsAdditional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5291243880/npm-package-cloudflare-pages-shared-3384Note that these links will no longer work once the GitHub Actions artifact expires. |
Codecov Report
@@ Coverage Diff @@
## main #3384 +/- ##
==========================================
- Coverage 75.42% 75.16% -0.26%
==========================================
Files 182 183 +1
Lines 11063 11069 +6
Branches 2902 2911 +9
==========================================
- Hits 8344 8320 -24
- Misses 2719 2749 +30
|
|
@Peter-Sparksuite thank you for submitting the feature request and PR. This looks like good functionality to have in Wrangler. As a heads-up - with our Pages/Workers convergence work later this year this Our team will review the code shortly. One thing I notice currently is that its failing on a linting error. You should be able to run |
|
@admah , Thank you for progressing this! Seems the changeset file was not quite the way the formatted preferred - it has been updated. Please approve running the checks, as I'm not seeing any issues being reported locally, currently. |
|
I'm slightly confused regarding the connection being made between this and what you've indicated at https://developers.cloudflare.com/pages/platform/serving-pages/#asset-retention Seems that the TTL/expiration you've linked to is what's going to be applied to data that's held in the cache that's near the requester. If I understand correctly, then that's fine and good. But, this change is about setting the TTL/expiration on KV assets that are no longer part of the deployed application, and this is only able to be done after a new deployment is done. ie: it's not about how long the data lives on the data centers near the end user that are used to distribute the data, but rather, the expiration of content that's no longer part of the actively deployed application. The related KV assets could live for a very long time before they become obsolete due to a subsequent deploy happening. eg: website has no need for changes for 3 months. I don't expect you're going to be expiring assets from the application. Setting an expiration on locally cached copies of the website assets would be fine, however, and if those assets are asked for again, the content will be fetched & cached afresh, I expect. |
|
Hey there @admah , it's been 10 days since your last comment as I write this. The formatting issue was sorted out the same day as your last comment, but it seems there's been no further activity as yet. Any idea as to when this will actually see some further progress? It would be really helpful to know. And: please provide some feedback regarding #3384 (comment) Much appreciated! |
|
Hi @Peter-Sparksuite - to set your expectation, Adam has been (and is) away for a bit at the moment so that is probably why he has not responded. |
Thank you for the information @petebacondarwin ! Any idea when @admah will be back? |
|
He should be able to continue review in July. |
|
@Peter-Sparksuite thanks for you patience on this as I was away. I'm good with this change, and have requested an engineering review before it gets merged. As to my comment regarding retention time, we're currently in the process of converging Pages and Workers which will involve creating a unified asset pipeline. I don't want to get into too many specifics, so I'm simply alluding to the fact that the way static assets are currently handled in Workers is being rebuilt and may not need this flag in the future. |
There was a problem hiding this comment.
OK I think that this is OK to land, given that it requires opt-in.
But we should note the reasons why asset expiration was removed from Wrangler 1 and 2 before making use of this option.
See
cloudflare/wrangler-legacy#2224 (comment)
In particular if you have a large number of assets that change often then you can hit the rate limit on the API that Wrangler is using, causing deployments to fail.
|
Hey there @petebacondarwin, in regards to:
I'm curious whether you saw what I mentioned about this possibility in the issue related to this pull request (last paragraph):
So I'm kinda wondering what people think about this scenario arising, and whether it's OK to just fail, or whether it should have some logic that retries individual failures repeatedly until they all succeed? Or, perhaps just let it error out, and expect the publisher, using some other mechanism, to clean things up? Perhaps, if something is suggested to address the problem, that can be addressed via a follow-up improvement, and not necessarily something that holds up this piece? |
The problem is, if I remember correctly, that the rate limiter will kick in for quite a long time, like 24 hours(?). As far as this PR is concerned, I don't think this should block it, since you have to actively opt-in to this behaviour, and if you have problems with rate-limiting, you can always choose to opt-out again. |
|
Hey there @petebacondarwin , I see there are some additional tests (E2E related) that were enabled, some of which are failing. Is there anything there I need to be concerned about / doing anything as a result? Much appreciated! :) |
|
Ah don't worry. The e2e tests will never run on a fork because they don't get access to the necessary secrets. |
* Add Wrangler deploy --old-asset-ttl option * Add changeset * Provide details in the format requested * Update formatting of changeset file
Fixes #3385
What this PR solves / how to test:
When a
wrangler deployhappens, the old assets are immediately deleted, causing users of previously obtained web applications that expect to find specific resources to no longer have access to them, resulting in users seeing 404 errors rather quickly.Wrangler 1.x attempted to address this by way of cloudflare/wrangler-legacy#2221, which caused the default behavior to be a delaying of the old asset removal, but, the cache could become large with repeated deploys causing expirations being extended repeatedly (well, that was one issue...), so it was reverted.
Even though this delayed expiration of old assets was removed in subsequent Wrangler 1.x builds, we see this as a needed behavior, and stuck with the version of Wrangler 1.x that had this delayed removal of old content.
Now, migrating to Wrangler 3.x, and finding that having delayed expiration of old assets is not available, we want to add this as a non-default optional behavior, so that those that want it can have it, while not affecting others.
This implementation also avoids the extending of existing expiration target times, which helps limit cache growth.
Test by running
wrangler deployand see that old assets are immediately removed.Test by running
wrangler deploy --old-asset-ttl 300.The console output will show some slight differences in the
Building list of assets to upload...section, in the case where assets are to be expired rather than removed.Later, after the upload of new assets is done, the heading of
Expiring [x] stale assets...(default behavior showsRemoving [x] stale assets), and the new listing of each of the individual assets as they are processed. Each asset listed will either have no suffix (meaning they are newly being set an expiration), or, a suffix of(already expiring at [x])which indicates we are not modifying their expiration and informs what the expiration target is.Here is an example:
Note: previous immediate removals did not show a list of the assets being deleted. However, when updating the expiration target, it is considered helpful to see the list, as, each asset having its expiration changed must be pulled and then pushed again with the expiration target set, and for sites having many resources, seeing this list grow over time will be helpful to be assured progress is being made.
Associated docs issue(s)/PR(s):
cloudflare/wrangler-legacy#2221 where similar was added to Wrangler 1.x
Author has included the following, where applicable:
tasty-beds-look.md
Reviewer is to perform the following, as applicable: