diff --git a/.changeset/retry-gateway-errors.md b/.changeset/retry-gateway-errors.md new file mode 100644 index 00000000000..7a228844359 --- /dev/null +++ b/.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 diff --git a/packages/cli-kit/src/private/node/api.test.ts b/packages/cli-kit/src/private/node/api.test.ts index ad0922fbb60..bfb2b772e66 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/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', () => { diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 7fdaeb54640..cfeb2c2e2e7 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/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 = { request: () => Promise 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): 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 = 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( 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( 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 { let retriesUsed = 0 + let gatewayRetriesUsed = 0 const limitRetriesTo = retryOptions.limitRetriesTo ?? DEFAULT_RETRY_LIMIT let result = await makeVerboseRequest(requestOptions) @@ -371,8 +411,11 @@ ${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 { @@ -380,6 +423,9 @@ ${result.sanitizedHeaders} } } retriesUsed += 1 + if (result.retryReason === 'gateway-error') { + gatewayRetriesUsed += 1 + } // Record command retries if (requestOptions.recordCommandRetries) { diff --git a/packages/cli-kit/src/private/node/api/gateway-status.ts b/packages/cli-kit/src/private/node/api/gateway-status.ts new file mode 100644 index 00000000000..6f5609fc430 --- /dev/null +++ b/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) +} diff --git a/packages/cli-kit/src/public/node/api/graphql.test.ts b/packages/cli-kit/src/public/node/api/graphql.test.ts index d0a4bf322b4..6bf91c3d1f8 100644 --- a/packages/cli-kit/src/public/node/api/graphql.test.ts +++ b/packages/cli-kit/src/public/node/api/graphql.test.ts @@ -131,6 +131,43 @@ afterEach(() => { }) describe('graphqlRequest', () => { + test.each([ + ['query', 'query QueryName { example }', true], + ['mutation', 'mutation MutationName($some: String!) { example }', false], + ])('marks a %s operation as idempotent when appropriate', async (_operation, query, requestIsIdempotent) => { + const retryAwareSpy = vi + .spyOn(api, 'retryAwareRequest') + .mockImplementation(async () => ({status: 200, headers: new Headers(), data: {}})) + + await graphqlRequest({ + query, + api: 'mockApi', + url: mockedAddress, + token: mockToken, + variables: mockVariables, + }) + + expect(retryAwareSpy).toHaveBeenCalledWith(expect.objectContaining({requestIsIdempotent}), expect.anything()) + retryAwareSpy.mockRestore() + }) + + test('treats an unparseable query as non-idempotent instead of throwing', async () => { + const retryAwareSpy = vi + .spyOn(api, 'retryAwareRequest') + .mockImplementation(async () => ({status: 200, headers: new Headers(), data: {}})) + + await graphqlRequest({ + query: 'this is not graphql', + api: 'mockApi', + url: mockedAddress, + token: mockToken, + variables: mockVariables, + }) + + expect(retryAwareSpy).toHaveBeenCalledWith(expect.objectContaining({requestIsIdempotent: false}), expect.anything()) + retryAwareSpy.mockRestore() + }) + test('calls debugLogRequestInfo once', async () => { let headers: any server.events.on('request:start', ({request}) => { @@ -328,6 +365,40 @@ describe('graphqlRequest', () => { }) describe('graphqlRequestDoc', () => { + test.each([ + ['query', true], + ['mutation', false], + ] as const)('marks a typed %s operation as idempotent when appropriate', async (operation, requestIsIdempotent) => { + const document = { + kind: 'Document', + definitions: [ + { + kind: 'OperationDefinition', + operation, + name: {kind: 'Name', value: 'OperationName'}, + selectionSet: { + kind: 'SelectionSet', + selections: [{kind: 'Field', name: {kind: 'Name', value: 'example'}}], + }, + }, + ], + } as unknown as TypedDocumentNode + const retryAwareSpy = vi + .spyOn(api, 'retryAwareRequest') + .mockImplementation(async () => ({status: 200, headers: new Headers(), data: {}})) + + await graphqlRequestDoc({ + query: document, + api: 'mockApi', + url: mockedAddress, + token: mockToken, + variables: mockVariables, + }) + + expect(retryAwareSpy).toHaveBeenCalledWith(expect.objectContaining({requestIsIdempotent}), expect.anything()) + retryAwareSpy.mockRestore() + }) + test('converts document before querying', async () => { // Given const document = { diff --git a/packages/cli-kit/src/public/node/api/graphql.ts b/packages/cli-kit/src/public/node/api/graphql.ts index e39f7386062..64efa7add38 100644 --- a/packages/cli-kit/src/public/node/api/graphql.ts +++ b/packages/cli-kit/src/public/node/api/graphql.ts @@ -25,6 +25,7 @@ import { ClientError, } from 'graphql-request' import {TypedDocumentNode} from '@graphql-typed-document-node/core' +import {Kind, parse, type DocumentNode} from 'graphql' // to replace TVariable type when there graphql query has no variables export type Exact> = {[K in keyof T]: T[K]} @@ -64,6 +65,7 @@ interface GraphQLRequestBaseOptions { type PerformGraphQLRequestOptions = GraphQLRequestBaseOptions & { queryAsString: string + requestIsIdempotent: boolean variables?: Variables unauthorizedHandler?: UnauthorizedHandler autoRateLimitRestore?: boolean @@ -101,6 +103,29 @@ export interface GraphQLResponseOptions { const MAX_RATE_LIMIT_RESTORE_DELAY_SECONDS = 0.3 +/** + * Whether a document is a single `query` operation, and therefore safe to send again after a + * gateway error. Anything else — a mutation, or a document bundling several operations — is treated + * as non-idempotent so it is never retried. + * + * @param query - The query as a string, or an already-parsed document. + * @returns True when the document contains exactly one operation and it is a query. + */ +function isGraphQLQuery(query: string | DocumentNode): boolean { + let document: DocumentNode + try { + document = typeof query === 'string' ? parse(query) : query + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + // A document we cannot parse is left for the API to reject, exactly as before. Treating it as + // non-idempotent keeps this check from changing where a malformed query surfaces. + return false + } + const operations = document.definitions.filter((definition) => definition.kind === Kind.OPERATION_DEFINITION) + + return operations.length === 1 && operations[0]?.operation === 'query' +} + async function createGraphQLClient({ url, addedHeaders, @@ -173,6 +198,7 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti unauthorizedHandler, cacheOptions, autoRateLimitRestore, + requestIsIdempotent, } = options const behaviour = requestMode(options.preferredBehaviour ?? 'default') @@ -217,7 +243,7 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti const request = () => retryAwareRequest( - {request: rawGraphQLRequest, url, ...behaviour}, + {request: rawGraphQLRequest, url, requestIsIdempotent, ...behaviour}, responseOptions?.handleErrors === false ? undefined : errorHandler(api), ) @@ -292,6 +318,7 @@ export async function graphqlRequest(options: GraphQLRequestOptions): Prom return performGraphQLRequest({ ...options, queryAsString: options.query as string, + requestIsIdempotent: isGraphQLQuery(options.query), }) } @@ -307,5 +334,6 @@ export async function graphqlRequestDoc( return performGraphQLRequest({ ...options, queryAsString: resolveRequestDocument(options.query).query, + requestIsIdempotent: isGraphQLQuery(options.query), }) }