Fix regression in retryOnAPIFailure preventing any requests from being retried (#7895)

* Fix regression in retryOnAPIFailure preventing any requests from being retried

Also fixes a regression in pipelines that prevented 401 errors from
being retried when waiting for an API token to become active.

* fixup! Retry TypeError

* fixup! Don't retry other errors

Author: jahands

.changeset/neat-needles-brush.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+Fix regression in retryOnAPIFailure preventing any requests from being retried
+
+Also fixes a regression in pipelines that prevented 401 errors from being retried when waiting for an API token to become active.

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

@@ -0,0 +1,85 @@
+import { APIError } from "../../parse";
+import { retryOnAPIFailure } from "../../utils/retry";
+
+describe("retryOnAPIFailure", () => {
+	it("should retry 5xx errors and succeed if the 3rd try succeeds", async () => {
+		let attempts = 0;
+
+		await retryOnAPIFailure(() => {
+			attempts++;
+			if (attempts < 3) {
+				throw new APIError({ status: 500, text: "500 error" });
+			}
+		});
+		expect(attempts).toBe(3);
+	});
+
+	it("should throw 5xx error after all retries fail", async () => {
+		let attempts = 0;
+
+		await expect(() =>
+			retryOnAPIFailure(() => {
+				attempts++;
+				throw new APIError({ status: 500, text: "500 error" });
+			})
+		).rejects.toMatchInlineSnapshot(`[APIError: 500 error]`);
+		expect(attempts).toBe(3);
+	});
+
+	it("should not retry non-5xx errors", async () => {
+		let attempts = 0;
+
+		await expect(() =>
+			retryOnAPIFailure(() => {
+				attempts++;
+				throw new APIError({ status: 401, text: "401 error" });
+			})
+		).rejects.toMatchInlineSnapshot(`[APIError: 401 error]`);
+		expect(attempts).toBe(1);
+	});
+
+	it("should retry TypeError", async () => {
+		let attempts = 0;
+
+		await expect(() =>
+			retryOnAPIFailure(() => {
+				attempts++;
+				throw new TypeError("type error");
+			})
+		).rejects.toMatchInlineSnapshot(`[TypeError: type error]`);
+		expect(attempts).toBe(3);
+	});
+
+	it("should not retry other errors", async () => {
+		let attempts = 0;
+
+		await expect(() =>
+			retryOnAPIFailure(() => {
+				attempts++;
+				throw new Error("some error");
+			})
+		).rejects.toMatchInlineSnapshot(`[Error: some error]`);
+		expect(attempts).toBe(1);
+	});
+
+	it("should retry custom APIError implementation with non-5xx error", async () => {
+		let checkedCustomIsRetryable = false;
+		class CustomAPIError extends APIError {
+			isRetryable(): boolean {
+				checkedCustomIsRetryable = true;
+				return true;
+			}
+		}
+
+		let attempts = 0;
+
+		await expect(() =>
+			retryOnAPIFailure(() => {
+				attempts++;
+				throw new CustomAPIError({ status: 401, text: "401 error" });
+			})
+		).rejects.toMatchInlineSnapshot(`[CustomAPIError: 401 error]`);
+		expect(attempts).toBe(3);
+		expect(checkedCustomIsRetryable).toBe(true);
+	});
+});

packages/wrangler/src/pipelines/index.ts

@@ -65,11 +65,21 @@ async function authorizeR2Bucket(
 	!__testSkipDelaysFlag &&
 		(await retryOnAPIFailure(
 			async () => {
-				await r2.send(
-					new HeadBucketCommand({
-						Bucket: bucketName,
-					})
-				);
+				try {
+					await r2.send(
+						new HeadBucketCommand({
+							Bucket: bucketName,
+						})
+					);
+				} catch (err) {
+					if (err instanceof Error && err.name === "401") {
+						throw new AuthAPIError({
+							status: 401,
+							text: "R2 HeadBucket request failed with status: 401",
+						});
+					}
+					throw err;
+				}
 			},
 			1000,
 			10
@@ -78,6 +88,17 @@ async function authorizeR2Bucket(
 	return serviceToken;
 }
 
+/**
+ * AuthAPIError always retries errors so that
+ * we always retry auth errors while waiting for an
+ * API token to propegate and start working.
+ */
+class AuthAPIError extends APIError {
+	override isRetryable(): boolean {
+		return true;
+	}
+}
+
 function getAccountR2Endpoint(accountId: string) {
 	return `https://${accountId}.r2.cloudflarestorage.com`;
 }

packages/wrangler/src/utils/retry.ts

@@ -21,10 +21,11 @@ export async function retryOnAPIFailure<T>(
 	try {
 		return await action();
 	} catch (err) {
-		if (
-			(err instanceof APIError && !err.isRetryable()) ||
-			!(err instanceof TypeError)
-		) {
+		if (err instanceof APIError) {
+			if (!err.isRetryable()) {
+				throw err;
+			}
+		} else if (!(err instanceof TypeError)) {
 			throw err;
 		}