Retry transient server errors for theme API reads

.changeset/theme-retry-transient-server-errors.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Theme commands now retry Admin API requests that fail with a temporary server error (`INTERNAL_SERVER_ERROR`, 502, 503 or 504), up to 3 times with backoff. If the error persists, they show a friendly "try again" message with the request ID instead of the raw API response. Mutations are never retried.

packages/cli-kit/src/private/node/analytics/graphql-error-codes.test.ts

@@ -1,4 +1,10 @@
-import {graphQLErrorCodes, hasRateLimitCode, isPermissionCode, isRateLimitCode} from './graphql-error-codes.js'
+import {
+  graphQLErrorCodes,
+  hasRateLimitCode,
+  hasTransientServerCode,
+  isPermissionCode,
+  isRateLimitCode,
+} from './graphql-error-codes.js'
 import {describe, expect, test} from 'vitest'
 
 describe('graphQLErrorCodes', () => {
@@ -38,4 +44,10 @@ describe('code predicates', () => {
     expect(hasRateLimitCode([{message: 'a'}, {extensions: {code: '429'}}])).toBe(true)
     expect(hasRateLimitCode([{extensions: {code: 'OTHER'}}])).toBe(false)
   })
+
+  test('hasTransientServerCode scans all errors, not just the first', () => {
+    expect(hasTransientServerCode([{message: 'a'}, {extensions: {code: 'INTERNAL_SERVER_ERROR'}}])).toBe(true)
+    expect(hasTransientServerCode([{extensions: {code: 'ACCESS_DENIED'}}])).toBe(false)
+    expect(hasTransientServerCode('Internal error')).toBe(false)
+  })
 })

packages/cli-kit/src/private/node/analytics/graphql-error-codes.ts

@@ -18,6 +18,13 @@ const RATE_LIMIT_CODES = new Set(['THROTTLED', '429'])
  */
 const PERMISSION_CODES = new Set(['ACCESS_DENIED', 'access_denied'])
 
+/**
+ * GraphQL `extensions.code` values we treat as a transient server-side failure: the request was
+ * valid, but the server failed to process it (for example, a database briefly running out of
+ * connections). Shopify APIs return these on an HTTP 200, so the status alone doesn't reveal them.
+ */
+const TRANSIENT_SERVER_CODES = new Set(['INTERNAL_SERVER_ERROR'])
+
 /**
  * Collects every routing-relevant code from a GraphQL `errors` value.
  *
@@ -67,6 +74,13 @@ export function isPermissionCode(code: string | undefined): boolean {
   return code !== undefined && PERMISSION_CODES.has(code)
 }
 
+/**
+ * Whether a single code is a transient server-side failure signal (`INTERNAL_SERVER_ERROR`).
+ */
+function isTransientServerCode(code: string | undefined): boolean {
+  return code !== undefined && TRANSIENT_SERVER_CODES.has(code)
+}
+
 /**
  * Whether any GraphQL error carries a rate-limit code. Convenience for suppression logic that only
  * needs a boolean over the raw `errors` value.
@@ -77,3 +91,13 @@ export function isPermissionCode(code: string | undefined): boolean {
 export function hasRateLimitCode(errors: unknown): boolean {
   return graphQLErrorCodes(errors).some(isRateLimitCode)
 }
+
+/**
+ * Whether any GraphQL error carries a transient server-side failure code.
+ *
+ * @param errors - The `errors` value from a GraphQL response.
+ * @returns True when a transient server code is present in any error.
+ */
+export function hasTransientServerCode(errors: unknown): boolean {
+  return graphQLErrorCodes(errors).some(isTransientServerCode)
+}

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

@@ -1,4 +1,4 @@
-import {retryAwareRequest, isNetworkError, isTransientNetworkError} from './api.js'
+import {retryAwareRequest, isNetworkError, isTransientNetworkError, isTransientServerError} from './api.js'
 import {recordRetry} from '../../public/node/analytics.js'
 import {ClientError} from 'graphql-request'
 import {describe, test, vi, expect, beforeEach, afterEach} from 'vitest'
@@ -814,3 +814,108 @@ describe('isNetworkError', () => {
     expect(isNetworkError({message: 'certificate error'})).toBe(false)
   })
 })
