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

Author: petebacondarwin

.changeset/tangy-donuts-jog.md

@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+retry subdomain requests to be more resilient to flakes

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

@@ -6081,6 +6081,27 @@ addEventListener('fetch', event => {});`
 			expect(std.err).toMatchInlineSnapshot(`""`);
 		});
 
+		it("should deploy successfully if the /subdomain POST request is flaky", async () => {
+			writeWranglerConfig();
+			writeWorkerSource();
+			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: false });
+			mockSubDomainRequest();
+			mockUpdateWorkerSubdomain({ enabled: true, flakeCount: 1 });
+
+			await runWrangler("deploy ./index");
+
+			expect(std.out).toMatchInlineSnapshot(`
+				"Total Upload: xx KiB / gzip: xx KiB
+				Worker Startup Time: 100 ms
+				Uploaded test-name (TIMINGS)
+				Deployed test-name triggers (TIMINGS)
+				  https://test-name.test-sub-domain.workers.dev
+				Current Version ID: Galaxy-Class"
+			`);
+			expect(std.err).toMatchInlineSnapshot(`""`);
+		});
+
 		it("should deploy to the workers.dev domain if workers_dev is `true`", async () => {
 			writeWranglerConfig({
 				workers_dev: true,

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

@@ -79,18 +79,21 @@ export function mockUpdateWorkerSubdomain({
 	env,
 	legacyEnv = false,
 	expectedScriptName = "test-name",
+	flakeCount = 0,
 }: {
 	enabled: boolean;
 	previews_enabled?: boolean;
 	env?: string | undefined;
 	legacyEnv?: boolean | undefined;
 	expectedScriptName?: string;
+	flakeCount?: number; // The first `flakeCount` requests will fail with a 500 error
 }) {
 	const url =
 		env && !legacyEnv
 			? `*/accounts/:accountId/workers/services/:scriptName/environments/:envName/subdomain`
 			: `*/accounts/:accountId/workers/scripts/:scriptName/subdomain`;
-	msw.use(
+
+	const handlers = [
 		http.post(
 			url,
 			async ({ request, params }) => {
@@ -108,6 +111,23 @@ export function mockUpdateWorkerSubdomain({
 				);
 			},
 			{ once: true }
-		)
-	);
+		),
+	];
+	while (flakeCount > 0) {
+		flakeCount--;
+		handlers.unshift(
+			http.post(
+				url,
+				() =>
+					HttpResponse.json(
+						createFetchResult(null, false, [
+							{ code: 10013, message: "An unknown error has occurred." },
+						]),
+						{ status: 500 }
+					),
+				{ once: true }
+			)
+		);
+	}
+	msw.use(...handlers);
 }

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

@@ -1,10 +1,17 @@
+import { logger } from "../../logger";
 import { APIError } from "../../parse";
 import { retryOnAPIFailure } from "../../utils/retry";
 import { mockConsoleMethods } from "../helpers/mock-console";
 
 describe("retryOnAPIFailure", () => {
 	const std = mockConsoleMethods();
 
+	beforeEach(() => {
+		const level = logger.loggerLevel;
+		logger.loggerLevel = "debug";
+		return () => (logger.loggerLevel = level);
+	});
+
 	it("should retry 5xx errors and succeed if the 3rd try succeeds", async () => {
 		let attempts = 0;
 
@@ -15,15 +22,13 @@ describe("retryOnAPIFailure", () => {
 			}
 		});
 		expect(attempts).toBe(3);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "Retrying API call after error...
-			Retrying API call after error...",
-			  "out": "",
-			  "warn": "",
-			}
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
+			Array [
+			  "Retrying API call after error...",
+			  "APIError: 500 error",
+			  "Retrying API call after error...",
+			  "APIError: 500 error",
+			]
 		`);
 	});
 
@@ -37,16 +42,15 @@ describe("retryOnAPIFailure", () => {
 			})
 		).rejects.toMatchInlineSnapshot(`[APIError: 500 error]`);
 		expect(attempts).toBe(3);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "Retrying API call after error...
