fix(service-error-classification): consider $retryable trait errors to be transient

Author: kuhe

.changeset/mean-paws-check.md

@@ -0,0 +1,6 @@
+---
+"@smithy/service-error-classification": patch
+"@smithy/util-retry": patch
+---
+
+make $retryable-trait errors considered transient in StandardRetryStrategyV2

packages/service-error-classification/src/index.ts

@@ -55,6 +55,7 @@ export const isThrottlingError = (error: SdkError) =>
  * the name "TimeoutError" to be checked by the TRANSIENT_ERROR_CODES condition.
  */
 export const isTransientError = (error: SdkError, depth = 0): boolean =>
+  isRetryableByTrait(error) ||
   isClockSkewCorrectedError(error) ||
   TRANSIENT_ERROR_CODES.includes(error.name) ||
   NODEJS_TIMEOUT_ERROR_CODES.includes((error as { code?: string })?.code || "") ||

packages/util-retry/package.json

@@ -16,7 +16,9 @@
     "format": "prettier --config ../../prettier.config.js --ignore-path ../../.prettierignore --write \"**/*.{ts,md,json}\"",
     "extract:docs": "api-extractor run --local",
     "test": "yarn g:vitest run",
-    "test:watch": "yarn g:vitest watch"
+    "test:watch": "yarn g:vitest watch",
+    "test:integration": "yarn g:vitest run -c vitest.config.integ.mts",
+    "test:integration:watch": "yarn g:vitest watch -c vitest.config.integ.mts"
   },
   "keywords": [
     "aws",

packages/util-retry/src/retries.integ.spec.ts

@@ -0,0 +1,140 @@
+import { cbor } from "@smithy/core/cbor";
+import { HttpResponse } from "@smithy/protocol-http";
+import { requireRequestsFrom } from "@smithy/util-test/src";
+import { Readable } from "node:stream";
+import { describe, expect, test as it } from "vitest";
+import { XYZService } from "xyz";
+
+describe("retries", () => {
+  function createCborResponse(body: any, status = 200) {
+    const bytes = cbor.serialize(body);
+    return new HttpResponse({
+      headers: {
+        "smithy-protocol": "rpc-v2-cbor",
+      },
+      body: Readable.from(bytes),
+      statusCode: status,
+    });
+  }
+
+  it("should retry throttling and transient-error status codes", async () => {
+    const client = new XYZService({
+      endpoint: "https://localhost/nowhere",
+    });
+
+    requireRequestsFrom(client)
+      .toMatch({
+        hostname: /localhost/,
+      })
+      .respondWith(
+        createCborResponse(
+          {
+            __type: "HaltError",
+          },
+          429
+        ),
+        createCborResponse(
+          {
+            __type: "HaltError",
+          },
+          500
+        ),
+        createCborResponse("", 200)
+      );
+
+    const response = await client.getNumbers().catch((e) => e);
+
+    expect(response.$metadata.attempts).toEqual(3);
+  });
+
+  it("should retry when a retryable trait is modeled", async () => {
+    const client = new XYZService({
+      endpoint: "https://localhost/nowhere",
+    });
+
+    requireRequestsFrom(client)
+      .toMatch({
+        hostname: /localhost/,
+      })
+      .respondWith(
+        createCborResponse(
+          {
+            __type: "RetryableError",
+          },
+          400 // not retryable status code
+        ),
+        createCborResponse(
+          {
+            __type: "RetryableError",
+          },
+          400 // not retryable status code
+        ),
+        createCborResponse("", 200)
+      );
+
+    const response = await client.getNumbers().catch((e) => e);
+
+    expect(response.$metadata.attempts).toEqual(3);
+  });
+
+  it("should retry retryable trait with throttling", async () => {
+    const client = new XYZService({
+      endpoint: "https://localhost/nowhere",
+    });
+
+    requireRequestsFrom(client)
+      .toMatch({
+        hostname: /localhost/,
+      })
+      .respondWith(
+        createCborResponse(
+          {
+            __type: "CodedThrottlingError",
+          },
+          429
+        ),
+        createCborResponse(
+          {
+            __type: "MysteryThrottlingError",
+          },
+          400 // not a retryable status code, but error is modeled as retryable.
+        ),
+        createCborResponse("", 200)
+      );
+
+    const response = await client.getNumbers().catch((e) => e);
+
+    expect(response.$metadata.attempts).toEqual(3);
+  });
+
+  it("should not retry if the error is not modeled with retryable trait and is not otherwise retryable", async () => {
+    const client = new XYZService({
+      endpoint: "https://localhost/nowhere",
+    });
+
+    requireRequestsFrom(client)
+      .toMatch({
+        hostname: /localhost/,
+      })
+      .respondWith(
+        createCborResponse(
+          {
+            __type: "HaltError",
+          },
+          429 // not modeled as retryable, but this is a retryable status code.
+        ),
+        createCborResponse(
+          {
+            __type: "HaltError",
+          },
+          400
+        ),
+        createCborResponse("", 200)
+      );
+
+    const response = await client.getNumbers().catch((e) => e);
+
+    // stopped at the second error.
+    expect(response.$metadata.attempts).toEqual(2);
+  });
+});

packages/util-retry/vitest.config.integ.mts

@@ -0,0 +1,8 @@
+import { defineConfig } from "vitest/config";
+
+export default defineConfig({
+  test: {
+    include: ["**/*.integ.spec.ts"],
+    environment: "node",
+  },
+});

private/my-local-model/src/commands/GetNumbersCommand.ts

@@ -56,6 +56,14 @@ export interface GetNumbersCommandOutput extends GetNumbersResponse, __MetadataB
  * @see {@link GetNumbersCommandOutput} for command's `response` shape.
  * @see {@link XYZServiceClientResolvedConfig | config} for XYZServiceClient's `config` shape.
  *
+ * @throws {@link CodedThrottlingError} (client fault)
+ *
+ * @throws {@link MysteryThrottlingError} (client fault)
+ *
+ * @throws {@link RetryableError} (client fault)
+ *
+ * @throws {@link HaltError} (client fault)
+ *
  * @throws {@link XYZServiceServiceException}
  * <p>Base exception class for all service exceptions from XYZService service.</p>
  *

private/my-local-model/src/models/models_0.ts

@@ -1,5 +1,29 @@
 // smithy-typescript generated code
+import { XYZServiceServiceException as __BaseException } from "./XYZServiceServiceException";
 import { NumericValue } from "@smithy/core/serde";
+import { ExceptionOptionType as __ExceptionOptionType } from "@smithy/smithy-client";
+
+/**
+ * @public
+ */
+export class CodedThrottlingError extends __BaseException {
+  readonly name: "CodedThrottlingError" = "CodedThrottlingError";
+  readonly $fault: "client" = "client";
+  $retryable = {
+    throttling: true,
+  };
+  /**
+   * @internal
+   */
+  constructor(opts: __ExceptionOptionType<CodedThrottlingError, __BaseException>) {
+    super({
+      name: "CodedThrottlingError",
+      $fault: "client",
+      ...opts,
+    });
+    Object.setPrototypeOf(this, CodedThrottlingError.prototype);
+  }
+}
 
 /**
  * @public
@@ -16,3 +40,64 @@ export interface GetNumbersResponse {
   bigDecimal?: NumericValue | undefined;
   bigInteger?: bigint | undefined;
 }
+
+/**
+ * @public
+ */
+export class HaltError extends __BaseException {
+  readonly name: "HaltError" = "HaltError";
+  readonly $fault: "client" = "client";
+  /**
+   * @internal
+   */
+  constructor(opts: __ExceptionOptionType<HaltError, __BaseException>) {
+    super({
+      name: "HaltError",
+      $fault: "client",
+      ...opts,
+    });
+    Object.setPrototypeOf(this, HaltError.prototype);
+  }
+}
+
+/**
+ * @public
+ */
+export class MysteryThrottlingError extends __BaseException {
+  readonly name: "MysteryThrottlingError" = "MysteryThrottlingError";
+  readonly $fault: "client" = "client";
+  $retryable = {
+    throttling: true,
+  };
+  /**
+   * @internal
+   */
+  constructor(opts: __ExceptionOptionType<MysteryThrottlingError, __BaseException>) {
+    super({
+      name: "MysteryThrottlingError",
+      $fault: "client",
+      ...opts,
+    });
+    Object.setPrototypeOf(this, MysteryThrottlingError.prototype);
+  }
+}
+
+/**
+ * @public
+ */
+export class RetryableError extends __BaseException {
+  readonly name: "RetryableError" = "RetryableError";
+  readonly $fault: "client" = "client";
+  $retryable = {};
+  /**
+   * @internal
+   */
+  constructor(opts: __ExceptionOptionType<RetryableError, __BaseException>) {
+    super({
+      name: "RetryableError",
+      $fault: "client",
+      ...opts,
+    });
+    Object.setPrototypeOf(this, RetryableError.prototype);
+  }
+}