+
+describe('retryAwareRequest with transient server errors', () => {
+  const internalServerErrorResponse = {
+    status: 200,
+    errors: [
+      {
+        message: 'Internal error. Looks like something went wrong on our end.',
+        extensions: {code: 'INTERNAL_SERVER_ERROR', requestId: 'request-id'},
+      } as any,
+    ],
+    headers: new Headers(),
+  }
+  const okResponse = {status: 200, data: {hello: 'world!'}, headers: new Headers()}
+
+  test('retries INTERNAL_SERVER_ERROR and records each attempt when enabled', async () => {
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(internalServerErrorResponse, {query: ''})
+      })
+      .mockImplementationOnce(() => {
+        throw new ClientError(internalServerErrorResponse, {query: ''})
+      })
+      .mockImplementationOnce(() => Promise.resolve(okResponse))
+    const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
+
+    const result = await retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com',
+        useNetworkLevelRetry: false,
+        retryTransientServerErrors: true,
+        recordCommandRetries: true,
+      },
+      undefined,
+      {scheduleDelay: mockScheduleDelayFn},
+    )
+
+    expect(result).toEqual(okResponse)
+    expect(mockRequestFn).toHaveBeenCalledTimes(3)
+    expect(recordRetry).toHaveBeenCalledWith('https://example.com', 'http-retry-1:transient-server-error:')
+    expect(recordRetry).toHaveBeenCalledWith('https://example.com', 'http-retry-2:transient-server-error:')
+  })
+
+  test('retries gateway errors when retryTransientServerErrors is enabled', async () => {
+    const gatewayErrorResponse = {status: 503, headers: new Headers()}
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(gatewayErrorResponse, {query: ''})
+      })
+      .mockImplementationOnce(() => Promise.resolve(okResponse))
+    const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
+
+    const result = await retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com',
+        useNetworkLevelRetry: false,
+        retryTransientServerErrors: true,
+      },
+      undefined,
+      {scheduleDelay: mockScheduleDelayFn},
+    )
+
+    expect(result).toEqual(okResponse)
+    expect(mockRequestFn).toHaveBeenCalledTimes(2)
+  })
+
+  test('gives up after 3 retries of a transient server error, even when the overall retry limit is higher', async () => {
+    vi.useRealTimers()
+    const mockRequestFn = vi.fn().mockImplementation(() => {
+      throw new ClientError(internalServerErrorResponse, {query: ''})
+    })
+    const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com',
+        useNetworkLevelRetry: false,
+        retryTransientServerErrors: true,
+      },
+      undefined,
+      {scheduleDelay: mockScheduleDelayFn},
+    )
+
+    await expect(result).rejects.toThrowError(ClientError)
+    expect(mockRequestFn).toHaveBeenCalledTimes(4)
+    expect(mockScheduleDelayFn.mock.calls.map(([, delay]) => delay)).toEqual([1000, 2000, 4000])
+  })
+})
+
+describe('isTransientServerError', () => {
+  test('does not identify other errors', () => {
+    const accessDenied = new ClientError(
+      {status: 200, errors: [{message: 'Access denied', extensions: {code: 'ACCESS_DENIED'}} as any]},
+      {query: ''},
+    )
+    expect(isTransientServerError(accessDenied)).toBe(false)
+    expect(isTransientServerError(new ClientError({status: 500}, {query: ''}))).toBe(false)
+    expect(isTransientServerError(new Error('INTERNAL_SERVER_ERROR'))).toBe(false)
+    expect(isTransientServerError(undefined)).toBe(false)
+  })
+})

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

@@ -1,6 +1,7 @@
 import {sanitizedHeadersOutput} from './api/headers.js'
 import {sanitizeURL} from './api/urls.js'
-import {hasRateLimitCode} from './analytics/graphql-error-codes.js'
+import {hasRateLimitCode, hasTransientServerCode} from './analytics/graphql-error-codes.js'
+import {isGatewayErrorStatus} from './api/status-checks.js'
 import {sleepWithBackoffUntil} from './sleep-with-backoff.js'
 import {outputDebug} from '../../public/node/output.js'
 import {recordRetry} from '../../public/node/analytics.js'
