googleapis / googleapis/google-cloud-node

[gaxios] totalTimeout does not stop retries when timeout is exceeded

Open
#7,660 1 comment 0 reactions 0 assignees View on GitHub
library: google-cloud-node-core size: s type: bug
Dominant language
TypeScript
Stars
3.2k
Forks
712
Avg merge
2d 3h
Merged PRs (30d)
99

Description

## Description

The `totalTimeout` option in `RetryConfig` does not work as documented. According to the type definition:

> "The total time starting from when the initial request is sent, after which an error will be returned, regardless of the retrying attempts made meanwhile."

However, the actual implementation only uses `totalTimeout` to shorten the delay between retries, but **never stops retries** when the timeout is exceeded.

## Expected Behavior

When `Date.now() - timeOfFirstRequest >= totalTimeout`, retries should stop and an error should be returned.

## Actual Behavior

Retries continue indefinitely (up to `retry` count) even after `totalTimeout` is exceeded. The only effect is that `maxAllowableDelay` becomes negative, causing immediate retries (since `setTimeout` treats negative values as 0).

## Root Cause

In `packages/gaxios/src/retry.ts`, the `shouldRetryRequest` function has no check for `totalTimeout`:

```typescript
function shouldRetryRequest(err: GaxiosError) {
const config = getConfig(err);

// Checks for: abort, config.retry === 0, noResponseRetries,
// httpMethodsToRetry, statusCodesToRetry, currentRetryAttempt

// ❌ Missing check for totalTimeout!

return true;
}
```

The `totalTimeout` is only used in `getNextRetryDelay`:

```typescript
const maxAllowableDelay =
config.totalTimeout! - (Date.now() - config.timeOfFirstRequest!);

return Math.min(calculatedDelay, maxAllowableDelay, config.maxRetryDelay!);
```

## Suggested Fix

Add a timeout check to `shouldRetryRequest`:

```typescript
function shouldRetryRequest(err: GaxiosError) {
const config = getConfig(err);

// ... existing checks ...

// Check if total timeout exceeded
if (config.totalTimeout && config.timeOfFirstRequest) {
const elapsed = Date.now() - config.timeOfFirstRequest;
if (elapsed >= config.totalTimeout) {
return false;
}
}

return true;
}
```

## Environment

- gaxios version: 7.1.3 (also affects earlier versions)
- Node.js version: 18+

## Impact

This affects any user relying on `totalTimeout` to bound the total retry duration. Without this fix, retries can continue much longer than expected.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.