diff --git a/listener/docs/WEBHOOK_RESPONSE_CLASSIFICATION.md b/listener/docs/WEBHOOK_RESPONSE_CLASSIFICATION.md new file mode 100644 index 00000000..d1d3e483 --- /dev/null +++ b/listener/docs/WEBHOOK_RESPONSE_CLASSIFICATION.md @@ -0,0 +1,39 @@ +# Webhook Response Classification + +Webhook deliveries are classified into exactly one of three categories by +`listener/src/services/webhook-response-classifier.ts`. That module is the single +source of truth for retry decisions; both the bounded retry helper +(`webhook-retry-helper.ts`) and the delivery service +(`webhook-delivery-service.ts`) defer to it so they can never disagree. + +| Category | Meaning | Retried? | +| ----------- | --------------------------------------------------------------------- | ----------------------------------------- | +| `success` | HTTP 2xx; the receiver accepted the payload. | No | +| `retryable` | Transient failure (HTTP 429, any 5xx, network / timeout errors). | Yes, within the bounded retry budget. | +| `permanent` | Failure that will not change on retry (3xx redirects, other 4xx, 1xx). | No | + +## Status code rules + +| HTTP status | Category | Rationale | +| ----------- | ----------- | ------------------------------------------ | +| 1xx | `permanent` | Unexpected for a webhook POST. | +| 200-299 | `success` | Payload accepted. | +| 3xx | `permanent` | Redirects are not followed for webhooks. | +| 429 | `retryable` | Rate limited; back off and try again. | +| Other 4xx | `permanent` | Bad request, auth failure, not found, ... | +| 5xx | `retryable` | Server-side / transient. | + +## Error rules + +Network-level failures and request timeouts (`AbortError`) are classified as +`retryable`. Any unrecognised outcome defaults to `permanent`, so a bug can only +ever under-retry rather than loop forever. + +## Usage + +```ts +import { classifyWebhookStatus, classifyWebhookError } from './webhook-response-classifier'; + +const category = classifyWebhookStatus(response.status); // 'success' | 'retryable' | 'permanent' +const retryable = category === 'retryable'; +``` diff --git a/listener/src/services/webhook-delivery-service.test.ts b/listener/src/services/webhook-delivery-service.test.ts index 4899fce6..75a74aee 100644 --- a/listener/src/services/webhook-delivery-service.test.ts +++ b/listener/src/services/webhook-delivery-service.test.ts @@ -241,6 +241,50 @@ describe('WebhookDeliveryService', () => { }); }); + // ── Classification (issue #643) ─────────────────────────────────────────── + + describe('response category', () => { + it('reports success for 2xx responses', async () => { + mockSendWebhook.mockResolvedValue(makeResponse(200)); + + const result = await service.deliver(TARGET_URL, PAYLOAD, REQUEST_ID); + + expect(result.category).toBe('success'); + }); + + it.each([429, 500, 502, 503, 504])('reports retryable for HTTP %i', async (status) => { + mockSendWebhook.mockResolvedValue(makeResponse(status, false)); + + const result = await service.deliver(TARGET_URL, PAYLOAD, REQUEST_ID); + + expect(result.category).toBe('retryable'); + }); + + it.each([400, 401, 403, 404, 422])('reports permanent for HTTP %i', async (status) => { + mockSendWebhook.mockResolvedValue(makeResponse(status, false)); + + const result = await service.deliver(TARGET_URL, PAYLOAD, REQUEST_ID); + + expect(result.category).toBe('permanent'); + }); + + it('reports retryable for a request timeout', async () => { + mockSendWebhook.mockRejectedValue(makeAbortError()); + + const result = await service.deliver(TARGET_URL, PAYLOAD, REQUEST_ID); + + expect(result.category).toBe('retryable'); + }); + + it('reports retryable for a network error', async () => { + mockSendWebhook.mockRejectedValue(new Error('ECONNREFUSED')); + + const result = await service.deliver(TARGET_URL, PAYLOAD, REQUEST_ID); + + expect(result.category).toBe('retryable'); + }); + }); + // ── Default options ─────────────────────────────────────────────────────── describe('default options', () => { diff --git a/listener/src/services/webhook-delivery-service.ts b/listener/src/services/webhook-delivery-service.ts index d38f74f9..4df7020f 100644 --- a/listener/src/services/webhook-delivery-service.ts +++ b/listener/src/services/webhook-delivery-service.ts @@ -17,6 +17,11 @@ */ import logger from '../utils/logger'; +import { + classifyWebhookError, + classifyWebhookStatus, + type WebhookResponseCategory, +} from './webhook-response-classifier'; import { sendWebhook, WebhookSendOptions } from './webhook-sender'; export interface WebhookDeliveryOptions { @@ -33,6 +38,8 @@ export interface WebhookDeliveryResult { statusCode?: number; /** Human-readable failure reason for logging. */ errorReason?: string; + /** Classified outcome: `success`, `retryable`, or `permanent`. */ + category: WebhookResponseCategory; } export class WebhookDeliveryService { @@ -73,19 +80,24 @@ export class WebhookDeliveryService { try { const response = await sendWebhook(targetUrl, payload, sendOpts); const durationMs = Date.now() - startMs; + const category = classifyWebhookStatus(response.status); - if (response.ok) { + if (category === 'success') { logger.info('Webhook delivered successfully', { ...logCtx, statusCode: response.status, durationMs, }); - return { success: true, statusCode: response.status }; + return { success: true, statusCode: response.status, category }; } - // 5xx — transient server error, worth retrying - if (response.status >= 500) { - logger.warn('Webhook delivery failed with server error (5xx) — will retry', { + if (category === 'retryable') { + // 5xx is the common transient case; 429 (rate limited) is also retryable. + const message = + response.status >= 500 + ? 'Webhook delivery failed with server error (5xx) — will retry' + : 'Webhook delivery failed with a transient error — will retry'; + logger.warn(message, { ...logCtx, statusCode: response.status, durationMs, @@ -94,6 +106,7 @@ export class WebhookDeliveryService { success: false, statusCode: response.status, errorReason: `HTTP ${response.status}`, + category, }; } @@ -107,6 +120,7 @@ export class WebhookDeliveryService { success: false, statusCode: response.status, errorReason: `HTTP ${response.status}`, + category, }; } catch (err) { const durationMs = Date.now() - startMs; @@ -131,7 +145,7 @@ export class WebhookDeliveryService { }); } - return { success: false, errorReason }; + return { success: false, errorReason, category: classifyWebhookError(err) }; } } } 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..8eb00ee0 --- /dev/null +++ b/listener/src/services/webhook-response-classifier.test.ts @@ -0,0 +1,56 @@ +/** + * Tests for the canonical webhook response classifier (issue #643). + * + * Covers representative HTTP status codes for each category, plus the error + * classifier used for network / timeout failures. + */ + +import { describe, it, expect } from '@jest/globals'; +import { + classifyWebhookError, + classifyWebhookStatus, + isRetryableStatus, +} from './webhook-response-classifier'; + +describe('classifyWebhookStatus', () => { + it.each([200, 201, 202, 204, 299])('classifies HTTP %i as success', (status) => { + expect(classifyWebhookStatus(status)).toBe('success'); + }); + + it.each([429, 500, 501, 502, 503, 504, 507])('classifies HTTP %i as retryable', (status) => { + expect(classifyWebhookStatus(status)).toBe('retryable'); + }); + + it.each([100, 301, 302, 400, 401, 403, 404, 408, 418, 422])( + 'classifies HTTP %i as permanent', + (status) => { + expect(classifyWebhookStatus(status)).toBe('permanent'); + }, + ); +}); + +describe('classifyWebhookError', () => { + it('treats network errors as retryable', () => { + expect(classifyWebhookError(new Error('ECONNREFUSED'))).toBe('retryable'); + }); + + it('treats request timeouts (AbortError) as retryable', () => { + const abort = new Error('The operation was aborted'); + abort.name = 'AbortError'; + expect(classifyWebhookError(abort)).toBe('retryable'); + }); + + it('treats an absent error as permanent', () => { + expect(classifyWebhookError(undefined)).toBe('permanent'); + expect(classifyWebhookError(null)).toBe('permanent'); + }); +}); + +describe('isRetryableStatus', () => { + it('mirrors classifyWebhookStatus for representative codes', () => { + expect(isRetryableStatus(429)).toBe(true); + expect(isRetryableStatus(503)).toBe(true); + expect(isRetryableStatus(200)).toBe(false); + expect(isRetryableStatus(404)).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..6bdbfdbb --- /dev/null +++ b/listener/src/services/webhook-response-classifier.ts @@ -0,0 +1,71 @@ +/** + * Canonical classification of webhook delivery responses (issue #643). + * + * Every webhook attempt has exactly one of three outcomes: + * + * - `success` - the receiver accepted the payload (HTTP 2xx). Stop. + * - `retryable` - a transient failure worth another attempt: HTTP 429, any + * 5xx (500/502/503/504/...), and network / timeout errors. + * - `permanent` - a client error that will not change on retry: redirects + * (3xx), other 4xx, and 1xx. Stop and surface the failure. + * + * Status -> category: + * + * 1xx -> permanent (unexpected for a webhook POST) + * 2xx -> success + * 3xx -> permanent (redirects are not followed for webhooks) + * 429 -> retryable (rate limited; back off and try again) + * other 4xx -> permanent (bad request, auth failure, not found, ...) + * 5xx -> retryable (server-side / transient) + * + * Network-level failures and timeouts (AbortError) are retryable; see + * {@link classifyWebhookError}. + * + * This module is the single source of truth for retry decisions so the retry + * helper and the delivery service can never disagree. It never mutates the + * response and defaults anything unrecognised to `permanent`, so a bug can only + * ever under-retry, never loop forever. + */ + +export type WebhookResponseCategory = 'success' | 'retryable' | 'permanent'; + +/** Explicitly retryable status codes (5xx is covered by the generic 5xx rule). */ +export const RETRYABLE_STATUS_CODES: ReadonlySet = new Set([429, 500, 502, 503, 504]); + +/** Permanent client-error status codes (never retried). */ +export const PERMANENT_STATUS_CODES: ReadonlySet = new Set([400, 401, 403, 404, 422]); + +/** + * Classify an HTTP status code into a webhook delivery category. + * + * @param status - HTTP response status code. + * @returns The category that describes whether delivery succeeded, should be + * retried, or has permanently failed. + */ +export function classifyWebhookStatus(status: number): WebhookResponseCategory { + if (status >= 200 && status < 300) { + return 'success'; + } + if (status === 429 || status >= 500) { + return 'retryable'; + } + return 'permanent'; +} + +/** + * Classify a thrown webhook error. + * + * Network-level failures and request timeouts (AbortError) are transient and + * therefore retryable; anything else is treated as a permanent failure. + * + * @param error - The error thrown by the webhook attempt, if any. + * @returns `'retryable'` for any thrown failure, else `'permanent'`. + */ +export function classifyWebhookError(error: unknown): WebhookResponseCategory { + return error ? 'retryable' : 'permanent'; +} + +/** True when the HTTP status code represents a retryable failure. */ +export function isRetryableStatus(status: number): boolean { + return classifyWebhookStatus(status) === 'retryable'; +} diff --git a/listener/src/services/webhook-retry-helper.ts b/listener/src/services/webhook-retry-helper.ts index 14bee3b9..48df3cc1 100644 --- a/listener/src/services/webhook-retry-helper.ts +++ b/listener/src/services/webhook-retry-helper.ts @@ -14,6 +14,7 @@ * A delay is added between attempts to reduce load on failing services. */ +import { classifyWebhookError, classifyWebhookStatus } from './webhook-response-classifier'; import { sendWebhook, WebhookSendOptions } from './webhook-sender'; /** Maximum number of retry attempts (not counting the initial attempt). */ @@ -22,16 +23,6 @@ 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. * @@ -40,35 +31,14 @@ const PERMANENT_CLIENT_ERRORS = new Set([400, 401, 403, 404, 422]); * @returns true if the failure is retryable */ function isRetryable(response?: Response, error?: unknown): boolean { - // Network errors and timeouts are retryable + // Network errors and timeouts are retryable; the classifier owns the policy. if (error) { - return true; + return classifyWebhookError(error) === 'retryable'; } - // Check HTTP status codes + // Delegate the HTTP status policy to the shared classifier. 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 classifyWebhookStatus(response.status) === 'retryable'; } return false;