narrow abort pattern and harden isNetworkError

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

src/lib/network-error.test.ts

@@ -1,20 +1,46 @@
+import {getMain, XrpcInternalError} from '@atproto/lex'
 import {describe, expect, test} from '@jest/globals'
 
+import {AbortError} from '#/lib/async/cancelable'
 import {isNetworkError} from '#/lib/network-error'
+import {com} from '#/lexicons'
+
+const method = getMain(com.atproto.server.describeServer)
 
 describe('isNetworkError', () => {
   test.each([
     'Network request failed',
     'TypeError: Failed to fetch',
     'Error: fetch failed: java.net.UnknownHostException',
+    'Load failed',
+    'Error: Upstream service unreachable',
+    'NetworkError when attempting to fetch resource',
     'The Internet connection appears to be offline',
     'The network connection was lost',
+    'Unable to resolve host "bsky.social": No address associated with hostname',
     'A server with the specified hostname could not be found',
     'TypeError: Network request timed out',
+    'ConnectException: Failed to connect to bsky.social/1.2.3.4:443',
+    'SSLHandshakeException: Connection closed by peer',
   ])('detects %s', message => {
     expect(isNetworkError(new Error(message))).toBe(true)
   })
 
+  test('detects a cancelled request', () => {
+    expect(isNetworkError(new AbortError())).toBe(true)
+  })
+
+  test('detects a stringified web abort', () => {
+    expect(isNetworkError('AbortError: The user aborted a request.')).toBe(true)
+  })
+
+  test.each([
+    'MultipartUploadError: Multipart upload aborted',
+    'TypeError: undefined is not an object (evaluating abortController.abort)',
+  ])('does not treat a lowercase abort as a network error: %s', message => {
+    expect(isNetworkError(message)).toBe(false)
+  })
+
   test('checks wrapped error causes', () => {
     const error = new Error('Unable to fulfill XRPC request', {
       cause: new Error('fetch failed: connection closed'),
@@ -23,6 +49,23 @@ describe('isNetworkError', () => {
     expect(isNetworkError(error)).toBe(true)
   })
 
+  test('checks the cause of a wrapper that does not embed it', () => {
+    const error = new XrpcInternalError(method, undefined, {
+      cause: new Error('fetch failed'),
+    })
+
+    expect(error.message).not.toContain('fetch failed')
+    expect(isNetworkError(error)).toBe(true)
+  })
+
+  test('keeps a wrapper whose cause is not a network error', () => {
+    const error = new XrpcInternalError(method, undefined, {
+      cause: new TypeError('URL.canParse is not a function'),
+    })
+
+    expect(isNetworkError(error)).toBe(false)
+  })
+
   test('does not treat arbitrary fetch-handler failures as network errors', () => {
     expect(
       isNetworkError(
@@ -36,4 +79,28 @@ describe('isNetworkError', () => {
   test('does not match words ending in load', () => {
     expect(isNetworkError(new Error('Multipart upload failed'))).toBe(false)
   })
+
+  test('terminates on a cycle of causes', () => {
+    const a = new Error('first') as Error & {cause?: unknown}
+    const b = new Error('second') as Error & {cause?: unknown}
+    a.cause = b
+    b.cause = a
+
+    expect(isNetworkError(a)).toBe(false)
+  })
+
+  test.each([
+    ['a null-prototype object', () => Object.create(null)],
+    [
+      'a value whose toString throws',
+      () => ({
+        toString() {
+          throw new Error('nope')
+        },
+      }),
+    ],
+  ])('returns false without throwing for %s', (_label, build) => {
+    expect(() => isNetworkError(build())).not.toThrow()
+    expect(isNetworkError(build())).toBe(false)
+  })
 })

src/lib/network-error.ts

@@ -1,5 +1,12 @@
 const NETWORK_ERROR_PATTERNS = [
-  /\babort(?:ed|error)?\b/i,
+  /*
+   * Case-sensitive on purpose. `AbortError` (web `DOMException`) and the
+   * `Aborted` message thrown by `#/lib/async/cancelable` are transport
+   * cancellations, but the lowercase word shows up in unrelated failures - a
+   * multipart upload the service marked `aborted`, or a `TypeError` naming
+   * `abortController.abort` - which must still be reported.
+   */
+  /\bAbort(?:ed|Error)?\b/,
   /network request failed/i,
   /failed to fetch/i,
   /fetch failed/i,
@@ -16,24 +23,64 @@ const NETWORK_ERROR_PATTERNS = [
   /sslhandshakeexception: connection closed/i,
 ]
 
+/**
+ * `String(value)` runs code we do not control - a custom `toString`, a
+ * `Symbol.toPrimitive`, a `Proxy` trap - and throws outright for a
+ * null-prototype object, so stringifying an arbitrary thrown value can itself
+ * throw. Returning `undefined` lets callers treat an unreadable value as "not a
+ * network error" rather than letting the throw escape `logger.error()` or
+ * Sentry's `beforeSend`, where it would lose the original report.
+ */
+export function safeStringify(value: unknown): string | undefined {
+  try {
+    // oxlint-disable-next-line typescript/no-base-to-string
+    return String(value)
+  } catch {
+    return undefined
+  }
+}
+
 /**
  * Detects transport failures across the error strings produced by web, native
  * fetch, and the XRPC clients. Error causes are checked because the XRPC
  * clients wrap the platform-specific fetch error.
+ *
+ * Never throws: reading `cause` off a hostile value can throw just like
+ * stringifying it can, and a throw here would escape whatever error path is
+ * asking the question.
  */
 export function isNetworkError(value: unknown): boolean {
-  return isNetworkErrorInner(value, new Set())
+  try {
+    return isNetworkErrorInner(value, undefined)
+  } catch {
+    return false
+  }
 }
 
-function isNetworkErrorInner(value: unknown, seen: Set<object>): boolean {
-  if (NETWORK_ERROR_PATTERNS.some(pattern => pattern.test(String(value)))) {
+/**
+ * `seen` stays undefined until the first recursion into an object cause, so the
+ * common case (a string or an error with no cause) allocates nothing.
+ */
+function isNetworkErrorInner(
+  value: unknown,
+  seen: Set<object> | undefined,
+): boolean {
+  const message = safeStringify(value)
+  if (
+    message !== undefined &&
+    NETWORK_ERROR_PATTERNS.some(pattern => pattern.test(message))
+  ) {
     return true
   }
 
-  if (typeof value !== 'object' || value === null || seen.has(value)) {
+  if (typeof value !== 'object' || value === null || seen?.has(value)) {
+    return false
+  }
+  if (!('cause' in value)) {
     return false
   }
 
-  seen.add(value)
-  return 'cause' in value && isNetworkErrorInner(value.cause, seen)
+  const nextSeen = seen ?? new Set<object>()
+  nextSeen.add(value)
+  return isNetworkErrorInner(value.cause, nextSeen)
 }

src/lib/strings/errors.ts

@@ -39,7 +39,12 @@ export function cleanError(e: unknown): string {
    */
   // oxlint-disable-next-line typescript/no-base-to-string
   const str = typeof e === 'string' ? e : e.toString()
-  if (isNetworkError(str)) {
+  /*
+   * Passed the original value, not `str`, so the wrapped-cause walk is
+   * reachable: the XRPC clients report the platform fetch failure as the
+   * `cause` of a generic message.
+   */
+  if (isNetworkError(e)) {
     return t`Unable to connect. Please check your internet connection and try again.`
   }
   /*