Merge pull request #6244 from Shopify/make-retry-errors-resilient

Harden network retry classification

Author: EvilGenius13

.changeset/swift-nails-battle.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Add more retryable errors and improve consistency of identifying retryable conditions

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

@@ -198,4 +198,148 @@ describe('retryAwareRequest', () => {
 
     await expect(networkRetryDisabled).rejects.toThrowError('ENOTFOUND')
   })
+
+  test('retries when request is aborted by client (AbortError message)', async () => {
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new Error('the operation was aborted')
+      })
+      .mockImplementationOnce(() => {
+        return Promise.resolve({
+          status: 200,
+          data: {ok: true},
+          headers: new Headers(),
+        })
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com/graphql.json',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 2000,
+      },
+      undefined,
+      {
+        defaultDelayMs: 10,
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {ok: true},
+    })
+    expect(mockRequestFn).toHaveBeenCalledTimes(2)
+  })
+
+  test('retries when fetch wrapper has blank reason in message', async () => {
+    const blankReasonMessage = 'request to https://example.com/admin/api/unstable/graphql.json failed, reason:'
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new Error(blankReasonMessage)
+      })
+      .mockImplementationOnce(() => {
+        return Promise.resolve({
+          status: 200,
+          data: {ok: true},
+          headers: new Headers(),
+        })
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn, _delay) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com/graphql.json',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 2000,
+      },
+      undefined,
+      {
+        defaultDelayMs: 10,
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {ok: true},
+    })
+    expect(mockRequestFn).toHaveBeenCalledTimes(2)
+  })
+
+  test('retries when blank reason contains trailing whitespace/newlines', async () => {
+    const blankReasonWithWhitespace =
+      'request to https://example.com/admin/api/unstable/graphql.json failed, reason:   \n\t'
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new Error(blankReasonWithWhitespace)
+      })
+      .mockImplementationOnce(() => {
+        return Promise.resolve({
+          status: 200,
+          data: {ok: true},
+          headers: new Headers(),
+        })
+      })
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://example.com/graphql.json',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 2000,
+      },
+      undefined,
+      {defaultDelayMs: 10, scheduleDelay: (fn) => fn()},
+    )
+
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {ok: true},
+    })
+    expect(mockRequestFn).toHaveBeenCalledTimes(2)
+  })
+
+  test('does not treat non-blank reason as retryable when no known patterns match', async () => {
+    vi.useRealTimers()
+    const nonBlankUnknownReason =
+      'request to https://example.com/admin/api/unstable/graphql.json failed, reason: gateway policy'
+
+    const mockRequestFn = vi.fn().mockImplementationOnce(() => {
+      throw new Error(nonBlankUnknownReason)
+    })
+
+    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(nonBlankUnknownReason)
+    expect(mockRequestFn).toHaveBeenCalledTimes(1)
+  })
 })

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

@@ -72,18 +72,21 @@ function isARetryableNetworkError(error: unknown): boolean {
   if (error instanceof Error) {
     const networkErrorMessages = [
       'socket hang up',
-      'ECONNRESET',
-      'ECONNABORTED',
-      'ENOTFOUND',
-      'ENETUNREACH',
+      'econnreset',
+      'econnaborted',
+      'enotfound',
+      'enetunreach',
       'network socket disconnected',
-      'ETIMEDOUT',
-      'ECONNREFUSED',
-      'EAI_FAIL',
-      'The operation was aborted.',
+      'etimedout',
+      'econnrefused',
+      'eai_again',
+      'epipe',
+      'the operation was aborted',
     ]
-    const anyMatches = networkErrorMessages.some((issueMessage) => error.message.includes(issueMessage))
-    return anyMatches
+    const errorMessage = error.message.toLowerCase()
+    const anyMatches = networkErrorMessages.some((issueMessage) => errorMessage.includes(issueMessage))
+    const missingReason = /^request to .* failed, reason:\s*$/.test(errorMessage)
+    return anyMatches || missingReason
   }
   return false
 }

packages/cli-kit/src/public/node/vendor/otel-js/service/BaseOtelService/BaseOtelService.ts

@@ -136,7 +136,8 @@ export class BaseOtelService implements OtelService {
           instrument.add(finalValue, finalLabels)
         }
         // We flush metrics after every record - we do not await as we fire & forget.
-        this.meterProvider.forceFlush({})
+        // Catch any export errors to prevent unhandled rejections from crashing the CLI
+        void this.meterProvider.forceFlush({}).catch(() => {})
       }
       record(firstValue, firstLabels)
       this.metrics.set(metricName, record)