Retry gateway errors on idempotent GraphQL requests

[craigmichaelmartin]

Resolves the retry half of Vault 68247 / shop/issues#73647 (Observe).

A 502/503/504 from the proxy in front of a Shopify API was classified as a terminal client-error in retryAwareRequest and thrown on the first attempt, so a single gateway blip killed the whole command. The issue was auto-reopened on event volume: 35 affected users in 7 days on getThemes alone, plus a matching signature on the store execute path (shop/issues#73646).

This restores the retry work from the first push of #8329 (commit 2edc7d0, force-pushed away when that PR was narrowed to reporting only), rebased onto current main, plus one additional fix described below.

Retry gateway statuses, gated on idempotency

graphqlRequest/graphqlRequestDoc mark a document idempotent when it holds exactly one query operation, so this path never resends a mutation. That gate is what makes retrying safe at all — a 502 can mean the upstream processed the request and only the response was lost, and 504 in particular means the gateway did forward the request to an origin that may have been mid-write. A malformed query is treated as non-idempotent rather than throwing, so the API keeps rejecting it exactly where it does today.

Gateway errors get their own budget of 3, not the default 10: unlike a 429 they carry no instruction about when to come back, and an upstream that is already failing should not be hit ten more times on its way down. It's a separate counter rather than an override of limitRetriesTo, so an explicit caller cap still composes.

HTTP 500 is deliberately excluded — the API itself answered and failed, so it keeps its existing non-retryable, reportable behaviour, and the existing test asserting that is untouched.

Second fix: the network-level retry was matching mutation payload text

While verifying the above I found a pre-existing hole that undermines the guarantee. runRequestWithNetworkLevelRetry does see the thrown ClientError and tests it with isTransientNetworkError, which searches error.message — and a ClientError's message embeds JSON.stringify({response, request}), so the request's own query and variables are part of the searched string:

request	502 outcome before this PR
getThemes query	not retried
ThemeFilesUpsert with plain Liquid	not retried
ThemeFilesUpsert with an asset containing setTimeout	retried, non-idempotently

A theme asset containing setTimeout matches 'timeout', so a 502 on the ThemeFilesUpsert mutation retried purely because of the file's contents — risking duplicate file writes. A ClientError means a response came back, so it is never a connection-level failure and must not be message-matched; retryable statuses are now classified deliberately where idempotency is known. The regression test pins it: with the guard removed it fails, re-POSTing the mutation until the retry window expires (5s).

This is scoped to the retry loop only — isTransientNetworkError itself is unchanged, so isNetworkError classification in public/node/api/admin.ts keeps its current behaviour.

Incidental latent fix

Hoisting the Retry-After parse out of the throttling branch also fixes a bug on that path: the old inline Number.parseInt yields NaN for the HTTP-date form of the header, and ?? does not treat NaN as absent, so NaN reached setTimeout and fired immediately — turning a 429 with a date-form Retry-After into 10 back-to-back retries with no delay. It's now normalised to undefined so the default delay applies.

Relationship to #8329

Complementary, no file overlap — #8329 changes public/node/error.ts (stop reporting gateway errors as crashes), this changes the request layer. Either can land first. Once both are in, #8329's inlined status === 502 || 503 || 504 should collapse onto the shared isGatewayErrorStatus helper added here, so the status list lives in one place.

Testing

• pnpm --filter @shopify/cli-kit vitest run src/private/node/api.test.ts src/public/node/api/graphql.test.ts — 52 passing, including 6 new retry tests (recovery, budget exhaustion, retry-after, non-idempotent, 500, payload-text regression) and 5 new idempotency-marking tests.
• Full @shopify/cli-kit suite: 1876 passing. The 2 failures in hooks/deprecations.test.ts are pre-existing on clean main (hardcoded "December 31, 2025" expiry), unrelated to this change.
• pnpm nx run-many --all --target=type-check (10 projects), --target=lint (14 projects), node bin/run-knip-ci.js, and pnpm refresh-code-documentation (tree stays clean) all pass.
• Verified end-to-end against a stubbed transport returning a real nginx 502 body: the getThemes query from the Observe signature succeeds on the second attempt; an equivalent mutation is not retried.

Known, not changed here

retryDelayMsFromHeaders treats Retry-After as milliseconds, but the header is defined in seconds — so the CLI waits 1000× too little. That's pre-existing on the 429 path (and pinned by an existing test asserting 200 → 200ms), so correcting the unit would change existing rate-limit timing and seemed worth raising separately rather than smuggling in here.

[github-actions]

Differences in type declarations

We 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:

• Some seemingly private modules might be re-exported through public modules.
• If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/private/node/api/gateway-status.d.ts

/**
 * Pure helpers for classifying HTTP statuses that come from the gateway in front of an API rather
 * than from the API itself.
 *
 * This module is intentionally dependency-free so that both the request layer (`../api.ts`) and the
 * crash-report suppression logic in `../../public/node/error.ts` can share it. `error.ts` cannot
 * import `../api.ts` directly — that would pull `graphql-request` into the module graph of every
 * command, and `api.ts → headers.ts → error.ts` is already a cycle — so the shared status logic
 * lives here, where it imports nothing from cli-kit.
 */
/**
 * Whether an HTTP status indicates a gateway-level failure in front of the API.
 *
 * @param status - The HTTP status of the response, if known.
 * @returns True when the status is 502, 503 or 504.
 */
export declare function isGatewayErrorStatus(status: number | undefined): boolean;

Existing type declarations

packages/cli-kit/dist/private/node/api.d.ts

@@ -12,6 +12,7 @@ export type NetworkRetryBehaviour = {
 type RequestOptions<T> = {
     request: () => Promise<T>;
     url: string;
+    requestIsIdempotent?: boolean;
 } & NetworkRetryBehaviour;
 /**
  * Checks if an error is a transient network error that is likely to recover with retries.