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 78126c604a4..edc2c76e7e3 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 @@ -53,8 +53,8 @@ export function graphQLErrorCodes(errors: unknown): string[] { /** * Whether a single code is a rate-limit signal (`THROTTLED` or `429`). * - * Mirrors the established shape detected by `errorsIncludeStatus429` in `private/node/api.ts`, - * where `extensions.code === '429'` signals rate limiting even at HTTP 200. + * Shared with the retry path (`isThrottled` in `private/node/api.ts`), where these codes signal + * rate limiting even at HTTP 200. */ export function isRateLimitCode(code: string | undefined): boolean { return code !== undefined && RATE_LIMIT_CODES.has(code) diff --git a/packages/cli-kit/src/private/node/api.test.ts b/packages/cli-kit/src/private/node/api.test.ts index 5dd325be6ca..ad0922fbb60 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/packages/cli-kit/src/private/node/api.test.ts @@ -92,6 +92,100 @@ describe('retryAwareRequest', () => { expect(mockScheduleDelayFn).toHaveBeenNthCalledWith(2, expect.anything(), 500) }) + test('retries THROTTLED GraphQL errors that carry no 429 status or code', async () => { + // Shopify GraphQL APIs (e.g. App Management) throttle with a 200 response + // whose GraphQL error has extensions.code "THROTTLED" — no 429 status, no + // retry-after header. + const throttledResponse = { + status: 200, + errors: [ + { + message: 'Throttled', + extensions: {code: 'THROTTLED'}, + } as any, + ], + headers: new Headers(), + } + + const mockRequestFn = vi + .fn() + .mockImplementationOnce(() => { + throw new ClientError(throttledResponse, {query: ''}) + }) + .mockImplementationOnce(() => { + return Promise.resolve({ + status: 200, + data: {hello: 'world!'}, + headers: new Headers(), + }) + }) + const mockScheduleDelayFn = vi.fn((fn, delay) => { + return fn() + }) + const result = retryAwareRequest( + { + request: mockRequestFn, + url: 'https://example.com', + useNetworkLevelRetry: false, + }, + undefined, + { + defaultDelayMs: 500, + scheduleDelay: mockScheduleDelayFn, + }, + ) + await vi.runAllTimersAsync() + + await expect(result).resolves.toEqual({ + headers: expect.anything(), + status: 200, + data: {hello: 'world!'}, + }) + + expect(mockRequestFn).toHaveBeenCalledTimes(2) + expect(mockScheduleDelayFn).toHaveBeenCalledWith(expect.anything(), 500) + }) + + test('does not retry errors whose message says Throttled without a rate-limit code', async () => { + // The message can echo user-controlled strings (e.g. an app named + // "Throttled") — only the server-set extensions.code marks rate limiting. + // This test gives a false warning from vitest if fake timers are used. It thinks the exception is uncaught. + vi.useRealTimers() + const messageOnlyResponse = { + status: 200, + errors: [ + { + message: 'Throttled app name is invalid', + } as any, + ], + headers: new Headers(), + } + const mockRequestFn = vi.fn().mockImplementation(() => { + throw new ClientError(messageOnlyResponse, {query: ''}) + }) + const mockScheduleDelayFn = vi.fn((fn, delay) => { + return fn() + }) + + const result = retryAwareRequest( + { + request: mockRequestFn, + url: 'https://example.com', + useNetworkLevelRetry: false, + }, + undefined, + { + defaultDelayMs: 500, + scheduleDelay: mockScheduleDelayFn, + }, + ) + + await expect(result).rejects.toThrowError(ClientError) + + expect(mockRequestFn).toHaveBeenCalledTimes(1) + expect(mockScheduleDelayFn).not.toHaveBeenCalled() + }) + test('fails after too many retries', async () => { // This test gives a false warning from vitest if fake timers are used. It thinks the exception is uncaught. vi.useRealTimers() diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 044dd0abfb2..7fdaeb54640 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -1,5 +1,6 @@ import {sanitizedHeadersOutput} from './api/headers.js' import {sanitizeURL} from './api/urls.js' +import {hasRateLimitCode} from './analytics/graphql-error-codes.js' import {sleepWithBackoffUntil} from './sleep-with-backoff.js' import {outputDebug} from '../../public/node/output.js' import {recordRetry} from '../../public/node/analytics.js' @@ -193,7 +194,7 @@ async function makeVerboseRequest( } const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders) - if (errorsIncludeStatus429(err)) { + if (isThrottled(err)) { let delayMs: number | undefined try { @@ -253,17 +254,15 @@ async function makeVerboseRequest( } } -function errorsIncludeStatus429(error: ClientError): boolean { +// Shopify GraphQL APIs signal rate limiting with `extensions.code` set to +// `THROTTLED` (often on a 200 response) or `429` — the same codes that +// crash-report suppression and analytics grouping already treat as rate +// limiting via this shared helper. +function isThrottled(error: ClientError): boolean { if (error.response.status === 429) { return true } - - // GraphQL returns a 401 with a string error message when auth fails - // Therefore error.response.errors can be a string or GraphQLError[] - if (typeof error.response.errors === 'string') { - return false - } - return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false + return hasRateLimitCode(error.response.errors) } export async function simpleRequestWithDebugLog(