feat: commercetools-aligned per-method retry defaults for mutations

src/lib/api/types.ts

@@ -9,6 +9,31 @@ export interface CommercetoolsApiConfig extends CommercetoolsAuthConfig {
   clientScopes?: string[]
 }
 
+/**
+ * Per-HTTP-method retry policy override.
+ * When not set, commercetools-aligned defaults apply automatically:
+ *   - POST/PATCH/DELETE → only retry on [502, 503, 504]; never on timeout or 500
+ *   - All other methods → retry on [500–504] and ECONNABORTED/ETIMEDOUT
+ */
+export interface MethodRetryPolicy {
+  /**
+   * HTTP response status codes that trigger a retry for this method.
+   * Default when not set:
+   *   POST/PATCH/DELETE → [502, 503, 504]  — gateway errors only (commercetools never processed)
+   *   all other methods → [500, 501, 502, 503, 504]
+   */
+  retryableStatusCodes?: number[]
+  /**
+   * Axios error codes (e.g. 'ECONNABORTED', 'ETIMEDOUT') that trigger a retry.
+   * Default when not set:
+   *   POST/PATCH/DELETE → []  — never (timeout means commercetools may have already processed it)
+   *   all other methods → ['ECONNABORTED', 'ETIMEDOUT']
+   * Note: when `!error.request` (request never left the client), retry is always
+   * allowed regardless — the server definitely never received the request.
+   */
+  retryableErrorCodes?: string[]
+}
+
 /**
  * Configuration for retrying a request when it fails
  */
@@ -32,4 +57,14 @@ export interface CommercetoolsRetryConfig {
    * We utilise the 'full' jitter + plus an additional decaying variance.
    */
   jitter?: boolean
+
+  /**
+   * Per-method retry policy overrides. When omitted, commercetools-aligned defaults apply:
+   * - POST/PATCH/DELETE: only retry on [502, 503, 504]; never on timeout or 500
+   * - All other methods: retry on [500–504] and ECONNABORTED/ETIMEDOUT
+   *
+   * Example — allow POST retry for inherently idempotent operations (e.g. OAuth2):
+   *   methodPolicies: { POST: { retryableErrorCodes: ['ECONNABORTED', 'ETIMEDOUT'] } }
+   */
+  methodPolicies?: Partial<Record<string, MethodRetryPolicy>>
 }

src/lib/request/is-retryable-error.ts

@@ -1,10 +1,32 @@
-import { RETRYABLE_STATUS_CODES } from '../constants.js'
+import { MethodRetryPolicy } from '../api/index.js'
+
+// commercetools docs: mutations may complete even after a 500. Gateway errors (502/503/504)
+// are safe — the LB rejected before forwarding to commercetools.
+const DEFAULT_RETRYABLE_STATUS_CODES = [500, 501, 502, 503, 504]
+const DEFAULT_RETRYABLE_ERROR_CODES = ['ECONNABORTED', 'ETIMEDOUT']
+const MUTATION_RETRYABLE_STATUS_CODES = [502, 503, 504]
+const MUTATION_RETRYABLE_ERROR_CODES: string[] = []
+const MUTATION_METHODS = new Set(['POST', 'PATCH', 'DELETE'])
+
+export interface IsRetryableErrorOptions {
+  method?: string
+  methodPolicies?: Partial<Record<string, MethodRetryPolicy>>
+}
 
 /**
  * Determine whether the given error means we should allow the request
  * to be retried (assuming retry config is provided).
+ *
+ * When `options.method` is provided, commercetools-aligned per-method defaults apply:
+ *   - POST/PATCH/DELETE: only retry on gateway errors (502/503/504); never on
+ *     timeout (ECONNABORTED/ETIMEDOUT) or 500, since commercetools may have already
+ *     processed the mutation.
+ *   - All other methods: retry on [500–504] and network errors.
+ *
+ * When `options.method` is omitted, legacy behavior is preserved for
+ * backward compatibility with call sites that have not yet been updated.
  */
