8000 retry subdomain requests to be more resilient to flakes (#10832) · cloudflare/workers-sdk@f9d37db · GitHub
[go: up one dir, main page]
Skip to content

Commit f9d37db

Browse files
petebacondarwinpetebacondarwin
authored
retry subdomain requests to be more resilient to flakes (#10832)
* retry subdomain requests to be more resilient to flakes * fixup! retry subdomain requests to be more resilient to flakes
1 parent cc47218 commit f9d37db

File tree

9 files changed

+141
-103
lines changed

9 files changed

+141
-103
lines changed

‎.changeset/tangy-donuts-jog.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"wrangler": patch
3+
---
4+
5+
retry subdomain requests to be more resilient to flakes

‎packages/wrangler/src/__tests__/deploy.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6081,6 +6081,27 @@ addEventListener('fetch', event => {});`
60816081
expect(std.err).toMatchInlineSnapshot(`""`);
60826082
});
60836083

6084+
it("should deploy successfully if the /subdomain POST request is flaky", async () => {
6085+
writeWranglerConfig();
6086+
writeWorkerSource();
6087+
mockUploadWorkerRequest();
6088+
mockGetWorkerSubdomain({ enabled: false });
6089+
mockSubDomainRequest();
6090+
mockUpdateWorkerSubdomain({ enabled: true, flakeCount: 1 });
6091+
6092+
await runWrangler("deploy ./index");
6093+
6094+
expect(std.out).toMatchInlineSnapshot(`
6095+
"Total Upload: xx KiB / gzip: xx KiB
6096+
Worker Startup Time: 100 ms
6097+
Uploaded test-name (TIMINGS)
6098+
Deployed test-name triggers (TIMINGS)
6099+
https://test-name.test-sub-domain.workers.dev
6100+
Current Version ID: Galaxy-Class"
6101+
`);
6102+
expect(std.err).toMatchInlineSnapshot(`""`);
6103+
});
6104+
60846105
it("should deploy to the workers.dev domain if workers_dev is `true`", async () => {
60856106
writeWranglerConfig({
60866107
workers_dev: true,

‎packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,18 +79,21 @@ export function mockUpdateWorkerSubdomain({
7979
env,
8080
legacyEnv = false,
8181
expectedScriptName = "test-name",
82+
flakeCount = 0,
8283
}: {
8384
enabled: boolean;
8485
previews_enabled?: boolean;
8586
env?: string | undefined;
8687
legacyEnv?: boolean | undefined;
8788
expectedScriptName?: string;
89+
flakeCount?: number; // The first `flakeCount` requests will fail with a 500 error
8890
}) {
8991
const url =
9092
env && !legacyEnv
9193
? `*/accounts/:accountId/workers/services/:scriptName/environments/:envName/subdomain`
9294
: `*/accounts/:accountId/workers/scripts/:scriptName/subdomain`;
93-
msw.use(
95+
96+
const handlers = [
9497
http.post(
9598
url,
9699
async ({ request, params }) => {
@@ -108,6 +111,23 @@ export function mockUpdateWorkerSubdomain({
108111
);
109112
},
110113
{ once: true }
111-
)
112-
);
114+
),
115+
];
116+
while (flakeCount > 0) {
117+
flakeCount--;
118+
handlers.unshift(
119+
http.post(
120+
url,
121+
() =>
122+
HttpResponse.json(
123+
createFetchResult(null, false, [
124+
{ code: 10013, message: "An unknown error has occurred." },
125+
]),
126+
{ status: 500 }
127+
),
128+
{ once: true }
129+
)
130+
);
131+
}
132+
msw.use(...handlers);
113133
}

‎packages/wrangler/src/__tests__/utils/retry.test.ts‎

Lines changed: 46 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
1+
import { logger } from "../../logger";
12
import { APIError } from "../../parse";
23
import { retryOnAPIFailure } from "../../utils/retry";
34
import { mockConsoleMethods } from "../helpers/mock-console";
45

56
describe("retryOnAPIFailure", () => {
67
const std = mockConsoleMethods();
78

9+
beforeEach(() => {
10+
const level = logger.loggerLevel;
11+
logger.loggerLevel = "debug";
12+
return () => (logger.loggerLevel = level);
13+
});
14+
815
it("should retry 5xx errors and succeed if the 3rd try succeeds", async () => {
916
let attempts = 0;
1017

@@ -15,15 +22,13 @@ describe("retryOnAPIFailure", () => {
1522
}
1623
});
1724
expect(attempts).toBe(3);
18-
expect(std).toMatchInlineSnapshot(`
19-
Object {
20-
"debug": "",
21-
"err": "",
22-
"info": "Retrying API call after error...
23-
Retrying API call after error...",
24-
"out": "",
25-
"warn": "",
26-
}
25+
expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
26+
Array [
27+
"Retrying API call after error...",
28+
"APIError: 500 error",
29+
"Retrying API call after error...",
30+
"APIError: 500 error",
31+
]
2732
`);
2833
});
2934

@@ -37,16 +42,15 @@ describe("retryOnAPIFailure", () => {
3742
})
3843
).rejects.toMatchInlineSnapshot(`[APIError: 500 error]`);
3944
expect(attempts).toBe(3);
40-
expect(std).toMatchInlineSnapshot(`
41-
Object {
42-
"debug": "",
43-
"err": "",
44-
"info": "Retrying API call after error...
45-
Retrying API call after error...
46-
Retrying API call after error...",
47-
"out": "",
48-
"warn": "",
49-
}
45+
expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
46+
Array [
47+
"Retrying API call after error...",
48+
"APIError: 500 error",
49+
"Retrying API call after error...",
50+
"APIError: 500 error",
51+
"Retrying API call after error...",
52+
"APIError: 500 error",
53+
]
5054
`);
5155
});
5256

@@ -60,15 +64,7 @@ describe("retryOnAPIFailure", () => {
6064
})
6165
).rejects.toMatchInlineSnapshot(`[APIError: 401 error]`);
6266
expect(attempts).toBe(1);
63-
expect(std).toMatchInlineSnapshot(`
64-
Object {
65-
"debug": "",
66-
"err": "",
67-
"info": "",
68-
"out": "",
69-
"warn": "",
70-
}
71-
`);
67+
expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`Array []`);
7268
});
7369

7470
it("should retry TypeError", async () => {
@@ -81,16 +77,12 @@ describe("retryOnAPIFailure", () => {
8177
})
8278
).rejects.toMatchInlineSnapshot(`[TypeError: type error]`);
8379
expect(attempts).toBe(3);
84-
expect(std).toMatchInlineSnapshot(`
85-
Object {
86-
"debug": "",
87-
"err": "",
88-
"info": "Retrying API call after error...
89-
Retrying API call after error...
90-
Retrying API call after error...",
91-
"out": "",
92-
"warn": "",
93-
}
80+
expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
81+
Array [
82+
"Retrying API call after error...",
83+
"Retrying API call after error...",
84+
"Retrying API call after error...",
85+
]
9486
`);
9587
});
9688

@@ -104,15 +96,7 @@ describe("retryOnAPIFailure", () => {
10496
})
10597
).rejects.toMatchInlineSnapshot(`[Error: some error]`);
10698
expect(attempts).toBe(1);
107-
expect(std).toMatchInlineSnapshot(`
108-
Object {
109-
"debug": "",
110-
"err": "",
111-
"info": "",
112-
"out": "",
113-
"warn": "",
114-
}
7F2 115-
`);
99+
expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`Array []`);
116100
});
117101

118102
it("should retry custom APIError implementation with non-5xx error", async () => {
@@ -134,16 +118,21 @@ describe("retryOnAPIFailure", () => {
134118
).rejects.toMatchInlineSnapshot(`[CustomAPIError: 401 error]`);
135119
expect(attempts).toBe(3);
136120
expect(checkedCustomIsRetryable).toBe(true);
137-
expect(std).toMatchInlineSnapshot(`
138-
Object {
139-
"debug": "",
140-
"err": "",
141-
"info": "Retrying API call after error...
142-
Retrying API call after error...
143-
Retrying API call after error...",
144-
"out": "",
145-
"warn": "",
146-
}
121+
ex 98A7 pect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
122+
Array [
123+
"Retrying API call after error...",
124+
"CustomAPIError: 401 error",
125+
"Retrying API call after error...",
126+
"CustomAPIError: 401 error",
127+
"Retrying API call after error...",
128+
"CustomAPIError: 401 error",
129+
]
147130
`);
148131
});
149132
});
133+
134+
function getRetryAndErrorLogs(debugOutput: string): string[] {
135+
return debugOutput
136+
.split("\n")
137+
.filter((line) => line.includes("Retrying") || line.includes("APIError"));
138+
}

‎packages/wrangler/src/__tests__/versions/versions.upload.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ describe("versions upload", () => {
250250
writeWorkerSource();
251251
setIsTTY(false);
252252

253-
const result = runWrangler("versions upload");
253+
const result = runWrangler("versions upload", { WRANGLER_LOG: "debug" });
254254

255255
await expect(result).resolves.toBeUndefined();
256256

@@ -265,7 +265,7 @@ describe("versions upload", () => {
265265
Worker Version ID: 51e4886e-2db7-4900-8d38-fbfecfeab993"
266266
`);
267267

268-
expect(std.info).toContain("Retrying API call after error...");
268+
expect(std.debug).toContain("Retrying API call after error...");
269269
});
270270

