Skip to content

Commit 937ac5a

Browse files
kuhekuhe
authored
fix(service-error-classification): consider $retryable trait errors to be transient (#1695)
* fix(service-error-classification): consider $retryable trait errors to be transient * test: add sigv4 credentials to integration test clients
1 parent 969d21e commit 937ac5a

File tree

15 files changed

+510
-67
lines changed

15 files changed

+510
-67
lines changed

‎.changeset/mean-paws-check.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@smithy/service-error-classification": patch
3+
"@smithy/util-retry": patch
4+
---
5+
6+
make $retryable-trait errors considered transient in StandardRetryStrategyV2

‎packages/middleware-apply-body-checksum/src/middleware-apply-body-checksum.integ.spec.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
66
describe("middleware-apply-body-checksum", () => {
77
describe(Weather.name, () => {
88
it("should add body-checksum", async () => {
9-
const client = new Weather({ endpoint: "https://foo.bar" });
9+
const client = new Weather({
10+
endpoint: "https://foo.bar",
11+
region: "us-west-2",
12+
credentials: {
13+
accessKeyId: "INTEG",
14+
secretAccessKey: "INTEG",
15+
},
16+
});
1017
requireRequestsFrom(client).toMatch({
1118
headers: {
1219
"content-md5": /^.{22}(==)?$/i,

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
66
describe("middleware-content-length", () => {
77
describe(Weather.name, () => {
88
it("should not add content-length if no body", async () => {
9-
const client = new Weather({ endpoint: "https://foo.bar" });
9+
const client = new Weather({
10+
endpoint: "https://foo.bar",
11+
region: "us-west-2",
12+
credentials: {
13+
accessKeyId: "INTEG",
14+
secretAccessKey: "INTEG",
15+
},
16+
});
1017
requireRequestsFrom(client).toMatch({
1118
headers: {
1219
"content-length": /undefined/,
@@ -24,7 +31,14 @@ describe("middleware-content-length", () => {
2431
// This tests that content-length gets set to `2`, only where bodies are
2532
// sent in the request.
2633
it("should add content-length if body present", async () => {
27-
const client = new Weather({ endpoint: "https://foo.bar" });
34+
const client = new Weather({
35+
endpoint: "https://foo.bar",
36+
region: "us-west-2",
37+
credentials: {
38+
accessKeyId: "INTEG",
39+
secretAccessKey: "INTEG",
40+
},
41+
});
2842
requireRequestsFrom(client).toMatch({
2943
headers: {
3044
"content-length": /2/,

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
66
describe("middleware-retry", () => {
77
describe(Weather.name, () => {
88
it("should set retry headers", async () => {
9-
const client = new Weather({ endpoint: "https://foo.bar" });
9+
const client = new Weather({
10+
endpoint: "https://foo.bar",
11+
region: "us-west-2",
12+
credentials: {
13+
accessKeyId: "INTEG",
14+
secretAccessKey: "INTEG",
15+
},
16+
});
1017

1118
requireRequestsFrom(client).toMatch({
1219
hostname: "foo.bar",

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
66
describe("middleware-serde", () => {
77
describe(Weather.name, () => {
88
it("should serialize TestProtocol", async () => {
9-
const client = new Weather({ endpoint: "https://foo.bar" });
9+
const client = new Weather({
10+
endpoint: "https://foo.bar",
11+
region: "us-west-2",
12+
credentials: {
13+
accessKeyId: "INTEG",
14+
secretAccessKey: "INTEG",
15+
},
16+
});
1017
requireRequestsFrom(client).toMatch({
1118
method: "PUT",
1219
hostname: "foo.bar",

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
TRANSIENT_ERROR_STATUS_CODES,
1010
} from "./constants";
1111

12-
export const isRetryableByTrait = (error: SdkError) => error.$retryable !== undefined;
12+
export const isRetryableByTrait = (error: SdkError) => error?.$retryable !== undefined;
1313

1414
/**
1515
* @deprecated use isClockSkewCorrectedError. This is only used in deprecated code.
@@ -55,6 +55,7 @@ export const isThrottlingError = (error: SdkError) =>
5555
* the name "TimeoutError" to be checked by the TRANSIENT_ERROR_CODES condition.
5656
*/
5757
export const isTransientError = (error: SdkError, depth = 0): boolean =>
58+
isRetryableByTrait(error) ||
5859
isClockSkewCorrectedError(error) ||
5960
TRANSIENT_ERROR_CODES.includes(error.name) ||
6061
NODEJS_TIMEOUT_ERROR_CODES.includes((error as { code?: string })?.code || "") ||

‎packages/util-retry/package.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
"format": "prettier --config ../../prettier.config.js --ignore-path ../../.prettierignore --write \"**/*.{ts,md,json}\"",
1717
"extract:docs": "api-extractor run --local",
1818
"test": "yarn g:vitest run",
19-
"test:watch": "yarn g:vitest watch"
19+
"test:watch": "yarn g:vitest watch",
20+
"test:integration": "yarn g:vitest run -c vitest.config.integ.mts",
21+
"test:integration:watch": "yarn g:vitest watch -c vitest.config.integ.mts"
2022
},
2123
"keywords": [
2224
"aws",

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

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
import { cbor } from "@smithy/core/cbor";
2+
import { HttpResponse } from "@smithy/protocol-http";
3+
import { requireRequestsFrom } from "@smithy/util-test/src";
4+
import { Readable } from "node:stream";
5+
import { describe, expect, test as it } from "vitest";
6+
import { XYZService } from "xyz";
7+
8+
describe("retries", () => {
9+
function createCborResponse(body: any, status = 200) {
10+
const bytes = cbor.serialize(body);
11+
return new HttpResponse({
12+
headers: {
13+
"smithy-protocol": "rpc-v2-cbor",
14+
},
15+
body: Readable.from(bytes),
16+
statusCode: status,
17+
});
18+
}
19+
20+
it("should retry throttling and transient-error status codes", async () => {
21+
const client = new XYZService({
22+
endpoint: "https://localhost/nowhere",
23+
});
24+
25+
requireRequestsFrom(client)
26+
.toMatch({
27+
hostname: /localhost/,
28+
})
29+
.respondWith(
30+
createCborResponse(
31+
{
32+
__type: "HaltError",
33+
},
34+
429
35+
),
36+
createCborResponse(
37+
{
38+
__type: "HaltError",
39+
},
40+
500
41+
),
42+
createCborResponse("", 200)
43+
);
44+
45+
const response = await client.getNumbers().catch((e) => e);
46+
47+
expect(response.$metadata.attempts).toEqual(3);
48+
});
49+
50+
it("should retry when a retryable trait is modeled", async () => {
51+
const client = new XYZService({
52+
endpoint: "https://localhost/nowhere",
53+
});
54+
55+
requireRequestsFrom(client)
56+
.toMatch({
57+
hostname: /localhost/,
58+
})
59+
.respondWith(
60+
createCborResponse(
61+
{
62+
__type: "RetryableError",
63+
},
64+
400 // not retryable status code
65+
),
66+
createCborResponse(
67+
{
68+
__type: "RetryableError",
69+
},
70+
400 // not retryable status code
71+
),
72+
createCborResponse("", 200)
73+
);
74+
75+
const response = await client.getNumbers().catch((e) => e);
76+
77+
expect(response.$metadata.attempts).toEqual(3);
78+
});
79+
80+
it("should retry retryable trait with throttling", async () => {
81+
const client = new XYZService({
82+
endpoint: "https://localhost/nowhere",
83+
});
84+
85+
requireRequestsFrom(client)
86+
.toMatch({
87+
hostname: /localhost/,
88+
})
89+
.respondWith(
90+
createCborResponse(
91+
{
92+
__type: "CodedThrottlingError",
93+
},
94+
429
95+
),
96+
createCborResponse(
97+
{
98+
__type: "MysteryThrottlingError",
99+
},
100+
400 // not a retryable status code, but error is modeled as retryable.
101+
),
102+
createCborResponse("", 200)
103+
);
104+
105+
const response = await client.getNumbers().catch((e) => e);
106+
107+
expect(response.$metadata.attempts).toEqual(3);
108+
});
109+
110+
it("should not retry if the error is not modeled with retryable trait and is not otherwise retryable", async () => {
111+
const client = new XYZService({
112+
endpoint: "https://localhost/nowhere",
113+
});
114+
115+
requireRequestsFrom(client)
116+
.toMatch({
117+
hostname: /localhost/,
118+
})
119+
.respondWith(
120+
createCborResponse(
121+
{
122+
__type: "HaltError",
123+
},
124+
429 // not modeled as retryable, but this is a retryable status code.
125+
),
126+
createCborResponse(
127+
{
128+
__type: "HaltError",
129+
},
130+
400
131+
),
132+
createCborResponse("", 200)
133+
);
134+
135+
const response = await client.getNumbers().catch((e) => e);
136+
137+
// stopped at the second error.
138+
expect(response.$metadata.attempts).toEqual(2);
139+
});
140+
});

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { defineConfig } from "vitest/config";
2+
3+
export default defineConfig({
4+
test: {
5+
include: ["**/*.integ.spec.ts"],
6+
environment: "node",
7+
},
8+
});

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,14 @@ import { requireRequestsFrom } from "../../../private/util-test/src/index";
1212
describe("util-stream", () => {
1313
describe(Weather.name, () => {
1414
it("should be uniform between string and Uint8Array payloads", async () => {
15-
const client = new Weather({ endpoint: "https://foo.bar" });
15+
const client = new Weather({
16+
endpoint: "https://foo.bar",
17+
region: "us-west-2",
18+
credentials: {
19+
accessKeyId: "INTEG",
20+
secretAccessKey: "INTEG",
21+
},
22+
});
1623
requireRequestsFrom(client).toMatch({
1724
method: "POST",
1825
hostname: "foo.bar",
@@ -47,7 +54,14 @@ describe("util-stream", () => {
4754
});
4855

4956
describe("blob helper integration", () => {
50-
const client = new Weather({ endpoint: "https://foo.bar" });
57+
const client = new Weather({
58+
endpoint: "https://foo.bar",
59+
region: "us-west-2",
60+
credentials: {
61+
accessKeyId: "INTEG",
62+
secretAccessKey: "INTEG",
63+
},
64+
});
5165

5266
requireRequestsFrom(client).toMatch({
5367
method: "POST",

0 commit comments

Comments
 (0)