diff --git a/src/app/api/core/utils/withErrorHandler.ts b/src/app/api/core/utils/withErrorHandler.ts index dedbc87f..e6c9bf8a 100644 --- a/src/app/api/core/utils/withErrorHandler.ts +++ b/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 @@ -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) ) { diff --git a/src/app/api/core/utils/withRetry.ts b/src/app/api/core/utils/withRetry.ts index 3a71b07a..65ce290b 100644 --- a/src/app/api/core/utils/withRetry.ts +++ b/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 = 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 = new Set([ + 500, 502, 503, 504, +]) + +const RETRYABLE_NETWORK_CODES: ReadonlySet = new Set([ + 'ECONNRESET', + 'ECONNREFUSED', + 'ETIMEDOUT', + 'UND_ERR_CONNECT_TIMEOUT', +]) + +const RETRYABLE_ERROR_NAMES: ReadonlySet = 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 ( fn: (...args: any[]) => Promise, args: any[], + options: RetryOptions = {}, ): Promise => { let isEventProcessorRegistered = false @@ -36,26 +147,23 @@ export const withRetry = async ( }, { - 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), }, ) } diff --git a/src/config/index.ts b/src/config/index.ts index 66e6194d..465c5d4a 100644 --- a/src/config/index.ts +++ b/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 || '' diff --git a/src/helper/fetch.helper.ts b/src/helper/fetch.helper.ts index 07c92e96..851c5d40 100644 --- a/src/helper/fetch.helper.ts +++ b/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 " 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 + + // 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 => { + 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, body: Record, + 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, + 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() } diff --git a/src/helper/swr.helper.ts b/src/helper/swr.helper.ts index 7f7d83f3..4665ffae 100644 --- a/src/helper/swr.helper.ts +++ b/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, { diff --git a/src/hook/useQuickbooks.ts b/src/hook/useQuickbooks.ts index f2673b46..99e516a2 100644 --- a/src/hook/useQuickbooks.ts +++ b/src/hook/useQuickbooks.ts @@ -289,7 +289,11 @@ export const useAppBridge = ({ 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 () => { diff --git a/src/hook/useSettings.ts b/src/hook/useSettings.ts index 6ea735ef..f8b4251a 100644 --- a/src/hook/useSettings.ts +++ b/src/hook/useSettings.ts @@ -121,6 +121,7 @@ export const useProductMappingSettings = () => { `/api/quickbooks/product/map?token=${token}`, {}, { mappingItems, changedItemReference }, + { timeoutMs: null }, ) } @@ -129,6 +130,7 @@ export const useProductMappingSettings = () => { `/api/quickbooks/setting?type=${SettingType.PRODUCT}&token=${token}`, {}, { ...productSetting, type: SettingType.PRODUCT }, + { timeoutMs: null }, ) } @@ -138,23 +140,16 @@ export const useProductMappingSettings = () => { showProductConfirm: false, })) setSettingShowConfirm(false) - const [tableRes, settingRes] = await Promise.all([ - tableMappingSubmit(), - settingSubmit(), - ]) - - if (tableRes && settingRes) { + try { + await Promise.all([tableMappingSubmit(), settingSubmit()]) mutate(`/api/quickbooks/product/map?token=${token}`) mutate( `/api/quickbooks/setting?type=${SettingType.PRODUCT}&token=${token}`, ) setChangedItemReference([]) - } else { + } catch (err) { setSettingShowConfirm(true) // show the update settings button if error - console.error('Error submitting product settings', { - tableRes, - settingRes, - }) + console.error('Error submitting product settings', err) } } @@ -558,16 +553,17 @@ export const useInvoiceDetailSettings = () => { const submitInvoiceSettings = async () => { setShowButton(false) - const res = await postFetcher( - `/api/quickbooks/setting?type=${SettingType.INVOICE}&token=${token}`, - {}, - { ...settingState, type: SettingType.INVOICE }, - ) - if (!res || res?.error) { - setShowButton(true) // show the update settings button if error - console.error('Error submitting Invoice settings', { res }) - } else { + try { + await postFetcher( + `/api/quickbooks/setting?type=${SettingType.INVOICE}&token=${token}`, + {}, + { ...settingState, type: SettingType.INVOICE }, + { timeoutMs: null }, + ) mutate(`/api/quickbooks/setting?type=invoice&token=${token}`) + } catch (err) { + setShowButton(true) // show the update settings button if error + console.error('Error submitting Invoice settings', err) } } diff --git a/src/utils/copilotAPI.ts b/src/utils/copilotAPI.ts index 4d37b904..4f2cc6a4 100644 --- a/src/utils/copilotAPI.ts +++ b/src/utils/copilotAPI.ts @@ -1,5 +1,10 @@ import { withRetry } from '@/app/api/core/utils/withRetry' -import { copilotAPIKey as apiKey, appId } from '@/config' +import { + copilotAPIKey as apiKey, + appId, + externalFetchTimeoutMs, +} from '@/config' +import { buildHttpFetchError } from '@/helper/fetch.helper' import { ClientRequest, ClientResponse, @@ -82,7 +87,11 @@ export class CopilotAPI { 'X-API-KEY': workspaceId ? `${workspaceId}/${apiKey}` : apiKey, accept: 'application/json', }, + signal: AbortSignal.timeout(externalFetchTimeoutMs), }) + + if (!resp.ok) throw await buildHttpFetchError(resp, url.toString()) + return await resp.json() } diff --git a/src/utils/error.ts b/src/utils/error.ts index 08e6616f..82325174 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -63,6 +63,17 @@ export const getMessageAndCodeFromError = ( ? refreshTokenExpireMessage : error.error return { message, code: httpStatus.BAD_REQUEST, source: 'intuit' } + } else if (error instanceof HttpFetchError) { + // Surface real upstream status to qb_sync_logs. Source inferred by URL + // substring (intuit.com / copilot.app); revisit if either host migrates. + const source: 'intuit' | 'copilot' | 'unknown' = error.url.includes( + 'intuit', + ) + ? 'intuit' + : error.url.includes('copilot') + ? 'copilot' + : 'unknown' + return { message: error.message, code: error.status, source } } else if (error instanceof Error && error.message) { return { message: error.message, code, source: 'unknown' } } else if (isAxiosError(error)) { @@ -85,3 +96,28 @@ export class RetryableError extends Error { this.status = status } } + +export class HttpFetchError extends Error { + readonly status: number + readonly statusText: string + readonly url: string + readonly body: unknown + + constructor(args: { + status: number + statusText: string + url: string + body: unknown + message?: string + }) { + super( + args.message ?? + `HTTP ${args.status} ${args.statusText || ''} from ${args.url}`.trim(), + ) + this.name = 'HttpFetchError' + this.status = args.status + this.statusText = args.statusText + this.url = args.url + this.body = args.body + } +} diff --git a/src/utils/intuit.ts b/src/utils/intuit.ts index 299510ed..6273985c 100644 --- a/src/utils/intuit.ts +++ b/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( fn: (...args: Args) => Promise, + options?: RetryOptions, ): (...args: Args) => Promise { - return (...args: Args): Promise => withRetry(fn.bind(this), args) + return (...args: Args): Promise => + withRetry(fn.bind(this), args, { idempotent: false, ...options }) } authorizeUri = this.wrapWithRetry(this._authorizeUri) diff --git a/src/utils/intuitAPI.ts b/src/utils/intuitAPI.ts index e652b33a..e00271d0 100644 --- a/src/utils/intuitAPI.ts +++ b/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' @@ -29,7 +29,6 @@ import { SingleIdAndTokenResponseSchema, } from '@/type/dto/intuitAPI.dto' import { escapeForQBQuery, getNameAsCustomer } from '@/utils/string' -import { RetryableError } from '@/utils/error' import CustomLogger from '@/utils/logger' import httpStatus from 'http-status' @@ -121,12 +120,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/query?query=${encodeURIComponent(query)}&minorversion=${intuitApiMinorVersion}` const res = await this.getFetchWithHeader(url) - if (!res) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#customQuery | message = no response', - ) - if (res?.Fault) { CustomLogger.error({ obj: res.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -146,12 +139,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/invoice?minorversion=${intuitApiMinorVersion}` const invoice = await this.postFetchWithHeaders(url, payload) - if (!invoice) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createInvoice | message = no response', - ) - if (invoice?.Fault) { CustomLogger.error({ obj: invoice.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -178,12 +165,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/customer?minorversion=${intuitApiMinorVersion}` const customer = await this.postFetchWithHeaders(url, payload) - if (!customer) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createCustomer | message = no response', - ) - if (customer?.Fault) { CustomLogger.error({ obj: customer.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -208,12 +189,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/item?minorversion=${intuitApiMinorVersion}` const item = await this.postFetchWithHeaders(url, payload) - if (!item) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createItem | message = no response', - ) - if (item?.Fault) { CustomLogger.error({ obj: item.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -346,9 +321,10 @@ export default class IntuitAPI { * Inactive records are intentionally excluded: QBO auto-suffixes their * DisplayName with " (deleted)" when deactivated, freeing the original name. * - * Intentionally NOT wrapped in wrapWithRetry — the inner customQuery calls - * already retry on 429; re-wrapping would amplify rate-limit bursts (worst - * case 4 × 3 × 4 = 48 requests) and make recovery worse. + * Intentionally NOT wrapped in wrapWithRetry — each of the three inner + * customQuery calls is already retried by withRetry (up to 5 attempts on + * 429/5xx/transient network). Re-wrapping would amplify rate-limit bursts + * (worst case 5 × 3 × 5 = 75 requests) and make recovery worse. * * Throws if every candidate is taken. */ @@ -471,12 +447,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/invoice?minorversion=${intuitApiMinorVersion}` const invoice = await this.postFetchWithHeaders(url, payload) - if (!invoice) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#InvoiceSparseUpdate | message = no response', - ) - if (invoice?.Fault) { CustomLogger.error({ obj: invoice.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -503,12 +473,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/customer?minorversion=${intuitApiMinorVersion}` const customer = await this.postFetchWithHeaders(url, payload) - if (!customer) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#customerSparseUpdate | message = no response', - ) - if (customer?.Fault) { CustomLogger.error({ obj: customer.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -535,12 +499,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/item?minorversion=${intuitApiMinorVersion}` const item = await this.postFetchWithHeaders(url, payload) - if (!item) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#itemFullUpdate | message = no response', - ) - if (item?.Fault) { CustomLogger.error({ obj: item.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -569,12 +527,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/account?minorversion=${intuitApiMinorVersion}` const account = await this.postFetchWithHeaders(url, payload) - if (!account) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#updateAccount | message = no response', - ) - if (account?.Fault) { CustomLogger.error({ obj: account.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -601,12 +553,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/payment?minorversion=${intuitApiMinorVersion}` const payment = await this.postFetchWithHeaders(url, payload) - if (!payment) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createPayment | message = no response', - ) - if (payment?.Fault) { CustomLogger.error({ obj: payment.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -631,12 +577,6 @@ export default class IntuitAPI { const query = `select Id, SyncToken, DocNumber from Invoice where DocNumber = '${escapeForQBQuery(invoiceNumber)}' maxresults 1` const invoice = await this.customQuery(query) - if (!invoice) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#getInvoice | message = no response', - ) - if (!invoice.Invoice) return null CustomLogger.info({ @@ -654,12 +594,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/invoice?operation=void&minorversion=${intuitApiMinorVersion}` const invoice = await this.postFetchWithHeaders(url, payload) - if (!invoice) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#voidInvoice | message = no response', - ) - if (invoice?.Fault) { CustomLogger.error({ obj: invoice.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -684,13 +618,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/invoice?operation=delete&minorversion=${intuitApiMinorVersion}` const invoice = await this.postFetchWithHeaders(url, payload) - if (!invoice) { - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#deleteInvoice | No invoice deletion confirmation was received from Quickbooks API', - ) - } - if (invoice?.Fault) { CustomLogger.error({ obj: invoice.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -715,12 +642,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/payment?operation=delete&minorversion=${intuitApiMinorVersion}` const payment = await this.postFetchWithHeaders(url, payload) - if (!payment) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#deletePayment | message = no response', - ) - if (payment?.Fault) { CustomLogger.error({ obj: payment.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -788,12 +709,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/account?minorversion=${intuitApiMinorVersion}` const account = await this.postFetchWithHeaders(url, payload) - if (!account) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createAccount | message = no response', - ) - if (account?.Fault) { CustomLogger.error({ obj: account.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -818,12 +733,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/purchase?minorversion=${intuitApiMinorVersion}` const purchase = await this.postFetchWithHeaders(url, payload) - if (!purchase) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#createPurchase | message = no response', - ) - if (purchase?.Fault) { CustomLogger.error({ obj: purchase.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -848,12 +757,6 @@ export default class IntuitAPI { const url = `${intuitBaseUrl}/v3/company/${this.tokens.intuitRealmId}/purchase?operation=delete&minorversion=${intuitApiMinorVersion}` const purchase = await this.postFetchWithHeaders(url, payload) - if (!purchase) - throw new APIError( - httpStatus.BAD_REQUEST, - 'IntuitAPI#deletePurchase | message = no response', - ) - if (purchase?.Fault) { CustomLogger.error({ obj: purchase.Fault?.Error, message: 'Error: ' }) throw new APIError( @@ -878,11 +781,7 @@ export default class IntuitAPI { const companyInfo = await this.customQuery(query) if (!companyInfo) - throw new RetryableError( - httpStatus.NOT_FOUND, - 'No company info found', - true, - ) + throw new APIError(httpStatus.NOT_FOUND, 'No company info found') const parsedCompanyInfo = CompanyInfoSchema.parse(companyInfo) return parsedCompanyInfo.CompanyInfo[0] @@ -890,15 +789,21 @@ export default class IntuitAPI { private wrapWithRetry( fn: (...args: Args) => Promise, + options?: RetryOptions, ): (...args: Args) => Promise { - return (...args: Args): Promise => withRetry(fn.bind(this), args) + return (...args: Args): Promise => + withRetry(fn.bind(this), args, { idempotent: false, ...options }) } - 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) - getSingleIncomeAccount = this.wrapWithRetry(this._getSingleIncomeAccount) + getSingleIncomeAccount = this._getSingleIncomeAccount.bind(this) getACustomer: { ( displayName: string, @@ -915,10 +820,10 @@ export default class IntuitAPI { id: string, includeInactive?: boolean, ): Promise - } = this.wrapWithRetry(this._getACustomer) as any - // Intentionally NOT wrapped in wrapWithRetry — a transient 429 mid-walk would - // replay from page 1 and amplify rate-limit pressure. The inner customQuery - // calls already retry on 429 (same reasoning as resolveUniqueCustomerName). + } = this._getACustomer.bind(this) as any + // Additional rationale beyond the wrap convention: a transient 429 mid-walk + // would replay from page 1 and amplify rate-limit pressure (same reasoning + // as resolveUniqueCustomerName). getCustomerByEmail = this._getCustomerByEmail.bind(this) getAnItem: { ( @@ -936,13 +841,13 @@ export default class IntuitAPI { id: string, includeInactive?: boolean, ): Promise - } = this.wrapWithRetry(this._getAnItem) as any - getAllItems = this.wrapWithRetry(this._getAllItems) + } = this._getAnItem.bind(this) as any + getAllItems = this._getAllItems.bind(this) invoiceSparseUpdate = this.wrapWithRetry(this._invoiceSparseUpdate) customerSparseUpdate = this.wrapWithRetry(this._customerSparseUpdate) itemFullUpdate = this.wrapWithRetry(this._itemFullUpdate) createPayment = this.wrapWithRetry(this._createPayment) - getInvoice = this.wrapWithRetry(this._getInvoice) + getInvoice = this._getInvoice.bind(this) voidInvoice = this.wrapWithRetry(this._voidInvoice) deleteInvoice = this.wrapWithRetry(this._deleteInvoice) getAnAccount: { @@ -961,11 +866,11 @@ export default class IntuitAPI { id: string, includeInactive?: boolean, ): Promise - } = this.wrapWithRetry(this._getAnAccount) as any + } = this._getAnAccount.bind(this) as any createAccount = this.wrapWithRetry(this._createAccount) updateAccount = this.wrapWithRetry(this._updateAccount) createPurchase = this.wrapWithRetry(this._createPurchase) deletePayment = this.wrapWithRetry(this._deletePayment) deletePurchase = this.wrapWithRetry(this._deletePurchase) - getCompanyInfo = this.wrapWithRetry(this._getCompanyInfo) + getCompanyInfo = this._getCompanyInfo.bind(this) } diff --git a/test/unit/core/withRetry.test.ts b/test/unit/core/withRetry.test.ts index 0a04f220..f28448e6 100644 --- a/test/unit/core/withRetry.test.ts +++ b/test/unit/core/withRetry.test.ts @@ -1,11 +1,112 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { RetryableError } from '@/utils/error' -import { withRetry } from '@/app/api/core/utils/withRetry' +import { withRetry, isRetryableError } from '@/app/api/core/utils/withRetry' vi.mock('@sentry/nextjs', () => ({ withScope: vi.fn(), })) +describe('isRetryableError', () => { + it('returns the retry flag from RetryableError', () => { + expect(isRetryableError(new RetryableError(500, 'x', true))).toBe(true) + expect(isRetryableError(new RetryableError(500, 'x', false))).toBe(false) + }) + + it('treats 429/500/502/503/504 as retryable, others as not', () => { + for (const status of [429, 500, 502, 503, 504]) { + expect(isRetryableError(Object.assign(new Error(), { status }))).toBe( + true, + ) + } + for (const status of [400, 401, 404, 501]) { + expect(isRetryableError(Object.assign(new Error(), { status }))).toBe( + false, + ) + } + }) + + it('treats undici-style fetch failure with network code on cause as retryable', () => { + const err = Object.assign(new TypeError('fetch failed'), { + cause: { code: 'ECONNRESET' }, + }) + expect(isRetryableError(err)).toBe(true) + }) + + it('treats AbortSignal.timeout TimeoutError as retryable', () => { + const err = Object.assign(new Error('timed out'), { name: 'TimeoutError' }) + expect(isRetryableError(err)).toBe(true) + }) + + it('does NOT retry AbortError — deliberate cancellation should propagate', () => { + const err = Object.assign(new Error('aborted'), { name: 'AbortError' }) + expect(isRetryableError(err)).toBe(false) + }) + + it('treats top-level network code as retryable', () => { + const err = Object.assign(new Error('boom'), { code: 'ETIMEDOUT' }) + expect(isRetryableError(err)).toBe(true) + }) + + it('returns false for non-error inputs', () => { + expect(isRetryableError(null)).toBe(false) + 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', () => { beforeEach(() => { vi.useFakeTimers() @@ -62,7 +163,7 @@ describe('withRetry', () => { expect(fn).toHaveBeenCalledTimes(1) }) - it('does not retry on non-429 status errors', async () => { + it('does not retry on permanent 4xx responses', async () => { const error = Object.assign(new Error('bad request'), { status: 400 }) const fn = vi.fn().mockRejectedValue(error) @@ -70,6 +171,55 @@ describe('withRetry', () => { expect(fn).toHaveBeenCalledTimes(1) }) + it.each([500, 502, 503, 504])( + 'retries on transient %i and succeeds', + async (status) => { + const error = Object.assign(new Error(`transient ${status}`), { status }) + const fn = vi + .fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('recovered') + + const promise = withRetry(fn, []) + await vi.runAllTimersAsync() + + expect(await promise).toBe('recovered') + expect(fn).toHaveBeenCalledTimes(2) + }, + ) + + it('retries on undici fetch-failed wrapping ECONNRESET and succeeds', async () => { + const error = Object.assign(new TypeError('fetch failed'), { + cause: { code: 'ECONNRESET' }, + }) + const fn = vi + .fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('recovered') + + const promise = withRetry(fn, []) + await vi.runAllTimersAsync() + + expect(await promise).toBe('recovered') + expect(fn).toHaveBeenCalledTimes(2) + }) + + it('retries on AbortSignal.timeout TimeoutError and succeeds', async () => { + const error = Object.assign(new Error('timed out'), { + name: 'TimeoutError', + }) + const fn = vi + .fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce('recovered') + + const promise = withRetry(fn, []) + await vi.runAllTimersAsync() + + expect(await promise).toBe('recovered') + expect(fn).toHaveBeenCalledTimes(2) + }) + it('exhausts retries and throws on persistent 429', async () => { const error = Object.assign(new Error('rate limited'), { status: 429 }) const fn = vi.fn().mockRejectedValue(error) @@ -81,7 +231,56 @@ describe('withRetry', () => { const result = await promise expect(result).toBeInstanceOf(Error) expect((result as Error).message).toBe('rate limited') - // 1 initial + 3 retries = 4 total - expect(fn).toHaveBeenCalledTimes(4) + // 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) }) }) diff --git a/test/unit/helper/fetch.helper.test.ts b/test/unit/helper/fetch.helper.test.ts new file mode 100644 index 00000000..81595d75 --- /dev/null +++ b/test/unit/helper/fetch.helper.test.ts @@ -0,0 +1,243 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { getFetcher, postFetcher } from '@/helper/fetch.helper' +import { HttpFetchError } from '@/utils/error' + +const okJson = (body: unknown) => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'content-type': 'application/json' }, + }) + +describe('fetch.helper', () => { + beforeEach(() => { + vi.stubGlobal('fetch', vi.fn()) + }) + + afterEach(() => { + vi.unstubAllGlobals() + vi.restoreAllMocks() + }) + + describe('getFetcher', () => { + it('returns parsed JSON on a 2xx response', async () => { + ;(fetch as any).mockResolvedValueOnce(okJson({ ok: true, n: 1 })) + + const result = await getFetcher('https://example.com/x', { + Authorization: 'Bearer t', + }) + + expect(result).toEqual({ ok: true, n: 1 }) + }) + + it('passes an AbortSignal to fetch', async () => { + ;(fetch as any).mockResolvedValueOnce(okJson({})) + + await getFetcher('https://example.com/x', {}) + + const init = (fetch as any).mock.calls[0][1] as RequestInit + expect(init.signal).toBeInstanceOf(AbortSignal) + }) + + it('captures status, url, and parsed body on the thrown error', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response('{"err":"boom"}', { + status: 500, + statusText: 'Server Error', + headers: { 'content-type': 'application/json' }, + }), + ) + + const url = 'https://example.com/y' + try { + await getFetcher(url, {}) + throw new Error('expected throw') + } catch (e) { + expect(e).toBeInstanceOf(HttpFetchError) + const err = e as HttpFetchError + expect(err.status).toBe(500) + expect(err.statusText).toBe('Server Error') + expect(err.url).toBe(url) + expect(err.body).toEqual({ err: 'boom' }) + } + }) + + it('surfaces Intuit Fault.Error detail into error.message', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response( + JSON.stringify({ + Fault: { + Error: [ + { + Message: 'Required param missing', + Detail: 'Name is required', + code: '2020', + }, + ], + type: 'ValidationFault', + }, + }), + { + status: 400, + statusText: 'Bad Request', + headers: { 'content-type': 'application/json' }, + }, + ), + ) + + try { + await getFetcher('https://sandbox-quickbooks.api.intuit.com/v3/x', {}) + throw new Error('expected throw') + } catch (e) { + const err = e as HttpFetchError + expect(err.message).toBe('Required param missing — Name is required') + // Status and URL are preserved on the error fields, not in the message. + expect(err.status).toBe(400) + expect(err.url).toBe('https://sandbox-quickbooks.api.intuit.com/v3/x') + } + }) + + it('surfaces Copilot { message } into error.message', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response(JSON.stringify({ message: 'workspace not found' }), { + status: 404, + statusText: 'Not Found', + headers: { 'content-type': 'application/json' }, + }), + ) + + try { + await getFetcher('https://api.copilot.app/v1/workspaces/x', {}) + throw new Error('expected throw') + } catch (e) { + const err = e as HttpFetchError + expect(err.message).toBe('workspace not found') + } + }) + + it('falls back to generic HTTP message when body has no recognizable detail', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response(JSON.stringify({ unrelated: 'shape' }), { + status: 502, + statusText: 'Bad Gateway', + headers: { 'content-type': 'application/json' }, + }), + ) + + try { + await getFetcher('https://example.com/y', {}) + throw new Error('expected throw') + } catch (e) { + const err = e as HttpFetchError + expect(err.message).toBe('HTTP 502 Bad Gateway') + expect(err.url).toBe('https://example.com/y') + } + }) + + it('falls back to raw text body when response is not JSON', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response('oops', { + status: 504, + statusText: 'Gateway Timeout', + headers: { 'content-type': 'text/html' }, + }), + ) + + try { + await getFetcher('https://example.com/y', {}) + throw new Error('expected throw') + } catch (e) { + const err = e as HttpFetchError + expect(err.status).toBe(504) + expect(err.body).toBe('oops') + } + }) + + it('aborts the request when the per-call timeoutMs elapses', async () => { + vi.useFakeTimers() + try { + // Real fetch honors the signal: reject when the signal aborts. + // AbortSignal.timeout() produces a TimeoutError (Node 18+). + ;(fetch as any).mockImplementationOnce( + (_url: string, init: RequestInit) => + new Promise((_resolve, reject) => { + const signal = init.signal as AbortSignal + const onAbort = () => { + const err = new Error('The operation was aborted') + ;(err as any).name = 'TimeoutError' + reject(err) + } + if (signal.aborted) onAbort() + else signal.addEventListener('abort', onAbort, { once: true }) + }), + ) + + const pending = getFetcher( + 'https://example.com/x', + {}, + { timeoutMs: 50 }, + ) + const settled = pending.catch((e) => e) + + await vi.advanceTimersByTimeAsync(60) + const result = await settled + expect(result).toBeInstanceOf(Error) + expect((result as Error).name).toBe('TimeoutError') + } finally { + vi.useRealTimers() + } + }) + + it('does not abort before the configured timeout elapses', async () => { + vi.useFakeTimers() + try { + ;(fetch as any).mockImplementationOnce( + (_url: string, _init: RequestInit) => + new Promise((resolve) => + setTimeout(() => resolve(okJson({ late: true })), 10), + ), + ) + + const pending = getFetcher( + 'https://example.com/x', + {}, + { timeoutMs: 1000 }, + ) + await vi.advanceTimersByTimeAsync(15) + await expect(pending).resolves.toEqual({ late: true }) + } finally { + vi.useRealTimers() + } + }) + }) + + describe('postFetcher', () => { + it('serializes body and returns parsed JSON on a 2xx response', async () => { + ;(fetch as any).mockResolvedValueOnce(okJson({ ok: true })) + + const result = await postFetcher( + 'https://example.com/x', + { 'content-type': 'application/json' }, + { foo: 'bar' }, + ) + + expect(result).toEqual({ ok: true }) + const init = (fetch as any).mock.calls[0][1] as RequestInit + expect(init.method).toBe('POST') + expect(init.body).toBe(JSON.stringify({ foo: 'bar' })) + }) + + it('throws HttpFetchError on non-2xx', async () => { + ;(fetch as any).mockResolvedValueOnce( + new Response('{"err":"x"}', { + status: 500, + statusText: 'Server Error', + headers: { 'content-type': 'application/json' }, + }), + ) + + await expect( + postFetcher('https://example.com/x', {}, { a: 1 }), + ).rejects.toBeInstanceOf(HttpFetchError) + }) + }) +})