feature: multipart uploads and retry - #246
Conversation
madebydavid
changed the title
There was a problem hiding this comment.
Pull request overview
Adds multipart ZIP uploads and retry handling to improve large-upload resilience.
Changes:
- Uses multipart uploads for ZIPs ≥16 MiB, with
--skipMultipartfallback. - Adds signed-part APIs, concurrency limiting, retries, progress, completion, and cleanup.
- Adds supporting types, dependency, and unit tests.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/api/index.ts |
Adds multipart API operations. |
src/commands/game/ship.tsx |
Adds the multipart opt-out flag. |
src/types/api.ts |
Defines multipart API types. |
src/types/index.ts |
Extends shipping flags. |
src/utils/errors.ts |
Adds response and retry helpers. |
src/utils/ship/index.ts |
Selects single or multipart upload. |
src/utils/ship/multipartUpload.ts |
Implements multipart upload orchestration. |
src/utils/ship/upload.ts |
Extracts single-upload handling and limits. |
test/utils/errors.test.ts |
Tests retry classification. |
test/utils/ship/multipartUpload.test.ts |
Tests part calculation and retry basics. |
package.json |
Adds p-limit. |
package-lock.json |
Locks the new dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/utils/ship/multipartUpload.ts:111
- A short
FileHandle.read()does not necessarily mean EOF, so the network-mount case mentioned here can abort an otherwise valid upload. Continue reading into the remaining portion of the buffer and fail only if a zero-byte read occurs before the expected part size is reached.
const {bytesRead} = await handle.read(body, 0, part.size, part.start)
// A network mount can return less than it was asked for. Uploading the
// short buffer would pad the part with zeros and corrupt the zip.
if (bytesRead !== part.size) {
throw new Error(`Part ${part.partNumber} read ${bytesRead} bytes, expected ${part.size}`)
src/utils/ship/multipartUpload.ts:233
clearQueue()only removes callbacks that have not started; up to eight active callbacks continue reading, PUTting, and reporting progress. This function then rejects immediately, somultipartUploadaborts the ticket andshipdeletes the ZIP while those PUTs are still running, potentially uploading parts after cleanup. Signal cancellation and await every in-flight task before rethrowing so abort happens only after uploads have stopped.
limit.clearQueue()
@copilot - took what you said onboard - could you review the changes to that part? |
... The implementation looks correct. The |
|
I have tested this by uploading a 700MB game zip and disabling my wifi and enabling it again during the upload - I also noted that this works with the old non-multi upload (still usable via |
madebydavid
changed the title
|
Tried a better test - switching to a mobile hotspot and back during the upload: |
|
Looking into parsing and retrying that 400 |
|
Updated - the multipart upload survives a network switch mid-upload. It takes a while to resume but seems reliable. |
|
i will merge this once backend and infra (updated lifecycle rule) changes have been deployed to production. |
|
@copilot review the latest changes - docs and unhiding the flag |
Changes look good. The |
This is related to #244
Goals
What's changed
--skipMultipart