[wrangler] Improve asset upload performance with single-file uploads#14305
[wrangler] Improve asset upload performance with single-file uploads#14305jbwcloudflare wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: c48cd7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
5b5e896 to
c549f98
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
c549f98 to
ba647bb
Compare
ba647bb to
13f5a9f
Compare
13f5a9f to
6676947
Compare
6676947 to
5d74650
Compare
| export function getEdgeKvUploadConcurrency(jwt: string): number { | ||
| try { | ||
| const value = Number(decodeJwtPayload(jwt).edge_kv_upload_concurrency); | ||
| return value > 0 ? Math.floor(value) : EDGE_KV_UPLOAD_CONCURRENCY; |
There was a problem hiding this comment.
🟡 getEdgeKvUploadConcurrency can return 0 for fractional JWT values between 0 and 1, breaking PQueue
When edge_kv_upload_concurrency in the JWT is a fractional value in the range (0, 1) — e.g. 0.5 — the check value > 0 passes but Math.floor(value) produces 0. PQueue requires concurrency ≥ 1; setting it to 0 will cause the queue to either throw or hang indefinitely, preventing any uploads from completing. While the value is server-controlled (making this unlikely in practice), a simple guard like Math.max(1, Math.floor(value)) or checking value >= 1 would make this robust.
| return value > 0 ? Math.floor(value) : EDGE_KV_UPLOAD_CONCURRENCY; | |
| return value >= 1 ? Math.floor(value) : EDGE_KV_UPLOAD_CONCURRENCY; |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
it's fine lol we will not set this to a fractional value between 0 and 1 on the server side
5d74650 to
c48cd7c
Compare
| }, | ||
| body: payload, | ||
| let res: UploadResponse; | ||
| if (useEdgeKvUpload) { |
There was a problem hiding this comment.
this is the new codepath: if the gate is on, use the new single-file upload endpoint
| } | ||
| ); | ||
| } else { | ||
| // Populate the payload only when actually uploading (this is limited to 3 concurrent uploads at 50 MiB per bucket meaning we'd only load in a max of ~150 MiB) |
There was a problem hiding this comment.
this is the old codepath: just moved down into the else
| const contentType = getContentType(absFilePath); | ||
| res = await fetchResult<UploadResponse>( | ||
| complianceConfig, | ||
| `/accounts/${accountId}/workers/assets/upload/${manifestEntry[1].hash}`, |
There was a problem hiding this comment.
didn't realize we were using the same endpoint but with different semantics, i kinda figured we'd have a new endpoint? not an issue though
There was a problem hiding this comment.
the old bulk one is:
/accounts/${accountId}/workers/assets/upload,
and the new single file-one is:
/accounts/${accountId}/workers/assets/upload/:hash
so they are different, unless i'm misunderstanding the question?
|
|
||
| const queue = new PQueue({ | ||
| concurrency: useEdgeKvUpload | ||
| ? getEdgeKvUploadConcurrency(initializeAssetsResponse.jwt) |
There was a problem hiding this comment.
do we need concurrency limits for single-file uploads? i guess probably if there's any level of buffering, and definitely better start there and increase it.
There was a problem hiding this comment.
not sure i'm following: edgeKvUpload is the single-file mode and this will currently set the concurrency limit to 25 by default (it's actually a setting in the JWT so we can increase it from EWC later if we want)
is that what you meant? or some kind of server side concurrency limit maybe?
Fixes #WC-4107
Begin onboarding wrangler to use the improved API endpoint for uploading Workers Assets.
This is being staged as a gradual rollout:
This allows us to gradually roll out and validate the new feature without a hard cutover during a new wrangler release, or the need for emergency wrangler patch releases if something goes wrong.
A picture of a cute animal (not mandatory, but encouraged)