fix(OUT-3544): strict retry classifier for non-idempotent write paths

The OUT-3544 broadening of the retry classifier (429 → 429 + 5xx + network)
applies uniformly to every wrapped call, including QBO write endpoints
(`createInvoice`, `createCustomer`, `createItem`, `createPayment`,
`createAccount`, `createPurchase`) which have no Intuit-side request-key
dedupe. A 5xx or network error after the upstream commits could cause
pRetry to replay the write and produce a duplicate financial record.

Adds an `idempotent` option to `withRetry` / `isRetryableError`. In strict
mode (`idempotent: false`), the classifier retries only on 429 and explicit
`RetryableError.retry === true`; 5xx, network codes, and AbortSignal
timeouts are all treated as possibly-after-commit and never replayed.

Inverts the convention in both QBO-facing wrappers:

- `IntuitAPI.wrapWithRetry` defaults to strict; `customQuery` (the only
  read in the wrapped set) opts back into broad retry. All 13 wrapped
  writes (create/update/void/delete) inherit strict by default, so any
  future write method added without options is automatically safe.
- `Intuit.wrapWithRetry` (OAuth) also defaults to strict. `createToken`
  and `refreshAccessToken` consume single-use credentials and would
  previously misdiagnose a 5xx-after-commit refresh as `invalid_grant`
  via `tokenRefresh.handleInvalidGrant`, throwing `QBReconnectRequiredError`
  for a healthy connection. Strict mode lets the transport error surface
  honestly.

Both wrappers merge `{ idempotent: false, ...options }` rather than
relying on a parameter default, so an explicitly-passed `{}` cannot
silently flip back to the global broad-retry default.

Out of scope (not addressed here):
- The same after-commit-with-dropped-response can still produce a
  duplicate when the 3-hour resync re-runs the write. Eliminating that
  requires QBO's `requestid` query parameter on POST endpoints — a
  separate ticket.
- `copilotAPI.ts` / `authenticate.ts` retain the global `idempotent: true`
  default. None of those calls are QBO writes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: SandipBajracharya

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

@@ -2,12 +2,17 @@ import pRetry, { FailedAttemptError } from 'p-retry'
 import * as Sentry from '@sentry/nextjs'
 import { RetryableError } from '@/utils/error'
 