271271
test("correctly detects python workers", async () => {

‎packages/wrangler/src/cfetch/index.ts‎

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,13 @@ export async function fetchResult<ResponseType>(
3131
abortSignal?: AbortSignal,
3232
apiToken?: ApiCredentials
3333
): Promise<ResponseType> {
34-
const json = await fetchInternal<FetchResult<ResponseType>>(
35-
complianceConfig,
36-
resource,
37-
init,
38-
queryParams,
39-
abortSignal,
40-
apiToken
41-
);
34+
const { response: json, status } = await fetchInternal<
35+
FetchResult<ResponseType>
36+
>(complianceConfig, resource, init, queryParams, abortSignal, apiToken);
4237
if (json.success) {
4338
return json.result;
4439
} else {
45-
throwFetchError(resource, json);
40+
throwFetchError(resource, json, status);
4641
}
4742
}
4843

@@ -54,7 +49,7 @@ export async function fetchGraphqlResult<ResponseType>(
5449
init: RequestInit = {},
5550
abortSignal?: AbortSignal
5651
): Promise<ResponseType> {
57-
const json = await fetchInternal<ResponseType>(
52+
const { response: json } = await fetchInternal<ResponseType>(
5853
complianceConfig,
5954
"/graphql",
6055
{ ...init, method: "POST" }, //Cloudflare API v4 doesn't allow GETs to /graphql
@@ -87,12 +82,9 @@ export async function fetchListResult<ResponseType>(
8782
queryParams = new URLSearchParams(queryParams);
8883
queryParams.set("cursor", cursor);
8984
}
90-
const json = await fetchInternal<FetchResult<ResponseType[]>>(
91-
complianceConfig,
92-
resource,
93-
init,
94-
queryParams
95-
);
85+
const { response: json, status } = await fetchInternal<
86+
FetchResult<ResponseType[]>
87+
>(complianceConfig, resource, init, queryParams);
9688
if (json.success) {
9789
results.push(...json.result);
9890
if (hasCursor(json.result_info)) {
@@ -101,7 +93,7 @@ export async function fetchListResult<ResponseType>(
10193
getMoreResults = false;
10294
}
10395
} else {
104-
throwFetchError(resource, json);
96+
throwFetchError(resource, json, status);
10597
}
10698
}
10799
return results;
@@ -127,12 +119,9 @@ export async function fetchPagedListResult<ResponseType>(
127119
queryParams = new URLSearchParams(queryParams);
128120
queryParams.set("page", String(page));
129121

130-
const json = await fetchInternal<FetchResult<ResponseType[]>>(
131-
complianceConfig,
132-
resource,
133-
init,
134-
queryParams
135-
);
122+
const { response: json, status } = await fetchInternal<
123+
FetchResult<ResponseType[]>
124+
>(complianceConfig, resource, init, queryParams);
136125
if (json.success) {
137126
results.push(...json.result);
138127
if (hasMorePages(json.result_info)) {
@@ -141,7 +130,7 @@ export async function fetchPagedListResult<ResponseType>(
141130
getMoreResults = false;
142131
}
143132
} else {
144-
throwFetchError(resource, json);
133+
throwFetchError(resource, json, status);
145134
}
146135
}
147136
return results;
@@ -171,7 +160,8 @@ export function hasMorePages(
171160

172161
function throwFetchError(
173162
resource: string,
174-
response: FetchResult<unknown>
163+
response: FetchResult<unknown>,
164+
status: number
175165
): never {
176166
// This is an error from within an MSW handler
177167
if (typeof vitest !== "undefined" && !("errors" in response)) {
@@ -187,6 +177,7 @@ function throwFetchError(
187177
...response.errors.map((err) => ({ text: renderError(err) })),
188178
...(response.messages?.map((text) => ({ text })) ?? []),
189179
],
180+
status,
190181
});
191182
// add the first error code directly to this error
192183
// so consumers can use it for specific behaviour

‎packages/wrangler/src/cfetch/internal.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ export async function fetchInternal<ResponseType>(
158158
queryParams?: URLSearchParams,
159159
abortSignal?: AbortSignal,
160160
apiToken?: ApiCredentials
161-
): Promise<ResponseType> {
161+
): Promise<{ response: ResponseType; status: number }> {
162162
const method = init.method ?? "GET";
163163
const response = await performApiFetch(
164164
complianceConfig,
@@ -181,12 +181,20 @@ export async function fetchInternal<ResponseType>(
181181
// HTTP 204 and HTTP 205 responses do not return a body. We need to special-case this
182182
// as otherwise parseJSON will throw an error back to the user.
183183
if (!jsonText && (response.status === 204 || response.status === 205)) {
184-
const emptyBody = `{"result": {}, "success": true, "errors": [], "messages": []}`;
185-
return parseJSON(emptyBody) as ResponseType;
184+
return {
185+
response: {
186+
result: {},
187+
success: true,
188+
errors: [],
189+
messages: [],
190+
} as ResponseType,
191+
status: response.status,
192+
};
186193
}
187194

188195
try {
189-
return parseJSON(jsonText) as ResponseType;
196+
const json = parseJSON(jsonText) as ResponseType;
197+
return { response: json, status: response.status };
190198
} catch {
191199
throw new APIError({
192200
text: "Received a malformed response from the API",

0 commit comments

Comments
 (0)
0