fix(storage): reject uploadFileInChunks when the automatic abort succeeds

[Om-singhaI]

fix(storage): reject uploadFileInChunks when the automatic abort succeeds

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #9193 🦕

What was wrong

TransferManager.uploadFileInChunks drives an XML multipart upload: initiate, upload the parts, complete. When any of those steps fails after an uploadId exists, the method aborts the upload session by default (autoAbortFailure unset or true). The catch block then did this:

try {
  await mpuHelper.abortUpload();
  return;
} catch (e) {
  throw new MultiPartUploadError(...);
}
throw new MultiPartUploadError(...);

So the only outcome that did not throw was "the upload failed and the abort succeeded". The promise resolved with undefined, and callers that treat a resolved promise as a written object carried on as if the file existed. The reporter hit exactly this and lost data silently. The method's doc comment promises the opposite ("otherwise a error containing the message, uploadId, and parts map"), and every other failure path (initiate fails, abort fails, autoAbortFailure: false) already throws a MultiPartUploadError.

The fix

Remove the early return so a successful abort falls through to the existing throw new MultiPartUploadError(message, uploadId, partsMap). The error now carries the original failure message plus the uploadId and the parts map, which is the documented contract and matches what the abort failure path and the autoAbortFailure: false path already do. No other logic changes.

Note on behaviour: a case that previously resolved now rejects. That is the point of the fix and it is what the documentation and the sample on cloud.google.com already describe, but I am calling it out explicitly so nobody is surprised. I framed this as a patch level fix rather than a breaking change because the resolved undefined was never documented and could not be distinguished from success; if the maintainers would rather treat it as breaking, the commit message can be adjusted.

Tests

test/transfer-manager.ts, describe('uploadFileInChunks'):

• should call abortUpload when a failure occurs after an uploadID is established previously used assert.doesNotThrow on a promise returning arrow function without awaiting anything, so it passed regardless of whether the promise resolved or rejected and effectively pinned the bug. It now awaits assert.rejects, checks that the rejection is a MultiPartUploadError whose message, uploadId and parts map match what the fake helper reported, and checks that abortUpload was called once. The fake now establishes the uploadId in initiateUpload, records a part in the parts map in uploadPart, and fails in completeUpload, so the parts map assertion is meaningful (a non empty map is propagated).
• New: should reject with the abort error when abortUpload also fails covers the inner catch, which had no unit test before: the abort rejects, and the method rejects with a MultiPartUploadError carrying the abort error's message, the uploadId and the parts map.
• should reject with an error with empty uploadId and partsMap (the autoAbortFailure: false test) called assert.rejects without await, so its assertion could never fail the test. It now awaits it. No other change to that test. I am happy to drop this one word change if you would rather keep the PR to the single path from the issue.

Why the fakes fail in completeUpload rather than uploadPart: completeUpload is awaited directly by the method. A fake whose uploadPart rejects synchronously rejects while the read loop is still waiting on the next chunk from disk, and the wrapper promise from p-limit sits unhandled for one event loop turn until Promise.all attaches a handler. With the tests now awaiting the result that shows up as two PromiseRejectionHandledWarning lines in the mocha output. Failing in completeUpload keeps the tests deterministic and the output clean. That short unhandled window in the source is pre existing and separate from this issue; I mention it under "Out of scope" below.

Verification

All runs were on handwritten/storage with npm run compile:cjs -- --sourceMap followed by mocha build/cjs/test/transfer-manager.js (the same file the package's npm test runs under c8, limited to the transfer manager suite; the unit tests use the sinon FakeXMLHelper harness and need no GCP credentials).

• Pristine main (48e0941): 44 passing. A standalone script that calls uploadFileInChunks with a helper whose uploadPart fails and whose abortUpload succeeds printed abortUpload called and succeeded followed by the promise resolving to undefined.
• Updated tests with the source change reverted to main (test file only changed): 44 passing, 1 failing. The failing test is should call abortUpload when a failure occurs after an uploadID is established with AssertionError [ERR_ASSERTION]: Missing expected rejection. The new abort failure test passes on main as well, as expected, because that path already threw.
• Updated tests with the fix: 45 passing, 0 failing, no PromiseRejectionHandledWarning output. The same standalone script now prints REJECTED: MultiPartUploadError part upload failed uploadId= upload-123 partsMap= Map(1) { 1 => 'etag-1' }.

Coverage: the only removed source line is the early return; the inner catch (abort failure) gains its first unit test, so line coverage of src/transfer-manager.ts goes up, not down.

Lint

I ran the monorepo linter the way presubmit.yaml does (node ./bin/linter.mjs, which diffs main...HEAD, runs ESLint with the root config, and type checks the package with tsc --noEmit). Type checking passes. Two pre existing findings are unrelated to this change and are identical on main:

• src/transfer-manager.ts reports 32 prettier/prettier "Delete ," errors both before and after this change (only the line numbers shift by one). The file on main is formatted with trailingComma: "all" (the root gts 6 config) while the package's own .prettierrc.cjs resolves the package local gts 5 config with trailingComma: "es5". My one line deletion neither adds nor removes any of these.
• test/transfer-manager.ts cannot be parsed by that linter at all because the package tsconfig.json does not include test/. Linting it with tsconfig.cjs.json as the project instead gives 0 prettier findings and three pre existing rule findings on lines this PR does not touch (duplicate fs imports at lines 41 and 42, and an explicit any at line 639). Before this PR that run also flagged @typescript-eslint/no-floating-promises at the un awaited assert.rejects in the autoAbortFailure: false test; that is the one word await included here.

npx prettier --check test/transfer-manager.ts with the package config passes.

Out of scope, noted for maintainers

• While writing the standalone reproduction I noticed that a part upload that rejects before the read loop reaches await Promise.all(promises) (that is, while the loop is still waiting on the next chunk from disk) produces an unhandled rejection on the p-limit wrapper promise for one event loop turn. Under Node's default --unhandled-rejections=throw that terminates the process instead of reaching the catch block at all. Real part uploads take far longer than a disk read, so this is rare in practice, but it is a separate latent issue and I did not touch it here.
• Open PR fix(storage): optimize stream download throughput and eliminate write latency #9158 and draft PR fix!: Update storage to use newer versions of gaxios, teeny-request, and google-auth-library #8827 both edit src/transfer-manager.ts and test/transfer-manager.ts. Neither touches the catch block changed here. fix(storage): optimize stream download throughput and eliminate write latency #9158 does reformat a large part of the test file, so whichever lands second will need a rebase of the test hunks; the one line source change is unaffected.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request removes an early return statement after aborting a multipart upload in transfer-manager.ts, ensuring proper error propagation. It also updates the unit tests to correctly await asynchronous rejections and adds a new test case to verify the behavior when the abort operation itself fails. No review comments were provided, so there is no additional feedback.