Skip to content

Stop reporting gateway errors as CLI crashes - #8329

Merged
craigmichaelmartin merged 1 commit into
mainfrom
fix-500-reports
Sep 1, 2026
Merged

Stop reporting gateway errors as CLI crashes#8329
craigmichaelmartin merged 1 commit into
mainfrom
fix-500-reports

Conversation

@isaacroldan

@isaacroldan isaacroldan commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Raw graphql-request ClientError objects can reach the top-level reporter when a call site uses handleErrors: false. For HTTP 502, 503, and 504, that records an infrastructure failure as an unexpected CLI bug.

Classify these gateway statuses as expected in shouldReportErrorAsUnexpected, consistent with the handled path that maps 5xx responses to AbortError. HTTP 500 keeps its existing classification.

Testing

  • pnpm --filter @shopify/cli-kit vitest run src/public/node/error.test.ts
  • pnpm --filter @shopify/cli-kit type-check
  • ESLint on the two changed files

@github-actions github-actions Bot added cla-needed Area: @shopify/cli @shopify/cli package issues labels Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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: it is imported by both the request layer
 * (`../api.ts`) and the crash-report suppression logic (`../../public/node/error.ts`).
 * `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.

@isaacroldan isaacroldan changed the title Treat gateway 5xx responses as expected and retryable Stop reporting gateway errors as CLI crashes Aug 14, 2026
@github-actions github-actions Bot added no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. and removed Area: @shopify/cli @shopify/cli package issues labels Aug 14, 2026
@isaacroldan
isaacroldan marked this pull request as ready for review August 14, 2026 10:20
@isaacroldan
isaacroldan requested a review from a team as a code owner August 14, 2026 10:20
Assisted-By: devx/cfa7efa4-831b-4142-a418-5480cf27ac8c
@craigmichaelmartin

Copy link
Copy Markdown
Contributor

Reviewed in the context of resolving Vault 68247 / shop/issues#73647, which this PR targets. No merge-blocking issues — this looks correct and I'd land it.

Two things I verified rather than assumed:

  1. The new branch actually fires for the reported signature. isExpectedApiError only proceeds when the error carries both response and request, and then reads response.status. The real Observe payload for #73647 is {"response":{"error":"<html>…502 Bad Gateway…nginx…","status":502,"headers":{}},"request":{"query":"query getThemes…"}} — so response.status is present even though the body is HTML, and the structural match holds. Worth stating because an HTML-bodied 502 has no response.errors, so the pre-existing hasRateLimitCode fallback could never have caught it; the explicit status check is doing the work.

  2. Keeping 500 reportable is the right line, and the existing "genuine failure" assertion stays untouched. 502/503/504 come from the proxy in front of the API and say nothing about the CLI; a 500 means the API itself answered and failed.

Non-blocking notes:

  • This fixes the reporting half only. The issue is typed Resiliency and was auto-reopened on event volume, so after this lands the Observe signal goes quiet while the user still loses their command to a transient blip and still sees the raw nginx HTML. I've opened Retry gateway errors on idempotent GraphQL requests #8450 as a companion PR, restoring the retry half from this branch's own first push (commit 2edc7d0, before it was narrowed) — 502/503/504 become retryable, gated on requestIsIdempotent so a mutation is never resent. The two together actually close 68247, and they don't overlap: yours touches public/node/error.ts, mine touches the request layer.
  • The three statuses are inlined here. Retry gateway errors on idempotent GraphQL requests #8450 adds back your dependency-free private/node/api/gateway-status.ts with isGatewayErrorStatus(). Whichever of us lands second, it'd be worth collapsing this condition onto that helper so the gateway status list lives in one place rather than two.
  • Tiny doc-comment nit, not worth a re-push on its own: dropping "Two distinct cases" leaves 401 and rate-limiting still reading as a pair, with the gateway sentence appended to the rate-limiting paragraph.

For context on why it's worth unblocking: the error has been re-firing since this PR went up on Aug 14 — 37 events / 35 users in the last 7-day window on the theme path alone.

@craigmichaelmartin
craigmichaelmartin added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 7968811 Sep 1, 2026
124 of 133 checks passed
@craigmichaelmartin
craigmichaelmartin deleted the fix-500-reports branch September 1, 2026 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants