fix(web): retry each upload part instead of failing the whole upload - #243
Conversation
A multi-gigabyte file is several hundred sequential PUTs. Previously the first non-OK response threw and aborted the entire upload without a second attempt, discarding everything already transferred - a single transient 500/503 from the object store, or a brief network drop, could cost an hour of work on a slow uplink. Each part is now attempted up to 8 times with exponential backoff and jitter (~254s of tolerance per part). The presigned URL is re-fetched on every attempt because presign_upload_part expires after an hour and a large upload can outlive that, so a URL cached before a long backoff may already be dead. User-initiated cancels are never retried. The part loop was duplicated verbatim between the new-asset and new-version upload paths; both now share uploadAllParts(), so the retry exists once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retry treated every failure as transient, so a rejected request - bad signature, expired policy, wrong length - was repeated eight times across four minutes before surfacing. That delays the error message without ever changing the outcome, and while it happens the upload looks frozen at its current percentage. Only network errors and 5xx are repeated now. 408 and 429 stay retryable: both explicitly invite a later attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixed belongs after Changed.
ravirajsinh45
left a comment
There was a problem hiding this comment.
Thanks @Lennart-Pingpong. This one is in good shape and we are happy with it.
We checked it out and ran it rather than reading alone: 35 test files, 271 tests, all green, and the one tsc error (globals.css side-effect import) reproduces identically on main, so it is not from this PR.
The details we specifically looked at and agree with: re-fetching the presigned URL inside each attempt is right, since presign_upload_part expires after an hour and a large upload can easily outlive that; treating 4xx as permanent while letting 408 and 429 through is the correct split; and never retrying after a cancel avoids the obvious foot-gun. Jitter on the backoff is a nice touch. Folding the duplicated part loop into one uploadAllParts is worth it on its own, and it is what #242 and #241 will build on.
We pushed one commit to your branch: the ### Fixed block sat above ### Changed, and Keep a Changelog orders it the other way. Nothing else changed.
One optional follow-up, not a blocker for this PR. The backoff is a plain setTimeout, so cancelling during a late attempt takes up to 128 seconds to register and issues one more presign call before it notices. An abort-aware sleep would make cancel feel immediate. Happy for that to be a separate change, or to leave it.
Merging once CI re-runs.
Summary
A multi-gigabyte upload is several hundred sequential PUTs. Until now the first
non-OK response threw and aborted the entire upload without a second attempt,
discarding every byte already transferred. A single transient 500/503 from the
object store — or a brief network drop — could therefore cost an hour of work on
a slow uplink.
We hit this on our own instance: a 3.75 GB upload died at 36 % (129 of 358 parts,
1.35 GB transferred, 21 minutes in) and had to start over from zero.
Each part is now retried with exponential backoff instead.
Changes
uploadPartOnce()— one attempt at a single part, returns its ETaguploadPart()— retries a part up toPART_MAX_ATTEMPTS(8) times withexponential backoff and jitter, ≈254s of tolerance per part
uploadAllParts()— the part loop, now sharedpresign_upload_partexpires after an hour and a large upload can outlive that, so a URL cached
before a long backoff may already be dead
AbortController) are never retried and surfaceimmediately
wrong length) fails identically on every attempt, so repeating it eight times
across four minutes only delays the error while the upload looks frozen. 408
and 429 stay retryable, since both explicitly invite a later attempt
upload paths — both now call
uploadAllParts(), so the retry logic exists once(this is why the diff removes ~47 lines while adding the retry)
apps/web/stores/__tests__/upload-store.test.tsVerified the tests actually pin the behaviour: setting
PART_MAX_ATTEMPTS = 1turns exactly the three retry tests red and leaves the ordering and cancellation
tests green.
Testing
pnpm test→ 269 tests passing,pnpm exec tsc --noEmitclean,pnpm buildsucceeds)Manual test. Uploaded a large file against a staging instance and pulled the
Wi-Fi for >30s mid-transfer, then reconnected. Verified server-side against S3
ListPartsrather than trusting the progress bar:The upload survived and continued at the next part. On
mainthe first failedPUT would have ended it and discarded all 335 MB. The gap is 89s rather than 30s
because the retry was mid-backoff when connectivity returned and waited out the
remaining interval.
Two honest caveats: the run was cancelled deliberately rather than carried
through to
/upload/complete(it was a 4.6 GB file on a slow uplink), and thistests recovery from a network outage — not a closed tab or a suspended machine,
which no amount of retrying can address.
One observation, not part of this PR: during the backoff the UI shows
nothing — no "retrying" state, no stalled-connection hint — so it looks like the
upload died while it is in fact waiting. A user would likely cancel and start
over, which is the opposite of what the retry is for. The store knows it is
retrying; it just doesn't tell the UI. Happy to follow up separately.
Checklist
CHANGELOG.mdentry added under## [Unreleased]→### FixedScreenshots
n/a — no UI changes.
Related: this is the client-side half of the problem described in #241.
Retry reduces how often an upload dies; it cannot help when the browser context
itself disappears, which is what resumable uploads would address.