Retry gateway errors on idempotent GraphQL requests

.changeset/retry-gateway-errors.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Retry gateway errors (502/503/504) on GraphQL queries instead of failing immediately

packages/cli-kit/src/private/node/api.test.ts

@@ -696,6 +696,161 @@ describe('retryAwareRequest', () => {
     expect(recordRetry).toHaveBeenCalledTimes(1)
     expect(recordRetry).toHaveBeenCalledWith('https://themes.example.com/auth', 'http-retry-1:can-retry:')
   })
+
+  test('retries a gateway error and resolves once the upstream recovers', async () => {
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError({status: 502, headers: new Headers()}, {query: ''})
+      })
+      .mockImplementation(() => {
+        return Promise.resolve({status: 200, data: {hello: 'world!'}, headers: new Headers()})
+      })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        requestIsIdempotent: true,
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: vi.fn((fn) => fn())},
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {hello: 'world!'},
+    })
+    expect(mockRequestFn).toHaveBeenCalledTimes(2)
+  })
+
+  test('gives up on a persistent gateway error well before the general retry limit', async () => {
+    const mockRequestFn = vi.fn().mockImplementation(() => {
+      throw new ClientError({status: 503, headers: new Headers()}, {query: ''})
+    })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        requestIsIdempotent: true,
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: vi.fn((fn) => fn())},
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).rejects.toThrowError(ClientError)
+    // The initial attempt plus the 3 gateway retries, rather than the default limit of 10.
+    expect(mockRequestFn).toHaveBeenCalledTimes(4)
+  })
+
+  test('uses the retry-after header for a gateway error', async () => {
+    const gatewayError = new ClientError({status: 503, headers: new Headers({'retry-after': '250'})}, {query: ''})
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw gatewayError
+      })
+      .mockResolvedValue({status: 200, data: {hello: 'world!'}, headers: new Headers()})
+    const scheduleDelay = vi.fn((fn) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        requestIsIdempotent: true,
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {scheduleDelay},
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {hello: 'world!'},
+    })
+    expect(scheduleDelay).toHaveBeenCalledWith(expect.anything(), 250)
+  })
+
+  test('does not retry a gateway error unless the request is known to be idempotent', async () => {
+    const mockRequestFn = vi.fn().mockImplementation(() => {
+      throw new ClientError({status: 504, headers: new Headers()}, {query: ''})
+    })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: vi.fn((fn) => fn())},
+    )
+
+    await expect(result).rejects.toThrowError(ClientError)
+    expect(mockRequestFn).toHaveBeenCalledTimes(1)
+  })
+
+  test('does not network-retry a mutation because its payload text contains a transient keyword', async () => {
+    // A ClientError's message embeds JSON.stringify({response, request}), so the mutation's own
+    // variables land in the string isTransientNetworkError searches. This asset value contains
+    // "setTimeout", which used to match 'timeout' and retry a non-idempotent request.
+    const mockRequestFn = vi.fn().mockImplementation(() => {
+      throw new ClientError(
+        {status: 502, headers: new Headers()},
+        {
+          query: 'mutation ThemeFilesUpsert($files: [FileInput!]!) { themeFilesUpsert(files: $files) { id } }',
+          variables: {files: [{filename: 'assets/app.js', body: {value: 'setTimeout(() => init(), 300)'}}]},
+        },
+      )
+    })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: vi.fn((fn) => fn())},
+    )
+
+    await expect(result).rejects.toThrowError(ClientError)
+    expect(mockRequestFn).toHaveBeenCalledTimes(1)
+  })
+
+  test('does not retry an HTTP 500', async () => {
+    const mockRequestFn = vi.fn().mockImplementation(() => {
+      throw new ClientError({status: 500, headers: new Headers()}, {query: ''})
+    })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        requestIsIdempotent: true,
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: vi.fn((fn) => fn())},
+    )
+
+    await expect(result).rejects.toThrowError(ClientError)
+    expect(mockRequestFn).toHaveBeenCalledTimes(1)
+  })
 })
 
 describe('isTransientNetworkError', () => {

packages/cli-kit/src/private/node/api.ts

@@ -1,4 +1,5 @@
 import {sanitizedHeadersOutput} from './api/headers.js'
+import {isGatewayErrorStatus} from './api/gateway-status.js'
 import {sanitizeURL} from './api/urls.js'
 import {hasRateLimitCode} from './analytics/graphql-error-codes.js'
 import {sleepWithBackoffUntil} from './sleep-with-backoff.js'
@@ -17,6 +18,13 @@ export const allAPIs: API[] = ['admin', 'storefront-renderer', 'partners', 'busi
 const DEFAULT_RETRY_DELAY_MS = 1000
 const DEFAULT_RETRY_LIMIT = 10
 
+/**
+ * Gateway errors on idempotent requests get a much smaller retry budget than rate limits. A 429
+ * response tells us exactly how long to wait, whereas a 502/503/504 means an upstream is already
+ * struggling and should not be hit ten more times on its way down.
+ */
+const GATEWAY_ERROR_RETRY_LIMIT = 3
+
 export type NetworkRetryBehaviour =
   | {
       useNetworkLevelRetry: true
@@ -31,6 +39,7 @@ export type NetworkRetryBehaviour =
 type RequestOptions<T> = {
   request: () => Promise<T>
   url: string
+  requestIsIdempotent?: boolean
 } & NetworkRetryBehaviour
 
 const interestingResponseHeaders = new Set([
@@ -46,6 +55,14 @@ function responseHeaderIsInteresting(header: string): boolean {
   return interestingResponseHeaders.has(header)
 }
 
+function retryDelayMsFromHeaders(responseHeaders: Record<string, string>): number | undefined {
+  const retryAfter = responseHeaders['retry-after']
+  if (!retryAfter) return undefined
+
+  const delayMs = Number.parseInt(retryAfter, 10)
+  return Number.isNaN(delayMs) ? undefined : delayMs
+}
+
 interface CommonResponse {
   duration: number
   sanitizedHeaders: string
@@ -54,10 +71,20 @@ interface CommonResponse {
 }
 
 type OkResponse<T> = CommonResponse & {status: 'ok'; response: T}
+
+/**
+ * `'client-error'` names the graphql-request `ClientError` wrapper, not an HTTP 4xx: it is the
+ * terminal bucket for every status that is not classified as retryable or unauthorized below.
+ */
 type ClientErrorResponse = CommonResponse & {status: 'client-error'; clientError: ClientError}
 type UnknownErrorResponse = CommonResponse & {status: 'unknown-error'; error: unknown}
+
+/** Why a response was classified as retryable, so each cause can carry its own retry budget. */
+type RetryReason = 'rate-limit' | 'gateway-error'
+
 type CanRetryErrorResponse = CommonResponse & {
   status: 'can-retry'
+  retryReason: RetryReason
   clientError: ClientError
   delayMs: number | undefined
 }
@@ -152,7 +179,15 @@ async function runRequestWithNetworkLevelRetry<T extends {headers: Headers; stat
       return await requestOptions.request()
     } catch (err) {
       lastSeenError = err
-      if (!isTransientNetworkError(err)) {
+      // A `ClientError` means the request reached the API and came back with a response, so it is
+      // never a connection-level failure. It must not be matched by message text: a ClientError's
+      // message embeds `JSON.stringify({response, request})`, so the request's own query and
+      // variables are part of the string `isTransientNetworkError` searches. A theme asset
+      // containing `setTimeout` matches `'timeout'` and used to make a 502 on the
+      // `ThemeFilesUpsert` mutation retry here, non-idempotently, purely because of the file's
+      // contents. Retryable statuses are classified deliberately in `makeVerboseRequest` instead,
+      // where the idempotency of the request is known.
+      if (err instanceof ClientError || !isTransientNetworkError(err)) {
         throw err
       }
 
@@ -195,22 +230,15 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
       const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders)
 
       if (isThrottled(err)) {
-        let delayMs: number | undefined
-
-        try {
-          delayMs = responseHeaders['retry-after'] ? Number.parseInt(responseHeaders['retry-after'], 10) : undefined
-          // eslint-disable-next-line no-catch-all/no-catch-all
-        } catch {
-          // ignore errors in extracting retry-after header
-        }
         return {
           status: 'can-retry',
+          retryReason: 'rate-limit',
           clientError: err,
           duration,
           sanitizedHeaders,
           sanitizedUrl,
           requestId: responseHeaders['x-request-id'],
-          delayMs,
+          delayMs: retryDelayMsFromHeaders(responseHeaders),
         }
       } else if (err.response.status === 401) {
         return {
@@ -222,6 +250,17 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
           requestId: responseHeaders['x-request-id'],
           delayMs: 500,
         }
+      } else if (requestOptions.requestIsIdempotent && isGatewayErrorStatus(err.response.status)) {
+        return {
+          status: 'can-retry',
+          retryReason: 'gateway-error',
+          clientError: err,
+          duration,
+          sanitizedHeaders,
+          sanitizedUrl,
+          requestId: responseHeaders['x-request-id'],
+          delayMs: retryDelayMsFromHeaders(responseHeaders),
+        }
       }
 
       return {
@@ -340,6 +379,7 @@ export async function retryAwareRequest<T extends {headers: Headers; status: num
   },
 ): Promise<T> {
   let retriesUsed = 0
+  let gatewayRetriesUsed = 0
   const limitRetriesTo = retryOptions.limitRetriesTo ?? DEFAULT_RETRY_LIMIT
 
   let result = await makeVerboseRequest(requestOptions)
@@ -371,15 +411,21 @@ ${result.sanitizedHeaders}
       throw result.clientError
     }
 
-    if (limitRetriesTo <= retriesUsed) {
-      outputDebug(`${limitRetriesTo} retries exhausted for request to ${result.sanitizedUrl}`)
+    const gatewayBudgetExhausted =
+      result.retryReason === 'gateway-error' && GATEWAY_ERROR_RETRY_LIMIT <= gatewayRetriesUsed
+    if (limitRetriesTo <= retriesUsed || gatewayBudgetExhausted) {
+      const exhaustedLimit = gatewayBudgetExhausted ? GATEWAY_ERROR_RETRY_LIMIT : limitRetriesTo
+      outputDebug(`${exhaustedLimit} retries exhausted for request to ${result.sanitizedUrl}`)
       if (errorHandler) {
         throw errorHandler(result.clientError, result.requestId)
       } else {
         throw result.clientError
       }
     }
     retriesUsed += 1
+    if (result.retryReason === 'gateway-error') {
+      gatewayRetriesUsed += 1
+    }
 
     // Record command retries
     if (requestOptions.recordCommandRetries) {

packages/cli-kit/src/private/node/api/gateway-status.ts

@@ -0,0 +1,29 @@
+/**
+ * Pure helpers for classifying HTTP statuses that come from the gateway in front of an API rather
+ * than from the API itself.
+ *
+ * This module is intentionally dependency-free so that both the request layer (`../api.ts`) and the
+ * crash-report suppression logic in `../../public/node/error.ts` can share it. `error.ts` cannot
+ * import `../api.ts` directly — that would pull `graphql-request` into the module graph of every
+ * command, and `api.ts → headers.ts → error.ts` is already a cycle — so the shared status logic
+ * lives here, where it imports nothing from cli-kit.
+ */
+
+/**
+ * Statuses commonly emitted by a proxy or load balancer that could not produce a usable response
+ * from an upstream service: 502 Bad Gateway, 503 Service Unavailable, 504 Gateway Timeout.
+ *
+ * 500 is deliberately excluded: it means the API itself answered and failed, so it keeps its
+ * existing non-retryable, reportable behaviour.
+ */
+const GATEWAY_ERROR_STATUSES = new Set([502, 503, 504])
+
+/**
+ * Whether an HTTP status indicates a gateway-level failure in front of the API.
+ *
+ * @param status - The HTTP status of the response, if known.
+ * @returns True when the status is 502, 503 or 504.
+ */
+export function isGatewayErrorStatus(status: number | undefined): boolean {
+  return status !== undefined && GATEWAY_ERROR_STATUSES.has(status)
+}