Retry GraphQL requests rejected as Throttled without a 429 code - #8317
Conversation
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 <noreply@anthropic.com>
| // the message so those are retried too. | ||
| return ( | ||
| error.response.errors?.some( | ||
| (graphqlError) => graphqlError.extensions?.code === '429' || /^throttled/i.test(graphqlError.message ?? ''), |
There was a problem hiding this comment.
Regex tests always raise some flags... Is there nothing like a canonical error code we can use? Or at least some tighter enforcement around the message text? So someone can't name an app "throttled" and then retry on user errors...
There was a problem hiding this comment.
yeah, you are right, this was a very naive solution. I'll look more into this, apparently the AI understood that the API is returning a 200 code with a throttled message, but that's not true
There was a problem hiding this comment.
Good call — there is a canonical code: throttle errors carry a server-set extensions.code: "THROTTLED". I assumed there was none because the CLI's rendered error only prints the message.
Reworked to match the code exactly, no message matching left. Added a negative test: a message that just says "Throttled" without the code is not retried.
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 <noreply@anthropic.com>
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/analytics/graphql-error-codes.d.ts@@ -21,8 +21,8 @@ export declare 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 declare function isRateLimitCode(code: string | undefined): boolean;
/**
|
WHY are these changes introduced?
retryAwareRequestonly retries rate-limited requests on HTTP 429 or a GraphQL error withextensions.code === '429'. But Shopify GraphQL APIs (App Management among them) throttle with a 200 response andextensions.code: "THROTTLED"— those requests fail instantly on first rejection.The CLI already treats
THROTTLEDas a rate-limit signal in crash-report suppression and analytics grouping. The retry path was the only place that didn't.This is currently the top E2E failure mode (32 of 79 failed shards last week), and real
app dev/app deployusers hit the same instant failure.WHAT is this pull request doing?
The retryability check now reuses the shared
hasRateLimitCodehelper, soextensions.code === 'THROTTLED'is retried exactly like'429'. The code is server-set (seecomplexity_throttle.rbandapp_error_handling.rbin shop/world), so unlike message matching, user-controlled strings can't trigger retries. Retry mechanics are unchanged: up to 10 retries,Retry-Afterwhen present, 1s default backoff.How to test your changes?
api.test.tsadds two cases: a 200 +THROTTLEDerror is retried and succeeds; an error whose message merely says "Throttled" without the code is not retried.🤖 Generated with Claude Code