-const RETRYABLE_HTTP_STATUSES: ReadonlySet<number> = new Set([
-  429, // rate limit
-  500, // internal server error (often transient at QBO)
-  502, // bad gateway (upstream proxy hiccup)
-  503, // service unavailable
-  504, // gateway timeout
+// 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([
@@ -23,36 +28,34 @@ const RETRYABLE_ERROR_NAMES: ReadonlySet<string> = new Set([
   // 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
+}
+
 /**
- * Centralized classifier for whether an error should trigger a retry.
- * Exported so it can be unit-tested independent of pRetry's timer plumbing.
- *
- * The error shapes inspected here come from different layers; a single
- * unified type doesn't exist, which is why the input is `unknown` and
- * each field is checked defensively:
+ * 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.
  *
- *   - `RetryableError` (ours, src/utils/error.ts) — explicit retry flag.
- *   - `HttpFetchError` (ours) and Copilot SDK's `StatusableError` —
- *     `status: number` set after a non-2xx response was received.
- *   - undici (Node fetch) network failures — thrown as
- *     `TypeError: fetch failed` with the underlying error on `.cause`
- *     (e.g. `{ code: 'ECONNRESET' }`). No HTTP response was ever built,
- *     so there is no status to inspect.
- *   - `AbortSignal.timeout()` rejects with a `DOMException` whose
- *     `name` is `'TimeoutError'` (retryable). `AbortController.abort()`
- *     produces `'AbortError'` and is NOT retried (deliberate cancellation).
- *   - Top-level `error.code` is checked defensively for legacy Node
- *     error paths; in current Node fetch the code lives under `.cause`.
- *
- * Retry-nesting hazard: do not call a `withRetry`-wrapped function from
- * inside another `withRetry`-wrapped function. With the broadened retry
- * set, worst-case wait is `outer × inner × per_call_timeout`, which can
- * blow past the 300s webhook execution budget. Inside `IntuitAPI._*`
- * methods that are themselves wrapped at the public level (see exports
- * at the bottom of `src/utils/intuitAPI.ts`), call the unwrapped `_*`
- * counterparts directly (e.g. `this._customQuery`, not `this.customQuery`).
+ * 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): boolean => {
+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
@@ -65,7 +68,16 @@ export const isRetryableError = (error: unknown): boolean => {
     cause?: unknown
   }
 
-  if (typeof err.status === 'number' && RETRYABLE_HTTP_STATUSES.has(err.status))
+  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))
@@ -104,6 +116,7 @@ export const isRetryableError = (error: unknown): boolean => {
 export const withRetry = async <T>(
   fn: (...args: any[]) => Promise<T>,
   args: any[],
+  options: RetryOptions = {},
 ): Promise<T> => {
   let isEventProcessorRegistered = false
 
@@ -150,7 +163,7 @@ export const withRetry = async <T>(
           error,
         )
       },
-      shouldRetry: (error: unknown) => isRetryableError(error),
+      shouldRetry: (error: unknown) => isRetryableError(error, options),
     },
   )
 }

src/utils/intuit.ts

@@ -1,5 +1,5 @@
 import { IntuitOAuthError } from '@/app/api/core/exceptions/custom'
-import { withRetry } from '@/app/api/core/utils/withRetry'
+import { RetryOptions, withRetry } from '@/app/api/core/utils/withRetry'
 import {
   intuitClientId,
   intuitClientSecret,
@@ -79,10 +79,16 @@ export default class Intuit {
     return tokenInfo
   }
 
+  // `createToken` / `refreshAccessToken` consume single-use credentials with
+  // no Intuit-side dedupe; a post-commit retry sees `invalid_grant` and
+  // would be misdiagnosed as revocation by `tokenRefresh.handleInvalidGrant`.
+  // So the wrapper defaults to strict, same as `IntuitAPI.wrapWithRetry`.
   private wrapWithRetry<Args extends unknown[], R>(
     fn: (...args: Args) => Promise<R>,
+    options?: RetryOptions,
   ): (...args: Args) => Promise<R> {
-    return (...args: Args): Promise<R> => withRetry(fn.bind(this), args)
+    return (...args: Args): Promise<R> =>
+      withRetry(fn.bind(this), args, { idempotent: false, ...options })
   }
 
   authorizeUri = this.wrapWithRetry(this._authorizeUri)

src/utils/intuitAPI.ts

@@ -1,5 +1,5 @@
 import APIError from '@/app/api/core/exceptions/api'
-import { withRetry } from '@/app/api/core/utils/withRetry'
+import { RetryOptions, withRetry } from '@/app/api/core/utils/withRetry'
 import { intuitApiMinorVersion, intuitBaseUrl } from '@/config'
 import { QBPortalConnectionSelectSchemaType } from '@/db/schema/qbPortalConnections'
 import { getFetcher, postFetcher } from '@/helper/fetch.helper'
@@ -789,14 +789,17 @@ export default class IntuitAPI {
 
   private wrapWithRetry<Args extends unknown[], R>(
     fn: (...args: Args) => Promise<R>,
+    options?: RetryOptions,
   ): (...args: Args) => Promise<R> {
-    return (...args: Args): Promise<R> => withRetry(fn.bind(this), args)
+    return (...args: Args): Promise<R> =>
+      withRetry(fn.bind(this), args, { idempotent: false, ...options })
   }
 
-  // Wrap convention: writes + customQuery are wrapped here. Read methods
-  // (get*) compose customQuery and stay unwrapped to avoid nested withRetry
-  // (see withRetry.ts).
-  customQuery = this.wrapWithRetry(this._customQuery)
+  // Writes default to `idempotent: false` (no QBO request-key dedupe — a
+  // post-commit retry would duplicate). `customQuery` is the one read in
+  // this set and opts back into broad retry. `get*` stay unwrapped and
+  // inherit retry via `customQuery` (see withRetry.ts on nesting).
+  customQuery = this.wrapWithRetry(this._customQuery, { idempotent: true })
   createInvoice = this.wrapWithRetry(this._createInvoice)
   createCustomer = this.wrapWithRetry(this._createCustomer)
   createItem = this.wrapWithRetry(this._createItem)

test/unit/core/withRetry.test.ts

@@ -52,6 +52,59 @@ describe('isRetryableError', () => {
     expect(isRetryableError(undefined)).toBe(false)
     expect(isRetryableError('oops')).toBe(false)
   })
+
+  describe('idempotent: false (write-safety mode)', () => {
+    it('retries 429 — server explicitly rejected without committing', () => {
+      expect(
+        isRetryableError(Object.assign(new Error(), { status: 429 }), {
+          idempotent: false,
+        }),
+      ).toBe(true)
+    })
+
+    it('honors explicit RetryableError.retry regardless of idempotent option', () => {
+      expect(
+        isRetryableError(new RetryableError(500, 'x', true), {
+          idempotent: false,
+        }),
+      ).toBe(true)
+      expect(
+        isRetryableError(new RetryableError(500, 'x', false), {
+          idempotent: false,
+        }),
+      ).toBe(false)
+    })
+
+    it('does NOT retry 5xx — commit-then-dropped-response would duplicate', () => {
+      for (const status of [500, 502, 503, 504]) {
+        expect(
+          isRetryableError(Object.assign(new Error(), { status }), {
+            idempotent: false,
+          }),
+        ).toBe(false)
+      }
+    })
+
+    it('does NOT retry network errors — ECONNRESET/ETIMEDOUT can occur mid-response', () => {
+      const econnreset = Object.assign(new TypeError('fetch failed'), {
+        cause: { code: 'ECONNRESET' },
+      })
+      expect(isRetryableError(econnreset, { idempotent: false })).toBe(false)
+
+      const etimedout = Object.assign(new Error('boom'), { code: 'ETIMEDOUT' })
+      expect(isRetryableError(etimedout, { idempotent: false })).toBe(false)
+
+      const fetchFailed = new TypeError('fetch failed')
+      expect(isRetryableError(fetchFailed, { idempotent: false })).toBe(false)
+    })
+
+    it('does NOT retry AbortSignal.timeout — could fire after request committed', () => {
+      const timeout = Object.assign(new Error('timed out'), {
+        name: 'TimeoutError',
+      })
+      expect(isRetryableError(timeout, { idempotent: false })).toBe(false)
+    })
+  })
 })
 
 describe('withRetry', () => {
@@ -181,4 +234,53 @@ describe('withRetry', () => {
     // 1 initial + 4 retries = 5 total
     expect(fn).toHaveBeenCalledTimes(5)
   })
+
+  it('does not retry 5xx when idempotent: false', async () => {
+    const error = Object.assign(new Error('upstream 503'), { status: 503 })
+    const fn = vi.fn().mockRejectedValue(error)
+
+    await expect(withRetry(fn, [], { idempotent: false })).rejects.toThrow(
+      'upstream 503',
+    )
+    expect(fn).toHaveBeenCalledTimes(1)
+  })
+
+  it('does not retry network errors when idempotent: false', async () => {
+    const error = Object.assign(new TypeError('fetch failed'), {
+      cause: { code: 'ECONNRESET' },
+    })
+    const fn = vi.fn().mockRejectedValue(error)
+
+    await expect(withRetry(fn, [], { idempotent: false })).rejects.toThrow(
+      'fetch failed',
+    )
+    expect(fn).toHaveBeenCalledTimes(1)
+  })
+
+  it('still retries 429 when idempotent: false — server explicitly rejected', async () => {
+    const error = Object.assign(new Error('rate limited'), { status: 429 })
+    const fn = vi
+      .fn()
+      .mockRejectedValueOnce(error)
+      .mockResolvedValueOnce('recovered')
+
+    const promise = withRetry(fn, [], { idempotent: false })
+    await vi.runAllTimersAsync()
+
+    expect(await promise).toBe('recovered')
+    expect(fn).toHaveBeenCalledTimes(2)
+  })
+
+  it('still retries RetryableError(retry=true) when idempotent: false', async () => {
+    const fn = vi
+      .fn()
+      .mockRejectedValueOnce(new RetryableError(500, 'transient', true))
+      .mockResolvedValueOnce('ok')
+
+    const promise = withRetry(fn, [], { idempotent: false })
+    await vi.runAllTimersAsync()
+
+    expect(await promise).toBe('ok')
+    expect(fn).toHaveBeenCalledTimes(2)
+  })
 })