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

[thiyaguk09]

Hoists the generation of persistentInvocationId to the beginning of the
upload process in Bucket.upload and File.save. This ensures that retried multipart upload attempts reuse the same invocation ID in the x-goog-api-client header, rather than generating a new one for each attempt.

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 implements persistent invocationId tracking across the upload lifecycle and refactors the StorageTransport.makeRequest method to improve project ID resolution and header management. Feedback focuses on critical issues in the makeRequest refactor, including a bug where the request body is incorrectly assigned to an undefined property, thread-safety risks from unconditionally clearing shared interceptors, and an inconsistent response handler that mutates data and breaks return type expectations.

[gemini-code-assist]

The interceptors.request.clear() call is now executed unconditionally for every request. This will remove any default interceptors set on the gaxiosInstance even if no new interceptors are provided in reqOpts. Furthermore, clearing and adding interceptors on a shared instance is not safe for concurrent requests. It should at least be wrapped in the if (reqOpts.interceptors) block as it was before, although a better approach would be to avoid modifying the shared instance's interceptors entirely.

Suggested change

	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);
	}
	}
	if (reqOpts.interceptors) {
	this.gaxiosInstance.interceptors.request.clear();
	for (const inter of reqOpts.interceptors) {
	this.gaxiosInstance.interceptors.request.add(inter);
	}
	}

[thiyaguk09]

required for retry operations fix in conformance Test PR

[Dhriti07]

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.

[thiyaguk09]

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]

The responseHandler introduces an inconsistency in the return type of makeRequest. If the response data is an object, it returns the data mutated with headers and status. If the response data is not an object (e.g., a string or null), it returns the entire GaxiosResponse object. This violates the expected return type T and will likely cause runtime errors in callers expecting raw data. Additionally, mutating the data object directly is risky as it might be a frozen object or a class instance that doesn't support these extra properties. It is recommended to return resp.data consistently as before.

      const responseHandler = (resp: GaxiosResponse<T>) => resp.data;

[thiyaguk09]

required for retry operations

[Dhriti07]

Please elaborate as stated in the above comment.

[thiyaguk09]

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.

[Dhriti07]

I agree with using isPlanObject to avoid mutating Buffer or Stream payloads.

However the issue mentioned above refers to the fallback "return resp;" statement. If the payload is a Buffer or Stream and the check fails, returning the entire GaxiosResponse wrapper instead of the raw data might break backward compatibility for binary downloads, since it previously resolved with just the data.

What do you think about returning data instead in that fallback case to keep the return types consistent?

[thiyaguk09]

Great catch. Simply returning data broke resumable uploads, which rely on resp.headers.location when the API returns an empty string. I've updated the logic to handle all three scenarios correctly:

• Plain objects: Mutate and return data.
• Buffers/Streams (non-plain objects): Return raw data to protect binary downloads.
• Primitives (empty strings/null): Return the full resp wrapper so session URIs can still be read from the headers.

[Dhriti07]

On the bodyStr.includes("etag") check

1. Is it possible that this could cause false positive incase a user uploads the word "etag"
2. If reqOpts.data is a object, bodyStr defaults to '', so it is possible that we would miss an existing etag entirely.

[thiyaguk09]

Spot on. The .includes('"etag"') check was brittle. I've updated this to properly inspect the payload as an object (safely parsing it first if it's a stringified JSON) to check for an actual etag key. This eliminates both false positives on raw text and false negatives on object payloads.

And as per the comment below, this refactor was removed here and added to PR #8235.

[Dhriti07]

these changes are present both in this pr and PR #8235. Since these changes look retry related are they required in this pr ?

[thiyaguk09]

You're completely right. I added these temporarily during refactoring to keep the conformance tests green, but missed removing them before updating this PR. I have removed them now!