From b001807a82616521680baf5429fec6af639aeb58 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 5 Aug 2026 05:49:25 +0000 Subject: [PATCH 1/9] fix(storage): pass signingEndpoint to URLSigner in file.getSignedUrl (#8982) 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](https://github.com/googleapis/{{metadata['repo']['name']}}/issues) 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 #8829 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- handwritten/storage/src/file.ts | 1 + handwritten/storage/src/signer.ts | 2 +- handwritten/storage/test/file.ts | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/handwritten/storage/src/file.ts b/handwritten/storage/src/file.ts index 27765d935a99..d5aa40bf96e1 100644 --- a/handwritten/storage/src/file.ts +++ b/handwritten/storage/src/file.ts @@ -3236,6 +3236,7 @@ class File extends ServiceObject { contentMd5: cfg.contentMd5, contentType: cfg.contentType, host: cfg.host, + signingEndpoint: cfg.signingEndpoint, }; if (cfg.cname) { diff --git a/handwritten/storage/src/signer.ts b/handwritten/storage/src/signer.ts index a657cef6133d..ba5c17c04b75 100644 --- a/handwritten/storage/src/signer.ts +++ b/handwritten/storage/src/signer.ts @@ -25,7 +25,7 @@ type GoogleAuthLike = Pick; * @deprecated Use {@link GoogleAuth} instead */ export interface AuthClient { - sign(blobToSign: string): Promise; + sign(blobToSign: string, signingEndpoint?: string): Promise; getCredentials(): Promise<{ client_email?: string; }>; diff --git a/handwritten/storage/test/file.ts b/handwritten/storage/test/file.ts index 26823995b907..d9f9185a16e8 100644 --- a/handwritten/storage/test/file.ts +++ b/handwritten/storage/test/file.ts @@ -3794,11 +3794,30 @@ describe('File', () => { contentType: config.contentType, cname: CNAME, virtualHostedStyle: true, + signingEndpoint: undefined, }); done(); }); }); + it('should pass signingEndpoint to URLSigner', done => { + const signingEndpoint = 'https://my-endpoint.com'; + const config = { + ...SIGNED_URL_CONFIG, + signingEndpoint, + }; + + file.getSignedUrl(config, (err: Error | null) => { + assert.ifError(err); + const getSignedUrlArgs = signerGetSignedUrlStub.getCall(0).args; + assert.strictEqual( + getSignedUrlArgs[0]['signingEndpoint'], + signingEndpoint + ); + done(); + }); + }); + it('should add "x-goog-resumable: start" header if action is resumable', done => { SIGNED_URL_CONFIG.action = 'resumable'; SIGNED_URL_CONFIG.extensionHeaders = { From 81612c7679a23535b90508e49e73e724cecb1787 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 5 Aug 2026 06:11:34 +0000 Subject: [PATCH 2/9] docs(storage): add comprehensive JSDoc documentation to GetSignedUrlConfig interface properties (#8782) 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](https://github.com/googleapis/{{metadata['repo']['name']}}/issues) 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 #7352 --------- Co-authored-by: Dhriti07 <56169283+Dhriti07@users.noreply.github.com> --- handwritten/storage/src/file.ts | 75 ++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/handwritten/storage/src/file.ts b/handwritten/storage/src/file.ts index d5aa40bf96e1..786998c5f4e4 100644 --- a/handwritten/storage/src/file.ts +++ b/handwritten/storage/src/file.ts @@ -148,21 +148,94 @@ export interface SignedPostPolicyV4Output { url: string; fields: PolicyFields; } - export interface GetSignedUrlConfig extends Pick { + /** + * The action to permit with the signed URL. + * - `'read'`: Allows downloading/viewing the file (HTTP GET). + * - `'write'`: Allows uploading/overwriting the file (HTTP PUT). + * - `'delete'`: Allows removing the file (HTTP DELETE). + * - `'resumable'`: Allows resumable uploads (HTTP POST). + * Note: When using `'resumable'`, the header `X-Goog-Resumable: start` must be sent in the client request. + */ action: 'read' | 'write' | 'delete' | 'resumable'; + + /** + * The signing version to use. + * @default 'v2' + */ version?: 'v2' | 'v4'; + + /** + * Determines the URL structure for accessing bucket resources. + * - `true`: Uses virtual hosted-style URLs (e.g., `https://mybucket.storage.googleapis.com/...`) + * - `false`: Uses path-style URLs (e.g., `https://storage.googleapis.com/mybucket/...`). + * Virtual hosted-style URLs are generally preferred. + * @default false + */ virtualHostedStyle?: boolean; + + /** + * The custom domain name (CNAME) mapped to this bucket (e.g., `"https://cdn.example.com"`). + */ cname?: string; + + /** + * The MD5 digest value in base64. If provided, the client request **must** + * include an identical `Content-MD5` HTTP header. + * If omitted, the client request must not include this header. + */ contentMd5?: string; + + /** + * The expected Content-Type of the file. If provided, the client request **must** + * include an identical `Content-Type` HTTP header. + * If omitted, the client request must not include this header. + */ contentType?: string; + + /** + * The expiration timestamp for the link. Any provided value is passed directly to `new Date()`. + * @throws {Error} If an expiration timestamp from the past is given. + * Note: `'v4'` signing supports a maximum duration of 7 days (604,800 seconds) from the creation time. + */ expires: string | number | Date; + + /** + * The timestamp when this link becomes usable. Any provided value is passed directly to `new Date()`. + * @default Date.now() + * Note: Only supported/applicable when `version` is set to `'v4'`. + */ accessibleAt?: string | number | Date; + + /** + * Canonical extension headers that the server will validate against the client's request. + * Requirements: + * - Header names must be prefixed with `x-goog-` and must be entirely lowercase. + * - Multi-valued headers passed as an array are converted into a comma-separated string (no spaces). + * The client must format them identically to prevent signature mismatches. + */ extensionHeaders?: http.OutgoingHttpHeaders; + + /** + * The filename to prompt the browser/user to save the file as upon access. + * Note: This option is ignored if `responseDisposition` is explicitly set. + */ promptSaveAs?: string; + + /** + * Maps to the `response-content-disposition` query parameter in the signed URL. + */ responseDisposition?: string; + + /** + * Maps to the `response-content-type` query parameter in the signed URL. + */ responseType?: string; + + /** + * Additional query parameters to include natively in the generated signed URL. + */ queryParams?: Query; } From 77dab53bf6d8a4b911f04e49eef444057d817619 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 5 Aug 2026 08:27:18 +0000 Subject: [PATCH 3/9] fix(storage): destroy local read stream on upload write failure to prevent resource leaks (#8752) 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](https://github.com/googleapis/{{metadata['repo']['name']}}/issues) 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 #7325 --- handwritten/storage/src/bucket.ts | 12 ++++++-- handwritten/storage/test/bucket.ts | 46 ++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/handwritten/storage/src/bucket.ts b/handwritten/storage/src/bucket.ts index 5c796789ebd3..527e7396f87f 100644 --- a/handwritten/storage/src/bucket.ts +++ b/handwritten/storage/src/bucket.ts @@ -4505,13 +4505,19 @@ class Bucket extends ServiceObject { if (options.onUploadProgress) { writable.on('progress', options.onUploadProgress); } - fs.createReadStream(pathString) - .on('error', bail) + const readStream = fs.createReadStream(pathString); + readStream + .on('error', err => { + readStream.destroy(); + writable.destroy(); + bail(err); + }) .pipe(writable) .on('error', err => { + readStream.destroy(); if ( this.storage.retryOptions.autoRetry && - this.storage.retryOptions.retryableErrorFn!(err) + this.storage.retryOptions.retryableErrorFn!(err as ApiError) ) { return reject(err); } else { diff --git a/handwritten/storage/test/bucket.ts b/handwritten/storage/test/bucket.ts index 555d8e8c1c9c..23874839e1a2 100644 --- a/handwritten/storage/test/bucket.ts +++ b/handwritten/storage/test/bucket.ts @@ -100,11 +100,18 @@ class FakeNotification { } let fsStatOverride: Function | null; +let fsCreateReadStreamOverride: Function | null; const fakeFs = { ...fs, stat: (filePath: string, callback: Function) => { return (fsStatOverride || fs.stat)(filePath, callback); }, + createReadStream: (filePath: string, options?: Parameters[1]) => { + return (fsCreateReadStreamOverride || fs.createReadStream)( + filePath, + options + ); + }, }; let pLimitOverride: Function | null; @@ -234,6 +241,7 @@ describe('Bucket', () => { beforeEach(() => { fsStatOverride = null; + fsCreateReadStreamOverride = null; pLimitOverride = null; bucket = new Bucket(STORAGE, BUCKET_NAME); }); @@ -3231,6 +3239,44 @@ describe('Bucket', () => { }); }); + it('should destroy the local read stream if write stream fails', done => { + const fakeFile = new FakeFile(bucket, 'file-name'); + const options = {destination: fakeFile, resumable: false}; + const originalCreateReadStream = fs.createReadStream; + let readStream: fs.ReadStream; + fsCreateReadStreamOverride = (path: string, opts: any) => { + readStream = originalCreateReadStream(path, opts); + return readStream; + }; + + fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => { + const ws = new stream.Writable({ + write(chunk, encoding, callback) { + callback(new Error('write error')); + }, + }); + return ws; + }; + + const textfilepath = path.join( + getDirName(), + '../../../test/testdata/textfile.txt' + ); + + bucket.upload(textfilepath, options, (err: Error) => { + try { + assert.strictEqual(err.message, 'write error'); + assert.ok(readStream); + assert.ok(readStream.destroyed); + done(); + } catch (e) { + done(e); + } finally { + fsCreateReadStreamOverride = null; + } + }); + }); + it('should allow overriding content type', done => { const fakeFile = new FakeFile(bucket, 'file-name'); const metadata = {contentType: 'made-up-content-type'}; From 67ef42373ec2cecaa11c9605c72bce2b0502faf9 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 24 Jun 2026 11:48:26 +0000 Subject: [PATCH 4/9] refactor(storage): make onResponse async and improve error formatting for retry failures --- handwritten/storage/src/resumable-upload.ts | 107 +++++++++++++++++-- handwritten/storage/test/resumable-upload.ts | 107 ++++++++++++++++--- 2 files changed, 189 insertions(+), 25 deletions(-) diff --git a/handwritten/storage/src/resumable-upload.ts b/handwritten/storage/src/resumable-upload.ts index 9ebbb6f37a85..3d126c5848be 100644 --- a/handwritten/storage/src/resumable-upload.ts +++ b/handwritten/storage/src/resumable-upload.ts @@ -1345,7 +1345,7 @@ export class Upload extends Writable { }, }; const res = await this.authClient.request(combinedReqOpts); - const successfulRequest = this.onResponse(res); + const successfulRequest = await this.onResponse(res); this.removeListener('error', errorCallback); return successfulRequest ? res : null; @@ -1354,7 +1354,7 @@ export class Upload extends Writable { /** * @return {bool} is the request good? */ - private onResponse(resp: GaxiosResponse) { + private async onResponse(resp: GaxiosResponse) { if ( resp.status !== 200 && this.retryOptions.retryableErrorFn!({ @@ -1363,7 +1363,7 @@ export class Upload extends Writable { name: resp.statusText, }) ) { - this.attemptDelayedRetry(resp); + await this.attemptDelayedRetry(resp); return false; } @@ -1386,9 +1386,7 @@ export class Upload extends Writable { if (retryDelay <= 0) { this.destroy( - new Error( - `Retry total time limit exceeded - ${JSON.stringify(resp.data)}`, - ), + formatRetryError('Retry total time limit exceeded', resp), ); return; } @@ -1409,9 +1407,7 @@ export class Upload extends Writable { } this.numRetries++; } else { - this.destroy( - new Error(`Retry limit exceeded - ${JSON.stringify(resp.data)}`), - ); + this.destroy(formatRetryError('Retry limit exceeded', resp)); } } @@ -1456,6 +1452,99 @@ export class Upload extends Writable { } } +function formatRetryError( + prefix: string, + resp: Pick, +): Error { + const parts: string[] = []; + + if (resp.status !== undefined && !isNaN(resp.status)) { + parts.push(`status: ${resp.status}`); + } + + const err = resp.data; + if (err !== undefined && err !== null) { + if (err instanceof Error) { + const gaxiosErr = err as GaxiosError; + const errParts: string[] = []; + if (gaxiosErr.message) { + errParts.push(gaxiosErr.message); + } + const status = gaxiosErr.status ?? gaxiosErr.response?.status; + if (status !== undefined && !isNaN(status) && status !== resp.status) { + errParts.push(`status: ${status}`); + } + const statusText = gaxiosErr.response?.statusText; + if (statusText) { + errParts.push(`statusText: ${statusText}`); + } + const responseData = gaxiosErr.response?.data; + if (responseData !== undefined && responseData !== null && responseData !== '') { + errParts.push( + `response: ${ + typeof responseData === 'object' + ? JSON.stringify(responseData) + : responseData + }`, + ); + } + if (gaxiosErr.code) { + errParts.push(`code: ${gaxiosErr.code}`); + } + if (errParts.length > 0) { + parts.push(...errParts); + } else { + parts.push(gaxiosErr.toString() || gaxiosErr.name || 'Unknown Error'); + } + } else if (typeof err === 'object') { + const errParts: string[] = []; + const gaxiosErrLike = err as any; + if (gaxiosErrLike.message) { + errParts.push(String(gaxiosErrLike.message)); + } + const status = gaxiosErrLike.status ?? gaxiosErrLike.response?.status; + if (status !== undefined && !isNaN(status) && status !== resp.status) { + errParts.push(`status: ${status}`); + } + const statusText = gaxiosErrLike.response?.statusText; + if (statusText) { + errParts.push(`statusText: ${statusText}`); + } + const responseData = gaxiosErrLike.response?.data; + if (responseData !== undefined && responseData !== null && responseData !== '') { + errParts.push( + `response: ${ + typeof responseData === 'object' + ? JSON.stringify(responseData) + : responseData + }`, + ); + } + if (gaxiosErrLike.code) { + errParts.push(`code: ${String(gaxiosErrLike.code)}`); + } + + if (errParts.length > 0) { + parts.push(...errParts); + } else { + const stringified = JSON.stringify(err); + if (stringified && stringified !== '{}') { + parts.push(stringified); + } + } + } else if (typeof err === 'string') { + if (err !== '') { + parts.push(err); + } + } else { + parts.push(String(err)); + } + } + + const suffix = parts.join(' - '); + return new Error(`${prefix} - ${suffix || 'Unknown Error'}`); +} + export function upload(cfg: UploadConfig) { return new Upload(cfg); } diff --git a/handwritten/storage/test/resumable-upload.ts b/handwritten/storage/test/resumable-upload.ts index 381044d64d9d..eb1ab149350c 100644 --- a/handwritten/storage/test/resumable-upload.ts +++ b/handwritten/storage/test/resumable-upload.ts @@ -2275,10 +2275,10 @@ describe('resumable-upload', () => { describe('500s', () => { const RESP = {status: 500, data: 'error message from server'}; - it('should increase the retry count if less than limit', () => { + it('should increase the retry count if less than limit', async () => { up.getRetryDelay = () => 1; assert.strictEqual(up.numRetries, 0); - assert.strictEqual(up.onResponse(RESP), false); + assert.strictEqual(await up.onResponse(RESP), false); assert.strictEqual(up.numRetries, 1); }); @@ -2287,15 +2287,17 @@ describe('resumable-upload', () => { up.destroy = (err: Error) => { assert.strictEqual( err.message, - `Retry limit exceeded - ${JSON.stringify(RESP.data)}` + `Retry limit exceeded - status: 500 - error message from server`, ); done(); }; - up.onResponse(RESP); - up.onResponse(RESP); - up.onResponse(RESP); - up.onResponse(RESP); + (async () => { + await up.onResponse(RESP); + await up.onResponse(RESP); + await up.onResponse(RESP); + await up.onResponse(RESP); + })().catch(done); }); describe('exponential back off', () => { @@ -2321,19 +2323,19 @@ describe('resumable-upload', () => { assert(delay <= maxTime); // make it keep retrying until the limit is reached - up.onResponse(RESP); + void up.onResponse(RESP); }; up.on('error', (err: Error) => { assert.strictEqual(up.numRetries, 3); assert.strictEqual( err.message, - `Retry limit exceeded - ${JSON.stringify(RESP.data)}` + `Retry limit exceeded - status: 500 - error message from server`, ); done(); }); - up.onResponse(RESP); + void up.onResponse(RESP); clock.runAll(); }); }); @@ -2348,23 +2350,22 @@ describe('resumable-upload', () => { assert.strictEqual(resp, RESP); done(); }); - up.onResponse(RESP); + void up.onResponse(RESP); }); - it('should return true', () => { + it('should return true', async () => { up.getRetryDelay = () => 1; - assert.strictEqual(up.onResponse(RESP), true); + assert.strictEqual(await up.onResponse(RESP), true); }); - it('should handle a custom status code when passed a retry function', () => { + it('should handle a custom status code when passed a retry function', async () => { up.getRetryDelay = () => 1; const RESP = {status: 1000}; const customHandlerFunction = (err: ApiError) => { return err.code === 1000; }; up.retryOptions.retryableErrorFn = customHandlerFunction; - - assert.strictEqual(up.onResponse(RESP), false); + assert.strictEqual(await up.onResponse(RESP), false); }); }); }); @@ -2490,6 +2491,80 @@ describe('resumable-upload', () => { up.attemptDelayedRetry({}); }); + + it('should include correct details for standard native Errors', done => { + up.numRetries = 3; + up.retryLimit = 3; + const nativeError = new Error('native connection issue'); + + up.on('error', (err: Error) => { + assert.strictEqual( + err.message, + 'Retry limit exceeded - native connection issue', + ); + done(); + }); + + up.attemptDelayedRetry({ + status: NaN, + data: nativeError, + }); + }); + + it('should include correct details for custom errors with empty messages', done => { + up.numRetries = 3; + up.retryLimit = 3; + const customError = new Error(''); + (customError as any).code = 'ERR_SOMETHING_SPECIAL'; + + up.on('error', (err: Error) => { + assert.strictEqual( + err.message, + 'Retry limit exceeded - code: ERR_SOMETHING_SPECIAL', + ); + done(); + }); + + up.attemptDelayedRetry({ + status: NaN, + data: customError, + }); + }); + + it('should include correct details for GaxiosErrors with empty/missing response bodies', done => { + up.numRetries = 3; + up.retryLimit = 3; + + const gaxiosError = new GaxiosError( + 'Request failed with status code 429', + { + method: 'POST', + url: 'https://example.com', + } as any, + { + status: 429, + statusText: 'Too Many Requests', + data: '', + config: {}, + headers: {}, + } as any + ); + + up.on('error', (err: Error) => { + // Let's check for the presence of key details rather than an exact string if we are unsure of exact GaxiosError fields, + // or assert the expected string. Let's assert the expected string: + assert(err.message.includes('Retry limit exceeded')); + assert(err.message.includes('Request failed with status code 429')); + assert(err.message.includes('status: 429') || err.message.includes('code: 429')); + assert(err.message.includes('statusText: Too Many Requests')); + done(); + }); + + up.attemptDelayedRetry({ + status: NaN, + data: gaxiosError, + }); + }); }); describe('PROTOCOL_REGEX', () => { From ef1545fb3945c5b362aa39effe4e3f980f54db11 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 24 Jun 2026 12:55:35 +0000 Subject: [PATCH 5/9] refactor(storage): unify error and object formatting in formatRetryError - Unify formatting of Errors and objects under a single block inside formatRetryError to reduce code duplication. - Ensure standard errors, GaxiosErrors, and custom errors with empty/missing properties are correctly formatted. --- handwritten/storage/src/resumable-upload.ts | 42 +++------------------ 1 file changed, 6 insertions(+), 36 deletions(-) diff --git a/handwritten/storage/src/resumable-upload.ts b/handwritten/storage/src/resumable-upload.ts index 3d126c5848be..7460811a9216 100644 --- a/handwritten/storage/src/resumable-upload.ts +++ b/handwritten/storage/src/resumable-upload.ts @@ -1458,52 +1458,20 @@ function formatRetryError( ): Error { const parts: string[] = []; - if (resp.status !== undefined && !isNaN(resp.status)) { + if (typeof resp.status === 'number' && !isNaN(resp.status)) { parts.push(`status: ${resp.status}`); } const err = resp.data; if (err !== undefined && err !== null) { - if (err instanceof Error) { - const gaxiosErr = err as GaxiosError; - const errParts: string[] = []; - if (gaxiosErr.message) { - errParts.push(gaxiosErr.message); - } - const status = gaxiosErr.status ?? gaxiosErr.response?.status; - if (status !== undefined && !isNaN(status) && status !== resp.status) { - errParts.push(`status: ${status}`); - } - const statusText = gaxiosErr.response?.statusText; - if (statusText) { - errParts.push(`statusText: ${statusText}`); - } - const responseData = gaxiosErr.response?.data; - if (responseData !== undefined && responseData !== null && responseData !== '') { - errParts.push( - `response: ${ - typeof responseData === 'object' - ? JSON.stringify(responseData) - : responseData - }`, - ); - } - if (gaxiosErr.code) { - errParts.push(`code: ${gaxiosErr.code}`); - } - if (errParts.length > 0) { - parts.push(...errParts); - } else { - parts.push(gaxiosErr.toString() || gaxiosErr.name || 'Unknown Error'); - } - } else if (typeof err === 'object') { - const errParts: string[] = []; + if (typeof err === 'object') { const gaxiosErrLike = err as any; + const errParts: string[] = []; if (gaxiosErrLike.message) { errParts.push(String(gaxiosErrLike.message)); } const status = gaxiosErrLike.status ?? gaxiosErrLike.response?.status; - if (status !== undefined && !isNaN(status) && status !== resp.status) { + if (typeof status === 'number' && !isNaN(status) && status !== resp.status) { errParts.push(`status: ${status}`); } const statusText = gaxiosErrLike.response?.statusText; @@ -1526,6 +1494,8 @@ function formatRetryError( if (errParts.length > 0) { parts.push(...errParts); + } else if (err instanceof Error) { + parts.push(err.toString() || err.name || 'Unknown Error'); } else { const stringified = JSON.stringify(err); if (stringified && stringified !== '{}') { From 3d7bdc98aa362332a42aa1966d8576a3f9bfa7af Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 25 Jun 2026 05:16:37 +0000 Subject: [PATCH 6/9] refactor: rename formatRetryError to buildRetryError for consistency --- handwritten/storage/src/resumable-upload.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/handwritten/storage/src/resumable-upload.ts b/handwritten/storage/src/resumable-upload.ts index 7460811a9216..10b496d2d260 100644 --- a/handwritten/storage/src/resumable-upload.ts +++ b/handwritten/storage/src/resumable-upload.ts @@ -1386,7 +1386,7 @@ export class Upload extends Writable { if (retryDelay <= 0) { this.destroy( - formatRetryError('Retry total time limit exceeded', resp), + buildRetryError('Retry total time limit exceeded', resp), ); return; } @@ -1407,7 +1407,7 @@ export class Upload extends Writable { } this.numRetries++; } else { - this.destroy(formatRetryError('Retry limit exceeded', resp)); + this.destroy(buildRetryError('Retry limit exceeded', resp)); } } @@ -1452,7 +1452,7 @@ export class Upload extends Writable { } } -function formatRetryError( +function buildRetryError( prefix: string, resp: Pick, ): Error { From 96f230abc1c59c3ed15a57a9aa8b4e7830e86a9b Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 25 Jun 2026 12:41:23 +0000 Subject: [PATCH 7/9] refactor: convert onResponse and related methods from async to synchronous execution --- handwritten/storage/src/resumable-upload.ts | 6 ++--- handwritten/storage/test/resumable-upload.ts | 28 +++++++++----------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/handwritten/storage/src/resumable-upload.ts b/handwritten/storage/src/resumable-upload.ts index 10b496d2d260..df63ec637b1d 100644 --- a/handwritten/storage/src/resumable-upload.ts +++ b/handwritten/storage/src/resumable-upload.ts @@ -1345,7 +1345,7 @@ export class Upload extends Writable { }, }; const res = await this.authClient.request(combinedReqOpts); - const successfulRequest = await this.onResponse(res); + const successfulRequest = this.onResponse(res); this.removeListener('error', errorCallback); return successfulRequest ? res : null; @@ -1354,7 +1354,7 @@ export class Upload extends Writable { /** * @return {bool} is the request good? */ - private async onResponse(resp: GaxiosResponse) { + private onResponse(resp: GaxiosResponse) { if ( resp.status !== 200 && this.retryOptions.retryableErrorFn!({ @@ -1363,7 +1363,7 @@ export class Upload extends Writable { name: resp.statusText, }) ) { - await this.attemptDelayedRetry(resp); + this.attemptDelayedRetry(resp); return false; } diff --git a/handwritten/storage/test/resumable-upload.ts b/handwritten/storage/test/resumable-upload.ts index eb1ab149350c..a324372f5e27 100644 --- a/handwritten/storage/test/resumable-upload.ts +++ b/handwritten/storage/test/resumable-upload.ts @@ -2275,10 +2275,10 @@ describe('resumable-upload', () => { describe('500s', () => { const RESP = {status: 500, data: 'error message from server'}; - it('should increase the retry count if less than limit', async () => { + it('should increase the retry count if less than limit', () => { up.getRetryDelay = () => 1; assert.strictEqual(up.numRetries, 0); - assert.strictEqual(await up.onResponse(RESP), false); + assert.strictEqual(up.onResponse(RESP), false); assert.strictEqual(up.numRetries, 1); }); @@ -2292,12 +2292,10 @@ describe('resumable-upload', () => { done(); }; - (async () => { - await up.onResponse(RESP); - await up.onResponse(RESP); - await up.onResponse(RESP); - await up.onResponse(RESP); - })().catch(done); + up.onResponse(RESP); + up.onResponse(RESP); + up.onResponse(RESP); + up.onResponse(RESP); }); describe('exponential back off', () => { @@ -2323,7 +2321,7 @@ describe('resumable-upload', () => { assert(delay <= maxTime); // make it keep retrying until the limit is reached - void up.onResponse(RESP); + up.onResponse(RESP); }; up.on('error', (err: Error) => { @@ -2335,7 +2333,7 @@ describe('resumable-upload', () => { done(); }); - void up.onResponse(RESP); + up.onResponse(RESP); clock.runAll(); }); }); @@ -2350,22 +2348,22 @@ describe('resumable-upload', () => { assert.strictEqual(resp, RESP); done(); }); - void up.onResponse(RESP); + up.onResponse(RESP); }); - it('should return true', async () => { + it('should return true', () => { up.getRetryDelay = () => 1; - assert.strictEqual(await up.onResponse(RESP), true); + assert.strictEqual(up.onResponse(RESP), true); }); - it('should handle a custom status code when passed a retry function', async () => { + it('should handle a custom status code when passed a retry function', () => { up.getRetryDelay = () => 1; const RESP = {status: 1000}; const customHandlerFunction = (err: ApiError) => { return err.code === 1000; }; up.retryOptions.retryableErrorFn = customHandlerFunction; - assert.strictEqual(await up.onResponse(RESP), false); + assert.strictEqual(up.onResponse(RESP), false); }); }); }); From 6cb233e6f5ba2a7365a5f987e98b5d0d7a24bbcd Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 5 Aug 2026 08:46:54 +0000 Subject: [PATCH 8/9] test: update assertions in resumable-upload tests to verify GaxiosError details --- handwritten/storage/test/resumable-upload.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/handwritten/storage/test/resumable-upload.ts b/handwritten/storage/test/resumable-upload.ts index a324372f5e27..bd7e7618e501 100644 --- a/handwritten/storage/test/resumable-upload.ts +++ b/handwritten/storage/test/resumable-upload.ts @@ -2549,8 +2549,7 @@ describe('resumable-upload', () => { ); up.on('error', (err: Error) => { - // Let's check for the presence of key details rather than an exact string if we are unsure of exact GaxiosError fields, - // or assert the expected string. Let's assert the expected string: + // Assert that the formatted error message includes key HTTP details from the GaxiosError. assert(err.message.includes('Retry limit exceeded')); assert(err.message.includes('Request failed with status code 429')); assert(err.message.includes('status: 429') || err.message.includes('code: 429')); From d79720e59484a8c40ea81b38c937e3e57bb24b8a Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Wed, 5 Aug 2026 08:53:23 +0000 Subject: [PATCH 9/9] test: add unit test for GaxiosError detail handling in resumable upload retries --- handwritten/storage/test/resumable-upload.ts | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/handwritten/storage/test/resumable-upload.ts b/handwritten/storage/test/resumable-upload.ts index bd7e7618e501..6e6985878b00 100644 --- a/handwritten/storage/test/resumable-upload.ts +++ b/handwritten/storage/test/resumable-upload.ts @@ -2562,6 +2562,45 @@ describe('resumable-upload', () => { data: gaxiosError, }); }); + + it('should include correct details for GaxiosErrors with populated error responses', done => { + up.numRetries = 3; + up.retryLimit = 3; + + const gaxiosError = new GaxiosError( + 'Request failed with status code 400', + { + method: 'POST', + url: 'https://example.com', + } as any, + { + status: 400, + statusText: 'Bad Request', + data: { + error: { + message: 'Invalid query parameter value', + code: 400, + }, + }, + config: {}, + headers: {}, + } as any + ); + + up.on('error', (err: Error) => { + // Assert that the formatted error message includes key HTTP details and the inner API error message. + assert(err.message.includes('Retry limit exceeded')); + assert(err.message.includes('Request failed with status code 400')); + assert(err.message.includes('status: 400') || err.message.includes('code: 400')); + assert(err.message.includes('Invalid query parameter value')); + done(); + }); + + up.attemptDelayedRetry({ + status: NaN, + data: gaxiosError, + }); + }); }); describe('PROTOCOL_REGEX', () => {