fix(gaxios): stop retrying once totalTimeout has elapsed

[kwy404]

Root cause: timeOfFirstRequest was only recorded in getRetryConfig, after the first request had already failed, so the time spent on that first attempt never counted. On top of that, shouldRetryRequest never compared the elapsed time with totalTimeout. Once the deadline passed, getNextRetryDelay returned a negative delay and the request was retried immediately until the retry count ran out, which is what #7660 describes.

Fix: record retryConfig.timeOfFirstRequest in #prepareRequest, before the first attempt is sent (retries call _request directly, so it is set once per request, and getRetryConfig still fills it in when retryConfig was not set), and return false from shouldRetryRequest once Date.now() - timeOfFirstRequest >= totalTimeout. The last sleep is still shortened so that the last retry runs at the deadline. Both parts are needed: with only one of them the new test still fails.

Test: "should not retry once totalTimeout has elapsed" sends one request that answers 500 after 200 ms, with totalTimeout set to 100. Before the fix gaxios retried anyway and the request failed with "Nock: No match for request". After the fix it rejects with the original 500 and makes no retry. The full gaxios unit suite (130 tests) passes, coverage goes up slightly, and eslint and prettier are clean.

• Make sure to open an issue as a bug/issue before writing your code
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #7660

[gemini-code-assist]

Code Review

This pull request implements a mechanism to stop retrying requests once a specified totalTimeout has elapsed, including a new test case to verify this behavior. The review feedback points out two important issues: first, directly mutating opts.retryConfig can lead to side effects if the configuration object is shared across multiple requests, and should be resolved by deep cloning the object; second, using non-null assertions on optional properties like timeOfFirstRequest and totalTimeout is unsafe and could result in NaN comparisons, which should be avoided by adding explicit undefined checks.

[gemini-code-assist]

Directly mutating opts.retryConfig can lead to severe side effects if the user-provided options object or the default configuration object (this.defaults.retryConfig) is shared or reused across multiple requests. If mutated, subsequent requests will inherit the timeOfFirstRequest from the first request, causing them to immediately skip retries because they falsely believe the total timeout has already elapsed. To prevent this, use a deep copy (e.g., deep clone or extend) of the retryConfig object instead of a shallow copy to protect the user's original configuration from accidental mutation.

    if (opts.retryConfig && !opts.retryConfig.timeOfFirstRequest) {
      opts.retryConfig = deepClone(opts.retryConfig);
      opts.retryConfig.timeOfFirstRequest = Date.now();
    }

References

1. When copying configuration objects that contain nested objects and are passed to underlying libraries that may mutate them, use a deep copy (e.g., deep extend or clone) instead of a shallow copy (e.g., Object.assign()) to protect the user's original configuration from accidental mutation.

[kwy404]

Thanks for looking. opts here is not the caller's object. #prepareRequest builds it with extend(true, {}, this.defaults, options), which deep copies retryConfig, so this only writes to a per request copy and never to the retryConfig passed in or to this.defaults.retryConfig. I checked it by reusing one retryConfig object, and separately instance defaults, for two requests spaced further apart than totalTimeout: the originals stay unchanged and the second request still retries. So I left this as is.

[gemini-code-assist]

Using the non-null assertion operator (!) on config.timeOfFirstRequest and config.totalTimeout is unsafe because these properties are optional and can be undefined. If totalTimeout is not configured, this check will perform arithmetic operations with undefined, resulting in NaN comparisons. It is safer to explicitly check that both properties are defined before performing the comparison.

  // If the total timeout has elapsed, return
  if (
    config.timeOfFirstRequest !== undefined &&
    config.totalTimeout !== undefined &&
    Date.now() - config.timeOfFirstRequest >= config.totalTimeout
  ) {
    return false;
  }

[kwy404]

shouldRetryRequest is not exported and only runs as the default shouldRetry from getRetryConfig, which fills in timeOfFirstRequest (Date.now()) and totalTimeout (Number.MAX_SAFE_INTEGER) before calling it, so both are always defined here. getNextRetryDelay below relies on the same two fields the same way. Even if one were undefined, the comparison would involve NaN and be false, so it would just fall back to the previous behavior. I kept it consistent with the rest of the file.