diff --git a/.changeset/theme-retry-transient-server-errors.md b/.changeset/theme-retry-transient-server-errors.md new file mode 100644 index 00000000000..a1fcdc7f57f --- /dev/null +++ b/.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. diff --git a/packages/cli-kit/src/private/node/analytics/graphql-error-codes.test.ts b/packages/cli-kit/src/private/node/analytics/graphql-error-codes.test.ts index 445cdde70b0..753952a566f 100644 --- a/packages/cli-kit/src/private/node/analytics/graphql-error-codes.test.ts +++ b/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) + }) }) diff --git a/packages/cli-kit/src/private/node/analytics/graphql-error-codes.ts b/packages/cli-kit/src/private/node/analytics/graphql-error-codes.ts index edc2c76e7e3..246055c1acf 100644 --- a/packages/cli-kit/src/private/node/analytics/graphql-error-codes.ts +++ b/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) +} diff --git a/packages/cli-kit/src/private/node/api.test.ts b/packages/cli-kit/src/private/node/api.test.ts index ad0922fbb60..aa8c09a9449 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/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) + }) +}) diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 969bbae3611..18403dc7e93 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/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 = { request: () => Promise @@ -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,6 +249,7 @@ async function makeVerboseRequest( } return { status: 'can-retry', + reason: 'throttled', clientError: err, duration, sanitizedHeaders, @@ -240,6 +257,17 @@ async function makeVerboseRequest( 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( requestOptions: RequestOptions, errorHandler?: (error: unknown, requestId: string | undefined) => unknown, @@ -368,6 +409,7 @@ export async function retryAwareRequest { 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 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 e5da87b4e7c..6bb08326a59 100644 --- a/packages/cli-kit/src/public/node/api/graphql.test.ts +++ b/packages/cli-kit/src/public/node/api/graphql.test.ts @@ -738,3 +738,85 @@ describe('graphqlRequest with caching', () => { expect(retryAwareSpy).toHaveBeenCalled() }) }) + +describe('graphqlRequest with transient server errors', () => { + const internalServerError = () => + HttpResponse.json<{errors: {message: string; extensions: {code: string}}[]}>({ + errors: [ + { + message: 'Internal error. Looks like something went wrong on our end.', + extensions: {code: 'INTERNAL_SERVER_ERROR'}, + }, + ], + }) + const retryBehaviour = { + useNetworkLevelRetry: false, + useAbortSignal: false, + retryTransientServerErrors: true, + } as const + + test('retries a query that fails with INTERNAL_SERVER_ERROR when the behaviour enables it', async () => { + let attempts = 0 + server.use( + mockApi.query('Flaky', () => { + attempts += 1 + if (attempts === 1) return internalServerError() + return HttpResponse.json({data: {Flaky: {example: 'hello'}}}) + }), + ) + + const res = await graphqlRequest({ + query: 'query Flaky { example }', + api: 'mockApi', + url: mockedAddress, + token: mockToken, + preferredBehaviour: retryBehaviour, + }) + + expect(res).toEqual({Flaky: {example: 'hello'}}) + expect(attempts).toEqual(2) + }) + + test('does not retry a mutation that fails with INTERNAL_SERVER_ERROR, even when the behaviour enables it', async () => { + let attempts = 0 + server.use( + mockApi.mutation('FlakyMutation', () => { + attempts += 1 + return internalServerError() + }), + ) + + await expect( + graphqlRequest({ + query: 'mutation FlakyMutation { example }', + api: 'mockApi', + url: mockedAddress, + token: mockToken, + preferredBehaviour: retryBehaviour, + responseOptions: {handleErrors: false}, + }), + ).rejects.toThrow() + expect(attempts).toEqual(1) + }) + + test('does not retry INTERNAL_SERVER_ERROR by default', async () => { + let attempts = 0 + server.use( + mockApi.query('Flaky', () => { + attempts += 1 + return internalServerError() + }), + ) + + await expect( + graphqlRequest({ + query: 'query Flaky { example }', + api: 'mockApi', + url: mockedAddress, + token: mockToken, + responseOptions: {handleErrors: false}, + }), + ).rejects.toThrow() + expect(attempts).toEqual(1) + }) +}) diff --git a/packages/cli-kit/src/public/node/api/graphql.ts b/packages/cli-kit/src/public/node/api/graphql.ts index 137bfd1b365..e73a54d5ee4 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 {parse} from 'graphql' // to replace TVariable type when there graphql query has no variables export type Exact> = {[K in keyof T]: T[K]} @@ -215,9 +216,12 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti } : undefined + // Retrying a transient server error is only safe when the request has no side effects. + const retryTransientServerErrors = Boolean(behaviour.retryTransientServerErrors) && isQueryOnly(queryAsString) + const request = () => retryAwareRequest( - {request: rawGraphQLRequest, url, ...behaviour}, + {request: rawGraphQLRequest, url, ...behaviour, retryTransientServerErrors}, responseOptions?.handleErrors === false ? undefined : errorHandler(api), ) @@ -278,6 +282,24 @@ function graphQLCachePrincipalHash(url: string, token: string | undefined): stri return sha256(JSON.stringify([url, token ?? ''])).toString('hex') } +/** + * Whether a GraphQL document only contains query operations (no mutations or subscriptions). + * + * @param queryAsString - The GraphQL document as a string. + * @returns True when every operation in the document is a query. False if it can't be parsed. + */ +function isQueryOnly(queryAsString: string): boolean { + try { + const operations = parse(queryAsString).definitions.filter( + (definition) => definition.kind === 'OperationDefinition', + ) + return operations.length > 0 && operations.every((operation) => operation.operation === 'query') + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + async function logLastRequestIdFromResponse(response: GraphQLResponse) { try { const requestId = response.headers.get('x-request-id') diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index c6da96b898a..b6022f8ecdf 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -3,6 +3,7 @@ import { themeDelete, themeDuplicate, fetchTheme, + fetchThemeAssets, fetchThemes, findDevelopmentThemeByName, ThemeParams, @@ -12,7 +13,9 @@ import { bulkUploadThemeAssets, AssetParams, deleteThemeAssets, + metafieldDefinitionsByOwnerType, parseThemeFileContent, + passwordProtected, } from './api.js' import {Operation} from './types.js' import {ThemeDelete} from '../../../cli/api/graphql/admin/generated/theme_delete.js' @@ -26,6 +29,8 @@ import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_f import {GetThemes} from '../../../cli/api/graphql/admin/generated/get_themes.js' import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' +import {MetafieldDefinitionsByOwnerType} from '../../../cli/api/graphql/admin/generated/metafield_definitions_by_owner_type.js' +import {OnlineStorePasswordProtection} from '../../../cli/api/graphql/admin/generated/online_store_password_protection.js' import {adminRequestDoc, supportedApiVersions} from '../api/admin.js' import {AbortError} from '../error.js' @@ -43,8 +48,24 @@ const expectedApiOptions = expect.objectContaining({ maxRetryTimeMs: 90000, useAbortSignal: false, useNetworkLevelRetry: true, + retryTransientServerErrors: true, }) +function internalServerError(requestId?: string) { + return new ClientError( + { + status: 200, + errors: [ + { + message: 'Internal error. Looks like something went wrong on our end.', + extensions: {code: 'INTERNAL_SERVER_ERROR', ...(requestId ? {requestId} : {})}, + } as any, + ], + }, + {query: ''}, + ) +} + describe('fetchTheme', () => { test('returns a store theme', async () => { vi.mocked(adminRequestDoc).mockResolvedValue({ @@ -97,6 +118,72 @@ describe('fetchTheme', () => { 'The authenticated account or access token is missing `read_themes` access scope.', ) }) + test('throws a friendly error instead of returning undefined when the server has a temporary problem', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue(internalServerError('request-id')) + + await expect(fetchTheme(123, session)).rejects.toThrow( + 'Shopify had a temporary problem and could not complete the request.', + ) + }) +}) + +describe('fetchThemeAssets', () => { + test('throws a friendly error with the request ID when the server keeps failing', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue(internalServerError('request-id')) + + const error = await fetchThemeAssets(123, ['templates/index.json'], session).catch((error) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(error.message).toEqual('Shopify had a temporary problem and could not complete the request.') + expect(error.tryMessage).toContain('request-id') + expect(adminRequestDoc).toHaveBeenCalledWith(expect.objectContaining({preferredBehaviour: expectedApiOptions})) + }) + + test('includes a gateway response request ID in the friendly error', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue( + new ClientError({status: 503, headers: new Headers({'x-request-id': 'gateway-request-id'})}, {query: ''}), + ) + + const error = await fetchThemeAssets(123, ['templates/index.json'], session).catch((error) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(error.tryMessage).toContain('gateway-request-id') + }) +}) + +describe('metafieldDefinitionsByOwnerType', () => { + test('uses theme retries and friendly handling for exhausted transient errors', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue(internalServerError('metafield-request-id')) + + const error = await metafieldDefinitionsByOwnerType('PRODUCT', session).catch((error) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(error.tryMessage).toContain('metafield-request-id') + expect(adminRequestDoc).toHaveBeenCalledWith({ + query: MetafieldDefinitionsByOwnerType, + session, + variables: {ownerType: 'PRODUCT'}, + responseOptions: {handleErrors: false}, + preferredBehaviour: expectedApiOptions, + }) + }) +}) + +describe('passwordProtected', () => { + test('uses theme retries and friendly handling for exhausted transient errors', async () => { + vi.mocked(adminRequestDoc).mockRejectedValue(internalServerError('password-request-id')) + + const error = await passwordProtected(session).catch((error) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(error.tryMessage).toContain('password-request-id') + expect(adminRequestDoc).toHaveBeenCalledWith({ + query: OnlineStorePasswordProtection, + session, + responseOptions: {handleErrors: false}, + preferredBehaviour: expectedApiOptions, + }) + }) }) describe('findDevelopmentThemeByName', () => { diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index 5bbd328b6aa..adb933141bc 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -30,6 +30,7 @@ import {AdminSession} from '../session.js' import {AbortError} from '../error.js' import {outputDebug} from '../output.js' import {recordTiming, recordEvent, recordError} from '../analytics.js' +import {isTransientServerError} from '../../../private/node/api.js' import {ClientError, type Variables} from 'graphql-request' import type {InlineToken, TokenItem} from '../ui.js' @@ -41,6 +42,7 @@ const THEME_API_NETWORK_BEHAVIOUR: RequestModeInput = { useAbortSignal: false, maxRetryTimeMs: 90 * 1000, recordCommandRetries: true, + retryTransientServerErrors: true, } const DEFAULT_THEME_ACCESS_REQUIREMENT = 'the required theme access scope' @@ -69,6 +71,8 @@ export async function fetchTheme(id: number, session: AdminSession): Promise ({ @@ -594,9 +600,11 @@ export async function metafieldDefinitionsByOwnerType(type: MetafieldOwnerType, export async function passwordProtected(session: AdminSession): Promise { recordEvent('theme-api:check-password-protection') - const {onlineStore} = await adminRequestDoc({ + const {onlineStore} = await requestThemeAdminDoc({ query: OnlineStorePasswordProtection, session, + responseOptions: {handleErrors: false}, + preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR, }) if (!onlineStore) { unexpectedGraphQLError("Unable to get details about the storefront's password protection") @@ -619,10 +627,43 @@ async function requestThemeAdminDoc( return response } catch (error) { abortIfMissingThemeAccessScope(error) + abortIfTransientServerError(error) throw error } } +/** + * Turns a transient server-side failure that is still failing after retries into a friendly, + * expected error instead of a crash with the raw GraphQL response. + * + * @param error - The error thrown by the Admin API request. + */ +function abortIfTransientServerError(error: unknown): void { + if (!isTransientServerError(error)) return + + const requestId = transientServerErrorRequestId(error) + throw recordError( + new AbortError( + 'Shopify had a temporary problem and could not complete the request.', + requestId + ? `Wait a moment and try again. If the problem continues, contact Shopify support and include this request ID: ${requestId}` + : 'Wait a moment and try again.', + ), + ) +} + +function transientServerErrorRequestId(error: ClientError): string | undefined { + const errors: unknown = error.response.errors + if (Array.isArray(errors)) { + for (const entry of errors) { + const requestId = (entry as {extensions?: {requestId?: unknown}} | undefined)?.extensions?.requestId + if (typeof requestId === 'string') return requestId + } + } + const headers = error.response.headers as {get?: (name: string) => string | null} | undefined + return headers?.get?.('x-request-id') ?? undefined +} + function abortIfMissingThemeAccessScope(error: unknown): void { if (!(error instanceof ClientError)) return