Skip to content

Commit fd13e7f

Browse files
shopify-riverlizkenyon
shopify-river
and
lizkenyon
committed
Retry transient server errors in theme API requests
Theme commands (`theme pull`, `theme dev`, ...) crashed on the first Admin API response carrying `extensions.code: INTERNAL_SERVER_ERROR` (returned on an HTTP 200), even though these are usually brief backend failures, such as a database briefly running out of connections, that succeed when retried. Because theme requests use `handleErrors: false`, the raw `ClientError` was reported as an unexpected error, with the whole GraphQL response as its message. - Add an opt-in `retryTransientServerErrors` request behaviour. When set, `INTERNAL_SERVER_ERROR` and HTTP 502/503/504 responses are retried up to 3 times with exponential backoff (1s, 2s, 4s). This is a separate, smaller budget than rate-limit retries, so an overloaded backend isn't hit with many quick retries. - GraphQL requests only honour the behaviour for documents that contain only queries. Mutations are never retried on these errors. - Enable it for theme API requests. - If a theme request still fails, throw a friendly AbortError that keeps the request ID instead of the raw response. `fetchTheme` no longer treats a temporary failure as "theme not found". Co-authored-by: Elizabeth Kenyon <elizabeth.kenyon@shopify.com>
1 parent f5effdf commit fd13e7f

9 files changed

Lines changed: 451 additions & 18 deletions

File tree

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/cli-kit': patch
3+
---
4+
5+
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‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import {graphQLErrorCodes, hasRateLimitCode, isPermissionCode, isRateLimitCode} from './graphql-error-codes.js'
1+
import {
2+
graphQLErrorCodes,
3+
hasRateLimitCode,
4+
hasTransientServerCode,
5+
isPermissionCode,
6+
isRateLimitCode,
7+
} from './graphql-error-codes.js'
28
import {describe, expect, test} from 'vitest'
39

410
describe('graphQLErrorCodes', () => {
@@ -38,4 +44,10 @@ describe('code predicates', () => {
3844
expect(hasRateLimitCode([{message: 'a'}, {extensions: {code: '429'}}])).toBe(true)
3945
expect(hasRateLimitCode([{extensions: {code: 'OTHER'}}])).toBe(false)
4046
})
47+
48+
test('hasTransientServerCode scans all errors, not just the first', () => {
49+
expect(hasTransientServerCode([{message: 'a'}, {extensions: {code: 'INTERNAL_SERVER_ERROR'}}])).toBe(true)
50+
expect(hasTransientServerCode([{extensions: {code: 'ACCESS_DENIED'}}])).toBe(false)
51+
expect(hasTransientServerCode('Internal error')).toBe(false)
52+
})
4153
})

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,13 @@ const RATE_LIMIT_CODES = new Set(['THROTTLED', '429'])
1818
*/
1919
const PERMISSION_CODES = new Set(['ACCESS_DENIED', 'access_denied'])
2020

