From 89d0ef82ae76e005d94d0bf8a00c7838b3cff9c1 Mon Sep 17 00:00:00 2001 From: Miron Machnicki Date: Wed, 17 Jun 2026 12:19:44 +0200 Subject: [PATCH 1/2] feat: DRO-32231 - apply CT-aligned per-method retry defaults for mutations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST/PATCH/DELETE mutations are no longer retried on ECONNABORTED/ETIMEDOUT or 500 by default — CT may have already processed the mutation server-side. Gateway errors (502/503/504) are still retried as the LB rejected the request before forwarding to CT. Changes: - types.ts: add MethodRetryPolicy interface and methodPolicies to CommercetoolsRetryConfig - is-retryable-error.ts: method-aware logic with CT-aligned per-method defaults - request.ts: pass httpMethod and methodPolicies to isRetryableError - tests: full coverage of new method-aware behavior and backward-compat cases --- src/lib/api/types.ts | 35 +++ src/lib/request/is-retryable-error.ts | 54 ++++- src/lib/request/request.ts | 3 +- .../__tests__/is-retryable-error.test.ts | 207 ++++++++++++++---- 4 files changed, 247 insertions(+), 52 deletions(-) diff --git a/src/lib/api/types.ts b/src/lib/api/types.ts index 4508fd4cba..280e1e1e4f 100644 --- a/src/lib/api/types.ts +++ b/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, CT-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 (CT 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 CT 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, CT-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> } diff --git a/src/lib/request/is-retryable-error.ts b/src/lib/request/is-retryable-error.ts index b76db7ba50..bc6ffd05be 100644 --- a/src/lib/request/is-retryable-error.ts +++ b/src/lib/request/is-retryable-error.ts @@ -1,10 +1,32 @@ -import { RETRYABLE_STATUS_CODES } from '../constants.js' +import { MethodRetryPolicy } from '../api/index.js' + +// CT docs: mutations may complete even after a 500. Gateway errors (502/503/504) +// are safe — the LB rejected before forwarding to CT. +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> +} /** * Determine whether the given error means we should allow the request * to be retried (assuming retry config is provided). + * + * When `options.method` is provided, CT-aligned per-method defaults apply: + * - POST/PATCH/DELETE: only retry on gateway errors (502/503/504); never on + * timeout (ECONNABORTED/ETIMEDOUT) or 500, since CT 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) } diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index 03574869e6..86540aed0c 100644 --- a/src/lib/request/request.ts +++ b/src/lib/request/request.ts @@ -94,6 +94,7 @@ async function executeRequest(options: RequestOptions): Promise { const timeout = options.timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS const additionalHeaders: Record = {} 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(options: RequestOptions): Promise { onAfterResponse(convertedError) } } - if (isRetryableError(error)) { + if (isRetryableError(error, { method: httpMethod, methodPolicies: retryConfig.methodPolicies })) { lastError = error } else { if (aggregateTimeoutId) { diff --git a/src/test/request/__tests__/is-retryable-error.test.ts b/src/test/request/__tests__/is-retryable-error.test.ts index 2ed068e184..2ecfc51642 100644 --- a/src/test/request/__tests__/is-retryable-error.test.ts +++ b/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 — CT-aligned defaults (no config required)', () => { + it('should NOT retry POST on ECONNABORTED by default — timeout means CT 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 — CT 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, CT 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) + }) }) }) From 898ece38b315f10a9274f85aa0432d8d8385a2db Mon Sep 17 00:00:00 2001 From: Miron Machnicki Date: Wed, 17 Jun 2026 12:45:51 +0200 Subject: [PATCH 2/2] feat: DRO-32231 - align commercetools wording and close patch coverage gap --- src/lib/api/types.ts | 8 ++++---- src/lib/request/is-retryable-error.ts | 8 ++++---- src/test/request/__tests__/is-retryable-error.test.ts | 8 ++++---- src/test/request/__tests__/request.test.ts | 7 +++---- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/lib/api/types.ts b/src/lib/api/types.ts index 280e1e1e4f..16f6802b36 100644 --- a/src/lib/api/types.ts +++ b/src/lib/api/types.ts @@ -11,7 +11,7 @@ export interface CommercetoolsApiConfig extends CommercetoolsAuthConfig { /** * Per-HTTP-method retry policy override. - * When not set, CT-aligned defaults apply automatically: + * 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 */ @@ -19,14 +19,14 @@ 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 (CT never processed) + * 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 CT may have already processed it) + * 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. @@ -59,7 +59,7 @@ export interface CommercetoolsRetryConfig { jitter?: boolean /** - * Per-method retry policy overrides. When omitted, CT-aligned defaults apply: + * 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 * diff --git a/src/lib/request/is-retryable-error.ts b/src/lib/request/is-retryable-error.ts index bc6ffd05be..96394e97ee 100644 --- a/src/lib/request/is-retryable-error.ts +++ b/src/lib/request/is-retryable-error.ts @@ -1,7 +1,7 @@ import { MethodRetryPolicy } from '../api/index.js' -// CT docs: mutations may complete even after a 500. Gateway errors (502/503/504) -// are safe — the LB rejected before forwarding to CT. +// 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] @@ -17,9 +17,9 @@ export interface IsRetryableErrorOptions { * Determine whether the given error means we should allow the request * to be retried (assuming retry config is provided). * - * When `options.method` is provided, CT-aligned per-method defaults apply: + * 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 CT may have already + * timeout (ECONNABORTED/ETIMEDOUT) or 500, since commercetools may have already * processed the mutation. * - All other methods: retry on [500–504] and network errors. * diff --git a/src/test/request/__tests__/is-retryable-error.test.ts b/src/test/request/__tests__/is-retryable-error.test.ts index 2ecfc51642..dcb4b7290d 100644 --- a/src/test/request/__tests__/is-retryable-error.test.ts +++ b/src/test/request/__tests__/is-retryable-error.test.ts @@ -81,8 +81,8 @@ describe('isRetryableError', () => { }) }) - describe('POST/PATCH mutations — CT-aligned defaults (no config required)', () => { - it('should NOT retry POST on ECONNABORTED by default — timeout means CT may have already processed it', () => { + 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) @@ -100,7 +100,7 @@ describe('isRetryableError', () => { expect(isRetryableError(error, { method: 'DELETE' })).toBe(false) }) - it('should NOT retry POST on 500 by default — CT may have already completed the mutation', () => { + it('should NOT retry POST on 500 by default — commercetools may have already completed the mutation', () => { const error = { isAxiosError: true, request: {}, response: { status: 500 } } expect(isRetryableError(error, { method: 'POST' })).toBe(false) @@ -112,7 +112,7 @@ describe('isRetryableError', () => { expect(isRetryableError(error, { method: 'PATCH' })).toBe(false) }) - it('should retry POST on 502 — gateway error, CT never received the request', () => { + 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) diff --git a/src/test/request/__tests__/request.test.ts b/src/test/request/__tests__/request.test.ts index b85627a57c..267ef54aa0 100644 --- a/src/test/request/__tests__/request.test.ts +++ b/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)