diff --git a/packages/@uppy/aws-s3/src/S3Uploader.ts b/packages/@uppy/aws-s3/src/S3Uploader.ts index bc0818a83..66e3beefa 100644 --- a/packages/@uppy/aws-s3/src/S3Uploader.ts +++ b/packages/@uppy/aws-s3/src/S3Uploader.ts @@ -80,6 +80,10 @@ export default class S3Uploader { #uploadId?: string #uploadHasStarted: boolean = false #abortController: AbortController | undefined + // Both pause() and abort() abort the same signal; this flag lets the + // signal-less create checkpoint tell a resumable pause apart from a + // terminal cancel. + #paused: boolean = false constructor(options: S3UploaderOptions) { if (options.file.data == null) { @@ -181,6 +185,8 @@ export default class S3Uploader { } async start(): Promise { + // A (re)start clears any prior pause. + this.#paused = false // Abort any pending operations (if not already aborted) this.#abortController?.abort() // Always create a fresh AbortController (also for resume) @@ -198,6 +204,7 @@ export default class S3Uploader { } pause(): void { + this.#paused = true this.#abortController?.abort() } @@ -206,6 +213,8 @@ export default class S3Uploader { * @param opts - `abortInS3`: Whether to also abort the multipart upload in S3. Default: true. Set to false to keep the multipart upload in S3 active, allowing for manual cleanup later and preventing accidental data loss if the user later tries to resume the upload. */ abort(opts?: { abortInS3?: boolean }): void { + // aboring request + this.#paused = false this.#abortController?.abort() // Clean up event listeners this.#eventManager.remove() @@ -296,13 +305,22 @@ export default class S3Uploader { this.#key = key this.#uploadId = uploadId - // The file may have been removed/cancelled while createMultipartUpload was - // in flight. We deliberately don't pass the abort signal into the create + // The file may have been paused/removed/cancelled while createMultipartUpload + // was in flight. We deliberately don't pass the abort signal into the create // request: if it were cancelled mid-flight, S3 might still create the upload // while we never receive the uploadId — an orphan we couldn't clean up. - // Instead we let create finish, then abort the upload in S3 so it isn't left - // behind, and skip persisting resume state for a cancelled upload. + // So we let create finish, then act on *why* the signal was aborted: if (this.#abortController?.signal.aborted) { + if (this.#paused) { + // Paused, not cancelled: keep the S3 upload alive and persist resume + // state so start()/Golden Retriever can continue from the parts. + this.#options.uppy.setFileState(this.#options.file.id, { + s3Multipart: { uploadId, key }, + }) + return + } + // Cancelled: abort the now-created upload in S3 so it isn't left behind, + // and skip persisting resume state. this.abort() return } diff --git a/packages/@uppy/aws-s3/tests/pause-during-create.test.ts b/packages/@uppy/aws-s3/tests/pause-during-create.test.ts new file mode 100644 index 000000000..22b98c066 --- /dev/null +++ b/packages/@uppy/aws-s3/tests/pause-during-create.test.ts @@ -0,0 +1,170 @@ +import { HttpResponse, http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest' +import 'whatwg-fetch' +import Core from '@uppy/core' +import AwsS3 from '../src/index.js' + +const MB = 1024 * 1024 +const s3Url = 'https://test-bucket.s3.us-east-1.amazonaws.com/:key' +const server = setupServer() + +const createMultipartXml = (uploadId: string, key: string) => + `${uploadId}${key}` + +describe('pause during createMultipartUpload', () => { + beforeAll(() => server.listen({ onUnhandledRequest: 'error' })) + afterEach(() => server.resetHandlers()) + afterAll(() => server.close()) + + it('must NOT abort the S3 upload when merely PAUSED (only cancel should)', async () => { + const operations: string[] = [] + let markCreateStarted!: () => void + const createStarted = new Promise((r) => { + markCreateStarted = r + }) + let releaseCreate!: () => void + const createGate = new Promise((r) => { + releaseCreate = r + }) + + const signRequest = vi.fn(async (req: any) => { + const params = new URLSearchParams() + if (req.uploadId) params.set('uploadId', req.uploadId) + if (req.partNumber) params.set('partNumber', String(req.partNumber)) + params.set('method', req.method) + return { + url: `https://test-bucket.s3.us-east-1.amazonaws.com/${req.key}?${params}`, + } + }) + + server.use( + http.post(s3Url, async ({ request }) => { + const hasUploadId = new URL(request.url).searchParams.has('uploadId') + if (!hasUploadId) { + operations.push('createMultipart') + markCreateStarted() + await createGate // hang create until the test releases it + return new HttpResponse(createMultipartXml('upload-1', 'test-key'), { + status: 200, + headers: { 'Content-Type': 'application/xml' }, + }) + } + operations.push('completeMultipart') + return new HttpResponse('', { status: 200 }) + }), + http.put(s3Url, () => { + operations.push('uploadPart') + return new HttpResponse('', { status: 200, headers: { ETag: '"e"' } }) + }), + http.get(s3Url, () => { + operations.push('listParts') + return new HttpResponse( + '', + { status: 200, headers: { 'Content-Type': 'application/xml' } }, + ) + }), + http.delete(s3Url, () => { + operations.push('abortMultipart') + return new HttpResponse('', { status: 204 }) + }), + ) + + const core = new Core().use(AwsS3, { + s3Endpoint: 'https://test-bucket.s3.us-east-1.amazonaws.com', + region: 'us-east-1', + signRequest, + shouldUseMultipart: true, + }) + core.addFile({ + source: 'test', + name: 'big.bin', + type: 'application/octet-stream', + data: new File([new Uint8Array(6 * MB)], 'big.bin'), + }) + const fileId = Object.keys(core.getState().files)[0] + + const uploadPromise = core.upload() + uploadPromise?.catch(() => {}) // may hang/reject on the bug — don't fail on it + + await createStarted // createMultipartUpload is now in flight + core.pauseResume(fileId) // PAUSE (not remove) during the create window + releaseCreate() // let create finish so S3 returns the uploadId + + await new Promise((r) => setTimeout(r, 50)) // let the post-create check run + + // A pause must keep the upload alive & resumable — it must NOT be deleted in S3. + expect(operations).not.toContain('abortMultipart') + // ...and resume state must be persisted so it can actually resume. + expect(core.getFile(fileId)?.s3Multipart).toBeDefined() + + core.cancelAll() + }) + + it('cancel overrides a prior pause: pause then remove during create still aborts in S3', async () => { + const operations: string[] = [] + let markCreateStarted!: () => void + const createStarted = new Promise((r) => { + markCreateStarted = r + }) + let releaseCreate!: () => void + const createGate = new Promise((r) => { + releaseCreate = r + }) + + const signRequest = vi.fn(async (req: any) => { + const params = new URLSearchParams() + if (req.uploadId) params.set('uploadId', req.uploadId) + params.set('method', req.method) + return { + url: `https://test-bucket.s3.us-east-1.amazonaws.com/${req.key}?${params}`, + } + }) + + server.use( + http.post(s3Url, async ({ request }) => { + const hasUploadId = new URL(request.url).searchParams.has('uploadId') + if (!hasUploadId) { + operations.push('createMultipart') + markCreateStarted() + await createGate + return new HttpResponse(createMultipartXml('upload-2', 'test-key'), { + status: 200, + headers: { 'Content-Type': 'application/xml' }, + }) + } + return new HttpResponse('', { status: 200 }) + }), + http.delete(s3Url, () => { + operations.push('abortMultipart') + return new HttpResponse('', { status: 204 }) + }), + ) + + const core = new Core().use(AwsS3, { + s3Endpoint: 'https://test-bucket.s3.us-east-1.amazonaws.com', + region: 'us-east-1', + signRequest, + shouldUseMultipart: true, + }) + core.addFile({ + source: 'test', + name: 'big.bin', + type: 'application/octet-stream', + data: new File([new Uint8Array(6 * MB)], 'big.bin'), + }) + const fileId = Object.keys(core.getState().files)[0] + + const uploadPromise = core.upload() + uploadPromise?.catch(() => {}) + + await createStarted + core.pauseResume(fileId) // pause first... + core.removeFile(fileId) // ...then cancel, both inside the create window + releaseCreate() + await uploadPromise + + // The cancel must win: the now-orphaned S3 upload gets aborted. + await vi.waitFor(() => expect(operations).toContain('abortMultipart')) + }) +})