fix(storage): Service.request_ sets { timeout: undefined }, clobbering the 60s requestDefaults.timeout and per-call options.timeout #9708

Please make sure you have searched for information in the following guides.

• Search the issues already opened: https://github.com/GoogleCloudPlatform/google-cloud-node/issues
• Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js
• Check our Troubleshooting guide: https://github.com/googleapis/google-cloud-node/blob/main/docs/troubleshooting.md
• Check our FAQ: https://github.com/googleapis/google-cloud-node/blob/main/docs/faq.md
• Check our libraries HOW-TO: https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md
• Check out our authentication guide: https://github.com/googleapis/google-auth-library-nodejs
• Check out handwritten samples for many of our APIs: https://github.com/GoogleCloudPlatform/nodejs-docs-samples
• Check the API's issue tracker: https://cloud.google.com/support/docs/issue-trackers

Library Name

@google-cloud/storage (handwritten/storage, versions 7.0.0 through 8.4.0)

A screenshot that you have tested with "Try this API".

N/A — this is a client-side option-merging bug in handwritten/storage/src/nodejs-common/service.ts before the HTTP request is sent over the network, not a server-side Cloud Storage API issue.

Link to the code that reproduces this issue. A link to a public Github Repository or gist with a minimal reproduction.

https://github.com/googleapis/google-cloud-node/blob/main/handwritten/storage/src/nodejs-common/service.ts#L222

A step-by-step description of how to reproduce the issue, based on the linked reproduction.

Scenario 1: Default 60,000 ms timeout (requestDefaults.timeout = 60000) is wiped out when new Storage() is called without { timeout }

1. Instantiate const storage = new Storage() without passing { timeout } (storage.timeout === undefined).
2. Perform any request that goes through Service.prototype.request_ (e.g., file.exists(), file.download(), file.createReadStream(), bucket.getFiles()).
3. In handwritten/storage/src/nodejs-common/service.ts:222:

   reqOpts = {...reqOpts, timeout: this.timeout};

   Because this.timeout is undefined, JS object spread creates an explicit own property { timeout: undefined } on reqOpts ('timeout' in reqOpts === true).
4. In handwritten/storage/src/nodejs-common/util.ts:64-71 and util.ts:819, util.makeRequest passes teenyRequest.defaults(requestDefaults) where requestDefaults.timeout = 60000.
5. In core/packages/teeny-request/src/index.ts:644:

   const opts = {...defaults, ...reqOpts};

   reqOpts.timeout (undefined) overwrites defaults.timeout (60000), resulting in opts.timeout === undefined (no HTTP timeout at all, even in @google-cloud/storage@7.x with teeny-request@9 / node-fetch@2, and even after teeny-request: the timeout option is ignored since 10.0.0 (node-fetch 3 has no timeout option) #9514 is resolved for teeny-request@11).

Scenario 2: Per-call options.timeout on file.createWriteStream({ resumable: false, timeout }) and bucket.upload(path, { resumable: false, timeout }) is overwritten

1. Call file.createWriteStream({ resumable: false, timeout: 15000 }) (or bucket.upload(path, { resumable: false, timeout: 15000 })).
2. In File.prototype.startSimpleUpload_ (handwritten/storage/src/file.ts:4731-4733):

   if (typeof options.timeout === 'number') {
     reqOpts.timeout = options.timeout;
   }

3. startSimpleUpload_ passes reqOpts into util.makeWritableStream, which invokes this.request(reqOpts, ...) (file.ts:4755) -> Service.prototype.request_ (service.ts:222):

   reqOpts = {...reqOpts, timeout: this.timeout};

4. Service.prototype.request_ unconditionally overwrites reqOpts.timeout (15000) with this.timeout (undefined if Storage was created without { timeout }).

A clear and concise description of what the bug is, and what you expected to happen.

Actual behavior:

1. When new Storage() is initialized without { timeout }, Service.prototype.request_ sets { timeout: undefined } as an own property on reqOpts, which clobbers requestDefaults.timeout = 60000 in teenyRequest.defaults(requestDefaults). As a result, requests have no HTTP timeout at all (combining with [retry-request] Request hangs indefinitely when totalTimeout is exceeded before retries are exhausted #9706 to cause an infinite hang if a socket is silently blackholed).
2. Passing timeout to file.createWriteStream({ resumable: false, timeout }) or bucket.upload(path, { resumable: false, timeout }) has no effect because Service.prototype.request_ overwrites reqOpts.timeout with this.timeout.

Expected behavior:

1. If neither reqOpts.timeout nor this.timeout is defined, Service.prototype.request_ should not set an explicit { timeout: undefined } property on reqOpts (preserving requestDefaults.timeout = 60000 in teeny-request).
2. If a per-request reqOpts.timeout is already specified (e.g. by startSimpleUpload_ from CreateWriteStreamOptions.timeout or makeWritableStream), Service.prototype.request_ should not clobber it when this.timeout is undefined:

   const timeout = reqOpts.timeout ?? this.timeout;
   reqOpts = {
     ...reqOpts,
     ...(timeout !== undefined && {timeout}),
   };

Related issues:

• teeny-request: the timeout option is ignored since 10.0.0 (node-fetch 3 has no timeout option) #9514 (teeny-request >= 10 ignoring timeout under node-fetch@3; note that teeny-request: the timeout option is ignored since 10.0.0 (node-fetch 3 has no timeout option) #9514 mentions nodejs-common/util.js setting timeout: 60000 for @google-cloud/storage, which is also blocked by this bug in service.ts:222 even in @google-cloud/storage@7.x with teeny-request@9)
• [retry-request] Request hangs indefinitely when totalTimeout is exceeded before retries are exhausted #9706 (retry-request hanging indefinitely when totalTimeout is exceeded after a socket without a per-request timeout waits for the OS TCP timeout)

A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. **

1. Regression from removing extend in commit ff88655cf8a (Publishing a string in to PubSub topic introduces quotes to the message #2204 / common/common-grpc: update retry-request #2244):
   Originally (and still today in core/common/src/service.ts:200), Service.prototype.request_ used extend:

   reqOpts = extend(true, {}, reqOpts, {timeout: this.timeout});

   Because extend ignores properties whose value is undefined, when this.timeout was undefined:

   • 'timeout' in reqOpts remained false (preserving requestDefaults.timeout = 60000 in teenyRequest.defaults), and
   • any existing reqOpts.timeout (such as options.timeout from startSimpleUpload_) was preserved.
     When extend was replaced with {...reqOpts, timeout: this.timeout} in ff88655cf8a, the unit test at handwritten/storage/test/nodejs-common/service.ts:442-448 (assert.strictEqual(reqOpts_.timeout, undefined)) continued to pass even though 'timeout' in reqOpts_ changed from false to true.

2. Public JSDoc explicitly documents timeout=60000 (Default: 60000):

   • handwritten/storage/src/file.ts:2032-2034 (CreateWriteStreamOptions.timeout):

     * @property {number} [timeout=60000] Set the HTTP request timeout in
     *     milliseconds. This option is not available for resumable uploads.
     *     Default: `60000`

   • handwritten/storage/src/bucket.ts:4325-4327 and bucket.ts:4403-4405 (UploadOptions.timeout):

     * @property {number} [timeout=60000] Set the HTTP request timeout in
     *     milliseconds. This option is not available for resumable uploads.
     *     Default: `60000`

   • handwritten/storage/src/nodejs-common/util.ts:64-71 defines requestDefaults = { timeout: 60000, ... }.