Merge pull request #6557 from Shopify/treat-network-errors-as-handled

Treat transient network errors as AbortError instead of BugError

Author: amcaplan

.changeset/network-errors-as-abort-errors.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Treat all network errors as user-facing AbortError instead of BugError in fetchApiVersions

packages/cli-kit/src/private/node/api.test.ts

@@ -1,4 +1,4 @@
-import {retryAwareRequest} from './api.js'
+import {retryAwareRequest, isNetworkError, isTransientNetworkError} from './api.js'
 import {ClientError} from 'graphql-request'
 import {describe, test, vi, expect, beforeEach, afterEach} from 'vitest'
 
@@ -342,4 +342,153 @@ describe('retryAwareRequest', () => {
     await expect(result).rejects.toThrowError(nonBlankUnknownReason)
     expect(mockRequestFn).toHaveBeenCalledTimes(1)
   })
+
+  test('does not retry certificate/TLS/SSL errors (permanent network errors)', async () => {
+    vi.useRealTimers()
+    const certificateErrors = [
+      'certificate has expired',
+      "Hostname/IP does not match certificate's altnames",
+      'TLS handshake failed',
+      'SSL certificate problem: unable to get local issuer certificate',
+    ]
+
+    await Promise.all(
+      certificateErrors.map(async (certError) => {
+        const mockRequestFn = vi.fn().mockImplementation(() => {
+          throw new Error(certError)
+        })
+
+        const result = retryAwareRequest(
+          {
+            request: mockRequestFn,
+            url: 'https://example.com/graphql.json',
+            useNetworkLevelRetry: true,
+            maxRetryTimeMs: 2000,
+          },
+          undefined,
+          {defaultDelayMs: 10, scheduleDelay: (fn) => fn()},
+        )
+
+        await expect(result).rejects.toThrowError(certError)
+        expect(mockRequestFn).toHaveBeenCalledTimes(1)
+      }),
+    )
+  })
+})
+
+describe('isTransientNetworkError', () => {
+  test('identifies transient network errors that should be retried', () => {
+    const transientErrors = [
+      'socket hang up',
+      'ECONNRESET',
+      'ECONNABORTED',
+      'ENOTFOUND',
+      'ENETUNREACH',
+      'network socket disconnected',
+      'ETIMEDOUT',
+      'ECONNREFUSED',
+      'EAI_AGAIN',
+      'EPIPE',
+      'the operation was aborted',
+      'timeout occurred',
+      'premature close',
+      'getaddrinfo ENOTFOUND',
+    ]
+
+    for (const errorMsg of transientErrors) {
+      expect(isTransientNetworkError(new Error(errorMsg))).toBe(true)
+    }
+  })
+
+  test('identifies blank reason network errors', () => {
+    const blankReasonErrors = [
+      'request to https://example.com failed, reason:',
+      'request to https://example.com failed, reason:   ',
+      'request to https://example.com failed, reason:\n\t',
+    ]
+
+    for (const errorMsg of blankReasonErrors) {
+      expect(isTransientNetworkError(new Error(errorMsg))).toBe(true)
+    }
+  })
+
+  test('does not identify certificate errors as transient (should not be retried)', () => {
+    const permanentErrors = [
+      'certificate has expired',
+      'cert verification failed',
+      'TLS handshake failed',
+      'SSL certificate problem',
+      "Hostname/IP does not match certificate's altnames",
+    ]
+
+    for (const errorMsg of permanentErrors) {
+      expect(isTransientNetworkError(new Error(errorMsg))).toBe(false)
+    }
+  })
+
+  test('does not identify non-network errors as transient', () => {
+    const nonNetworkErrors = [
+      'Invalid JSON',
+      'Syntax error',
+      'undefined is not a function',
+      'request failed with status 500',
+    ]
+
+    for (const errorMsg of nonNetworkErrors) {
+      expect(isTransientNetworkError(new Error(errorMsg))).toBe(false)
+    }
+  })
+
+  test('returns false for non-Error objects', () => {
+    expect(isTransientNetworkError('string error')).toBe(false)
+    expect(isTransientNetworkError(null)).toBe(false)
+    expect(isTransientNetworkError(undefined)).toBe(false)
+    expect(isTransientNetworkError({message: 'ENOTFOUND'})).toBe(false)
+  })
+})
+
+describe('isNetworkError', () => {
+  test('identifies all transient network errors', () => {
+    const transientErrors = ['ECONNRESET', 'ETIMEDOUT', 'ENOTFOUND', 'socket hang up', 'premature close']
+
+    for (const errorMsg of transientErrors) {
+      expect(isNetworkError(new Error(errorMsg))).toBe(true)
+    }
+  })
+
+  test('identifies permanent network errors (certificate/TLS/SSL)', () => {
+    const permanentErrors = [
+      'certificate has expired',
+      'cert verification failed',
+      'TLS handshake failed',
+      'SSL certificate problem',
+      "Hostname/IP does not match certificate's altnames",
+      'unable to verify the first certificate',
+      'self signed certificate in certificate chain',
+    ]
+
+    for (const errorMsg of permanentErrors) {
+      expect(isNetworkError(new Error(errorMsg))).toBe(true)
+    }
+  })
+
+  test('does not identify non-network errors', () => {
+    const nonNetworkErrors = [
+      'Invalid JSON',
+      'Syntax error',
+      'undefined is not a function',
+      'request failed with status 500',
+    ]
+
+    for (const errorMsg of nonNetworkErrors) {
+      expect(isNetworkError(new Error(errorMsg))).toBe(false)
+    }
+  })
+
+  test('returns false for non-Error objects', () => {
+    expect(isNetworkError('string error')).toBe(false)
+    expect(isNetworkError(null)).toBe(false)
+    expect(isNetworkError(undefined)).toBe(false)
+    expect(isNetworkError({message: 'certificate error'})).toBe(false)
+  })
 })

packages/cli-kit/src/private/node/api.ts

@@ -68,9 +68,20 @@ type VerboseResponse<T> =
   | CanRetryErrorResponse
   | UnauthorizedErrorResponse
 
-function isARetryableNetworkError(error: unknown): boolean {
+/**
+ * Checks if an error is a transient network error that is likely to recover with retries.
+ *
+ * Use this function for retry logic. Use isNetworkError for error classification.
+ *
+ * Examples of transient errors (worth retrying):
+ * - Connection timeouts, resets, and aborts
+ * - DNS failures (enotfound, getaddrinfo, eai_again) - can be temporary
+ * - Socket disconnects and hang ups
+ * - Premature connection closes
+ */
+export function isTransientNetworkError(error: unknown): boolean {
   if (error instanceof Error) {
-    const networkErrorMessages = [
+    const transientErrorMessages = [
       'socket hang up',
       'econnreset',
       'econnaborted',
@@ -82,15 +93,45 @@ function isARetryableNetworkError(error: unknown): boolean {
       'eai_again',
       'epipe',
       'the operation was aborted',
+      'timeout',
+      'premature close',
+      'getaddrinfo',
     ]
     const errorMessage = error.message.toLowerCase()
-    const anyMatches = networkErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage))
+    const anyMatches = transientErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage))
     const missingReason = /^request to .* failed, reason:\s*$/.test(errorMessage)
     return anyMatches || missingReason
   }
   return false
 }
 
+/**
+ * Checks if an error is any kind of network-related error (connection issues, timeouts, DNS failures,
+ * TLS/certificate errors, etc.) rather than an application logic error.
+ *
+ * These errors should be reported as user-facing errors (AbortError) rather than bugs (BugError),
+ * regardless of whether they are transient or permanent.
+ *
+ * Examples include:
+ * - Transient: connection timeouts, socket hang ups, temporary DNS failures
+ * - Permanent: certificate validation failures, misconfigured SSL
+ */
+export function isNetworkError(error: unknown): boolean {
+  // First check if it's a transient network error
+  if (isTransientNetworkError(error)) {
+    return true
+  }
+
+  // Then check for permanent network errors (SSL/TLS/certificate issues)
+  if (error instanceof Error) {
+    const permanentNetworkErrorMessages = ['certificate', 'cert', 'tls', 'ssl', 'altnames']
+    const errorMessage = error.message.toLowerCase()
+    return permanentNetworkErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage))
+  }
+
+  return false
+}
+
 async function runRequestWithNetworkLevelRetry<T extends {headers: Headers; status: number}>(
   requestOptions: RequestOptions<T>,
 ): Promise<T> {
@@ -105,7 +146,7 @@ async function runRequestWithNetworkLevelRetry<T extends {headers: Headers; stat
       return await requestOptions.request()
     } catch (err) {
       lastSeenError = err
-      if (!isARetryableNetworkError(err)) {
+      if (!isTransientNetworkError(err)) {
         throw err
       }
       outputDebug(`Retrying request to ${requestOptions.url} due to network error ${err}`)

packages/cli-kit/src/public/node/api/admin.ts

@@ -14,6 +14,7 @@ import {
   restRequestUrl,
   isThemeAccessSession,
 } from '../../../private/node/api/rest.js'
+import {isNetworkError} from '../../../private/node/api.js'
 import {RequestModeInput, shopifyFetch} from '../http.js'
 import {PublicApiVersions} from '../../../cli/api/graphql/admin/generated/public_api_versions.js'
 
@@ -182,13 +183,26 @@ async function fetchApiVersions(session: AdminSession, preferredBehaviour?: Requ
       throw new AbortError(
         `Error connecting to your store ${session.storeFqdn}: ${error.message} ${error.response.status} ${error.response.data}`,
       )
-    } else {
-      throw new BugError(
-        `Unknown error connecting to your store ${session.storeFqdn}: ${
+    }
+
+    // Check for network-level errors (connection issues, timeouts, DNS failures, TLS/certificate errors, etc.)
+    // All network errors should be treated as user-facing errors, not CLI bugs
+    // Note: Some of these may have been retried already by lower-level retry logic
+    if (isNetworkError(error)) {
+      throw new AbortError(
+        `Network error connecting to your store ${session.storeFqdn}: ${
           error instanceof Error ? error.message : String(error)
         }`,
+        'Check your internet connection and try again.',
       )
     }
+
+    // Unknown errors are likely bugs in the CLI
+    throw new BugError(
+      `Unknown error connecting to your store ${session.storeFqdn}: ${
+        error instanceof Error ? error.message : String(error)
+      }`,
+    )
   }
 }