[retry-request] Request hangs indefinitely when totalTimeout is exceeded before retries are exhausted #9706

Description

In core/packages/retry-request, when an attempt fails (either with a no-response error or a retryable HTTP status code) after totalTimeout has already elapsed, retry-request stops retrying but never invokes the callback or emits an error/response event on the stream, causing the caller's callback/Promise or stream to hang indefinitely.

According to core/packages/retry-request/index.js:

"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."

Expected Behavior

When Date.now() - timeOfFirstRequest >= totalTimeout * 1000 (nextRetryDelay <= 0), retry-request should stop retrying and forward the last error or response to the caller (callback(err, response, body) in callback mode, or retryStream.emit('error', err) / retryStream.emit('response', response) in stream mode).

Actual Behavior

When nextRetryDelay <= 0, retryAfterDelay() returns early without scheduling a next attempt and without notifying the caller. The request hangs forever.

In practice, this is easily triggered in @google-cloud/storage (where totalTimeout defaults to 600 seconds / 10 minutes and no socket timeout is set by default): if a TCP connection is silently dropped by a NAT gateway/firewall (blackholed socket), the Linux kernel aborts the socket with ETIMEDOUT after ~11.3 minutes (SO_KEEPALIVE) or ~15.4 minutes (tcp_retries2 = 15). Because >10 minutes have elapsed on the first attempt, retry-request swallows the ETIMEDOUT error and leaves file.createReadStream() / file.exists() hanging indefinitely.

Root Cause

In googleapis/retry-request#38, an early return was added to retryAfterDelay() in core/packages/retry-request/index.js:

  function retryAfterDelay(currentRetryAttempt) {
    if (streamMode) {
      resetStreams();
    }

    const nextRetryDelay = getNextRetryDelay({
      maxRetryDelay: opts.maxRetryDelay,
      retryDelayMultiplier: opts.retryDelayMultiplier,
      retryNumber: currentRetryAttempt,
      timeOfFirstRequest,
      totalTimeout: opts.totalTimeout,
    });
    debug(`Next retry delay: ${nextRetryDelay}`);

    if (nextRetryDelay <= 0) {
      numNoResponseAttempts = opts.noResponseRetries + 1;
      return;
    }

    setTimeout(makeRequest, nextRetryDelay);
  }

However, both call sites in onResponse() call retryAfterDelay(...) and immediately return:

    if (err) {
      numNoResponseAttempts++;

      if (numNoResponseAttempts <= opts.noResponseRetries) {
        retryAfterDelay(numNoResponseAttempts);
      } else {
        if (streamMode) {
          retryStream.emit('error', err);
          retryStream.end();
        } else {
          callback(err, response, body);
        }
      }

      return;
    }

and:

    if (
      adjustedCurrentRetryAttempt < opts.retries &&
      opts.shouldRetryFn(response)
    ) {
      retryAfterDelay(currentRetryAttempt);
      return;
    }

Because onResponse() has already evaluated the if condition before calling retryAfterDelay(), setting numNoResponseAttempts = opts.noResponseRetries + 1 and returning from retryAfterDelay() skips setTimeout(makeRequest, nextRetryDelay) without ever executing the else/fallthrough branch in onResponse(). (Additionally, in stream mode resetStreams() is called before checking nextRetryDelay <= 0.)

Reproduction

const assert = require('assert');
const retryRequest = require('retry-request');

let numAttempts = 0;
const error = new Error('ENOTFOUND');

retryRequest(
  'http://theblahstore',
  {
    totalTimeout: 0,
    request: function (_, callback) {
      numAttempts++;
      callback(error);
    },
  },
  err => {
    // Never called!
    assert.strictEqual(numAttempts, 1);
    assert.strictEqual(err, error);
  },
);

Suggested Fix

Have retryAfterDelay() check nextRetryDelay <= 0 before resetStreams() and return a boolean (false when nextRetryDelay <= 0, true when the timer is scheduled), and only return early in onResponse() when retryAfterDelay(...) returns true.