From afddba31a087ebf8c9b165540be22fad4a75155b Mon Sep 17 00:00:00 2001 From: Goodyslim <332204418+Goodyslim@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:13:53 +0100 Subject: [PATCH] feat(webhooks): classify delivery responses as success, retryable, or permanent Add a single documented classifier (webhook-response-classifier.ts) that maps an HTTP response or thrown error to success (2xx), retryable (network/timeout, 408/425/429, any 5xx), or permanent (other 4xx, 1xx/3xx). The delivery service and the bounded retry helper both delegate to it, so they can no longer disagree: permanent client failures are never retried and transient failures are. WebhookDeliveryResult now carries the explicit classification. Closes #643 --- .../src/services/webhook-delivery-service.ts | 28 +++++-- .../webhook-response-classifier.test.ts | 84 +++++++++++++++++++ .../services/webhook-response-classifier.ts | 72 ++++++++++++++++ listener/src/services/webhook-retry-helper.ts | 53 +++--------- 4 files changed, 188 insertions(+), 49 deletions(-) create mode 100644 listener/src/services/webhook-response-classifier.test.ts create mode 100644 listener/src/services/webhook-response-classifier.ts diff --git a/listener/src/services/webhook-delivery-service.ts b/listener/src/services/webhook-delivery-service.ts index d38f74f9..4636bad0 100644 --- a/listener/src/services/webhook-delivery-service.ts +++ b/listener/src/services/webhook-delivery-service.ts @@ -18,6 +18,10 @@ import logger from '../utils/logger'; import { sendWebhook, WebhookSendOptions } from './webhook-sender'; +import { + classifyWebhookResponse, + type WebhookResponseClassification, +} from './webhook-response-classifier'; export interface WebhookDeliveryOptions { /** Request timeout in milliseconds (default: 10 000). */ @@ -29,6 +33,8 @@ export interface WebhookDeliveryOptions { export interface WebhookDeliveryResult { /** True when the server responded with a 2xx status code. */ success: boolean; + /** Explicit outcome category (issue #643): success | retryable | permanent. */ + classification: WebhookResponseClassification; /** HTTP status code, or undefined when a network error occurred. */ statusCode?: number; /** Human-readable failure reason for logging. */ @@ -73,19 +79,25 @@ export class WebhookDeliveryService { try { const response = await sendWebhook(targetUrl, payload, sendOpts); const durationMs = Date.now() - startMs; + const classification = classifyWebhookResponse({ statusCode: response.status }); - if (response.ok) { + if (classification === 'success') { logger.info('Webhook delivered successfully', { ...logCtx, statusCode: response.status, durationMs, }); - return { success: true, statusCode: response.status }; + return { success: true, statusCode: response.status, classification }; } - // 5xx — transient server error, worth retrying - if (response.status >= 500) { - logger.warn('Webhook delivery failed with server error (5xx) — will retry', { + if (classification === 'retryable') { + // 5xx keeps its historical message; other transient codes (408/425/429) + // get a code-accurate message. + const message = + response.status >= 500 + ? 'Webhook delivery failed with server error (5xx) — will retry' + : `Webhook delivery failed with a transient error (HTTP ${response.status}) — will retry`; + logger.warn(message, { ...logCtx, statusCode: response.status, durationMs, @@ -93,11 +105,12 @@ export class WebhookDeliveryService { return { success: false, statusCode: response.status, + classification, errorReason: `HTTP ${response.status}`, }; } - // 4xx — client error, permanent failure + // permanent — a client error that retrying cannot fix logger.error('Webhook delivery failed with client error (4xx) — no retry', { ...logCtx, statusCode: response.status, @@ -106,6 +119,7 @@ export class WebhookDeliveryService { return { success: false, statusCode: response.status, + classification, errorReason: `HTTP ${response.status}`, }; } catch (err) { @@ -131,7 +145,7 @@ export class WebhookDeliveryService { }); } - return { success: false, errorReason }; + return { success: false, classification: 'retryable', errorReason }; } } } diff --git a/listener/src/services/webhook-response-classifier.test.ts b/listener/src/services/webhook-response-classifier.test.ts new file mode 100644 index 00000000..61999f47 --- /dev/null +++ b/listener/src/services/webhook-response-classifier.test.ts @@ -0,0 +1,84 @@ +/** + * Tests for the shared webhook response classifier (issue #643). + * + * Covers representative HTTP status codes for each category plus the + * error-takes-precedence rule and the retryable predicate. + */ + +import { describe, it, expect } from '@jest/globals'; +import { + classifyWebhookResponse, + isRetryableClassification, + RETRYABLE_STATUS_CODES, +} from './webhook-response-classifier'; + +describe('classifyWebhookResponse', () => { + describe('success (2xx)', () => { + it.each([200, 201, 202, 204, 299])('classifies HTTP %i as success', (status) => { + expect(classifyWebhookResponse({ statusCode: status })).toBe('success'); + }); + }); + + describe('retryable', () => { + it.each([500, 501, 502, 503, 504, 599])( + 'classifies server error HTTP %i as retryable', + (status) => { + expect(classifyWebhookResponse({ statusCode: status })).toBe('retryable'); + }, + ); + + it.each([...RETRYABLE_STATUS_CODES])( + 'classifies transient HTTP %i as retryable', + (status) => { + expect(classifyWebhookResponse({ statusCode: status })).toBe('retryable'); + }, + ); + + it('classifies a network error (no status) as retryable', () => { + expect(classifyWebhookResponse({ error: new Error('ECONNRESET') })).toBe('retryable'); + }); + + it('classifies a timeout (AbortError) as retryable', () => { + const error = new Error('The operation was aborted'); + error.name = 'AbortError'; + expect(classifyWebhookResponse({ error })).toBe('retryable'); + }); + + it('classifies a non-Error rejection as retryable', () => { + expect(classifyWebhookResponse({ error: 'string rejection' })).toBe('retryable'); + }); + }); + + describe('permanent', () => { + it.each([400, 401, 403, 404, 405, 409, 410, 418, 422, 451])( + 'classifies client error HTTP %i as permanent', + (status) => { + expect(classifyWebhookResponse({ statusCode: status })).toBe('permanent'); + }, + ); + + it.each([301, 302, 304])('classifies redirect HTTP %i as permanent', (status) => { + expect(classifyWebhookResponse({ statusCode: status })).toBe('permanent'); + }); + + it('classifies a missing status with no error as permanent', () => { + expect(classifyWebhookResponse({})).toBe('permanent'); + }); + }); + + describe('precedence', () => { + it('an error takes precedence over any status code', () => { + expect( + classifyWebhookResponse({ statusCode: 500, error: new Error('boom') }), + ).toBe('retryable'); + }); + }); + + describe('isRetryableClassification', () => { + it('is true only for the retryable category', () => { + expect(isRetryableClassification('retryable')).toBe(true); + expect(isRetryableClassification('success')).toBe(false); + expect(isRetryableClassification('permanent')).toBe(false); + }); + }); +}); diff --git a/listener/src/services/webhook-response-classifier.ts b/listener/src/services/webhook-response-classifier.ts new file mode 100644 index 00000000..2d03e4f8 --- /dev/null +++ b/listener/src/services/webhook-response-classifier.ts @@ -0,0 +1,72 @@ +/** + * Webhook response classification (issue #643). + * + * A single, documented rule set that maps an HTTP response (or a thrown + * network error) into exactly one of three categories: + * + * | Category | Signals | Retry? | + * |-------------|----------------------------------------------------------------|--------| + * | `success` | HTTP 2xx | n/a | + * | `retryable` | network / timeout error; HTTP 408, 425, 429; any HTTP 5xx | yes | + * | `permanent` | any other HTTP 4xx; 1xx/3xx; missing status with no error | no | + * + * Retryable means the same request may succeed later (transient). Permanent + * means retrying cannot help without a change to the request, so it must not + * be retried indefinitely. + */ + +export type WebhookResponseClassification = 'success' | 'retryable' | 'permanent'; + +/** + * Non-5xx status codes that are still transient and therefore retryable. + * - 408 Request Timeout + * - 425 Too Early + * - 429 Too Many Requests + */ +export const RETRYABLE_STATUS_CODES: ReadonlySet = new Set([408, 425, 429]); + +/** The signal available after one HTTP attempt. */ +export interface WebhookResponseSignal { + /** HTTP status code, or `undefined` when the request never got a response. */ + statusCode?: number; + /** The thrown error, when the request failed before producing a response. */ + error?: unknown; +} + +/** + * Classify a single webhook attempt. + * + * A thrown error always means the attempt is retryable (network failure or + * timeout). Otherwise the status code decides, per the table above. + */ +export function classifyWebhookResponse( + signal: WebhookResponseSignal, +): WebhookResponseClassification { + if (signal.error !== undefined && signal.error !== null) { + return 'retryable'; + } + + const status = signal.statusCode; + if (status === undefined || status === null) { + // No response and no error: nothing to retry safely. + return 'permanent'; + } + + if (status >= 200 && status < 300) { + return 'success'; + } + if (status >= 500) { + return 'retryable'; + } + if (RETRYABLE_STATUS_CODES.has(status)) { + return 'retryable'; + } + return 'permanent'; +} + +/** Convenience predicate for the common "should this be retried?" question. */ +export function isRetryableClassification( + classification: WebhookResponseClassification, +): boolean { + return classification === 'retryable'; +} diff --git a/listener/src/services/webhook-retry-helper.ts b/listener/src/services/webhook-retry-helper.ts index 14bee3b9..f7563132 100644 --- a/listener/src/services/webhook-retry-helper.ts +++ b/listener/src/services/webhook-retry-helper.ts @@ -15,6 +15,10 @@ */ import { sendWebhook, WebhookSendOptions } from './webhook-sender'; +import { + classifyWebhookResponse, + isRetryableClassification, +} from './webhook-response-classifier'; /** Maximum number of retry attempts (not counting the initial attempt). */ const MAX_RETRY_ATTEMPTS = 2; @@ -22,56 +26,21 @@ const MAX_RETRY_ATTEMPTS = 2; /** Delay in milliseconds between retry attempts. */ const RETRY_DELAY_MS = 1000; -/** - * HTTP status codes that are considered retryable (transient failures). - */ -const RETRYABLE_STATUS_CODES = new Set([429, 500, 502, 503, 504]); - -/** - * HTTP status codes that are permanent client errors (not retryable). - */ -const PERMANENT_CLIENT_ERRORS = new Set([400, 401, 403, 404, 422]); - /** * Determines if an error or response should trigger a retry. * + * Delegates to the shared classifier (issue #643) so the retry helper and the + * delivery service agree on what "retryable" means. Only responses classified + * as retryable are retried; success and permanent failures are returned. + * * @param response - The HTTP response, if available * @param error - The error thrown, if any * @returns true if the failure is retryable */ function isRetryable(response?: Response, error?: unknown): boolean { - // Network errors and timeouts are retryable - if (error) { - return true; - } - - // Check HTTP status codes - if (response) { - // Success responses don't need retry - if (response.ok) { - return false; - } - - // Permanent client errors should not be retried - if (PERMANENT_CLIENT_ERRORS.has(response.status)) { - return false; - } - - // Explicit retryable status codes - if (RETRYABLE_STATUS_CODES.has(response.status)) { - return true; - } - - // Any other 5xx error is retryable - if (response.status >= 500) { - return true; - } - - // Other status codes (e.g., redirects, other 4xx) are not retried - return false; - } - - return false; + return isRetryableClassification( + classifyWebhookResponse({ statusCode: response?.status, error }), + ); } /**