fix(storage): resolve transport and retry issues by thiyaguk09 · Pull Request #8235 · googleapis/google-cloud-node

thiyaguk09 · 2026-05-07T12:00:44Z

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 #<issue_number_goes_here> 🦕

gemini-code-assist

Code Review

This pull request refactors request handling and retry logic by transitioning from the common Service class to a specialized StorageTransport. Key changes include the introduction of granular idempotency logic for HTTP methods, improved encryption header management, and a custom parameter serializer. Review feedback identifies a high-severity race condition involving shared interceptors on the Gaxios instance and inconsistent return types in the response handler. Additionally, improvements were suggested regarding the efficiency of JSON parsing within retry loops and the safety of non-null assertions in encryption headers.

gemini-code-assist · 2026-05-07T12:03:21Z

+    this.gaxiosInstance.interceptors.request.clear();
    if (reqOpts.interceptors) {
-      this.gaxiosInstance.interceptors.request.clear();
      for (const inter of reqOpts.interceptors) {
        this.gaxiosInstance.interceptors.request.add(inter);
      }
    }


Clearing and adding interceptors on a shared this.gaxiosInstance before each asynchronous request creates a race condition. If multiple makeRequest calls are in flight, one call may clear the interceptors intended for another. Additionally, verify if this.authClient.request actually uses this.gaxiosInstance; if it doesn't, these interceptors are ineffective.

required for retry operations fix in conformance Test PR

What is the retry operations fix and why do we need to make this change in the current PR ? Same for the other comments below.

To support custom headers required by conformance tests without creating race conditions where concurrent async requests overwrite each other's configuration on a shared instance, we modified makeRequest to isolate custom interceptors using a fresh, request-scoped Gaxios instance, while explicitly stripping the adapter property during cloning to prevent infinite recursion and heap out-of-memory crashes between GoogleAuth and Gaxios.

gemini-code-assist · 2026-05-07T12:03:21Z

+      const responseHandler = (resp: GaxiosResponse<T>) => {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const data = resp.data as any;
+        if (data !== null && typeof data === 'object') {
+          data.headers = resp.headers;
+          data.status = resp.status;
+          return data;
+        }
+        return resp;
+      };


The responseHandler returns inconsistent types. If resp.data is an object, it returns the data with injected headers and status properties. Otherwise, it returns the full GaxiosResponse object. This inconsistency will likely cause runtime errors for consumers expecting a uniform return structure. Furthermore, injecting properties into resp.data is risky if the data is a Buffer or Stream.

required for retry operations

Please elaborate as stated in the above comment.

To preserve backward compatibility for legacy SDK methods that expect resource metadata objects to carry .headers and .status properties without introducing runtime errors or mutating binary data, we added a strict validation check to ensure these properties are strictly injected into plain JSON response objects and never onto Buffer or Stream payloads.

gemini-code-assist · 2026-05-07T12:03:21Z

