From 1e0a3612f751be9264f529a98fecea192c6c43e2 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Mon, 24 Aug 2026 16:24:58 +0100 Subject: [PATCH 1/8] Adding multipart upload for ZIP files > 16Mb --- package-lock.json | 50 ++++-- package.json | 1 + src/api/index.ts | 37 ++++ src/commands/game/ship.tsx | 6 + src/types/api.ts | 18 ++ src/types/index.ts | 1 + src/utils/ship/index.ts | 46 ++--- src/utils/ship/multipartUpload.ts | 224 ++++++++++++++++++++++++ src/utils/ship/upload.ts | 44 ++++- test/utils/ship/multipartUpload.test.ts | 42 +++++ 10 files changed, 437 insertions(+), 32 deletions(-) create mode 100644 src/utils/ship/multipartUpload.ts create mode 100644 test/utils/ship/multipartUpload.test.ts diff --git a/package-lock.json b/package-lock.json index 07f84f6d..7c354f1b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -35,6 +35,7 @@ "marked-terminal": "^6.2.0", "node-forge": "^1.3.1", "open": "^10.1.0", + "p-limit": "^6.2.0", "prompts": "^2.4.2", "qrcode": "^1.5.4", "react": "^18.3.1", @@ -10431,16 +10432,15 @@ } }, "node_modules/p-limit": { - "version": "3.1.0", - "resolved": "https://registry.npmjs.org/p-limit/-/p-limit-3.1.0.tgz", - "integrity": "sha512-TYOanM3wGwNGsZN2cVTYPArw454xnXj5qmWF1bEoAc4+cU/ol7GVh7odevjp1FNHduHc3KZMcFduxU5Xc6uJRQ==", - "dev": true, + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/p-limit/-/p-limit-6.2.0.tgz", + "integrity": "sha512-kuUqqHNUqoIWp/c467RI4X6mmyuojY5jGutNU0wVTmEOOfcuwLqyMVoAi9MKi2Ak+5i9+nhmrK4ufZE8069kHA==", "license": "MIT", "dependencies": { - "yocto-queue": "^0.1.0" + "yocto-queue": "^1.1.1" }, "engines": { - "node": ">=10" + "node": ">=18" }, "funding": { "url": "https://github.com/sponsors/sindresorhus" @@ -10462,6 +10462,35 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/p-locate/node_modules/p-limit": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/p-limit/-/p-limit-3.1.0.tgz", + "integrity": "sha512-TYOanM3wGwNGsZN2cVTYPArw454xnXj5qmWF1bEoAc4+cU/ol7GVh7odevjp1FNHduHc3KZMcFduxU5Xc6uJRQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "yocto-queue": "^0.1.0" + }, + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/p-locate/node_modules/yocto-queue": { + "version": "0.1.0", + "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz", + "integrity": "sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/p-try": { "version": "2.2.0", "resolved": "https://registry.npmjs.org/p-try/-/p-try-2.2.0.tgz", @@ -14270,13 +14299,12 @@ } }, "node_modules/yocto-queue": { - "version": "0.1.0", - "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz", - "integrity": "sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q==", - "dev": true, + "version": "1.2.2", + "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-1.2.2.tgz", + "integrity": "sha512-4LCcse/U2MHZ63HAJVE+v71o7yOdIe4cZ70Wpf8D/IyjDKYQLV5GD46B+hSTjJsvV5PztjvHoU580EftxjDZFQ==", "license": "MIT", "engines": { - "node": ">=10" + "node": ">=12.20" }, "funding": { "url": "https://github.com/sponsors/sindresorhus" diff --git a/package.json b/package.json index 5dea529a..93ab4443 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "marked-terminal": "^6.2.0", "node-forge": "^1.3.1", "open": "^10.1.0", + "p-limit": "^6.2.0", "prompts": "^2.4.2", "qrcode": "^1.5.4", "react": "^18.3.1", diff --git a/src/api/index.ts b/src/api/index.ts index c887e5df..297d0124 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -13,6 +13,8 @@ import { GoogleAuthResponse, GoogleStatusResponse, Job, + MultipartPartUrl, + MultipartUploadTicket, PageAndSortParams, Platform, Project, @@ -21,6 +23,7 @@ import { Self, TermsResponse, UploadDetails, + UploadedPart, UploadTicket, } from '@cli/types' import {castArrayObjectDates, castJobDates, castObjectDates} from '@cli/utils/dates.js' @@ -109,6 +112,40 @@ export async function getNewUploadTicket(projectId: string): Promise { + const headers = getAuthedHeaders() + const opt = {headers} + const {data} = await axios.post(`${API_URL}/upload/${projectId}/multipart`, {size}, opt) + return data as MultipartUploadTicket +} + +// Returns a signed URL for each part number. A signed URL lasts one hour, so a +// slow upload asks for a new URL rather than starting again. +export async function getMultipartPartUrls( + uploadTicketId: string, + partNumbers: number[], +): Promise { + const headers = getAuthedHeaders() + const opt = {headers} + const {data} = await axios.post(`${API_URL}/upload/multipart/${uploadTicketId}/parts`, {partNumbers}, opt) + return (data as {parts: MultipartPartUrl[]}).parts +} + +// Joins the uploaded parts into the final object +export async function completeMultipartUpload(uploadTicketId: string, parts: UploadedPart[]): Promise { + const headers = getAuthedHeaders() + const opt = {headers} + await axios.post(`${API_URL}/upload/multipart/${uploadTicketId}/complete`, {parts}, opt) +} + +// Cancels a multipart upload. This deletes the parts already uploaded. +export async function abortMultipartUpload(uploadTicketId: string): Promise { + const headers = getAuthedHeaders() + const opt = {headers} + await axios.post(`${API_URL}/upload/multipart/${uploadTicketId}/abort`, {}, opt) +} + // Tells the backend to start running the jobs for an upload-ticket type StartJobsOptions = { platform?: Platform diff --git a/src/commands/game/ship.tsx b/src/commands/game/ship.tsx index 0f6d9398..ac8a093a 100644 --- a/src/commands/game/ship.tsx +++ b/src/commands/game/ship.tsx @@ -46,6 +46,12 @@ export default class GameShip extends BaseGameCommand { options: ['android', 'ios'], required: false, }), + skipMultipart: Flags.boolean({ + default: false, + description: 'Upload the zip in one request instead of several parts in parallel', + hidden: true, + required: false, + }), skipPublish: Flags.boolean({ default: false, description: 'Skip the publish step', diff --git a/src/types/api.ts b/src/types/api.ts index bea7b02b..8f0b34f1 100644 --- a/src/types/api.ts +++ b/src/types/api.ts @@ -73,6 +73,24 @@ export interface UploadTicket { url: string } +// A MultipartUploadTicket is a request to upload a file in parts. The parts go +// to the signed URLs returned by getMultipartPartUrls. +export interface MultipartUploadTicket { + id: string + maxParts: number + partSize: number +} + +export interface MultipartPartUrl { + partNumber: number + url: string +} + +export interface UploadedPart { + etag: string + partNumber: number +} + export interface Upload { bucketName: string createdAt: DateTime diff --git a/src/types/index.ts b/src/types/index.ts index 36502487..38c297b3 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -36,6 +36,7 @@ export type ShipGameFlags = { downloadAPK?: string follow?: boolean platform?: 'android' | 'ios' + skipMultipart?: boolean skipPublish?: boolean verbose?: boolean useDemoCredentials?: boolean diff --git a/src/utils/ship/index.ts b/src/utils/ship/index.ts index a6600f21..f4adc844 100644 --- a/src/utils/ship/index.ts +++ b/src/utils/ship/index.ts @@ -2,14 +2,15 @@ import fs from 'node:fs' import {v4 as uuid} from 'uuid' -import {getNewUploadTicket, getProject, startJobsFromUpload} from '@cli/api/index.js' -import type {Job, Platform, ProjectConfig, ShipGameFlags, UploadDetails, UploadTicket} from '@cli/types' +import {getProject, startJobsFromUpload} from '@cli/api/index.js' +import type {Job, Platform, ProjectConfig, ShipGameFlags, UploadDetails} from '@cli/types' import {detectGodotVersion, getGodotVersionDrift} from '@cli/utils/godot.js' import {getCWDGitInfo, getFileHash} from '@cli/utils/index.js' import {getFilesToShip} from './glob.js' +import {MULTIPART_MIN_SIZE, multipartUpload} from './multipartUpload.js' import type {ShipOptions} from './types.js' -import {uploadZip} from './upload.js' +import {MAX_SINGLE_UPLOAD_SIZE, type ProgressData, singleUpload} from './upload.js' import {formatProgressLog, getPlatforms} from './utils.js' import {createZip} from './zip.js' @@ -35,6 +36,11 @@ const getMajorDriftError = (detected: string, configured: string) => const getMinorDriftWarning = (detected: string, configured: string) => `${getVersionMismatch(detected, configured)}\n\n` + getVersionFixHint(detected) +const getTooLargeForSingleUploadError = (size: number) => + `This zip is ${(size / 1000 / 1000 / 1000).toFixed(1)}GB. ` + + `One request can send at most ${MAX_SINGLE_UPLOAD_SIZE / 1000 / 1000 / 1000}GB.\n\n` + + `Remove --skipMultipart to upload it in parts.` + // Main function to ship the game export async function ship({command, log, warnLog, shipFlags}: ShipOptions): Promise { const commandFlags = command.getFlags() as ShipGameFlags @@ -99,27 +105,31 @@ export async function ship({command, log, warnLog, shipFlags}: ShipOptions): Pro }, }) - let response: any let zipFileMd5 = '' - let uploadTicket: UploadTicket | null = null + let uploadTicketId = '' try { const {size} = fs.statSync(tmpZipFile) - vlog('Requesting upload ticket...') - uploadTicket = await getNewUploadTicket(projectConfig.project.id) - log('Uploading zip file...') - const zipStream = fs.createReadStream(tmpZipFile) - - response = await uploadZip({ - url: uploadTicket.url, - zipStream, + const uploadProps = { + filePath: tmpZipFile, + projectId: projectConfig.project.id, + vlog, zipSize: size, - onProgress: (data) => { + onProgress: (data: ProgressData) => { log(formatProgressLog('Uploading', data, 'loadedBytes', 'totalBytes', false)) }, - }) + } + + // A small zip goes up in one request. Splitting it buys nothing. + const useMultipart = size >= MULTIPART_MIN_SIZE && !finalFlags.skipMultipart + + if (!useMultipart && size > MAX_SINGLE_UPLOAD_SIZE) { + throw new Error(getTooLargeForSingleUploadError(size)) + } + + uploadTicketId = useMultipart ? await multipartUpload(uploadProps) : await singleUpload(uploadProps) vlog('Computing zip file hash...') zipFileMd5 = await getFileHash(tmpZipFile) @@ -136,10 +146,6 @@ export async function ship({command, log, warnLog, shipFlags}: ShipOptions): Pro } } - if (!response.ok) { - throw new Error(`Upload failed: ${response.status} ${response.statusText}`) - } - log(`Upload complete`) vlog('Fetching Git info...') @@ -160,7 +166,7 @@ export async function ship({command, log, warnLog, shipFlags}: ShipOptions): Pro gameEngineVersion: finalFlags.gameEngineVersion, } - const jobs = await startJobsFromUpload(uploadTicket.id, startJobsOptions) + const jobs = await startJobsFromUpload(uploadTicketId, startJobsOptions) vlog('Job submission complete.') diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts new file mode 100644 index 00000000..262381bd --- /dev/null +++ b/src/utils/ship/multipartUpload.ts @@ -0,0 +1,224 @@ +import fs from 'node:fs' + +import pLimit from 'p-limit' + +import { + abortMultipartUpload, + completeMultipartUpload, + getMultipartPartUrls, + getNewMultipartUpload, +} from '@cli/api/index.js' +import type {MultipartPartUrl, MultipartUploadTicket, UploadedPart} from '@cli/types' + +import type {LogFunction} from './types.d.js' +import type {ProgressData} from './upload.js' + +// Below this size a single PUT is simpler. 16 MiB is two parts at the smallest +// part size the backend uses. +export const MULTIPART_MIN_SIZE = 16 * 1024 * 1024 + +// One part at a time is slower than a single PUT, so keep this above 1. +export const DEFAULT_CONCURRENCY = 8 + +export const MAX_PART_ATTEMPTS = 4 +export const RETRY_BASE_DELAY_MS = 500 + +// The backend signs at most this many parts in one request +const SIGN_BATCH_SIZE = 1000 + +interface Part { + partNumber: number + size: number + start: number +} + +// fetch rejects a view backed by a SharedArrayBuffer, so the part body is +// pinned to a plain ArrayBuffer. A bare Uint8Array is too wide for it. +type PartBody = Uint8Array + +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) + +// Splits the file into the parts to upload. Every part is partSize bytes, +// except the last one. +export function calculateParts(zipSize: number, partSize: number): Part[] { + const parts: Part[] = [] + for (let start = 0; start < zipSize; start += partSize) { + parts.push({ + partNumber: parts.length + 1, + size: Math.min(partSize, zipSize - start), + start, + }) + } + + return parts +} + +// Reads one part of the file into memory. The bytes stay in memory until the +// part uploads, so a retry does not read the file again. +async function readPart(filePath: string, part: Part): Promise { + const handle = await fs.promises.open(filePath, 'r') + try { + const body = new Uint8Array(part.size) + 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}`) + } + + return body + } finally { + await handle.close() + } +} + +// Signs every part number. The backend limits how many it signs in one request, +// so the part numbers go up in batches. +async function getPartUrls(uploadTicketId: string, partNumbers: number[]): Promise { + const batches: number[][] = [] + for (let i = 0; i < partNumbers.length; i += SIGN_BATCH_SIZE) { + batches.push(partNumbers.slice(i, i + SIGN_BATCH_SIZE)) + } + + const signedBatches = await Promise.all(batches.map((batch) => getMultipartPartUrls(uploadTicketId, batch))) + + return signedBatches.flat() +} + +// A retry runs after the attempt before it, so these awaits belong in the loop. +/* eslint-disable no-await-in-loop */ +// Uploads one part and returns its ETag. Retries with an increasing delay. +async function uploadPart( + uploadTicketId: string, + part: Part, + signedUrl: string, + body: PartBody, +): Promise { + let url = signedUrl + let lastError: Error = new Error(`Part ${part.partNumber} did not upload`) + + for (let attempt = 1; attempt <= MAX_PART_ATTEMPTS; attempt += 1) { + if (attempt > 1) { + await delay(RETRY_BASE_DELAY_MS * 2 ** (attempt - 2)) + } + + try { + const response = await fetch(url, { + body, + headers: {'Content-Length': String(body.length)}, + method: 'PUT', + }) + + if (response.ok) { + const etag = response.headers.get('etag') + if (etag) return etag + lastError = new Error(`Part ${part.partNumber} uploaded but returned no ETag`) + } else { + lastError = new Error(`Part ${part.partNumber} failed: ${response.status} ${response.statusText}`) + // A signed URL lasts one hour. A slow upload can outlive it. + if (response.status === 403) { + const [fresh] = await getMultipartPartUrls(uploadTicketId, [part.partNumber]) + url = fresh.url + } + } + } catch (error) { + lastError = error as Error + } + } + + throw lastError +} +/* eslint-enable no-await-in-loop */ + +interface UploadPartsProps { + concurrency?: number + filePath: string + onProgress: (data: ProgressData) => void + ticket: MultipartUploadTicket + zipSize: number +} + +// Uploads the file in parts, several at a time. Progress is reported as each +// part finishes, which at the default part size is often enough for the log. +export async function uploadParts({ + concurrency = DEFAULT_CONCURRENCY, + filePath, + onProgress, + ticket, + zipSize, +}: UploadPartsProps): Promise { + const parts = calculateParts(zipSize, ticket.partSize) + if (parts.length > ticket.maxParts) { + throw new Error(`The zip file needs ${parts.length} parts but the server allows ${ticket.maxParts}`) + } + + const urls = await getPartUrls( + ticket.id, + parts.map((part) => part.partNumber), + ) + const urlByPartNumber = new Map(urls.map((url) => [url.partNumber, url.url])) + + const startTime = Date.now() + let uploadedBytes = 0 + + const limit = pLimit(concurrency) + + try { + const uploaded = await Promise.all( + parts.map((part) => + limit(async (): Promise => { + const signedUrl = urlByPartNumber.get(part.partNumber) + if (!signedUrl) throw new Error(`The server did not sign part ${part.partNumber}`) + + const body = await readPart(filePath, part) + const etag = await uploadPart(ticket.id, part, signedUrl, body) + + uploadedBytes += part.size + const elapsedSeconds = (Date.now() - startTime) / 1000 + onProgress({ + elapsedSeconds, + loadedBytes: uploadedBytes, + progress: uploadedBytes / zipSize, + speedMBps: elapsedSeconds < 0.001 ? 0 : uploadedBytes / elapsedSeconds / 1024 / 1024, + totalBytes: zipSize, + }) + + return {etag, partNumber: part.partNumber} + }), + ), + ) + + await completeMultipartUpload(ticket.id, uploaded) + } catch (error) { + // The parts already uploaded cost storage until something removes them + await abortMultipartUpload(ticket.id).catch(() => {}) + throw error + } +} + +interface MultipartUploadProps { + concurrency?: number + filePath: string + onProgress: (data: ProgressData) => void + projectId: string + vlog: LogFunction + zipSize: number +} + +// Uploads the zip in parts and returns the ID of the upload ticket +export async function multipartUpload({ + concurrency, + filePath, + onProgress, + projectId, + vlog, + zipSize, +}: MultipartUploadProps): Promise { + vlog('Requesting multipart upload ticket...') + const ticket = await getNewMultipartUpload(projectId, zipSize) + + vlog(`Uploading in parts of ${Math.round(ticket.partSize / 1024 / 1024)}MB...`) + await uploadParts({concurrency, filePath, onProgress, ticket, zipSize}) + + return ticket.id +} diff --git a/src/utils/ship/upload.ts b/src/utils/ship/upload.ts index ce7fcecc..4d426046 100644 --- a/src/utils/ship/upload.ts +++ b/src/utils/ship/upload.ts @@ -1,7 +1,16 @@ +import fs from 'node:fs' import {Readable, Transform} from 'stream' +import {getNewUploadTicket} from '@cli/api/index.js' + +import type {LogFunction} from './types.d.js' + export const ON_PROGRESS_THROTTLE_MS = 2000 +// Digital Ocean Spaces rejects a PUT above this size. Only multipart can send +// a larger file. +export const MAX_SINGLE_UPLOAD_SIZE = 5 * 1000 * 1000 * 1000 + export function createProgressStream( totalSize: number, onProgress: (sent: number, total: number) => void, @@ -25,7 +34,7 @@ export function createProgressStream( }) } -interface ProgressData { +export interface ProgressData { progress: number loadedBytes: number totalBytes: number @@ -77,3 +86,36 @@ export function uploadZip({url, zipStream, zipSize, onProgress}: UploadProps): P return response } + +interface SingleUploadProps { + filePath: string + onProgress: (data: ProgressData) => void + projectId: string + vlog: LogFunction + zipSize: number +} + +// Uploads the zip in one request and returns the ID of the upload ticket +export async function singleUpload({ + filePath, + onProgress, + projectId, + vlog, + zipSize, +}: SingleUploadProps): Promise { + vlog('Requesting upload ticket...') + const uploadTicket = await getNewUploadTicket(projectId) + + const response = await uploadZip({ + onProgress, + url: uploadTicket.url, + zipSize, + zipStream: fs.createReadStream(filePath), + }) + + if (!response.ok) { + throw new Error(`Upload failed: ${response.status} ${response.statusText}`) + } + + return uploadTicket.id +} diff --git a/test/utils/ship/multipartUpload.test.ts b/test/utils/ship/multipartUpload.test.ts new file mode 100644 index 00000000..db67c171 --- /dev/null +++ b/test/utils/ship/multipartUpload.test.ts @@ -0,0 +1,42 @@ +import {expect} from 'chai' + +import {calculateParts} from '@cli/utils/ship/multipartUpload.js' + +const MIB = 1024 * 1024 + +describe('calculateParts (ship/multipartUpload)', () => { + it('numbers the parts from 1 and covers the whole file', () => { + const parts = calculateParts(20 * MIB, 8 * MIB) + + expect(parts.map((part) => part.partNumber)).to.deep.equal([1, 2, 3]) + expect(parts.map((part) => part.start)).to.deep.equal([0, 8 * MIB, 16 * MIB]) + expect(parts.reduce((total, part) => total + part.size, 0)).to.equal(20 * MIB) + }) + + it('makes the last part smaller than the rest', () => { + const parts = calculateParts(20 * MIB, 8 * MIB) + + expect(parts[0].size).to.equal(8 * MIB) + expect(parts[1].size).to.equal(8 * MIB) + expect(parts[2].size).to.equal(4 * MIB) + }) + + it('makes one part when the file is smaller than a part', () => { + const parts = calculateParts(3 * MIB, 8 * MIB) + + expect(parts).to.have.length(1) + expect(parts[0]).to.deep.equal({partNumber: 1, size: 3 * MIB, start: 0}) + }) + + it('makes no gap and no overlap when the size divides exactly', () => { + const parts = calculateParts(16 * MIB, 8 * MIB) + + expect(parts).to.have.length(2) + expect(parts[1].start).to.equal(parts[0].start + parts[0].size) + expect(parts[1].size).to.equal(8 * MIB) + }) + + it('returns no parts for an empty file', () => { + expect(calculateParts(0, 8 * MIB)).to.deep.equal([]) + }) +}) From 5df4c8959f2855a5ecafec20533d0ace16af4fb6 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 11:17:37 +0100 Subject: [PATCH 2/8] Adding a withRetry method for the upload --- src/utils/errors.ts | 25 +++++++ src/utils/ship/multipartUpload.ts | 88 +++++++++++++++++-------- test/utils/errors.test.ts | 36 ++++++++++ test/utils/ship/multipartUpload.test.ts | 53 ++++++++++++++- 4 files changed, 174 insertions(+), 28 deletions(-) create mode 100644 test/utils/errors.test.ts diff --git a/src/utils/errors.ts b/src/utils/errors.ts index ce757013..bc83df1b 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -9,6 +9,31 @@ export function isNetworkError(exception: any) { return ['ECONNABORTED', 'ERR_NETWORK'].includes(`${exception.code}`) } +// A 4xx means the request itself was wrong, so sending it again gives the same +// answer. These three are the exceptions. A 403 means a signed URL expired. +// A 408 and a 429 both ask the client to come back later. +const RETRYABLE_CLIENT_STATUSES = new Set([403, 408, 429]) + +// fetch answers a failed request with a Response instead of throwing, so the +// response needs converting before it can travel like any other error. +// axios sets `status` on the errors it throws, so this sets the same field. +export function getResponseError(response: Response, what: string) { + return Object.assign(new Error(`${what} failed: ${response.status} ${response.statusText}`), { + status: response.status, + }) +} + +// Decides whether another attempt at a failed request is worth making +export function isRetryable(error: unknown) { + const {status} = error as {status?: number} + + // No status means the request never got an answer. A dropped connection and a + // timeout both land here, and both deserve another attempt. + if (status === undefined) return true + + return status >= 500 || RETRYABLE_CLIENT_STATUSES.has(status) +} + // Util to extract API error messages if present export function getErrorMessage(error: any) { try { diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts index 262381bd..da77c010 100644 --- a/src/utils/ship/multipartUpload.ts +++ b/src/utils/ship/multipartUpload.ts @@ -9,6 +9,7 @@ import { getNewMultipartUpload, } from '@cli/api/index.js' import type {MultipartPartUrl, MultipartUploadTicket, UploadedPart} from '@cli/types' +import {getResponseError, isRetryable} from '@cli/utils/errors.js' import type {LogFunction} from './types.d.js' import type {ProgressData} from './upload.js' @@ -20,8 +21,11 @@ export const MULTIPART_MIN_SIZE = 16 * 1024 * 1024 // One part at a time is slower than a single PUT, so keep this above 1. export const DEFAULT_CONCURRENCY = 8 -export const MAX_PART_ATTEMPTS = 4 -export const RETRY_BASE_DELAY_MS = 500 +// Eight attempts wait at most 61.5 seconds in total. That covers a router +// restart, or a handover between two wifi points. +export const MAX_ATTEMPTS = 8 +const BASE_DELAY_MS = 500 +const MAX_DELAY_MS = 30_000 // The backend signs at most this many parts in one request const SIGN_BATCH_SIZE = 1000 @@ -38,6 +42,40 @@ type PartBody = Uint8Array const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +type OnRetry = (error: unknown, attempt: number, delayMs: number) => void + +// Calls run until it returns, or until the attempts run out. +// Each attempt waits after the one before it, so these awaits belong in a loop. +/* eslint-disable no-await-in-loop */ +export async function withRetry(run: () => Promise, onRetry: OnRetry): Promise { + let lastError: unknown = new Error('The operation did not run') + + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt += 1) { + try { + return await run() + } catch (error) { + lastError = error + if (attempt === MAX_ATTEMPTS || !isRetryable(error)) break + + // Wait longer after each failure, up to the cap. Half of the wait is + // random, so that parts which failed together do not retry together. + const backoff = Math.min(MAX_DELAY_MS, BASE_DELAY_MS * 2 ** (attempt - 1)) + const waitMs = backoff / 2 + Math.random() * (backoff / 2) + onRetry(error, attempt, waitMs) + await delay(waitMs) + } + } + + throw lastError +} +/* eslint-enable no-await-in-loop */ + +// Logs a failed attempt, so a slow upload shows why it is slow +const logRetry = (vlog: LogFunction, what: string): OnRetry => (error, attempt, delayMs) => { + const message = error instanceof Error ? error.message : String(error) + vlog(`${what} attempt ${attempt} failed (${message}). Retrying in ${(delayMs / 1000).toFixed(1)}s...`) +} + // Splits the file into the parts to upload. Every part is partSize bytes, // except the last one. export function calculateParts(zipSize: number, partSize: number): Part[] { @@ -85,56 +123,49 @@ async function getPartUrls(uploadTicketId: string, partNumbers: number[]): Promi return signedBatches.flat() } -// A retry runs after the attempt before it, so these awaits belong in the loop. -/* eslint-disable no-await-in-loop */ -// Uploads one part and returns its ETag. Retries with an increasing delay. +// Uploads one part and returns its ETag async function uploadPart( uploadTicketId: string, part: Part, signedUrl: string, body: PartBody, + vlog: LogFunction, ): Promise { let url = signedUrl - let lastError: Error = new Error(`Part ${part.partNumber} did not upload`) - - for (let attempt = 1; attempt <= MAX_PART_ATTEMPTS; attempt += 1) { - if (attempt > 1) { - await delay(RETRY_BASE_DELAY_MS * 2 ** (attempt - 2)) - } - try { + return withRetry( + async () => { const response = await fetch(url, { body, headers: {'Content-Length': String(body.length)}, method: 'PUT', }) - if (response.ok) { - const etag = response.headers.get('etag') - if (etag) return etag - lastError = new Error(`Part ${part.partNumber} uploaded but returned no ETag`) - } else { - lastError = new Error(`Part ${part.partNumber} failed: ${response.status} ${response.statusText}`) + if (!response.ok) { // A signed URL lasts one hour. A slow upload can outlive it. if (response.status === 403) { const [fresh] = await getMultipartPartUrls(uploadTicketId, [part.partNumber]) url = fresh.url } + + throw getResponseError(response, `Part ${part.partNumber}`) } - } catch (error) { - lastError = error as Error - } - } - throw lastError + const etag = response.headers.get('etag') + if (!etag) throw new Error(`Part ${part.partNumber} uploaded but returned no ETag`) + + return etag + }, + logRetry(vlog, `Part ${part.partNumber}`), + ) } -/* eslint-enable no-await-in-loop */ interface UploadPartsProps { concurrency?: number filePath: string onProgress: (data: ProgressData) => void ticket: MultipartUploadTicket + vlog: LogFunction zipSize: number } @@ -145,6 +176,7 @@ export async function uploadParts({ filePath, onProgress, ticket, + vlog, zipSize, }: UploadPartsProps): Promise { const parts = calculateParts(zipSize, ticket.partSize) @@ -171,7 +203,7 @@ export async function uploadParts({ if (!signedUrl) throw new Error(`The server did not sign part ${part.partNumber}`) const body = await readPart(filePath, part) - const etag = await uploadPart(ticket.id, part, signedUrl, body) + const etag = await uploadPart(ticket.id, part, signedUrl, body, vlog) uploadedBytes += part.size const elapsedSeconds = (Date.now() - startTime) / 1000 @@ -188,7 +220,9 @@ export async function uploadParts({ ), ) - await completeMultipartUpload(ticket.id, uploaded) + // Every byte is uploaded by now. Losing this call to one bad moment would + // throw all of it away, so it gets the same retry as a part. + await withRetry(() => completeMultipartUpload(ticket.id, uploaded), logRetry(vlog, 'Completing the upload')) } catch (error) { // The parts already uploaded cost storage until something removes them await abortMultipartUpload(ticket.id).catch(() => {}) @@ -218,7 +252,7 @@ export async function multipartUpload({ const ticket = await getNewMultipartUpload(projectId, zipSize) vlog(`Uploading in parts of ${Math.round(ticket.partSize / 1024 / 1024)}MB...`) - await uploadParts({concurrency, filePath, onProgress, ticket, zipSize}) + await uploadParts({concurrency, filePath, onProgress, ticket, vlog, zipSize}) return ticket.id } diff --git a/test/utils/errors.test.ts b/test/utils/errors.test.ts new file mode 100644 index 00000000..4fd1ad75 --- /dev/null +++ b/test/utils/errors.test.ts @@ -0,0 +1,36 @@ +import {expect} from 'chai' + +import {getResponseError, isRetryable} from '@cli/utils/errors.js' + +describe('getResponseError (utils/errors)', () => { + it('names what failed and keeps the status', () => { + const error = getResponseError(new Response('no', {status: 403, statusText: 'Forbidden'}), 'Part 3') + + expect(error).to.be.instanceOf(Error) + expect(error.message).to.equal('Part 3 failed: 403 Forbidden') + expect(error.status).to.equal(403) + }) +}) + +describe('isRetryable (utils/errors)', () => { + it('retries an error that has no status', () => { + expect(isRetryable(new Error('fetch failed'))).to.equal(true) + }) + + it('retries a server error and the three wait-and-see client errors', () => { + expect(isRetryable({status: 503})).to.equal(true) + expect(isRetryable({status: 403})).to.equal(true) + expect(isRetryable({status: 408})).to.equal(true) + expect(isRetryable({status: 429})).to.equal(true) + }) + + it('does not retry the other client errors', () => { + expect(isRetryable({status: 400})).to.equal(false) + expect(isRetryable({status: 401})).to.equal(false) + expect(isRetryable({status: 404})).to.equal(false) + }) + + it('reads the status axios sets on the errors it throws', () => { + expect(isRetryable(getResponseError(new Response('no', {status: 404}), 'Part 1'))).to.equal(false) + }) +}) diff --git a/test/utils/ship/multipartUpload.test.ts b/test/utils/ship/multipartUpload.test.ts index db67c171..971d1a56 100644 --- a/test/utils/ship/multipartUpload.test.ts +++ b/test/utils/ship/multipartUpload.test.ts @@ -1,6 +1,6 @@ import {expect} from 'chai' -import {calculateParts} from '@cli/utils/ship/multipartUpload.js' +import {calculateParts, withRetry} from '@cli/utils/ship/multipartUpload.js' const MIB = 1024 * 1024 @@ -40,3 +40,54 @@ describe('calculateParts (ship/multipartUpload)', () => { expect(calculateParts(0, 8 * MIB)).to.deep.equal([]) }) }) + +describe('withRetry (ship/multipartUpload)', () => { + const noop = () => {} + + it('returns the value without retrying when the first attempt works', async () => { + let calls = 0 + const result = await withRetry(async () => { + calls += 1 + return 'ok' + }, noop) + + expect(result).to.equal('ok') + expect(calls).to.equal(1) + }) + + it('stops at once when the error is not worth retrying', async () => { + let calls = 0 + + try { + await withRetry(async () => { + calls += 1 + throw Object.assign(new Error('not found'), {status: 404}) + }, noop) + expect.fail('withRetry should have thrown') + } catch (error) { + expect((error as Error).message).to.equal('not found') + } + + expect(calls).to.equal(1) + }) + + it('retries a retryable error, and tells onRetry which attempt failed', async () => { + const seen: number[] = [] + let calls = 0 + + const result = await withRetry( + async () => { + calls += 1 + if (calls === 1) throw Object.assign(new Error('expired'), {status: 403}) + return 'ok' + }, + (_error, attempt) => { + seen.push(attempt) + }, + ) + + expect(result).to.equal('ok') + expect(calls).to.equal(2) + expect(seen).to.deep.equal([1]) + }) +}) From fc2906a96f69760f54b7b34c14b46594f18bfdb8 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 11:27:41 +0100 Subject: [PATCH 3/8] Reuse the error lib function --- src/utils/ship/upload.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/utils/ship/upload.ts b/src/utils/ship/upload.ts index 4d426046..12aff1db 100644 --- a/src/utils/ship/upload.ts +++ b/src/utils/ship/upload.ts @@ -2,6 +2,7 @@ import fs from 'node:fs' import {Readable, Transform} from 'stream' import {getNewUploadTicket} from '@cli/api/index.js' +import {getResponseError} from '@cli/utils/errors.js' import type {LogFunction} from './types.d.js' @@ -113,9 +114,7 @@ export async function singleUpload({ zipStream: fs.createReadStream(filePath), }) - if (!response.ok) { - throw new Error(`Upload failed: ${response.status} ${response.statusText}`) - } + if (!response.ok) throw getResponseError(response, 'Upload') return uploadTicket.id } From c9052ced8750badc9164fd6668a278cf2f7868f7 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 11:54:51 +0100 Subject: [PATCH 4/8] Adjusting abort, queue and error codes --- src/utils/errors.ts | 7 ++++--- src/utils/ship/multipartUpload.ts | 22 +++++++++++++++++----- test/utils/errors.test.ts | 9 +++++++-- test/utils/ship/multipartUpload.test.ts | 2 +- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/utils/errors.ts b/src/utils/errors.ts index bc83df1b..f314625c 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -10,9 +10,10 @@ export function isNetworkError(exception: any) { } // A 4xx means the request itself was wrong, so sending it again gives the same -// answer. These three are the exceptions. A 403 means a signed URL expired. -// A 408 and a 429 both ask the client to come back later. -const RETRYABLE_CLIENT_STATUSES = new Set([403, 408, 429]) +// answer. These two are the exceptions. Both ask the client to come back later. +// A 403 is not here. On an authenticated call it never recovers, and a caller +// that can recover from one, such as a stale signed URL, handles it itself. +const RETRYABLE_CLIENT_STATUSES = new Set([408, 429]) // fetch answers a failed request with a Response instead of throwing, so the // response needs converting before it can travel like any other error. diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts index da77c010..587cb96c 100644 --- a/src/utils/ship/multipartUpload.ts +++ b/src/utils/ship/multipartUpload.ts @@ -142,10 +142,13 @@ async function uploadPart( }) if (!response.ok) { - // A signed URL lasts one hour. A slow upload can outlive it. + // A signed URL lasts one hour, and a slow upload can outlive it. This is + // the one 403 worth another attempt, so it throws an error with no + // status rather than one isRetryable would refuse. if (response.status === 403) { const [fresh] = await getMultipartPartUrls(uploadTicketId, [part.partNumber]) url = fresh.url + throw new Error(`Part ${part.partNumber} failed: the signed URL expired`) } throw getResponseError(response, `Part ${part.partNumber}`) @@ -224,8 +227,10 @@ export async function uploadParts({ // throw all of it away, so it gets the same retry as a part. await withRetry(() => completeMultipartUpload(ticket.id, uploaded), logRetry(vlog, 'Completing the upload')) } catch (error) { - // The parts already uploaded cost storage until something removes them - await abortMultipartUpload(ticket.id).catch(() => {}) + // Promise.all rejects on the first failed part, but p-limit keeps starting + // the parts behind it. Without this the rest of the file uploads to an + // upload that is already being cancelled. + limit.clearQueue() throw error } } @@ -251,8 +256,15 @@ export async function multipartUpload({ vlog('Requesting multipart upload ticket...') const ticket = await getNewMultipartUpload(projectId, zipSize) - vlog(`Uploading in parts of ${Math.round(ticket.partSize / 1024 / 1024)}MB...`) - await uploadParts({concurrency, filePath, onProgress, ticket, vlog, zipSize}) + try { + vlog(`Uploading in parts of ${Math.round(ticket.partSize / 1024 / 1024)}MB...`) + await uploadParts({concurrency, filePath, onProgress, ticket, vlog, zipSize}) + } catch (error) { + // The parts already uploaded cost storage until something removes them. The + // bucket also clears an upload that was never completed after 7 days. + await abortMultipartUpload(ticket.id).catch(() => {}) + throw error + } return ticket.id } diff --git a/test/utils/errors.test.ts b/test/utils/errors.test.ts index 4fd1ad75..a320492c 100644 --- a/test/utils/errors.test.ts +++ b/test/utils/errors.test.ts @@ -17,9 +17,8 @@ describe('isRetryable (utils/errors)', () => { expect(isRetryable(new Error('fetch failed'))).to.equal(true) }) - it('retries a server error and the three wait-and-see client errors', () => { + it('retries a server error and the two wait-and-see client errors', () => { expect(isRetryable({status: 503})).to.equal(true) - expect(isRetryable({status: 403})).to.equal(true) expect(isRetryable({status: 408})).to.equal(true) expect(isRetryable({status: 429})).to.equal(true) }) @@ -30,6 +29,12 @@ describe('isRetryable (utils/errors)', () => { expect(isRetryable({status: 404})).to.equal(false) }) + // An authenticated call cannot recover from a 403. A caller that can, such as + // uploadPart with a stale signed URL, handles its own 403. + it('does not retry a 403', () => { + expect(isRetryable({status: 403})).to.equal(false) + }) + it('reads the status axios sets on the errors it throws', () => { expect(isRetryable(getResponseError(new Response('no', {status: 404}), 'Part 1'))).to.equal(false) }) diff --git a/test/utils/ship/multipartUpload.test.ts b/test/utils/ship/multipartUpload.test.ts index 971d1a56..42ccf791 100644 --- a/test/utils/ship/multipartUpload.test.ts +++ b/test/utils/ship/multipartUpload.test.ts @@ -78,7 +78,7 @@ describe('withRetry (ship/multipartUpload)', () => { const result = await withRetry( async () => { calls += 1 - if (calls === 1) throw Object.assign(new Error('expired'), {status: 403}) + if (calls === 1) throw Object.assign(new Error('busy'), {status: 429}) return 'ok' }, (_error, attempt) => { From 8cf85df56cad5ed8cb5cda28d585f6e4b1a40b74 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 13:35:50 +0100 Subject: [PATCH 5/8] readPart adjustments for potential NFS issues based on copilot feedback --- src/utils/ship/multipartUpload.ts | 25 ++++++++--- test/utils/ship/multipartUpload.test.ts | 56 ++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts index 587cb96c..815aa6ba 100644 --- a/src/utils/ship/multipartUpload.ts +++ b/src/utils/ship/multipartUpload.ts @@ -93,15 +93,27 @@ export function calculateParts(zipSize: number, partSize: number): Part[] { // Reads one part of the file into memory. The bytes stay in memory until the // part uploads, so a retry does not read the file again. -async function readPart(filePath: string, part: Part): Promise { +// Each read starts where the one before it stopped, so these awaits belong in +// the loop. +/* eslint-disable no-await-in-loop */ +export async function readPart(filePath: string, part: Part): Promise { const handle = await fs.promises.open(filePath, 'r') try { const body = new Uint8Array(part.size) - 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}`) + let filled = 0 + + // A network mount can return less than it was asked for. That is not the + // end of the file, so ask again for the rest. Only a read of no bytes means + // there is nothing left to read. + while (filled < part.size) { + const {bytesRead} = await handle.read(body, filled, part.size - filled, part.start + filled) + if (bytesRead === 0) break + filled += bytesRead + } + + // Uploading a short part would pad it with zeros and corrupt the zip + if (filled !== part.size) { + throw new Error(`Part ${part.partNumber} read ${filled} bytes, expected ${part.size}`) } return body @@ -109,6 +121,7 @@ async function readPart(filePath: string, part: Part): Promise { await handle.close() } } +/* eslint-enable no-await-in-loop */ // Signs every part number. The backend limits how many it signs in one request, // so the part numbers go up in batches. diff --git a/test/utils/ship/multipartUpload.test.ts b/test/utils/ship/multipartUpload.test.ts index 42ccf791..70913c5d 100644 --- a/test/utils/ship/multipartUpload.test.ts +++ b/test/utils/ship/multipartUpload.test.ts @@ -1,6 +1,10 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + import {expect} from 'chai' -import {calculateParts, withRetry} from '@cli/utils/ship/multipartUpload.js' +import {calculateParts, readPart, withRetry} from '@cli/utils/ship/multipartUpload.js' const MIB = 1024 * 1024 @@ -91,3 +95,53 @@ describe('withRetry (ship/multipartUpload)', () => { expect(seen).to.deep.equal([1]) }) }) + +describe('readPart (ship/multipartUpload)', () => { + const filePath = path.join(os.tmpdir(), 'shipthis-readPart-test.bin') + + // Byte i of the file is i % 251, so a wrong offset is easy to spot + const contents = Uint8Array.from({length: 4096}, (_, i) => i % 251) + + beforeEach(() => fs.writeFileSync(filePath, contents)) + afterEach(() => fs.rmSync(filePath, {force: true})) + + it('reads the bytes at the start of the part, not the start of the file', async () => { + const body = await readPart(filePath, {partNumber: 2, size: 100, start: 1000}) + + expect(body).to.have.length(100) + expect([...body]).to.deep.equal([...contents.subarray(1000, 1100)]) + }) + + it('keeps reading when the filesystem gives back less than it was asked for', async () => { + // A network mount can answer with part of the request. Make every read + // return at most 30 bytes, so filling a 100 byte part needs several. + const handle = await fs.promises.open(filePath, 'r') + const fileHandle = Object.getPrototypeOf(handle) + const realRead = fileHandle.read + await handle.close() + + let reads = 0 + fileHandle.read = function (buffer: Uint8Array, offset: number, length: number, position: number) { + reads += 1 + return realRead.call(this, buffer, offset, Math.min(length, 30), position) + } + + try { + const body = await readPart(filePath, {partNumber: 1, size: 100, start: 0}) + + expect(reads).to.be.greaterThan(1) + expect([...body]).to.deep.equal([...contents.subarray(0, 100)]) + } finally { + fileHandle.read = realRead + } + }) + + it('fails when the file ends before the part is full', async () => { + try { + await readPart(filePath, {partNumber: 9, size: 500, start: 4000}) + expect.fail('readPart should have thrown') + } catch (error) { + expect((error as Error).message).to.equal('Part 9 read 96 bytes, expected 500') + } + }) +}) From f9a6d17cb36c1b546371d4f11f1572020dcf4557 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 13:43:08 +0100 Subject: [PATCH 6/8] eslint disable not needed --- src/utils/ship/multipartUpload.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts index 815aa6ba..3af9cae6 100644 --- a/src/utils/ship/multipartUpload.ts +++ b/src/utils/ship/multipartUpload.ts @@ -46,7 +46,6 @@ type OnRetry = (error: unknown, attempt: number, delayMs: number) => void // Calls run until it returns, or until the attempts run out. // Each attempt waits after the one before it, so these awaits belong in a loop. -/* eslint-disable no-await-in-loop */ export async function withRetry(run: () => Promise, onRetry: OnRetry): Promise { let lastError: unknown = new Error('The operation did not run') @@ -68,7 +67,6 @@ export async function withRetry(run: () => Promise, onRetry: OnRetry): Pro throw lastError } -/* eslint-enable no-await-in-loop */ // Logs a failed attempt, so a slow upload shows why it is slow const logRetry = (vlog: LogFunction, what: string): OnRetry => (error, attempt, delayMs) => { @@ -95,7 +93,6 @@ export function calculateParts(zipSize: number, partSize: number): Part[] { // part uploads, so a retry does not read the file again. // Each read starts where the one before it stopped, so these awaits belong in // the loop. -/* eslint-disable no-await-in-loop */ export async function readPart(filePath: string, part: Part): Promise { const handle = await fs.promises.open(filePath, 'r') try { @@ -121,7 +118,6 @@ export async function readPart(filePath: string, part: Part): Promise await handle.close() } } -/* eslint-enable no-await-in-loop */ // Signs every part number. The backend limits how many it signs in one request, // so the part numbers go up in batches. From e7faab4e2b43b1091ccf8309194acc0598f760e0 Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 15:15:56 +0100 Subject: [PATCH 7/8] Retry based on s3 response code. --- src/utils/errors.ts | 56 ++++++++++++++++++++----------- src/utils/ship/multipartUpload.ts | 4 +-- src/utils/ship/upload.ts | 4 +-- test/utils/errors.test.ts | 54 +++++++++++++++++++++++++---- 4 files changed, 89 insertions(+), 29 deletions(-) diff --git a/src/utils/errors.ts b/src/utils/errors.ts index f314625c..7cf772df 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -9,30 +9,48 @@ export function isNetworkError(exception: any) { return ['ECONNABORTED', 'ERR_NETWORK'].includes(`${exception.code}`) } -// A 4xx means the request itself was wrong, so sending it again gives the same -// answer. These two are the exceptions. Both ask the client to come back later. -// A 403 is not here. On an authenticated call it never recovers, and a caller -// that can recover from one, such as a stale signed URL, handles it itself. -const RETRYABLE_CLIENT_STATUSES = new Set([408, 429]) - -// fetch answers a failed request with a Response instead of throwing, so the -// response needs converting before it can travel like any other error. -// axios sets `status` on the errors it throws, so this sets the same field. -export function getResponseError(response: Response, what: string) { - return Object.assign(new Error(`${what} failed: ${response.status} ${response.statusText}`), { - status: response.status, - }) +// A 4xx means the request was wrong, so sending it again gives the same answer. +// These two ask the client to come back later. A 403 is not here - a caller that +// can recover from one, such as a stale signed URL, handles it itself. +const RETRYABLE_CLIENT_STATUSES = [408, 429] + +// S3 uses 400 for a socket that went quiet, which is temporary. The status +// cannot tell that apart from a request that was really wrong, so the name does. +const RETRYABLE_S3_CODES = [ + 'InternalError', + 'RequestTimeout', + 'RequestTimeoutException', + 'ServiceUnavailable', + 'SlowDown', +] + +// The two fields isRetryable reads. axios already sets `status` on what it throws. +type RequestError = Error & {code?: string; status?: number} + +// Converts a failed S3 request into an error. fetch does not throw on a bad +// status, and `400 Bad Request` on its own tells nobody anything, so the name +// and sentence from the small XML body S3 sends go into the message. +export async function getS3Error(response: Response, what: string) { + const body = await response.text().catch(() => '') + const code = /([^<]+)<\/Code>/.exec(body)?.[1] + const message = /([^<]+)<\/Message>/.exec(body)?.[1] + const detail = [code ?? response.statusText, message].filter(Boolean).join(' - ') + + const error: RequestError = new Error(`${what} failed: ${response.status} ${detail}`) + error.code = code + error.status = response.status + + return error } // Decides whether another attempt at a failed request is worth making export function isRetryable(error: unknown) { - const {status} = error as {status?: number} - - // No status means the request never got an answer. A dropped connection and a - // timeout both land here, and both deserve another attempt. + const {code, status} = error as RequestError + // An S3 name is more exact than the status, so it answers first + if (code !== undefined) return RETRYABLE_S3_CODES.includes(code) + // No status means the request never got an answer, which is worth another try if (status === undefined) return true - - return status >= 500 || RETRYABLE_CLIENT_STATUSES.has(status) + return status >= 500 || RETRYABLE_CLIENT_STATUSES.includes(status) } // Util to extract API error messages if present diff --git a/src/utils/ship/multipartUpload.ts b/src/utils/ship/multipartUpload.ts index 3af9cae6..5a6a10af 100644 --- a/src/utils/ship/multipartUpload.ts +++ b/src/utils/ship/multipartUpload.ts @@ -9,7 +9,7 @@ import { getNewMultipartUpload, } from '@cli/api/index.js' import type {MultipartPartUrl, MultipartUploadTicket, UploadedPart} from '@cli/types' -import {getResponseError, isRetryable} from '@cli/utils/errors.js' +import {getS3Error, isRetryable} from '@cli/utils/errors.js' import type {LogFunction} from './types.d.js' import type {ProgressData} from './upload.js' @@ -160,7 +160,7 @@ async function uploadPart( throw new Error(`Part ${part.partNumber} failed: the signed URL expired`) } - throw getResponseError(response, `Part ${part.partNumber}`) + throw await getS3Error(response, `Part ${part.partNumber}`) } const etag = response.headers.get('etag') diff --git a/src/utils/ship/upload.ts b/src/utils/ship/upload.ts index 12aff1db..44b71b51 100644 --- a/src/utils/ship/upload.ts +++ b/src/utils/ship/upload.ts @@ -2,7 +2,7 @@ import fs from 'node:fs' import {Readable, Transform} from 'stream' import {getNewUploadTicket} from '@cli/api/index.js' -import {getResponseError} from '@cli/utils/errors.js' +import {getS3Error} from '@cli/utils/errors.js' import type {LogFunction} from './types.d.js' @@ -114,7 +114,7 @@ export async function singleUpload({ zipStream: fs.createReadStream(filePath), }) - if (!response.ok) throw getResponseError(response, 'Upload') + if (!response.ok) throw await getS3Error(response, 'Upload') return uploadTicket.id } diff --git a/test/utils/errors.test.ts b/test/utils/errors.test.ts index a320492c..f607fe51 100644 --- a/test/utils/errors.test.ts +++ b/test/utils/errors.test.ts @@ -1,15 +1,34 @@ import {expect} from 'chai' -import {getResponseError, isRetryable} from '@cli/utils/errors.js' +import {getS3Error, isRetryable} from '@cli/utils/errors.js' -describe('getResponseError (utils/errors)', () => { - it('names what failed and keeps the status', () => { - const error = getResponseError(new Response('no', {status: 403, statusText: 'Forbidden'}), 'Part 3') +// The shape Spaces really answers with, taken from a live failed request +const s3Error = (code: string, message: string) => + `${code}${message}` + + `bucket/key.zipnot available` + +describe('getS3Error (utils/errors)', () => { + it('names what failed and keeps the status', async () => { + const error = await getS3Error(new Response('no', {status: 403, statusText: 'Forbidden'}), 'Part 3') expect(error).to.be.instanceOf(Error) expect(error.message).to.equal('Part 3 failed: 403 Forbidden') expect(error.status).to.equal(403) }) + + it('says what S3 said, rather than the status text', async () => { + const body = s3Error( + 'RequestTimeout', + 'Your socket connection to the server was not read from or written to within the timeout period.', + ) + const error = await getS3Error(new Response(body, {status: 400, statusText: 'Bad Request'}), 'Part 8') + + expect(error.code).to.equal('RequestTimeout') + expect(error.message).to.equal( + 'Part 8 failed: 400 RequestTimeout - Your socket connection to the server was not read from ' + + 'or written to within the timeout period.', + ) + }) }) describe('isRetryable (utils/errors)', () => { @@ -35,7 +54,30 @@ describe('isRetryable (utils/errors)', () => { expect(isRetryable({status: 403})).to.equal(false) }) - it('reads the status axios sets on the errors it throws', () => { - expect(isRetryable(getResponseError(new Response('no', {status: 404}), 'Part 1'))).to.equal(false) + it('reads the status axios sets on the errors it throws', async () => { + expect(isRetryable(await getS3Error(new Response('', {status: 404}), 'Part 1'))).to.equal(false) + }) + + // A dropped network leaves a part half sent, and S3 answers 400 RequestTimeout. + // Giving up there loses the whole upload, which is what a 700MB test did. + it('retries a 400 that S3 named RequestTimeout', async () => { + const body = s3Error('RequestTimeout', 'Your socket connection to the server was not read from or written to.') + const error = await getS3Error(new Response(body, {status: 400, statusText: 'Bad Request'}), 'Part 8') + + expect(isRetryable(error)).to.equal(true) + }) + + it('does not retry a 400 that S3 named InvalidPart', async () => { + const body = s3Error('InvalidPart', 'One or more of the specified parts could not be found.') + const error = await getS3Error(new Response(body, {status: 400, statusText: 'Bad Request'}), 'Complete') + + expect(isRetryable(error)).to.equal(false) + }) + + it('falls back to the status when the body names nothing', async () => { + const error = await getS3Error(new Response('not xml', {status: 503}), 'Part 1') + + expect(error.code).to.equal(undefined) + expect(isRetryable(error)).to.equal(true) }) }) From abd7d9a64d3bbf6853f84f42acca7f9ac83fba6b Mon Sep 17 00:00:00 2001 From: David Sutherland Date: Tue, 25 Aug 2026 15:43:27 +0100 Subject: [PATCH 8/8] Unhide the --skipMultipart flag and document the multi-part upload behaviour --- docs/game/ship.md | 24 +++++++++++++++++++++++- src/commands/game/ship.tsx | 3 +-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/docs/game/ship.md b/docs/game/ship.md index 8c8a54cb..c9053e44 100644 --- a/docs/game/ship.md +++ b/docs/game/ship.md @@ -45,6 +45,25 @@ shipthis game ship --platform ios --follow --useDemoCredentials --download game. shipthis game ship --platform android --follow --useDemoCredentials --downloadAPK game.apk ``` +### Uploading a large game + +ShipThis makes a zip of your game. For a zip of 16MB or more, ShipThis sends the zip in +several parts at the same time. This is faster than one request. + +Each part is separate. If the network fails, ShipThis sends that part again. The parts that +arrived stay on the server. + +ShipThis sends a zip smaller than 16MB in one request. Parts do not make a small zip faster. + +To send the zip in one request, use `--skipMultipart`. This method is slower, and the zip +must be smaller than 5GB. Use this flag only if the upload in parts fails. + +```bash +shipthis game ship --platform android --skipMultipart +``` + +To see each part, and to see ShipThis send a part again, add `--verbose`. + ### Overriding the Godot version You can specify a different Godot version to use only for the current job. This can be helpful if you are upgrading your game to use a newer version of Godot. @@ -58,7 +77,8 @@ shipthis game ship --platform android --follow --gameEngineVersion 4.5.1 --downl ```help USAGE $ shipthis game ship [-g ] [--download --platform android|ios] [--downloadAPK ] - [--follow ] [--skipPublish] [--verbose] [--useDemoCredentials ] [--gameEngineVersion ] [--dryRun] + [--follow ] [--skipMultipart] [--skipPublish] [--verbose] [--useDemoCredentials ] + [--gameEngineVersion ] [--dryRun] FLAGS -g, --gameId= The ID of the game @@ -70,6 +90,8 @@ FLAGS --gameEngineVersion= Override the specified game engine version for this build --platform=