21+
/**
22+
* GraphQL `extensions.code` values we treat as a transient server-side failure: the request was
23+
* valid, but the server failed to process it (for example, a database briefly running out of
24+
* connections). Shopify APIs return these on an HTTP 200, so the status alone doesn't reveal them.
25+
*/
26+
const TRANSIENT_SERVER_CODES = new Set(['INTERNAL_SERVER_ERROR'])
27+
2128
/**
2229
* Collects every routing-relevant code from a GraphQL `errors` value.
2330
*
@@ -67,6 +74,13 @@ export function isPermissionCode(code: string | undefined): boolean {
6774
return code !== undefined && PERMISSION_CODES.has(code)
6875
}
6976

77+
/**
78+
* Whether a single code is a transient server-side failure signal (`INTERNAL_SERVER_ERROR`).
79+
*/
80+
function isTransientServerCode(code: string | undefined): boolean {
81+
return code !== undefined && TRANSIENT_SERVER_CODES.has(code)
82+
}
83+
7084
/**
7185
* Whether any GraphQL error carries a rate-limit code. Convenience for suppression logic that only
7286
* needs a boolean over the raw `errors` value.
@@ -77,3 +91,13 @@ export function isPermissionCode(code: string | undefined): boolean {
7791
export function hasRateLimitCode(errors: unknown): boolean {
7892
return graphQLErrorCodes(errors).some(isRateLimitCode)
7993
}
94+
95+
/**
96+
* Whether any GraphQL error carries a transient server-side failure code.
97+
*
98+
* @param errors - The `errors` value from a GraphQL response.
99+
* @returns True when a transient server code is present in any error.
100+
*/
101+
export function hasTransientServerCode(errors: unknown): boolean {
102+
return graphQLErrorCodes(errors).some(isTransientServerCode)
103+
}

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

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {retryAwareRequest, isNetworkError, isTransientNetworkError} from './api.js'
1+
import {retryAwareRequest, isNetworkError, isTransientNetworkError, isTransientServerError} from './api.js'
22
import {recordRetry} from '../../public/node/analytics.js'
33
import {ClientError} from 'graphql-request'
44
import {describe, test, vi, expect, beforeEach, afterEach} from 'vitest'
@@ -814,3 +814,108 @@ describe('isNetworkError', () => {
814814
expect(isNetworkError({message: 'certificate error'})).toBe(false)
815815
})
816816
})
817+
818+
describe('retryAwareRequest with transient server errors', () => {
819+
const internalServerErrorResponse = {
820+
status: 200,
821+
errors: [
822+
{
823+
message: 'Internal error. Looks like something went wrong on our end.',
824+
extensions: {code: 'INTERNAL_SERVER_ERROR', requestId: 'request-id'},
825+
} as any,
826+
],
827+
headers: new Headers(),
828+
}
829+
const okResponse = {status: 200, data: {hello: 'world!'}, headers: new Headers()}
830+
831+
test('retries INTERNAL_SERVER_ERROR and records each attempt when enabled', async () => {
832+
const mockRequestFn = vi
833+
.fn()
834+
.mockImplementationOnce(() => {
835+
throw new ClientError(internalServerErrorResponse, {query: ''})
836+
})
837+
.mockImplementationOnce(() => {
838+
throw new ClientError(internalServerErrorResponse, {query: ''})
839+
})
840+
.mockImplementationOnce(() => Promise.resolve(okResponse))
841+
const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
842+
843+
const result = await retryAwareRequest(
844+
{
845+
request: mockRequestFn,
846+
url: 'https://example.com',
847+
useNetworkLevelRetry: false,
848+
retryTransientServerErrors: true,
849+
recordCommandRetries: true,
850+
},
851+
undefined,
852+
{scheduleDelay: mockScheduleDelayFn},
853+
)
854+
855+
expect(result).toEqual(okResponse)
856+
expect(mockRequestFn).toHaveBeenCalledTimes(3)
857+
expect(recordRetry).toHaveBeenCalledWith('https://example.com', 'http-retry-1:transient-server-error:')
858+
expect(recordRetry).toHaveBeenCalledWith('https://example.com', 'http-retry-2:transient-server-error:')
859+
})
860+
861+
test('retries gateway errors when retryTransientServerErrors is enabled', async () => {
862+
const gatewayErrorResponse = {status: 503, headers: new Headers()}
863+
const mockRequestFn = vi
864+
.fn()
865+
.mockImplementationOnce(() => {
866+
throw new ClientError(gatewayErrorResponse, {query: ''})
867+
})
868+
.mockImplementationOnce(() => Promise.resolve(okResponse))
869+
const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
870+
871+
const result = await retryAwareRequest(
872+
{
873+
request: mockRequestFn,
874+
url: 'https://example.com',
875+
useNetworkLevelRetry: false,
876+
retryTransientServerErrors: true,
877+
},
878+
undefined,
879+
{scheduleDelay: mockScheduleDelayFn},
880+
)
881+
882+
expect(result).toEqual(okResponse)
883+
expect(mockRequestFn).toHaveBeenCalledTimes(2)
884+
})
885+
886+
test('gives up after 3 retries of a transient server error, even when the overall retry limit is higher', async () => {
887+
vi.useRealTimers()
888+
const mockRequestFn = vi.fn().mockImplementation(() => {
889+
throw new ClientError(internalServerErrorResponse, {query: ''})
890+
})
891+
const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
892+
893+
const result = retryAwareRequest(
894+
{
895+
request: mockRequestFn,
896+
url: 'https://example.com',
897+
useNetworkLevelRetry: false,
898+
retryTransientServerErrors: true,
899+
},
900+
undefined,
901+
{scheduleDelay: mockScheduleDelayFn},
902+
)
903+
904+
await expect(result).rejects.toThrowError(ClientError)
905+
expect(mockRequestFn).toHaveBeenCalledTimes(4)
906+
expect(mockScheduleDelayFn.mock.calls.map(([, delay]) => delay)).toEqual([1000, 2000, 4000])
907+
})
908+
})
909+
910+
describe('isTransientServerError', () => {
911+
test('does not identify other errors', () => {
912+
const accessDenied = new ClientError(
913+
{status: 200, errors: [{message: 'Access denied', extensions: {code: 'ACCESS_DENIED'}} as any]},
914+
{query: ''},
915+
)
916+
expect(isTransientServerError(accessDenied)).toBe(false)
917+
expect(isTransientServerError(new ClientError({status: 500}, {query: ''}))).toBe(false)
918+
expect(isTransientServerError(new Error('INTERNAL_SERVER_ERROR'))).toBe(false)
919+
expect(isTransientServerError(undefined)).toBe(false)
920+
})
921+
})

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

