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

.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/middleware-apply-body-checksum/src/middleware-apply-body-checksum.integ.spec.ts

@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
 describe("middleware-apply-body-checksum", () => {
   describe(Weather.name, () => {
     it("should add body-checksum", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
       requireRequestsFrom(client).toMatch({
         headers: {
           "content-md5": /^.{22}(==)?$/i,

packages/middleware-content-length/src/middleware-content-length.integ.spec.ts

@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
 describe("middleware-content-length", () => {
   describe(Weather.name, () => {
     it("should not add content-length if no body", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
       requireRequestsFrom(client).toMatch({
         headers: {
           "content-length": /undefined/,
@@ -24,7 +31,14 @@ describe("middleware-content-length", () => {
     // This tests that content-length gets set to `2`, only where bodies are
     // sent in the request.
     it("should add content-length if body present", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
       requireRequestsFrom(client).toMatch({
         headers: {
           "content-length": /2/,

packages/middleware-retry/src/middleware-retry.integ.spec.ts

@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
 describe("middleware-retry", () => {
   describe(Weather.name, () => {
     it("should set retry headers", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
 
       requireRequestsFrom(client).toMatch({
         hostname: "foo.bar",

packages/middleware-serde/src/middleware-serde.integ.spec.ts

@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
 describe("middleware-serde", () => {
   describe(Weather.name, () => {
     it("should serialize TestProtocol", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
       requireRequestsFrom(client).toMatch({
         method: "PUT",
         hostname: "foo.bar",

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

@@ -9,7 +9,7 @@ import {
   TRANSIENT_ERROR_STATUS_CODES,
 } from "./constants";
 
-export const isRetryableByTrait = (error: SdkError) => error.$retryable !== undefined;
+export const isRetryableByTrait = (error: SdkError) => error?.$retryable !== undefined;
 
 /**
  * @deprecated use isClockSkewCorrectedError. This is only used in deprecated code.
@@ -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",
+  },
+});

packages/util-stream/src/util-stream.integ.spec.ts

@@ -12,7 +12,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
 describe("util-stream", () => {
   describe(Weather.name, () => {
     it("should be uniform between string and Uint8Array payloads", async () => {
-      const client = new Weather({ endpoint: "https://foo.bar" });
+      const client = new Weather({
+        endpoint: "https://foo.bar",
+        region: "us-west-2",
+        credentials: {
+          accessKeyId: "INTEG",
+          secretAccessKey: "INTEG",
+        },
+      });
       requireRequestsFrom(client).toMatch({
         method: "POST",
         hostname: "foo.bar",
@@ -47,7 +54,14 @@ describe("util-stream", () => {
   });
 
   describe("blob helper integration", () => {
-    const client = new Weather({ endpoint: "https://foo.bar" });
+    const client = new Weather({
+      endpoint: "https://foo.bar",
+      region: "us-west-2",
+      credentials: {
+        accessKeyId: "INTEG",
+        secretAccessKey: "INTEG",
+      },
+    });
 
     requireRequestsFrom(client).toMatch({
       method: "POST",

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>
  *