@@ -16,17 +17,31 @@ export const allAPIs: API[] = ['admin', 'storefront-renderer', 'partners', 'busi
 
 const DEFAULT_RETRY_DELAY_MS = 1000
 const DEFAULT_RETRY_LIMIT = 10
+// Transient server errors (e.g. `INTERNAL_SERVER_ERROR`, 502/503/504) get a smaller retry budget
+// with exponential backoff, so a struggling backend isn't hit by many quick retries.
+const TRANSIENT_SERVER_ERROR_RETRY_LIMIT = 3
+const TRANSIENT_SERVER_ERROR_BASE_DELAY_MS = 1000
+
+interface CommonRetryBehaviour {
+  recordCommandRetries?: boolean
+  /**
+   * Retry, with backoff, requests that fail with a transient server-side error: a GraphQL
+   * `INTERNAL_SERVER_ERROR` (usually on an HTTP 200) or an HTTP 502/503/504.
+   *
+   * Only enable this for requests that are safe to repeat. GraphQL requests only honour it for
+   * queries; mutations are never retried on these errors.
+   */
+  retryTransientServerErrors?: boolean
+}
 
 export type NetworkRetryBehaviour =
-  | {
+  | ({
       useNetworkLevelRetry: true
       maxRetryTimeMs: number
-      recordCommandRetries?: boolean
-    }
-  | {
+    } & CommonRetryBehaviour)
+  | ({
       useNetworkLevelRetry: false
-      recordCommandRetries?: boolean
-    }
+    } & CommonRetryBehaviour)
 
 type RequestOptions<T> = {
   request: () => Promise<T>
@@ -58,6 +73,7 @@ type ClientErrorResponse = CommonResponse & {status: 'client-error'; clientError
 type UnknownErrorResponse = CommonResponse & {status: 'unknown-error'; error: unknown}
 type CanRetryErrorResponse = CommonResponse & {
   status: 'can-retry'
+  reason: 'throttled' | 'transient-server-error'
   clientError: ClientError
   delayMs: number | undefined
 }
@@ -233,13 +249,25 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
         }
         return {
           status: 'can-retry',
+          reason: 'throttled',
           clientError: err,
           duration,
           sanitizedHeaders,
           sanitizedUrl,
           requestId: responseHeaders['x-request-id'],
           delayMs,
         }
+      } else if (requestOptions.retryTransientServerErrors && isTransientServerError(err)) {
+        return {
+          status: 'can-retry',
+          reason: 'transient-server-error',
+          clientError: err,
+          duration,
+          sanitizedHeaders,
+          sanitizedUrl,
+          requestId: responseHeaders['x-request-id'],
+          delayMs: undefined,
+        }
       } else if (err.response.status === 401) {
         return {
           status: 'unauthorized',
@@ -293,6 +321,19 @@ function isThrottled(error: ClientError): boolean {
   return hasRateLimitCode(error.response.errors)
 }
 
+/**
+ * Checks whether an error is a transient server-side failure that is likely to succeed if retried:
+ * a GraphQL `INTERNAL_SERVER_ERROR` (Shopify APIs usually return it on an HTTP 200), or an HTTP
+ * 502, 503 or 504.
+ *
+ * @param error - The error to check.
+ * @returns Whether the error is a transient server-side failure.
+ */
+export function isTransientServerError(error: unknown): error is ClientError {
+  if (!(error instanceof ClientError)) return false
+  return hasTransientServerCode(error.response.errors) || isGatewayErrorStatus(error.response.status)
+}
+
 export async function simpleRequestWithDebugLog<T extends {headers: Headers; status: number}>(
   requestOptions: RequestOptions<T>,
   errorHandler?: (error: unknown, requestId: string | undefined) => unknown,
@@ -368,6 +409,7 @@ export async function retryAwareRequest<T extends {headers: Headers; status: num
   },
 ): Promise<T> {
   let retriesUsed = 0
+  let transientServerErrorRetriesUsed = 0
   const limitRetriesTo = retryOptions.limitRetriesTo ?? DEFAULT_RETRY_LIMIT
 
   let result = await makeVerboseRequest(requestOptions)
@@ -399,8 +441,12 @@ ${result.sanitizedHeaders}
       throw result.clientError
     }
 
-    if (limitRetriesTo <= retriesUsed) {
-      outputDebug(`${limitRetriesTo} retries exhausted for request to ${result.sanitizedUrl}`)
+    const isTransientServerErrorRetry = result.reason === 'transient-server-error'
+    const retriesExhausted =
+      limitRetriesTo <= retriesUsed ||
+      (isTransientServerErrorRetry && TRANSIENT_SERVER_ERROR_RETRY_LIMIT <= transientServerErrorRetriesUsed)
+    if (retriesExhausted) {
+      outputDebug(`${retriesUsed} retries exhausted for request to ${result.sanitizedUrl}`)
       if (errorHandler) {
         throw errorHandler(result.clientError, result.requestId)
       } else {
@@ -409,13 +455,22 @@ ${result.sanitizedHeaders}
     }
     retriesUsed += 1
 
+    let retryDelayMs: number
+    if (isTransientServerErrorRetry) {
+      // exponential backoff: 1s, 2s, 4s, ...
+      retryDelayMs =
+        (retryOptions.defaultDelayMs ?? TRANSIENT_SERVER_ERROR_BASE_DELAY_MS) * 2 ** transientServerErrorRetriesUsed
+      transientServerErrorRetriesUsed += 1
+    } else {
+      // prefer to wait based on a header if given; the caller's preference if not; and a default if neither.
+      retryDelayMs = result.delayMs ?? retryOptions.defaultDelayMs ?? DEFAULT_RETRY_DELAY_MS
+    }
+
     // Record command retries
     if (requestOptions.recordCommandRetries) {
-      recordRetry(requestOptions.url, `http-retry-${retriesUsed}:${result.status}:`)
+      const retryKind = isTransientServerErrorRetry ? result.reason : result.status
+      recordRetry(requestOptions.url, `http-retry-${retriesUsed}:${retryKind}:`)
     }
-
-    // prefer to wait based on a header if given; the caller's preference if not; and a default if neither.
-    const retryDelayMs = result.delayMs ?? retryOptions.defaultDelayMs ?? DEFAULT_RETRY_DELAY_MS
     outputDebug(`Scheduling retry request #${retriesUsed} to ${result.sanitizedUrl} in ${retryDelayMs} ms`)
 
     // eslint-disable-next-line no-await-in-loop