diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ac9aa32..60a16027 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **Pinned `starlette` and `botocore` so self-hosted Docker builds are reproducible** — both were floating transitives, so rebuilding the same commit on a different day could silently install different versions with no diff and no PR. FastAPI declares `starlette>=0.46.0` with no upper bound, meaning builds were free to cross a Starlette major (the ASGI layer under the SSE endpoint and the middleware stack); `botocore` is where S3 request signing lives, and unreviewed moves there have broken S3 compatibility before. Both are pinned to the versions already resolving, so no installed version changes. +### Fixed +- **A single transient storage error no longer discards an entire multipart upload** — each part is now retried up to 8 times with exponential backoff and jitter (≈254s of tolerance per part) instead of failing the whole upload on the first non-OK response. A multi-gigabyte file is several hundred sequential requests, so one 500/503 from the object store or a brief network drop was enough to throw away everything already transferred; on a slow uplink that can be an hour of work. The presigned URL is re-fetched on every attempt, since `presign_upload_part` expires after an hour and a large upload can outlive that. User-initiated cancels are never retried, and a 4xx is thrown straight through instead of being repeated eight times over four minutes — it would fail identically every time, so retrying it only delays the error. The part loop, previously duplicated between new-asset and new-version uploads, is now shared. + ## [1.7.5] - 2026-07-23 ### Fixed diff --git a/apps/web/stores/__tests__/upload-store.test.ts b/apps/web/stores/__tests__/upload-store.test.ts new file mode 100644 index 00000000..98dff87d --- /dev/null +++ b/apps/web/stores/__tests__/upload-store.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' + +vi.mock('@/lib/api', () => ({ + api: { + post: vi.fn(), + get: vi.fn(), + }, +})) + +import { api } from '@/lib/api' +import { uploadAllParts } from '../upload-store' + +const CHUNK_SIZE = 10 * 1024 * 1024 + +/** Builds a File of `bytes` length; 15 MB spans two 10 MB parts. */ +function makeFile(bytes: number): File { + return new File([new Uint8Array(bytes)], 'clip.mp4', { type: 'video/mp4' }) +} + +function okResponse(etag: string) { + return { ok: true, headers: { get: () => etag } } +} + +function failResponse(status = 503, statusText = 'Service Unavailable') { + return { ok: false, status, statusText, headers: { get: () => null } } +} + +describe('uploadAllParts', () => { + let controller: AbortController + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + controller = new AbortController() + // Every attempt fetches its own presigned URL. + vi.mocked(api.post).mockResolvedValue({ presigned_url: 'https://s3.example/part' }) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('uploads each part in order and reports the collected ETags', async () => { + global.fetch = vi.fn() + .mockResolvedValueOnce(okResponse('"etag-1"')) + .mockResolvedValueOnce(okResponse('"etag-2"')) as unknown as typeof fetch + + const onProgress = vi.fn() + const promise = uploadAllParts(makeFile(CHUNK_SIZE + 5_000_000), 'key', 'upload-1', controller, onProgress) + await vi.runAllTimersAsync() + + expect(await promise).toEqual([ + { PartNumber: 1, ETag: '"etag-1"' }, + { PartNumber: 2, ETag: '"etag-2"' }, + ]) + expect(onProgress).toHaveBeenLastCalledWith(95) + }) + + it('retries a part that fails transiently and still succeeds', async () => { + global.fetch = vi.fn() + .mockResolvedValueOnce(failResponse()) + .mockResolvedValueOnce(failResponse()) + .mockResolvedValueOnce(okResponse('"etag-1"')) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + await vi.runAllTimersAsync() + + expect(await promise).toEqual([{ PartNumber: 1, ETag: '"etag-1"' }]) + expect(global.fetch).toHaveBeenCalledTimes(3) + }) + + it('re-fetches the presigned URL on every attempt, since it can expire mid-backoff', async () => { + global.fetch = vi.fn() + .mockResolvedValueOnce(failResponse()) + .mockResolvedValueOnce(okResponse('"etag-1"')) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + await vi.runAllTimersAsync() + await promise + + const presignCalls = vi.mocked(api.post).mock.calls.filter((c) => c[0] === '/upload/presign-part') + expect(presignCalls).toHaveLength(2) + }) + + it('gives up after the attempt limit and surfaces the last error', async () => { + global.fetch = vi.fn().mockResolvedValue(failResponse(500, 'Internal Server Error')) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + const assertion = expect(promise).rejects.toThrow(/Part 1 failed/) + await vi.runAllTimersAsync() + await assertion + + expect(global.fetch).toHaveBeenCalledTimes(8) + }) + + it('does not retry a 4xx, which would fail identically every time', async () => { + global.fetch = vi.fn().mockResolvedValue(failResponse(403, 'Forbidden')) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + const assertion = expect(promise).rejects.toThrow(/Forbidden/) + await vi.runAllTimersAsync() + await assertion + + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('does retry a 429, which explicitly invites a later attempt', async () => { + global.fetch = vi.fn() + .mockResolvedValueOnce(failResponse(429, 'Too Many Requests')) + .mockResolvedValueOnce(okResponse('"etag-1"')) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + await vi.runAllTimersAsync() + + expect(await promise).toEqual([{ PartNumber: 1, ETag: '"etag-1"' }]) + expect(global.fetch).toHaveBeenCalledTimes(2) + }) + + it('does not retry once the upload was cancelled', async () => { + global.fetch = vi.fn().mockImplementation(() => { + controller.abort() + return Promise.reject(new DOMException('The operation was aborted', 'AbortError')) + }) as unknown as typeof fetch + + const promise = uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()) + const assertion = expect(promise).rejects.toThrow(/aborted/) + await vi.runAllTimersAsync() + await assertion + + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('throws before any request when cancelled upfront', async () => { + global.fetch = vi.fn() as unknown as typeof fetch + controller.abort() + + await expect( + uploadAllParts(makeFile(1024), 'key', 'upload-1', controller, vi.fn()), + ).rejects.toThrow(/cancelled/) + + expect(global.fetch).not.toHaveBeenCalled() + }) +}) diff --git a/apps/web/stores/upload-store.ts b/apps/web/stores/upload-store.ts index 9b4619b5..cb6c51b0 100644 --- a/apps/web/stores/upload-store.ts +++ b/apps/web/stores/upload-store.ts @@ -5,6 +5,127 @@ import type { AssetResponse } from '@/types' const CHUNK_SIZE = 10 * 1024 * 1024 // 10 MB const HISTORY_PAGE_SIZE = 20 +const PART_MAX_ATTEMPTS = 8 // per part, including the first try +const PART_RETRY_BASE_MS = 2000 // 2s, 4s, 8s … ≈254s of total tolerance per part + +/** + * A failure that repeating cannot fix — a 4xx from the storage backend means the + * request itself was rejected (bad signature, expired policy, wrong length), and + * every further attempt is rejected identically. + */ +interface PartUploadError extends Error { + permanent?: boolean +} + +/** 408 Request Timeout and 429 Too Many Requests explicitly invite a later attempt. */ +function isPermanentStatus(status: number): boolean { + return status >= 400 && status < 500 && status !== 408 && status !== 429 +} + +/** Uploads one part exactly once. Returns its ETag. */ +async function uploadPartOnce( + file: File, + s3Key: string, + uploadId: string, + partNumber: number, + controller: AbortController, +): Promise { + const start = (partNumber - 1) * CHUNK_SIZE + const chunk = file.slice(start, Math.min(start + CHUNK_SIZE, file.size)) + + // The presigned URL is fetched per attempt, not cached across retries: + // presign_upload_part expires after an hour and a large upload can outlive + // that, so a URL obtained before a long backoff may already be dead. + const { presigned_url } = await api.post<{ presigned_url: string }>('/upload/presign-part', { + s3_key: s3Key, + upload_id: uploadId, + part_number: partNumber, + }) + + const putResponse = await fetch(presigned_url, { + method: 'PUT', + body: chunk, + signal: controller.signal, + }) + + if (!putResponse.ok) { + const error: PartUploadError = new Error(`Part ${partNumber} failed: ${putResponse.statusText}`) + error.permanent = isPermanentStatus(putResponse.status) + throw error + } + + return putResponse.headers.get('ETag') ?? '' +} + +/** + * Uploads one part, retrying with exponential backoff and jitter. + * + * A multi-gigabyte file is several hundred requests. Previously the first + * non-OK response aborted the entire upload without a second attempt, so a + * single transient 500/503 from the storage backend — or a brief network drop — + * discarded everything already transferred. + * + * Only failures that can plausibly recover are repeated: network errors and 5xx. + * A 4xx is thrown straight through, because retrying it eight times over four + * minutes would delay the error message without ever changing the outcome. + * + * Jitter keeps concurrent uploads from retrying in lockstep. A user-initiated + * cancel is never retried. + */ +async function uploadPart( + file: File, + s3Key: string, + uploadId: string, + partNumber: number, + controller: AbortController, +): Promise { + let lastError: unknown + + for (let attempt = 1; attempt <= PART_MAX_ATTEMPTS; attempt++) { + try { + return await uploadPartOnce(file, s3Key, uploadId, partNumber, controller) + } catch (err) { + if (controller.signal.aborted) throw err + if (err instanceof DOMException && err.name === 'AbortError') throw err + if ((err as PartUploadError)?.permanent) throw err + + lastError = err + if (attempt === PART_MAX_ATTEMPTS) break + + const delay = PART_RETRY_BASE_MS * 2 ** (attempt - 1) + Math.random() * 250 + await new Promise((resolve) => setTimeout(resolve, delay)) + } + } + + throw lastError +} + +/** + * Uploads every part of a multipart upload in order and returns the part list + * for /upload/complete. Exported for tests. + */ +export async function uploadAllParts( + file: File, + s3Key: string, + uploadId: string, + controller: AbortController, + onProgress: (percent: number) => void, +): Promise> { + const totalChunks = Math.ceil(file.size / CHUNK_SIZE) + const parts: Array<{ PartNumber: number; ETag: string }> = [] + + for (let partNumber = 1; partNumber <= totalChunks; partNumber++) { + if (controller.signal.aborted) { + throw new DOMException('Upload cancelled', 'AbortError') + } + + const etag = await uploadPart(file, s3Key, uploadId, partNumber, controller) + parts.push({ PartNumber: partNumber, ETag: etag }) + onProgress(Math.round((partNumber / totalChunks) * 95)) + } + + return parts +} export type UploadStatus = 'pending' | 'uploading' | 'processing' | 'complete' | 'failed' | 'cancelled' @@ -179,39 +300,9 @@ const storeCreator: StateCreator = updateFile(id, { uploadId: upload_id, assetId: asset_id, versionId: version_id }) - const totalChunks = Math.ceil(file.size / CHUNK_SIZE) - const parts: Array<{ PartNumber: number; ETag: string }> = [] - - for (let partNumber = 1; partNumber <= totalChunks; partNumber++) { - if (controller.signal.aborted) { - throw new DOMException('Upload cancelled', 'AbortError') - } - - const start = (partNumber - 1) * CHUNK_SIZE - const end = Math.min(start + CHUNK_SIZE, file.size) - const chunk = file.slice(start, end) - - const { presigned_url } = await api.post<{ presigned_url: string }>('/upload/presign-part', { - s3_key, - upload_id, - part_number: partNumber, - }) - - const putResponse = await fetch(presigned_url, { - method: 'PUT', - body: chunk, - signal: controller.signal, - }) - - if (!putResponse.ok) { - throw new Error(`Part ${partNumber} failed: ${putResponse.statusText}`) - } - - const etag = putResponse.headers.get('ETag') ?? '' - parts.push({ PartNumber: partNumber, ETag: etag }) - - updateFile(id, { progress: Math.round((partNumber / totalChunks) * 95) }) - } + const parts = await uploadAllParts(file, s3_key, upload_id, controller, (percent) => + updateFile(id, { progress: percent }), + ) await api.post('/upload/complete', { s3_key, @@ -293,20 +384,9 @@ const storeCreator: StateCreator = version_id = initRes.version_id updateFile(id, { uploadId: upload_id, versionId: version_id }) - const totalChunks = Math.ceil(file.size / CHUNK_SIZE) - const parts: Array<{ PartNumber: number; ETag: string }> = [] - for (let partNumber = 1; partNumber <= totalChunks; partNumber++) { - if (controller.signal.aborted) throw new DOMException('Upload cancelled', 'AbortError') - const start = (partNumber - 1) * CHUNK_SIZE - const chunk = file.slice(start, Math.min(start + CHUNK_SIZE, file.size)) - const { presigned_url } = await api.post<{ presigned_url: string }>('/upload/presign-part', { - s3_key, upload_id, part_number: partNumber, - }) - const putResponse = await fetch(presigned_url, { method: 'PUT', body: chunk, signal: controller.signal }) - if (!putResponse.ok) throw new Error(`Part ${partNumber} failed: ${putResponse.statusText}`) - parts.push({ PartNumber: partNumber, ETag: putResponse.headers.get('ETag') ?? '' }) - updateFile(id, { progress: Math.round((partNumber / totalChunks) * 95) }) - } + const parts = await uploadAllParts(file, s3_key, upload_id, controller, (percent) => + updateFile(id, { progress: percent }), + ) await api.post('/upload/complete', { s3_key, upload_id, asset_id: assetId, version_id, parts }) const isMedia = file.type.startsWith('video/') || file.type.startsWith('audio/') || file.type.startsWith('image/')