Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
94 changes: 94 additions & 0 deletions packages/cli-kit/src/private/node/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
17 changes: 8 additions & 9 deletions packages/cli-kit/src/private/node/api.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -193,7 +194,7 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
}
const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders)

if (errorsIncludeStatus429(err)) {
if (isThrottled(err)) {
let delayMs: number | undefined

try {
Expand Down Expand Up @@ -253,17 +254,15 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
}
}

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<T extends {headers: Headers; status: number}>(
Expand Down
Loading