Lines changed: 68 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {sanitizedHeadersOutput} from './api/headers.js'
22
import {sanitizeURL} from './api/urls.js'
3-
import {hasRateLimitCode} from './analytics/graphql-error-codes.js'
3+
import {hasRateLimitCode, hasTransientServerCode} from './analytics/graphql-error-codes.js'
4+
import {isGatewayErrorStatus} from './api/status-checks.js'
45
import {sleepWithBackoffUntil} from './sleep-with-backoff.js'
56
import {outputDebug} from '../../public/node/output.js'
67
import {recordRetry} from '../../public/node/analytics.js'
@@ -16,17 +17,31 @@ export const allAPIs: API[] = ['admin', 'storefront-renderer', 'partners', 'busi
1617

1718
const DEFAULT_RETRY_DELAY_MS = 1000
1819
const DEFAULT_RETRY_LIMIT = 10
20+
// Transient server errors (e.g. `INTERNAL_SERVER_ERROR`, 502/503/504) get a smaller retry budget
21+
// with exponential backoff, so a struggling backend isn't hit by many quick retries.
22+
const TRANSIENT_SERVER_ERROR_RETRY_LIMIT = 3
23+
const TRANSIENT_SERVER_ERROR_BASE_DELAY_MS = 1000
24+
25+
interface CommonRetryBehaviour {
26+
recordCommandRetries?: boolean
27+
/**
28+
* Retry, with backoff, requests that fail with a transient server-side error: a GraphQL
29+
* `INTERNAL_SERVER_ERROR` (usually on an HTTP 200) or an HTTP 502/503/504.
30+
*
31+
* Only enable this for requests that are safe to repeat. GraphQL requests only honour it for
32+
* queries; mutations are never retried on these errors.
33+
*/
34+
retryTransientServerErrors?: boolean
35+
}
1936

2037
export type NetworkRetryBehaviour =
21-
| {
38+
| ({
2239
useNetworkLevelRetry: true
2340
maxRetryTimeMs: number
24-
recordCommandRetries?: boolean
25-
}
26-
| {
41+
} & CommonRetryBehaviour)
42+
| ({
2743
useNetworkLevelRetry: false
28-
recordCommandRetries?: boolean
29-
}
44+
} & CommonRetryBehaviour)
3045

3146
type RequestOptions<T> = {
3247
request: () => Promise<T>
@@ -58,6 +73,7 @@ type ClientErrorResponse = CommonResponse & {status: 'client-error'; clientError
5873
type UnknownErrorResponse = CommonResponse & {status: 'unknown-error'; error: unknown}
5974
type CanRetryErrorResponse = CommonResponse & {
6075
status: 'can-retry'
76+
reason: 'throttled' | 'transient-server-error'
6177
clientError: ClientError
6278
delayMs: number | undefined
6379
}
@@ -233,13 +249,25 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
233249
}
234250
return {
235251
status: 'can-retry',
252+
reason: 'throttled',
236253
clientError: err,
237254
duration,
238255
sanitizedHeaders,
239256
sanitizedUrl,
240257
requestId: responseHeaders['x-request-id'],
241258
delayMs,
242259
}
260+
} else if (requestOptions.retryTransientServerErrors && isTransientServerError(err)) {
261+
return {
262+
status: 'can-retry',
263+
reason: 'transient-server-error',
264+
clientError: err,
265+
duration,
266+
sanitizedHeaders,
267+
sanitizedUrl,
268+
requestId: responseHeaders['x-request-id'],
269+
delayMs: undefined,
270+
}
243271
} else if (err.response.status === 401) {
244272
return {
245273
status: 'unauthorized',
@@ -293,6 +321,19 @@ function isThrottled(error: ClientError): boolean {
293321
return hasRateLimitCode(error.response.errors)
294322
}
295323

324+
/**
325+
* Checks whether an error is a transient server-side failure that is likely to succeed if retried:
326+
* a GraphQL `INTERNAL_SERVER_ERROR` (Shopify APIs usually return it on an HTTP 200), or an HTTP
327+
* 502, 503 or 504.
328+
*
329+
* @param error - The error to check.
330+
* @returns Whether the error is a transient server-side failure.
331+
*/
332+
export function isTransientServerError(error: unknown): error is ClientError {
333+
if (!(error instanceof ClientError)) return false
334+
return hasTransientServerCode(error.response.errors) || isGatewayErrorStatus(error.response.status)
335+
}
336+
296337
export async function simpleRequestWithDebugLog<T extends {headers: Headers; status: number}>(
297338
requestOptions: RequestOptions<T>,
298339
errorHandler?: (error: unknown, requestId: string | undefined) => unknown,
@@ -368,6 +409,7 @@ export async function retryAwareRequest<T extends {headers: Headers; status: num
368409
},
369410
): Promise<T> {
370411
let retriesUsed = 0
412+
let transientServerErrorRetriesUsed = 0
371413
const limitRetriesTo = retryOptions.limitRetriesTo ?? DEFAULT_RETRY_LIMIT
372414

373415
let result = await makeVerboseRequest(requestOptions)
@@ -399,8 +441,12 @@ ${result.sanitizedHeaders}
399441
throw result.clientError
400442
}
401443

402-
if (limitRetriesTo <= retriesUsed) {
403-
outputDebug(`${limitRetriesTo} retries exhausted for request to ${result.sanitizedUrl}`)
444+
const isTransientServerErrorRetry = result.reason === 'transient-server-error'
445+
const retriesExhausted =
446+
limitRetriesTo <= retriesUsed ||
447+
(isTransientServerErrorRetry && TRANSIENT_SERVER_ERROR_RETRY_LIMIT <= transientServerErrorRetriesUsed)
448+
if (retriesExhausted) {
449+
outputDebug(`${retriesUsed} retries exhausted for request to ${result.sanitizedUrl}`)
404450
if (errorHandler) {
405451
throw errorHandler(result.clientError, result.requestId)
406452
} else {
@@ -409,13 +455,22 @@ ${result.sanitizedHeaders}
409455
}
410456
retriesUsed += 1
411457

458+
let retryDelayMs: number
459+
if (isTransientServerErrorRetry) {
460+
// exponential backoff: 1s, 2s, 4s, ...
461+
retryDelayMs =
462+
(retryOptions.defaultDelayMs ?? TRANSIENT_SERVER_ERROR_BASE_DELAY_MS) * 2 ** transientServerErrorRetriesUsed
463+
transientServerErrorRetriesUsed += 1
464+
} else {
465+
// prefer to wait based on a header if given; the caller's preference if not; and a default if neither.
466+
retryDelayMs = result.delayMs ?? retryOptions.defaultDelayMs ?? DEFAULT_RETRY_DELAY_MS
467+
}
468+
412469
// Record command retries
413470
if (requestOptions.recordCommandRetries) {
414-
recordRetry(requestOptions.url, `http-retry-${retriesUsed}:${result.status}:`)
471+
const retryKind = isTransientServerErrorRetry ? result.reason : result.status
472+
recordRetry(requestOptions.url, `http-retry-${retriesUsed}:${retryKind}:`)
415473
}
416-
417-
// prefer to wait based on a header if given; the caller's preference if not; and a default if neither.
418-
const retryDelayMs = result.delayMs ?? retryOptions.defaultDelayMs ?? DEFAULT_RETRY_DELAY_MS
419474
outputDebug(`Scheduling retry request #${retriesUsed} to ${result.sanitizedUrl} in ${retryDelayMs} ms`)
420475

421476
// eslint-disable-next-line no-await-in-loop

0 commit comments

Comments
 (0)