When SDK introduces client side rate limiting in default RetryStrategy, the error is swallowed

[trivikr]

Checkboxes for prior research

• I've gone through Developer Guide and API reference
• I've checked AWS Forums and StackOverflow.
• I've searched for previous similar issues and didn't find any solution.

Describe the bug

When SDK introduces client side rate limiting in Retry Strategy, the error is swallowed

SDK version number

All, used v3.335.0 in testing

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

All, used v16.20.0 in testing

Reproduction Steps

Here is a repro which adds custom Handler which returns a Timeout for every call, and a retryStrategy with just 100ms delay between retries.

import { Kinesis } from "@aws-sdk/client-kinesis"; // v3.335.0
import { NodeHttp2Handler } from "@aws-sdk/node-http-handler";
import { ConfiguredRetryStrategy } from "@aws-sdk/util-retry";

// Simlulate a timeout error.
class NodeHttp2HandlerReturnsTimeout extends NodeHttp2Handler {
  async handle(request, options) {
    const timeoutError = new Error("Request timed out");
    timeoutError.name = "TimeoutError";
    throw timeoutError;
  }
}

const client = new Kinesis({
  region: "us-west-2",
  requestHandler: new NodeHttp2HandlerReturnsTimeout(),
  retryStrategy: new ConfiguredRetryStrategy(
    4, // max attempts.
    () => 100 // delay only for 100ms between retries to speed up client side rate limiting.
  ),
});

while (true) {
  try {
    await client.listStreams({});
  } catch (error) {
    if (error.$metadata.attempts === 1) {
      // SDK introduced client side rate limiting.
      throw error;
    }
  }
}

Observed Behavior

Error is thrown by the SDK with no info on why the error was thrown

file:///Users/trivikr/workspace/test/test.mjs:8
    const timeoutError = new Error("Request timed out");
                         ^

Error [TimeoutError]: Request timed out
    at NodeHttp2HandlerReturnsTimeout.handle (file:///Users/trivikr/workspace/test/test.mjs:8:26)
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/client-kinesis/dist-cjs/commands/ListStreamsCommand.js:36:58
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:5:32
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:14:26
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
    at async file:///Users/trivikr/workspace/test/test.mjs:27:5 {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}

Expected Behavior

Error thrown by the SDK with info on why the error was thrown

/Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:40
                    const errorToThrow = new Error(refreshError.message, { cause: lastError });
                                         ^

Error: No retry token available
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:40:42
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
    at async file:///Users/trivikr/workspace/test/test.mjs:27:5 {
  '$metadata': { attempts: 1, totalRetryDelay: 0 },
  [cause]: Error [TimeoutError]: Request timed out
      at NodeHttp2HandlerReturnsTimeout.handle (file:///Users/trivikr/workspace/test/test.mjs:8:26)
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/client-kinesis/dist-cjs/commands/ListStreamsCommand.js:36:58
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:5:32
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:14:26
      at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
      at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
      at async file:///Users/trivikr/workspace/test/test.mjs:27:5
}

This error was thrown by updating the catch clause in node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js as follows:

                catch (refreshError) {
                    const errorToThrow = new Error(refreshError.message, { cause: lastError });
                    errorToThrow.$metadata = {
                        attempts: attempts + 1,
                        totalRetryDelay,
                    };
                    throw errorToThrow;
                }

Possible Solution

Update the catch clause in retryMiddleware to not swallow the error thrown by refreshRetryTokenForRetry

Additional Information/Context

• Source code which throws "No retry token available" error:

  aws-sdk-js-v3/packages/util-retry/src/StandardRetryStrategy.ts

  Line 36 in 7f8d884

  throw new Error("No retry token available");

• The refreshError is swallowed and lastError is thrown with metadata:

  aws-sdk-js-v3/packages/middleware-retry/src/retryMiddleware.ts

  Lines 60 to 67 in 7f8d884

  	} catch (refreshError) {
  	if (!lastError.$metadata) {
  	lastError.$metadata = {};
  	}
  	lastError.$metadata.attempts = attempts + 1;
  	lastError.$metadata.totalRetryDelay = totalRetryDelay;
  	throw lastError;
  	}

[github-actions]

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

[cipak]

Hello @trivikr, I've been assigned with solving this issue you logged a while ago. I looked into your suggested solution, and also another solution that would involve including a list of all encountered errors (from all attempts) into the $metadata of the thrown error, which would give even more information to the developer about why the operation failed. The retry code now resides in smithy-typescript, so a PR with a fix would have to be created there (https://github.com/smithy-lang/smithy-typescript/blob/main/packages/middleware-retry/src/retryMiddleware.ts#L75).

However. On second thought, I feel like the "moral" cause of the operation failing, in your example, is still the timeout, not the depletion of the retry tokens. Otherwise, the "refresh" error will always be the thrown error, even when maxAttempts is set to 1. Currently, one can "detect" a refresh error by looking at the $metadata.attempts field of the received error, and comparing the numeric value with their maxAttempts limit.

On the other hand, you did include the timeout error in the refresh error via the {cause: lastError} option, although this requires creating a new error object (not a big deal I guess). So if you still think the error to be thrown should be the refresh error, I'm happy to create a PR on smithy-typescript and hope for the best:)

[cipak]

The current JS SDK documentation gives an example of TimeoutError being thrown in case of a failed operation, when the max number of attempts has been reached:

https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/programming-with-javascript.html#programming-with-javascript-error-handling