From e06668f0f6513b60cc037f4a327ad97b3e8dbed8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:36:06 +0200 Subject: [PATCH 1/4] Retry GraphQL requests rejected as Throttled without a 429 code retryAwareRequest already retries rate-limited requests, but only when the response is HTTP 429 or a GraphQL error with extensions.code '429'. Some Shopify APIs (App Management among them) throttle with a 200 response whose GraphQL error message is "Throttled" and no code, so the CLI failed instantly on first rejection. CI logs from throttled E2E jobs running with DEBUG=1 show zero retry attempts, confirming the path was never taken. Match the "Throttled" message as retryable too, reusing the existing retry limit and default backoff. This currently accounts for the top E2E failure mode (32 of 79 failed shards last week) and affects real `app dev`/`app deploy` users the same way. Co-Authored-By: Claude Fable 5 --- .changeset/retry-throttled-graphql-errors.md | 5 ++ packages/cli-kit/src/private/node/api.test.ts | 52 +++++++++++++++++++ packages/cli-kit/src/private/node/api.ts | 13 +++-- 3 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 .changeset/retry-throttled-graphql-errors.md diff --git a/.changeset/retry-throttled-graphql-errors.md b/.changeset/retry-throttled-graphql-errors.md new file mode 100644 index 00000000000..637219ff930 --- /dev/null +++ b/.changeset/retry-throttled-graphql-errors.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Retry automatically when Shopify APIs reject a request as throttled 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 5dd325be6ca..e67b28ceb1f 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/packages/cli-kit/src/private/node/api.test.ts @@ -92,6 +92,58 @@ describe('retryAwareRequest', () => { expect(mockScheduleDelayFn).toHaveBeenNthCalledWith(2, expect.anything(), 500) }) + test('retries throttled GraphQL errors that carry no 429 status or code', async () => { + // App Management throttles with a 200 response whose GraphQL error message + // is "Throttled" — no 429 status, no extensions code, no retry-after header. + const throttledResponse = { + status: 200, + errors: [ + { + message: '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('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..6337e6889f1 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -193,7 +193,7 @@ async function makeVerboseRequest( } const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders) - if (errorsIncludeStatus429(err)) { + if (errorsIncludeThrottling(err)) { let delayMs: number | undefined try { @@ -253,7 +253,7 @@ async function makeVerboseRequest( } } -function errorsIncludeStatus429(error: ClientError): boolean { +function errorsIncludeThrottling(error: ClientError): boolean { if (error.response.status === 429) { return true } @@ -263,7 +263,14 @@ function errorsIncludeStatus429(error: ClientError): boolean { if (typeof error.response.errors === 'string') { return false } - return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false + // Some Shopify APIs (e.g. App Management) throttle with a 200 response whose + // GraphQL error message is "Throttled", with no 429 status or code — match + // the message so those are retried too. + return ( + error.response.errors?.some( + (graphqlError) => graphqlError.extensions?.code === '429' || /^throttled/i.test(graphqlError.message ?? ''), + ) ?? false + ) } export async function simpleRequestWithDebugLog( From 70a2fea918cbb06550e86797f14e75a7b9bd0da9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:53:42 +0200 Subject: [PATCH 2/4] Remove changeset --- .changeset/retry-throttled-graphql-errors.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/retry-throttled-graphql-errors.md diff --git a/.changeset/retry-throttled-graphql-errors.md b/.changeset/retry-throttled-graphql-errors.md deleted file mode 100644 index 637219ff930..00000000000 --- a/.changeset/retry-throttled-graphql-errors.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/cli-kit': patch ---- - -Retry automatically when Shopify APIs reject a request as throttled instead of failing immediately From e49e22602e66a8c357d718d8e32e65648a44972c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:55:05 +0200 Subject: [PATCH 3/4] Rename throttle check to isThrottled --- packages/cli-kit/src/private/node/api.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 6337e6889f1..3f45ccd5814 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -193,7 +193,7 @@ async function makeVerboseRequest( } const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders) - if (errorsIncludeThrottling(err)) { + if (isThrottled(err)) { let delayMs: number | undefined try { @@ -253,7 +253,7 @@ async function makeVerboseRequest( } } -function errorsIncludeThrottling(error: ClientError): boolean { +function isThrottled(error: ClientError): boolean { if (error.response.status === 429) { return true } From 323f33238897988db3df19833498bc8188358fa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 14 Aug 2026 11:15:27 +0200 Subject: [PATCH 4/4] Match throttling by extensions.code THROTTLED instead of message text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shopify GraphQL APIs attach extensions.code THROTTLED to throttle errors (complexity_throttle.rb, app_error_handling.rb in shop/world), so exact-match the server-set code via the shared hasRateLimitCode helper — the same definition crash-report suppression and analytics grouping already use. The message regex was both spoofable by user-controlled strings and missed App Management's 'Usage throttled' variant. Co-Authored-By: Claude Fable 5 --- .../node/analytics/graphql-error-codes.ts | 4 +- packages/cli-kit/src/private/node/api.test.ts | 48 +++++++++++++++++-- packages/cli-kit/src/private/node/api.ts | 20 +++----- 3 files changed, 53 insertions(+), 19 deletions(-) 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 e67b28ceb1f..ad0922fbb60 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/packages/cli-kit/src/private/node/api.test.ts @@ -92,14 +92,16 @@ describe('retryAwareRequest', () => { expect(mockScheduleDelayFn).toHaveBeenNthCalledWith(2, expect.anything(), 500) }) - test('retries throttled GraphQL errors that carry no 429 status or code', async () => { - // App Management throttles with a 200 response whose GraphQL error message - // is "Throttled" — no 429 status, no extensions code, no retry-after header. + 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(), @@ -144,6 +146,46 @@ describe('retryAwareRequest', () => { 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 3f45ccd5814..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' @@ -253,24 +254,15 @@ async function makeVerboseRequest( } } +// 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 - } - // Some Shopify APIs (e.g. App Management) throttle with a 200 response whose - // GraphQL error message is "Throttled", with no 429 status or code — match - // the message so those are retried too. - return ( - error.response.errors?.some( - (graphqlError) => graphqlError.extensions?.code === '429' || /^throttled/i.test(graphqlError.message ?? ''), - ) ?? false - ) + return hasRateLimitCode(error.response.errors) } export async function simpleRequestWithDebugLog(