-export function isRetryableError(error: any): boolean {
+export function isRetryableError(error: any, options?: IsRetryableErrorOptions): boolean {
   // If the error isn't an axios error, then something serious
   // went wrong. Probably a coding error in this package. We should
   // never really hit this scenario.
@@ -17,14 +39,26 @@ export function isRetryableError(error: any): boolean {
   if (error.code === 'ERR_CANCELED') {
     return false
   }
-  // If axios makes a request successfully, the `request` property will
-  // be defined. Equally, if it received a response, the `response` property
-  // will be defined. If either is not defined then we assume there was
-  // a serious connectivity issue and allow the request to be retried.
-  if (!error.request || !error.response) {
+  // Request never left the client — the server never received it.
+  // Always safe to retry regardless of HTTP method.
+  if (!error.request) {
     return true
   }
-  // Finally we only allow requests to be retried if the status code
-  // returned is in the given list
-  return RETRYABLE_STATUS_CODES.includes(error.response.status)
+
+  const method = options?.method?.toUpperCase()
+  const policy = method ? options?.methodPolicies?.[method] : undefined
+  const isMutation = method !== undefined && MUTATION_METHODS.has(method)
+
+  if (!error.response) {
+    // No method provided → legacy call site: preserve existing behavior (retry on any
+    // network error) for backward compatibility with unupdated callers.
+    if (method === undefined) return true
+    const retryableErrorCodes =
+      policy?.retryableErrorCodes ?? (isMutation ? MUTATION_RETRYABLE_ERROR_CODES : DEFAULT_RETRYABLE_ERROR_CODES)
+    return error.code != null && retryableErrorCodes.includes(error.code)
+  }
+
+  const retryableStatusCodes =
+    policy?.retryableStatusCodes ?? (isMutation ? MUTATION_RETRYABLE_STATUS_CODES : DEFAULT_RETRYABLE_STATUS_CODES)
+  return retryableStatusCodes.includes(error.response.status)
 }

src/lib/request/request.ts

@@ -94,6 +94,7 @@ async function executeRequest<T = any>(options: RequestOptions): Promise<T> {
   const timeout = options.timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS
   const additionalHeaders: Record<string, string> = {}
   const requestConfig: CommercetoolsRequest = plainClone(options.request)
+  const httpMethod = (requestConfig.method ?? 'GET').toString().toUpperCase()
   let retryCount = 0
   let lastError: any
   let aggregateTimeoutId: NodeJS.Timeout | undefined
@@ -191,7 +192,7 @@ async function executeRequest<T = any>(options: RequestOptions): Promise<T> {
           onAfterResponse(convertedError)
         }
       }
-      if (isRetryableError(error)) {
+      if (isRetryableError(error, { method: httpMethod, methodPolicies: retryConfig.methodPolicies })) {
         lastError = error
       } else {
         if (aggregateTimeoutId) {

src/test/request/__tests__/is-retryable-error.test.ts

@@ -1,75 +1,200 @@
 import { isRetryableError } from '../../../lib/request/index.js'
 
 describe('isRetryableError', () => {
-  it('should return true when the error is not an axios error', async () => {
-    const error = { test: 1 }
+  describe('legacy behavior (no method provided)', () => {
+    it('should return true when the error is not an axios error', async () => {
+      const error = { test: 1 }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
-  })
+      expect(result).toBe(true)
+    })
 
-  it('should return true when the axios error is missing a request object', async () => {
-    const error = { isAxiosError: true, response: {} }
+    it('should return true when the axios error is missing a request object', async () => {
+      const error = { isAxiosError: true, response: {} }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
-  })
+      expect(result).toBe(true)
+    })
 
-  it('should return true when the axios error is missing a response object', async () => {
-    const error = { isAxiosError: true, request: {} }
+    it('should return true when the axios error is missing a response object', async () => {
+      const error = { isAxiosError: true, request: {} }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
-  })
+      expect(result).toBe(true)
+    })
 
-  it('should return true when the axios response status code is 500', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 500 } }
+    it('should return true when the axios response status code is 500', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 500 } }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
-  })
+      expect(result).toBe(true)
+    })
 
-  it('should return true when the axios response status code is 501', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 501 } }
+    it('should return true when the axios response status code is 501', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 501 } }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
-  })
+      expect(result).toBe(true)
+    })
+
+    it('should return true when the axios response status code is 502', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 502 } }
+
+      const result = isRetryableError(error)
+
+      expect(result).toBe(true)
+    })
+
+    it('should return true when the axios response status code is 503', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 503 } }
+
+      const result = isRetryableError(error)
+
+      expect(result).toBe(true)
+    })
 
-  it('should return true when the axios response status code is 502', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 502 } }
+    it('should return true when the axios response status code is 504', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 504 } }
 
-    const result = isRetryableError(error)
+      const result = isRetryableError(error)
 
-    expect(result).toBe(true)
+      expect(result).toBe(true)
+    })
+
+    it('should return false when the axios response status code is 400', async () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 400 } }
+
+      const result = isRetryableError(error)
+
+      expect(result).toBe(false)
+    })
+
+    it('should preserve legacy behavior when method is omitted — backward compat for unupdated callers', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
+
+      expect(isRetryableError(error)).toBe(true)
+    })
   })
 
-  it('should return true when the axios response status code is 503', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 503 } }
+  describe('POST/PATCH mutations — commercetools-aligned defaults (no config required)', () => {
+    it('should NOT retry POST on ECONNABORTED by default — timeout means commercetools may have already processed it', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
+
+      expect(isRetryableError(error, { method: 'POST' })).toBe(false)
+    })
+
+    it('should NOT retry PATCH on ETIMEDOUT by default', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ETIMEDOUT' }
+
+      expect(isRetryableError(error, { method: 'PATCH' })).toBe(false)
+    })
+
+    it('should NOT retry DELETE on ECONNABORTED by default', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
+
+      expect(isRetryableError(error, { method: 'DELETE' })).toBe(false)
+    })
+
+    it('should NOT retry POST on 500 by default — commercetools may have already completed the mutation', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 500 } }
 