-			Retrying API call after error...
-			Retrying API call after error...",
-			  "out": "",
-			  "warn": "",
-			}
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
+			Array [
+			  "Retrying API call after error...",
+			  "APIError: 500 error",
+			  "Retrying API call after error...",
+			  "APIError: 500 error",
+			  "Retrying API call after error...",
+			  "APIError: 500 error",
+			]
 		`);
 	});
 
@@ -60,15 +64,7 @@ describe("retryOnAPIFailure", () => {
 			})
 		).rejects.toMatchInlineSnapshot(`[APIError: 401 error]`);
 		expect(attempts).toBe(1);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "",
-			  "out": "",
-			  "warn": "",
-			}
-		`);
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`Array []`);
 	});
 
 	it("should retry TypeError", async () => {
@@ -81,16 +77,12 @@ describe("retryOnAPIFailure", () => {
 			})
 		).rejects.toMatchInlineSnapshot(`[TypeError: type error]`);
 		expect(attempts).toBe(3);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "Retrying API call after error...
-			Retrying API call after error...
-			Retrying API call after error...",
-			  "out": "",
-			  "warn": "",
-			}
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
+			Array [
+			  "Retrying API call after error...",
+			  "Retrying API call after error...",
+			  "Retrying API call after error...",
+			]
 		`);
 	});
 
@@ -104,15 +96,7 @@ describe("retryOnAPIFailure", () => {
 			})
 		).rejects.toMatchInlineSnapshot(`[Error: some error]`);
 		expect(attempts).toBe(1);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "",
-			  "out": "",
-			  "warn": "",
-			}
-		`);
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`Array []`);
 	});
 
 	it("should retry custom APIError implementation with non-5xx error", async () => {
@@ -134,16 +118,21 @@ describe("retryOnAPIFailure", () => {
 		).rejects.toMatchInlineSnapshot(`[CustomAPIError: 401 error]`);
 		expect(attempts).toBe(3);
 		expect(checkedCustomIsRetryable).toBe(true);
-		expect(std).toMatchInlineSnapshot(`
-			Object {
-			  "debug": "",
-			  "err": "",
-			  "info": "Retrying API call after error...
-			Retrying API call after error...
-			Retrying API call after error...",
-			  "out": "",
-			  "warn": "",
-			}
+		expect(getRetryAndErrorLogs(std.debug)).toMatchInlineSnapshot(`
+			Array [
+			  "Retrying API call after error...",
+			  "CustomAPIError: 401 error",
+			  "Retrying API call after error...",
+			  "CustomAPIError: 401 error",
+			  "Retrying API call after error...",
+			  "CustomAPIError: 401 error",
+			]
 		`);
 	});
 });
+
+function getRetryAndErrorLogs(debugOutput: string): string[] {
+	return debugOutput
+		.split("\n")
+		.filter((line) => line.includes("Retrying") || line.includes("APIError"));
+}

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

@@ -250,7 +250,7 @@ describe("versions upload", () => {
 		writeWorkerSource();
 		setIsTTY(false);
 
-		const result = runWrangler("versions upload");
+		const result = runWrangler("versions upload", { WRANGLER_LOG: "debug" });
 
 		await expect(result).resolves.toBeUndefined();
 
@@ -265,7 +265,7 @@ describe("versions upload", () => {
 			Worker Version ID: 51e4886e-2db7-4900-8d38-fbfecfeab993"
 		`);
 
-		expect(std.info).toContain("Retrying API call after error...");
+		expect(std.debug).toContain("Retrying API call after error...");
 	});
 
 	test("correctly detects python workers", async () => {

packages/wrangler/src/cfetch/index.ts

@@ -31,18 +31,13 @@ export async function fetchResult<ResponseType>(
 	abortSignal?: AbortSignal,
 	apiToken?: ApiCredentials
 ): Promise<ResponseType> {
-	const json = await fetchInternal<FetchResult<ResponseType>>(
-		complianceConfig,
-		resource,
-		init,
-		queryParams,
-		abortSignal,
-		apiToken
-	);
+	const { response: json, status } = await fetchInternal<
+		FetchResult<ResponseType>
+	>(complianceConfig, resource, init, queryParams, abortSignal, apiToken);
 	if (json.success) {
 		return json.result;
 	} else {
-		throwFetchError(resource, json);
+		throwFetchError(resource, json, status);
 	}
 }
 
@@ -54,7 +49,7 @@ export async function fetchGraphqlResult<ResponseType>(
 	init: RequestInit = {},
 	abortSignal?: AbortSignal
 ): Promise<ResponseType> {
-	const json = await fetchInternal<ResponseType>(
+	const { response: json } = await fetchInternal<ResponseType>(
 		complianceConfig,
 		"/graphql",
 		{ ...init, method: "POST" }, //Cloudflare API v4 doesn't allow GETs to /graphql
@@ -87,12 +82,9 @@ export async function fetchListResult<ResponseType>(
 			queryParams = new URLSearchParams(queryParams);
 			queryParams.set("cursor", cursor);
 		}
-		const json = await fetchInternal<FetchResult<ResponseType[]>>(
-			complianceConfig,
-			resource,
-			init,
-			queryParams
-		);
+		const { response: json, status } = await fetchInternal<
+			FetchResult<ResponseType[]>
+		>(complianceConfig, resource, init, queryParams);
 		if (json.success) {
 			results.push(...json.result);
 			if (hasCursor(json.result_info)) {
@@ -101,7 +93,7 @@ export async function fetchListResult<ResponseType>(
 				getMoreResults = false;
 			}
 		} else {
-			throwFetchError(resource, json);
+			throwFetchError(resource, json, status);
 		}
 	}
 	return results;
@@ -127,12 +119,9 @@ export async function fetchPagedListResult<ResponseType>(
 		queryParams = new URLSearchParams(queryParams);
 		queryParams.set("page", String(page));
 
-		const json = await fetchInternal<FetchResult<ResponseType[]>>(
-			complianceConfig,
-			resource,
-			init,
-			queryParams
-		);
+		const { response: json, status } = await fetchInternal<
+			FetchResult<ResponseType[]>
+		>(complianceConfig, resource, init, queryParams);
 		if (json.success) {
 			results.push(...json.result);
 			if (hasMorePages(json.result_info)) {
@@ -141,7 +130,7 @@ export async function fetchPagedListResult<ResponseType>(
 				getMoreResults = false;
 			}
 		} else {
-			throwFetchError(resource, json);
+			throwFetchError(resource, json, status);
 		}
 	}
 	return results;
@@ -171,7 +160,8 @@ export function hasMorePages(
 
 function throwFetchError(
 	resource: string,
-	response: FetchResult<unknown>
+	response: FetchResult<unknown>,
+	status: number
 ): never {
 	// This is an error from within an MSW handler
 	if (typeof vitest !== "undefined" && !("errors" in response)) {
@@ -187,6 +177,7 @@ function throwFetchError(
 			...response.errors.map((err) => ({ text: renderError(err) })),
 			...(response.messages?.map((text) => ({ text })) ?? []),
 		],
+		status,
 	});
 	// add the first error code directly to this error
 	// so consumers can use it for specific behaviour

packages/wrangler/src/cfetch/internal.ts

@@ -158,7 +158,7 @@ export async function fetchInternal<ResponseType>(
 	queryParams?: URLSearchParams,
 	abortSignal?: AbortSignal,
 	apiToken?: ApiCredentials
-): Promise<ResponseType> {
+): Promise<{ response: ResponseType; status: number }> {
 	const method = init.method ?? "GET";
 	const response = await performApiFetch(
 		complianceConfig,
@@ -181,12 +181,20 @@ export async function fetchInternal<ResponseType>(
 	// HTTP 204 and HTTP 205 responses do not return a body. We need to special-case this
 	// as otherwise parseJSON will throw an error back to the user.
 	if (!jsonText && (response.status === 204 || response.status === 205)) {
-		const emptyBody = `{"result": {}, "success": true, "errors": [], "messages": []}`;
-		return parseJSON(emptyBody) as ResponseType;
+		return {
+			response: {
+				result: {},
+				success: true,
+				errors: [],
+				messages: [],
+			} as ResponseType,
+			status: response.status,
+		};
 	}
 
 	try {
-		return parseJSON(jsonText) as ResponseType;
+		const json = parseJSON(jsonText) as ResponseType;
+		return { response: json, status: response.status };
 	} catch {
 		throw new APIError({
 			text: "Received a malformed response from the API",