diff --git a/.changeset/tangy-donuts-jog.md b/.changeset/tangy-donuts-jog.md new file mode 100644 index 00000000000..30688063b4d --- /dev/null +++ b/.changeset/tangy-donuts-jog.md @@ -0,0 +1,5 @@ +--- +"wrangler": patch +--- + +retry subdomain requests to be more resilient to flakes diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts index bad2b2276f1..3ca20315260 100644 --- a/packages/wrangler/src/__tests__/deploy.test.ts +++ b/packages/wrangler/src/__tests__/deploy.test.ts @@ -6048,6 +6048,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, diff --git a/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts b/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts index 3185346ba6d..a0d745f29d7 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts +++ b/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); } diff --git a/packages/wrangler/src/__tests__/utils/retry.test.ts b/packages/wrangler/src/__tests__/utils/retry.test.ts index 5881e8010a7..bfb63cffad0 100644 --- a/packages/wrangler/src/__tests__/utils/retry.test.ts +++ b/packages/wrangler/src/__tests__/utils/retry.test.ts @@ -1,3 +1,4 @@ +import { logger } from "../../logger"; import { APIError } from "../../parse"; import { retryOnAPIFailure } from "../../utils/retry"; import { mockConsoleMethods } from "../helpers/mock-console"; @@ -5,6 +6,12 @@ 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")); +} diff --git a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts index f622948afc1..85108c619a2 100644 --- a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts +++ b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts @@ -184,7 +184,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(); @@ -198,7 +198,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 () => { diff --git a/packages/wrangler/src/cfetch/index.ts b/packages/wrangler/src/cfetch/index.ts index a6261bb473e..96ae14c39d0 100644 --- a/packages/wrangler/src/cfetch/index.ts +++ b/packages/wrangler/src/cfetch/index.ts @@ -26,16 +26,13 @@ export async function fetchResult( queryParams?: URLSearchParams, abortSignal?: AbortSignal ): Promise { - const json = await fetchInternal>( - resource, - init, - queryParams, - abortSignal - ); + const { response: json, status } = await fetchInternal< + FetchResult + >(resource, init, queryParams, abortSignal); if (json.success) { return json.result; } else { - throwFetchError(resource, json); + throwFetchError(resource, json, status); } } @@ -46,7 +43,7 @@ export async function fetchGraphqlResult( init: RequestInit = {}, abortSignal?: AbortSignal ): Promise { - const json = await fetchInternal( + const { response: json } = await fetchInternal( "/graphql", { ...init, method: "POST" }, //Cloudflare API v4 doesn't allow GETs to /graphql undefined, @@ -77,11 +74,9 @@ export async function fetchListResult( queryParams = new URLSearchParams(queryParams); queryParams.set("cursor", cursor); } - const json = await fetchInternal>( - resource, - init, - queryParams - ); + const { response: json, status } = await fetchInternal< + FetchResult + >(resource, init, queryParams); if (json.success) { results.push(...json.result); if (hasCursor(json.result_info)) { @@ -90,7 +85,7 @@ export async function fetchListResult( getMoreResults = false; } } else { - throwFetchError(resource, json); + throwFetchError(resource, json, status); } } return results; @@ -115,11 +110,9 @@ export async function fetchPagedListResult( queryParams = new URLSearchParams(queryParams); queryParams.set("page", String(page)); - const json = await fetchInternal>( - resource, - init, - queryParams - ); + const { response: json, status } = await fetchInternal< + FetchResult + >(resource, init, queryParams); if (json.success) { results.push(...json.result); if (hasMorePages(json.result_info)) { @@ -128,7 +121,7 @@ export async function fetchPagedListResult( getMoreResults = false; } } else { - throwFetchError(resource, json); + throwFetchError(resource, json, status); } } return results; @@ -158,7 +151,8 @@ export function hasMorePages( function throwFetchError( resource: string, - response: FetchResult + response: FetchResult, + status: number ): never { // This is an error from within an MSW handler if (typeof vitest !== "undefined" && !("errors" in response)) { @@ -174,6 +168,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 diff --git a/packages/wrangler/src/cfetch/internal.ts b/packages/wrangler/src/cfetch/internal.ts index dc8c396f5ce..32f3c3a6ed5 100644 --- a/packages/wrangler/src/cfetch/internal.ts +++ b/packages/wrangler/src/cfetch/internal.ts @@ -72,7 +72,7 @@ export async function fetchInternal( init: RequestInit = {}, queryParams?: URLSearchParams, abortSignal?: AbortSignal -): Promise { +): Promise<{ response: ResponseType; status: number }> { const method = init.method ?? "GET"; const response = await performApiFetch( resource, @@ -95,13 +95,21 @@ export async function fetchInternal( // 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; - } catch (err) { + const json = parseJSON(jsonText) as ResponseType; + return { response: json, status: response.status }; + } catch { throw new APIError({ text: "Received a malformed response from the API", notes: [ diff --git a/packages/wrangler/src/utils/retry.ts b/packages/wrangler/src/utils/retry.ts index aa3ad65a579..9ace9c5c60e 100644 --- a/packages/wrangler/src/utils/retry.ts +++ b/packages/wrangler/src/utils/retry.ts @@ -29,7 +29,7 @@ export async function retryOnAPIFailure( throw err; } - logger.info(chalk.dim(`Retrying API call after error...`)); + logger.debug(chalk.dim(`Retrying API call after error...`)); logger.debug(err); if (attempts <= 1) {