-    const result = isRetryableError(error)
+      expect(isRetryableError(error, { method: 'POST' })).toBe(false)
+    })
 
-    expect(result).toBe(true)
+    it('should NOT retry PATCH on 501 by default', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 501 } }
+
+      expect(isRetryableError(error, { method: 'PATCH' })).toBe(false)
+    })
+
+    it('should retry POST on 502 — gateway error, commercetools never received the request', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 502 } }
+
+      expect(isRetryableError(error, { method: 'POST' })).toBe(true)
+    })
+
+    it('should retry PATCH on 503', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 503 } }
+
+      expect(isRetryableError(error, { method: 'PATCH' })).toBe(true)
+    })
+
+    it('should retry DELETE on 504', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 504 } }
+
+      expect(isRetryableError(error, { method: 'DELETE' })).toBe(true)
+    })
+
+    it('should ALWAYS retry POST when request was never sent — server never received it', () => {
+      const error = { isAxiosError: true }
+
+      expect(isRetryableError(error, { method: 'POST' })).toBe(true)
+    })
+
+    it('should ALWAYS retry PATCH when request was never sent', () => {
+      const error = { isAxiosError: true }
+
+      expect(isRetryableError(error, { method: 'PATCH' })).toBe(true)
+    })
   })
 
-  it('should return true when the axios response status code is 504', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 504 } }
+  describe('GET/HEAD/OPTIONS — safe read-only methods', () => {
+    it('should retry GET on ECONNABORTED — safe read-only method', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
 
-    const result = isRetryableError(error)
+      expect(isRetryableError(error, { method: 'GET' })).toBe(true)
+    })
 
-    expect(result).toBe(true)
+    it('should retry GET on ETIMEDOUT', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ETIMEDOUT' }
+
+      expect(isRetryableError(error, { method: 'GET' })).toBe(true)
+    })
+
+    it('should retry GET on 500', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 500 } }
+
+      expect(isRetryableError(error, { method: 'GET' })).toBe(true)
+    })
+
+    it('should not retry GET on unknown error code', () => {
+      const error = { isAxiosError: true, request: {}, code: 'UNKNOWN_ERROR' }
+
+      expect(isRetryableError(error, { method: 'GET' })).toBe(false)
+    })
   })
 
-  it('should return false when the axios response status code is 400', async () => {
-    const error = { isAxiosError: true, request: {}, response: { status: 400 } }
+  describe('methodPolicies override', () => {
+    it('should override POST to retry on timeout via methodPolicies for explicitly idempotent flows', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
+      const methodPolicies = { POST: { retryableErrorCodes: ['ECONNABORTED', 'ETIMEDOUT'] } }
+
+      expect(isRetryableError(error, { method: 'POST', methodPolicies })).toBe(true)
+    })
+
+    it('should override PATCH to retry on 500 via methodPolicies', () => {
+      const error = { isAxiosError: true, request: {}, response: { status: 500 } }
+      const methodPolicies = { PATCH: { retryableStatusCodes: [500, 502, 503, 504] } }
+
+      expect(isRetryableError(error, { method: 'PATCH', methodPolicies })).toBe(true)
+    })
+
+    it('should apply override only to the matching method — other methods use defaults', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
+      const methodPolicies = { POST: { retryableErrorCodes: ['ECONNABORTED'] } }
+
+      expect(isRetryableError(error, { method: 'PATCH', methodPolicies })).toBe(false)
+    })
 
-    const result = isRetryableError(error)
+    it('should treat method lookup as case-insensitive', () => {
+      const error = { isAxiosError: true, request: {}, code: 'ECONNABORTED' }
 
-    expect(result).toBe(false)
+      expect(isRetryableError(error, { method: 'post' })).toBe(false)
+    })
   })
 })

src/test/request/__tests__/request.test.ts

@@ -50,11 +50,10 @@ describe('request', () => {
       scope.isDone()
       expect(result).toEqual({ success: true })
     })
-
-    it('should make a DELETE request when the config specifies a DELETE request', async () => {
-      const scope = nock('https://localhost').delete('/test').reply(200, { success: true })
+    it('should default to GET when method is omitted', async () => {
+      const scope = nock('https://localhost').get('/test').reply(200, { success: true })
       const requestConfig = getRequestConfig()
-      requestConfig.request.method = 'DELETE'
+      delete (requestConfig.request as unknown as { method?: string }).method
 
       const result = await request(requestConfig)