OUT-3544: Add retry mechanisms for transient network errors

src/app/api/core/utils/withErrorHandler.ts

@@ -13,7 +13,11 @@ import {
   isIntuitOAuthError,
 } from '@/app/api/core/exceptions/custom'
 import * as Sentry from '@sentry/nextjs'
-import { getMessageAndCodeFromError, RetryableError } from '@/utils/error'
+import {
+  getMessageAndCodeFromError,
+  HttpFetchError,
+  RetryableError,
+} from '@/utils/error'
 import { getCategory } from '@/utils/synclog'
 
 type RequestHandler = (req: NextRequest, params: any) => Promise<NextResponse>
@@ -72,6 +76,9 @@ export const withErrorHandler = (handler: RequestHandler): RequestHandler => {
       } else if (error instanceof RetryableError) {
         status = error.status
         message = error.message || message
+      } else if (error instanceof HttpFetchError) {
+        status = error.status
+        message = error.message || message
       } else if (isIntuitOAuthError(error)) {
         message = error.error
         status = httpStatus.BAD_REQUEST
@@ -91,6 +98,7 @@ export const withErrorHandler = (handler: RequestHandler): RequestHandler => {
       } else if (
         error instanceof APIError ||
         error instanceof CopilotApiError ||
+        error instanceof HttpFetchError ||
         isAxiosError(error) ||
         isIntuitOAuthError(error)
       ) {

src/app/api/core/utils/withRetry.ts

@@ -1,11 +1,122 @@
-import { StatusableError } from '@/type/CopilotApiError'
 import pRetry, { FailedAttemptError } from 'p-retry'
 import * as Sentry from '@sentry/nextjs'
 import { RetryableError } from '@/utils/error'
 
+// 429 means the server explicitly rejected the request without processing it,
+// so retrying is always safe regardless of idempotency.
+const ALWAYS_RETRY_STATUSES: ReadonlySet<number> = new Set([429])
+
+// 5xx and network/timeout errors straddle "did not commit" and "committed but
+// response dropped." Safe to replay on idempotent reads; on non-idempotent
+// writes lacking a server-side request-key primitive, a retry after the upstream
+// committed would duplicate the write — so the classifier returns false for
+// these in `idempotent: false` mode.
+const IDEMPOTENT_ONLY_RETRY_STATUSES: ReadonlySet<number> = new Set([
+  500, 502, 503, 504,
+])
+
+const RETRYABLE_NETWORK_CODES: ReadonlySet<string> = new Set([
+  'ECONNRESET',
+  'ECONNREFUSED',
+  'ETIMEDOUT',
+  'UND_ERR_CONNECT_TIMEOUT',
+])
+
+const RETRYABLE_ERROR_NAMES: ReadonlySet<string> = new Set([
+  'TimeoutError', // AbortSignal.timeout() rejection (Node 18+)
+  // 'AbortError' is intentionally excluded — it signals a deliberate
+  // AbortController.abort() and retrying would defeat the cancellation.
+])
+
+export type RetryOptions = {
+  /**
+   * False for non-idempotent writes (QBO create/update/void/delete) whose
+   * upstream has no request-key dedupe. In strict mode the classifier
+   * retries only on 429 and explicit `RetryableError.retry === true`;
+   * 5xx, network errors, and AbortSignal timeouts all bubble. Defaults
+   * to true (safe-to-replay reads).
+   */
+  idempotent?: boolean
+}
+
+/**
+ * Classifies whether an error should trigger a retry. 429 and explicit
+ * `RetryableError.retry === true` retry in both modes. Strict mode
+ * (`options.idempotent === false`) short-circuits past 429: 5xx, network
+ * codes, AbortSignal timeouts, and undici fetch-failed envelopes are all
+ * treated as possibly-after-commit and never replayed.
+ *
+ * Retry-nesting hazard: don't call a wrapped function from inside another.
+ * Inside `IntuitAPI._*` methods call the unwrapped `_*` counterparts
+ * (e.g. `this._customQuery`, not `this.customQuery`).
+ */
+export const isRetryableError = (
+  error: unknown,
+  options: RetryOptions = {},
+): boolean => {
+  const { idempotent = true } = options
+
+  if (error instanceof RetryableError) return error.retry
+
+  if (typeof error !== 'object' || error === null) return false
+
+  const err = error as {
+    status?: unknown
+    code?: unknown
+    name?: unknown
+    message?: unknown
+    cause?: unknown
+  }
+
+  if (typeof err.status === 'number' && ALWAYS_RETRY_STATUSES.has(err.status))
+    return true
+
+  // Strict mode: nothing past this point is post-commit-safe.
+  if (!idempotent) return false
+
+  if (
+    typeof err.status === 'number' &&
+    IDEMPOTENT_ONLY_RETRY_STATUSES.has(err.status)
+  )
+    return true
+
+  if (typeof err.name === 'string' && RETRYABLE_ERROR_NAMES.has(err.name))
+    return true
+
+  if (typeof err.code === 'string' && RETRYABLE_NETWORK_CODES.has(err.code))
+    return true
+
+  // Check `cause` BEFORE the generic 'fetch failed' message match so undici
+  // envelopes with a known-permanent sub-code (e.g. ENOTFOUND for DNS, ENETUNREACH)
+  // short-circuit to non-retryable instead of being retried 5× pointlessly.
+  if (err.cause && typeof err.cause === 'object') {
+    const cause = err.cause as { code?: unknown; name?: unknown }
+    if (
+      typeof cause.code === 'string' &&
+      RETRYABLE_NETWORK_CODES.has(cause.code)
+    )
+      return true
+    if (typeof cause.name === 'string' && RETRYABLE_ERROR_NAMES.has(cause.name))
+      return true
+    // cause is present with a recognized shape but not in either retry set →
+    // treat as permanent (DNS failure, cert error, etc.).
+    if (typeof cause.code === 'string' || typeof cause.name === 'string')
+      return false
+  }
+
+  // Fallback: undici wraps low-level network errors as `TypeError: fetch failed`.
+  // Only reach this branch when no usable cause was attached — treat as a
+  // generic transient network blip.
+  if (typeof err.message === 'string' && err.message === 'fetch failed')
+    return true
+
+  return false
+}
+
 export const withRetry = async <T>(
   fn: (...args: any[]) => Promise<T>,
   args: any[],
+  options: RetryOptions = {},
 ): Promise<T> => {
   let isEventProcessorRegistered = false
 
@@ -36,26 +147,23 @@ export const withRetry = async <T>(
     },
 
     {
-      retries: 3,
+      // Tuned so that maxTimeout actually engages on the final attempt.
+      // With minTimeout=500, factor=4, retries=4 the back-off sequence is
+      // 500ms / 2000ms / 8000ms / 10000ms (capped) = ~20.5s of waiting
+      // across 5 total attempts. Combined with the 30s fetch timeout, a
+      // single op caps at ~170s — well within the 300s webhook budget
+      // and gives the upstream a meaningful recovery window.
+      retries: 4,
       minTimeout: 500,
-      maxTimeout: 2000,
-      factor: 2, // Exponential factor for timeout delay. Tweak this if issues still persist
+      maxTimeout: 10_000,
+      factor: 4,
       onFailedAttempt: (error: FailedAttemptError) => {
         console.warn(
           `CopilotAPI#withRetry | Attempt ${error.attemptNumber} failed. There are ${error.retriesLeft} retries left. Error:`,
           error,
         )
       },
-      shouldRetry: (error: any) => {
-        if (error instanceof RetryableError) {
-          return error.retry
-        }
-
-        // Typecasting because Copilot doesn't export an error class
-        const err = error as StatusableError
-        // Retry only if statusCode === 429
-        return err.status === 429
-      },
+      shouldRetry: (error: unknown) => isRetryableError(error, options),
     },
   )
 }

src/config/index.ts

@@ -37,6 +37,22 @@ export const intuitBaseUrl =
 export const intuitApiMinorVersion =
   process.env.INTUIT_API_MINOR_VERSION || '75'
 
+// Default timeout (ms) for outbound fetches to external APIs (QuickBooks,
+// Copilot). Prevents requests from hanging indefinitely against a slow or
+// unresponsive upstream. Per-call overrides are supported by the fetch helpers.
+// Guarded against NaN / non-positive values so a malformed env var doesn't
+// crash `AbortSignal.timeout()` on every external call.
+const parsePositiveMs = (raw: string | undefined, fallback: number): number => {
+  const n = Number(raw)
+  return Number.isFinite(n) && n > 0 ? n : fallback
+}
+
+// Tune EXTERNAL_FETCH_TIMEOUT_MS down (floor ~15s, ceiling ~45s per withRetry budget) once Sentry shows real QBO/Copilot P99 latency.
+export const externalFetchTimeoutMs = parsePositiveMs(
+  process.env.EXTERNAL_FETCH_TIMEOUT_MS,
+  30_000,
+)
+
 // Supabase
 export const supabaseProjectUrl =
   process.env.NEXT_PUBLIC_SUPABASE_PROJECT_URL || ''

src/helper/fetch.helper.ts

@@ -1,21 +1,106 @@
+import { externalFetchTimeoutMs } from '@/config'
+import { HttpFetchError } from '@/utils/error'
+
+export type FetcherOptions = {
+  // number = use this timeout; null = disable timeout entirely (no AbortSignal
+  // attached); undefined = fall back to externalFetchTimeoutMs.
+  timeoutMs?: number | null
+}
+
+// Pulls a human-readable detail out of common upstream error shapes so the
+// thrown HttpFetchError surfaces *why* the call failed in `error.message`
+// (which is what gets written to qb_sync_logs). Without this, the message
+// degrades to a generic "HTTP 400 Bad Request from <url>" and the real
+// reason — e.g. Intuit's "Required param missing" — only lives in
+// `error.body`, which most downstream consumers don't inspect.
+const extractUpstreamDetail = (body: unknown): string | undefined => {
+  if (!body || typeof body !== 'object') return undefined
+  const b = body as Record<string, any>
+
+  // Intuit (QBO):
+  //   { Fault: { Error: [{ Message, Detail, code }], type: '...' } }
+  // Detail is usually the diagnostic; Message is a short label.
+  const intuitErr = b?.Fault?.Error?.[0]
+  if (intuitErr && typeof intuitErr === 'object') {
+    const parts = [intuitErr.Message, intuitErr.Detail]
+      .filter((p) => typeof p === 'string' && p.length > 0)
+      .join(' — ')
+    if (parts) return parts
+  }
+
+  // Copilot and many other JSON APIs:  { message: '...' }  or  { error: '...' }
+  if (typeof b.message === 'string' && b.message) return b.message
+  if (typeof b.error === 'string' && b.error) return b.error
+
+  return undefined
+}
+
+// Exported so other clients with their own fetch wrappers (e.g. CopilotAPI's
+// manualFetch) can produce consistently-shaped HttpFetchError instances
+// without duplicating the JSON/text body-parsing logic.
+export const buildHttpFetchError = async (
+  response: Response,
+  url: string,
+): Promise<HttpFetchError> => {
+  const rawBody = await response.text().catch(() => '')
+  let body: unknown = rawBody
+  if (rawBody) {
+    try {
+      body = JSON.parse(rawBody)
+    } catch {
+      // not JSON; keep raw text
+    }
+  }
+
+  // Prefer the upstream detail as the message when present — it's what
+  // qb_sync_logs.message and any user-facing surface actually want to show.
+  // The HTTP status lives on error.status / qb_sync_logs.code and the URL
+  // lives on error.url, so we don't lose them by omitting them here.
+  const detail = extractUpstreamDetail(body)
+  const message =
+    detail ?? `HTTP ${response.status} ${response.statusText || ''}`.trim()
+
+  return new HttpFetchError({
+    status: response.status,
+    statusText: response.statusText,
+    url,
+    body,
+    message,
+  })
+}
+
+const resolveSignal = (opts: FetcherOptions): AbortSignal | undefined => {
+  if (opts.timeoutMs === null) return undefined
+  return AbortSignal.timeout(opts.timeoutMs ?? externalFetchTimeoutMs)
+}
+
 export const postFetcher = async (
   url: string,
   headers: Record<string, string>,
   body: Record<string, any>,
+  opts: FetcherOptions = {},
 ) => {
   const response = await fetch(url, {
     method: 'POST',
     headers,
-    body: JSON.stringify(body), // body data type must match "Content-Type" header
+    body: JSON.stringify(body),
+    signal: resolveSignal(opts),
   })
 
+  if (!response.ok) throw await buildHttpFetchError(response, url)
   return response.json()
 }
 
 export const getFetcher = async (
   url: string,
   headers: Record<string, string>,
+  opts: FetcherOptions = {},
 ) => {
-  const response = await fetch(url, { headers })
+  const response = await fetch(url, {
+    headers,
+    signal: resolveSignal(opts),
+  })
+
+  if (!response.ok) throw await buildHttpFetchError(response, url)
   return response.json()
 }

src/helper/swr.helper.ts

@@ -1,7 +1,9 @@
 import { getFetcher } from '@/helper/fetch.helper'
 import useSWR, { SWRConfiguration } from 'swr'
 
-const fetcher = (url: string) => getFetcher(url, {})
+// In-app SWR fetches against this app's own routes — no client-side timeout.
+// SWR handles its own error-retry; aborts here would just produce false negatives.
+const fetcher = (url: string) => getFetcher(url, {}, { timeoutMs: null })
 
 export const useSwrHelper = (key: any, opts: SWRConfiguration = {}) =>
   useSWR(key, fetcher, {

src/hook/useQuickbooks.ts

@@ -95,7 +95,7 @@
     return () => {
       supabase.removeChannel(realtimeSyncChannel)
     }
   }, [])
 
   // handle reconnect logic
   useEffect(() => {
@@ -107,7 +107,7 @@
     if (reconnect) {
       handleAppConnect()
     }
   }, [reconnect])
 
   const getAuthUrl = async (type?: string) => {
     setLoading(true)
@@ -289,7 +289,11 @@
       enable: false,
     }
     const url = `/api/quickbooks/token/change-enable-status?token=${token}`
-    await postFetcher(url, {}, payload)
+    try {
+      await postFetcher(url, {}, payload, { timeoutMs: null })
+    } catch (err) {
+      console.error('Error disconnecting QuickBooks account', err)
+    }
   }
 
   const downloadCsvAction = async () => {