From bbad4a4d48600b5c569f29d36a58e8e1b0879b21 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 8 Oct 2026 08:00:44 +0000 Subject: [PATCH 1/3] fix: aggregate error messages when both multi-part upload and abort operations fail --- handwritten/storage/src/transfer-manager.ts | 5 +- handwritten/storage/test/transfer-manager.ts | 48 +++++++++++++++++--- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 9ff9368ce91f..b3d1a42fb077 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -969,10 +969,9 @@ export class TransferManager { ) { try { await mpuHelper.abortUpload(); - return; - } catch (e) { + } catch (abortErr) { throw new MultiPartUploadError( - (e as Error).message, + `${(e as Error).message}\n${(abortErr as Error).message}`, mpuHelper.uploadId!, mpuHelper.partsMap! ); diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index 78554c2cc766..f3167d9c8aa4 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/test/transfer-manager.ts @@ -847,13 +847,13 @@ describe('Transfer Manager', () => { ); }); - it('should call abortUpload when a failure occurs after an uploadID is established', async () => { + it('should call abortUpload and reject with the original error when a failure occurs after an uploadID is established', async () => { + const fakeId = '123'; const expectedErr = new MultiPartUploadError( 'Hello World', - '', + fakeId, new Map() ); - const fakeId = '123'; mockGeneratorFunction = (bucket, fileName, uploadId, partsMap) => { fakeHelper = sandbox.createStubInstance(FakeXMLHelper); @@ -862,7 +862,7 @@ describe('Transfer Manager', () => { fakeHelper.initiateUpload.resolves(); fakeHelper.uploadPart.callsFake(() => { fakeHelper.uploadId = fakeId; - return Promise.reject(expectedErr); + return Promise.reject(new Error(expectedErr.message)); }); fakeHelper.completeUpload.resolves(); fakeHelper.abortUpload.callsFake(() => { @@ -872,9 +872,45 @@ describe('Transfer Manager', () => { return fakeHelper; }; - assert.doesNotThrow(() => - transferManager.uploadFileInChunks(filePath, {}, mockGeneratorFunction) + await assert.rejects( + transferManager.uploadFileInChunks(filePath, {}, mockGeneratorFunction), + expectedErr + ); + assert.strictEqual(fakeHelper.abortUpload.calledOnce, true); + }); + + it('should reject with an error containing both messages when both uploadPart and abortUpload fail', async () => { + const fakeId = '123'; + const uploadErr = new Error('Chunk upload failed'); + const abortErr = new Error('Abort upload failed'); + const expectedErr = new MultiPartUploadError( + `${uploadErr.message}\n${abortErr.message}`, + fakeId, + new Map() + ); + + mockGeneratorFunction = (bucket, fileName, uploadId, partsMap) => { + fakeHelper = sandbox.createStubInstance(FakeXMLHelper); + fakeHelper.uploadId = uploadId || ''; + fakeHelper.partsMap = partsMap || new Map(); + fakeHelper.initiateUpload.resolves(); + fakeHelper.uploadPart.callsFake(() => { + fakeHelper.uploadId = fakeId; + return Promise.reject(uploadErr); + }); + fakeHelper.completeUpload.resolves(); + fakeHelper.abortUpload.callsFake(() => { + assert.strictEqual(fakeHelper.uploadId, fakeId); + return Promise.reject(abortErr); + }); + return fakeHelper; + }; + + await assert.rejects( + transferManager.uploadFileInChunks(filePath, {}, mockGeneratorFunction), + expectedErr ); + assert.strictEqual(fakeHelper.abortUpload.calledOnce, true); }); it('should set the appropriate `GCCL_GCS_CMD_KEY`', async () => { From 5d90320b86afbab914df805b3f7bbe3345a7318e Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 8 Oct 2026 08:29:05 +0000 Subject: [PATCH 2/3] fix: improve error message handling in multipart upload failure path to handle non-Error objects --- handwritten/storage/src/transfer-manager.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index b3d1a42fb077..f464e3ebee68 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -963,6 +963,7 @@ export class TransferManager { await Promise.all(promises); return await mpuHelper.completeUpload(); } catch (e) { + const uploadMsg = (e as Error)?.message ?? String(e); if ( (options.autoAbortFailure === undefined || options.autoAbortFailure) && mpuHelper.uploadId @@ -970,15 +971,16 @@ export class TransferManager { try { await mpuHelper.abortUpload(); } catch (abortErr) { + const abortMsg = (abortErr as Error)?.message ?? String(abortErr); throw new MultiPartUploadError( - `${(e as Error).message}\n${(abortErr as Error).message}`, + `${uploadMsg}\n${abortMsg}`, mpuHelper.uploadId!, mpuHelper.partsMap! ); } } throw new MultiPartUploadError( - (e as Error).message, + uploadMsg, mpuHelper.uploadId!, mpuHelper.partsMap! ); From fa26d8672eaca9431c3dcabf0f258eafd1f583c0 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 20 Oct 2026 07:23:50 +0000 Subject: [PATCH 3/3] fix: preserve upload state and error handling logic in multipart upload failure catch block --- handwritten/storage/src/transfer-manager.ts | 8 +--- handwritten/storage/test/transfer-manager.ts | 43 +++++++++++++++++++- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index f464e3ebee68..472f8106e6d0 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -963,7 +963,7 @@ export class TransferManager { await Promise.all(promises); return await mpuHelper.completeUpload(); } catch (e) { - const uploadMsg = (e as Error)?.message ?? String(e); + let uploadMsg = (e as Error)?.message ?? String(e); if ( (options.autoAbortFailure === undefined || options.autoAbortFailure) && mpuHelper.uploadId @@ -972,11 +972,7 @@ export class TransferManager { await mpuHelper.abortUpload(); } catch (abortErr) { const abortMsg = (abortErr as Error)?.message ?? String(abortErr); - throw new MultiPartUploadError( - `${uploadMsg}\n${abortMsg}`, - mpuHelper.uploadId!, - mpuHelper.partsMap! - ); + uploadMsg += `\n${abortMsg}`; } } throw new MultiPartUploadError( diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index f3167d9c8aa4..d729b9c4e4a5 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/test/transfer-manager.ts @@ -849,10 +849,11 @@ describe('Transfer Manager', () => { it('should call abortUpload and reject with the original error when a failure occurs after an uploadID is established', async () => { const fakeId = '123'; + const fakePartsMap = new Map([[1, 'etag-1']]); const expectedErr = new MultiPartUploadError( 'Hello World', fakeId, - new Map() + fakePartsMap ); mockGeneratorFunction = (bucket, fileName, uploadId, partsMap) => { @@ -862,6 +863,7 @@ describe('Transfer Manager', () => { fakeHelper.initiateUpload.resolves(); fakeHelper.uploadPart.callsFake(() => { fakeHelper.uploadId = fakeId; + fakeHelper.partsMap = fakePartsMap; return Promise.reject(new Error(expectedErr.message)); }); fakeHelper.completeUpload.resolves(); @@ -879,14 +881,50 @@ describe('Transfer Manager', () => { assert.strictEqual(fakeHelper.abortUpload.calledOnce, true); }); + it('should not call abortUpload when autoAbortFailure is false and a failure occurs after an uploadID is established', async () => { + const fakeId = '123'; + const fakePartsMap = new Map([[1, 'etag-1']]); + const expectedErr = new MultiPartUploadError( + 'Hello World', + fakeId, + fakePartsMap + ); + + mockGeneratorFunction = (bucket, fileName, uploadId, partsMap) => { + fakeHelper = sandbox.createStubInstance(FakeXMLHelper); + fakeHelper.uploadId = uploadId || ''; + fakeHelper.partsMap = partsMap || new Map(); + fakeHelper.initiateUpload.resolves(); + fakeHelper.uploadPart.callsFake(() => { + fakeHelper.uploadId = fakeId; + fakeHelper.partsMap = fakePartsMap; + return Promise.reject(new Error(expectedErr.message)); + }); + fakeHelper.completeUpload.resolves(); + fakeHelper.abortUpload.resolves(); + return fakeHelper; + }; + + await assert.rejects( + transferManager.uploadFileInChunks( + filePath, + {autoAbortFailure: false}, + mockGeneratorFunction + ), + expectedErr + ); + assert.strictEqual(fakeHelper.abortUpload.notCalled, true); + }); + it('should reject with an error containing both messages when both uploadPart and abortUpload fail', async () => { const fakeId = '123'; + const fakePartsMap = new Map([[1, 'etag-1']]); const uploadErr = new Error('Chunk upload failed'); const abortErr = new Error('Abort upload failed'); const expectedErr = new MultiPartUploadError( `${uploadErr.message}\n${abortErr.message}`, fakeId, - new Map() + fakePartsMap ); mockGeneratorFunction = (bucket, fileName, uploadId, partsMap) => { @@ -896,6 +934,7 @@ describe('Transfer Manager', () => { fakeHelper.initiateUpload.resolves(); fakeHelper.uploadPart.callsFake(() => { fakeHelper.uploadId = fakeId; + fakeHelper.partsMap = fakePartsMap; return Promise.reject(uploadErr); }); fakeHelper.completeUpload.resolves();