+  // Optimized Precondition Check
+  let bodyEtag = false;
+  try {
+    const parsedBody = typeof data === 'string' ? JSON.parse(data) : data;


Parsing the request body using JSON.parse inside the retry logic is inefficient for large payloads and will be executed on every retryable error check. Consider checking for preconditions earlier or in a way that avoids repeated parsing of the body. Additionally, this logic may fail or behave unexpectedly if data is a Buffer or Stream.

required for retry operations fix in conformance Test PR

Please elaborate as stated in the above comment.

To eliminate the severe performance degradation and stream-breaking risks of repeatedly running JSON.parse inside the retry loop to detect preconditions like an etag, we now pre-calculate this presence once during the request preparation phase in StorageTransport.makeRequest and cache it as a simple boolean flag on the request configuration for the retry evaluator to read instantly.

gemini-code-assist · 2026-05-07T12:03:21Z

+    return {
+      'x-goog-encryption-algorithm': 'AES256',
+      'x-goog-encryption-key': this.encryptionKey.toString('base64'),
+      'x-goog-encryption-key-sha256': this.encryptionKeyHash!,


The non-null assertion this.encryptionKeyHash! is unsafe. If encryptionKeyHash is missing, the header will be set to undefined. It's better to provide a fallback value.

Suggested change

'x-goog-encryption-key-sha256': this.encryptionKeyHash!,

'x-goog-encryption-key-sha256': this.encryptionKeyHash || '',

Resolved without response. Reopening again.

Done. Replaced the non-null assertion with an empty string fallback (|| '') to safely handle cases where encryptionKeyHash is missing and prevent undefined headers.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

…transport-retry-fix

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

…transport-retry-fix

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

…transport-retry-fix

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

…transport-retry-fix

…request interception and test bench integration

…e etag check - Updated `responseHandler` to correctly handle different payload types: - Plain objects are mutated with `.headers` and `.status` and returned. - Binary payloads (Buffer/Stream) return raw data to prevent dangerous mutations. - Primitives (e.g., empty strings) return the full `GaxiosResponse` wrapper to preserve access to headers like `Location` for resumable upload initiation. - Fixed `hasPrecondition` logic to safely parse stringified JSON or inspect objects directly for an `etag` property. This prevents false positives on raw text payloads containing the word "etag" and false negatives on object payloads.

…transport-retry-fix

thiyaguk09 added 3 commits May 7, 2026 09:10

fix(storage): standardize URL formatting and enhance transport retry

b2e12e7

fix storage transport & retry issues

a55baa3

fix

d50bc9e

thiyaguk09 requested review from a team as code owners May 7, 2026 12:00

product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label May 7, 2026

gemini-code-assist Bot reviewed May 7, 2026

View reviewed changes

thiyaguk09 and others added 2 commits May 7, 2026 17:49

Update handwritten/storage/src/file.ts

618abb6

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

fix(storage): interceptors test

2a2ef77

thiyaguk09 mentioned this pull request May 12, 2026

feat(storage): enhance StorageTransport retry logic and parameter serialization #8119

Closed

4 tasks

thiyaguk09 added 2 commits May 14, 2026 12:54

fix(storage): standardize URL formatting and enhance transport retry

65f9f33

refactor(storage): remove Service.ts and migrate logic to StorageTran…

1ae557f

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

thiyaguk09 force-pushed the storage-node-18 branch from 011f5b5 to 1ae557f Compare May 14, 2026 13:19

thiyaguk09 added 3 commits May 14, 2026 13:26

Merge remote-tracking branch 'upstream/storage-node-18' into storage-…

f3b81f0

…transport-retry-fix

fix(storage): standardize URL formatting and enhance transport retry

bd33380

refactor(storage): remove Service.ts and migrate logic to StorageTran…

cc411a0

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

thiyaguk09 force-pushed the storage-node-18 branch from 1ae557f to cc411a0 Compare May 14, 2026 15:48

thiyaguk09 added 3 commits May 14, 2026 16:04

Merge remote-tracking branch 'upstream/storage-node-18' into storage-…

d8385a4

…transport-retry-fix

fix(storage): standardize URL formatting and enhance transport retry

128bf0a

refactor(storage): remove Service.ts and migrate logic to StorageTran…

9010041

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

thiyaguk09 force-pushed the storage-node-18 branch from cc411a0 to 9010041 Compare May 15, 2026 03:20

thiyaguk09 added 3 commits May 15, 2026 03:39

Merge remote-tracking branch 'upstream/storage-node-18' into storage-…

cf7060a

…transport-retry-fix

fix(storage): standardize URL formatting and enhance transport retry

7b6d68b

refactor(storage): remove Service.ts and migrate logic to StorageTran…

0e8f067

…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18

thiyaguk09 force-pushed the storage-node-18 branch from 9010041 to 0e8f067 Compare May 18, 2026 02:42

thiyaguk09 added 2 commits May 18, 2026 02:50

Merge remote-tracking branch 'upstream/storage-node-18' into storage-…

7fe696a

…transport-retry-fix

feat: implement robust storage conformance test retry framework with …

e399bea

…request interception and test bench integration

thiyaguk09 requested a review from Dhriti07 May 18, 2026 13:35

Dhriti07 mentioned this pull request May 19, 2026

fix(storage): Invocation ID is not retained on multipart upload retries #8190

Open

4 tasks

thiyaguk09 force-pushed the storage-node-18 branch from 0e8f067 to 72c17d7 Compare May 19, 2026 09:38

Merge remote-tracking branch 'upstream/storage-node-18' into storage-…

e506e77

…transport-retry-fix

	'x-goog-encryption-key-sha256': this.encryptionKeyHash!,
	'x-goog-encryption-key-sha256': this.encryptionKeyHash \|\| '',

Conversation

thiyaguk09 commented May 7, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants