diff --git a/.changeset/network-errors-as-abort-errors.md b/.changeset/network-errors-as-abort-errors.md new file mode 100644 index 00000000000..ebdba6f5242 --- /dev/null +++ b/.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 diff --git a/packages/cli-kit/src/private/node/api.test.ts b/packages/cli-kit/src/private/node/api.test.ts index e5586b0ca67..776441dcd2b 100644 --- a/packages/cli-kit/src/private/node/api.test.ts +++ b/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) + }) }) diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index ea344592a0c..ee37dea7e4c 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -68,9 +68,20 @@ type VerboseResponse = | 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( requestOptions: RequestOptions, ): Promise { @@ -105,7 +146,7 @@ async function runRequestWithNetworkLevelRetry