diff --git a/memory-bank/change-log.md b/memory-bank/change-log.md index 7fc31800..24dbf092 100644 --- a/memory-bank/change-log.md +++ b/memory-bank/change-log.md @@ -1263,3 +1263,40 @@ Validation: Follow-ups: - Optional next step: map these lifecycle fields into central dashboards/alerts (retry rate, status-class distribution, p95 elapsedMs). + +--- + +### CL-034: TASK22242 per-endpoint relay overrides + idempotency-aware retry gating + +date: 2026-03-24 +author: Cline +scope: `pages/api/middleware/relayForwarding.js`, `tests/phase21/relay-forwarding-hardening.test.cjs` +type: change +rationale: Deliver the next functional relay enhancement by enabling route-level retry tuning while adding safe-by-default retry gating for non-idempotent methods. +impact: Improves control and safety of relay retries without breaking existing endpoint contracts. +status: completed + +Summary: + +- Added **relay policy override support** (`relayPolicy`) to shared relay helpers (`relayGet`, `relayGetData`, `forwardGetData`): + - per-call override of `timeoutMs`, `maxRetries`, `retryBaseDelayMs`, `retryMaxDelayMs` + - optional method override via `relayPolicy.method` +- Added **idempotency-aware retry gating scaffolding**: + - retries allowed by default only for idempotent methods (`GET`, `HEAD`, `OPTIONS`) + - non-idempotent retry behavior controlled by: + - env flag `RELAY_ALLOW_NON_IDEMPOTENT_RETRIES` (default false) + - per-call override `relayPolicy.allowNonIdempotentRetries` +- Extended relay telemetry fields to include method and non-idempotent policy posture in start/failure/retry events. +- Preserved existing route behavior: + - existing GET endpoint flows continue to use retries per configured bounds + - no endpoint response contract changes + +Validation: + +- `node tests/phase21/relay-forwarding-hardening.test.cjs` -> pass (10/10) +- `node tests/phase21/endpoint-handler-contract.test.cjs` -> pass (152/152) +- `npm run lint` -> warnings only (pre-existing `react-hooks/exhaustive-deps`; no new lint errors) + +Follow-ups: + +- Future non-GET relay adoption should explicitly opt in/out per route using `relayPolicy` and include targeted negative-path tests. diff --git a/pages/api/middleware/relayForwarding.js b/pages/api/middleware/relayForwarding.js index 55371577..420eab3a 100644 --- a/pages/api/middleware/relayForwarding.js +++ b/pages/api/middleware/relayForwarding.js @@ -13,6 +13,7 @@ const DEFAULT_TIMEOUT_MS = 8000; const DEFAULT_MAX_RETRIES = 2; const DEFAULT_RETRY_BASE_DELAY_MS = 200; const DEFAULT_RETRY_MAX_DELAY_MS = 1200; +const DEFAULT_ALLOW_NON_IDEMPOTENT_RETRIES = false; const MAX_TIMEOUT_MS = 30000; const MAX_RETRIES = 4; const MAX_RETRY_DELAY_MS = 5000; @@ -29,11 +30,24 @@ const RETRYABLE_ERROR_CODES = new Set([ "EPIPE" ]); +const IDEMPOTENT_RETRY_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); + const parsePositiveInt = (value, fallback) => { const parsed = Number.parseInt(value, 10); return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback; }; +const parseBoolean = (value, fallback) => { + if (typeof value === "boolean") return value; + if (typeof value === "string") { + const normalized = value.trim().toLowerCase(); + if (normalized === "true") return true; + if (normalized === "false") return false; + } + + return fallback; +}; + const clamp = (value, min, max) => { return Math.min(max, Math.max(min, value)); }; @@ -65,6 +79,24 @@ const resolveNumericOverride = ({ return clamp(Math.floor(overrideValue), lowerBound, max); }; +const resolveBooleanOverride = ({ overrideValue, fallbackValue }) => { + if (typeof overrideValue === "boolean") return overrideValue; + return fallbackValue; +}; + +const resolveRelayMethod = (value) => { + if (typeof value !== "string" || value.trim().length === 0) { + return "GET"; + } + + return value.trim().toUpperCase(); +}; + +const canRetryForMethod = ({ method, allowNonIdempotentRetries }) => { + if (allowNonIdempotentRetries) return true; + return IDEMPOTENT_RETRY_METHODS.has(method); +}; + const getRelayConfig = () => { return { timeoutMs: sanitizeNumberConfig({ @@ -93,7 +125,11 @@ const getRelayConfig = () => { min: 0, max: MAX_RETRY_DELAY_MS, allowZero: true - }) + }), + allowNonIdempotentRetries: parseBoolean( + process.env.RELAY_ALLOW_NON_IDEMPOTENT_RETRIES, + DEFAULT_ALLOW_NON_IDEMPOTENT_RETRIES + ) }; }; @@ -122,8 +158,15 @@ const getStatusClass = (status) => { return `${Math.floor(status / 100)}xx`; }; -const shouldRetryRelayError = ({ error, attempt, maxRetries }) => { +const shouldRetryRelayError = ({ + error, + attempt, + maxRetries, + method, + allowNonIdempotentRetries +}) => { if (attempt >= maxRetries) return false; + if (!canRetryForMethod({ method, allowNonIdempotentRetries })) return false; const status = error?.response?.status; if (NON_RETRYABLE_STATUS_CODES.has(status)) return false; @@ -139,12 +182,14 @@ export const relayGet = async ({ res, errorResponse, transformData, - requestOptionsBuilder + requestOptionsBuilder, + relayPolicy }) => { try { const { data, accessToken } = await relayGetData({ queryUrl, - requestOptionsBuilder + requestOptionsBuilder, + relayPolicy }); return respondSuccess( @@ -164,12 +209,14 @@ export const relayGet = async ({ export const relayGetData = async ({ queryUrl, requestOptionsBuilder, - accessToken + accessToken, + relayPolicy }) => { return forwardGetData({ queryUrl, requestOptionsBuilder, - accessToken + accessToken, + relayPolicy }); }; @@ -182,19 +229,26 @@ export const forwardGetData = async ({ timeoutMs, maxRetries, retryBaseDelayMs, - retryMaxDelayMs + retryMaxDelayMs, + method, + relayPolicy }) => { const relayConfig = getRelayConfig(); + const relayPolicyConfig = relayPolicy || {}; + + const resolvedMethod = resolveRelayMethod( + relayPolicyConfig.method || method + ); const resolvedTimeoutMs = resolveNumericOverride({ - overrideValue: timeoutMs, + overrideValue: relayPolicyConfig.timeoutMs ?? timeoutMs, fallbackValue: relayConfig.timeoutMs, min: 100, max: MAX_TIMEOUT_MS }); const resolvedMaxRetries = resolveNumericOverride({ - overrideValue: maxRetries, + overrideValue: relayPolicyConfig.maxRetries ?? maxRetries, fallbackValue: relayConfig.maxRetries, min: 0, max: MAX_RETRIES, @@ -202,7 +256,7 @@ export const forwardGetData = async ({ }); const resolvedRetryBaseDelayMs = resolveNumericOverride({ - overrideValue: retryBaseDelayMs, + overrideValue: relayPolicyConfig.retryBaseDelayMs ?? retryBaseDelayMs, fallbackValue: relayConfig.retryBaseDelayMs, min: 0, max: MAX_RETRY_DELAY_MS, @@ -210,13 +264,20 @@ export const forwardGetData = async ({ }); const resolvedRetryMaxDelayMs = resolveNumericOverride({ - overrideValue: retryMaxDelayMs, + overrideValue: relayPolicyConfig.retryMaxDelayMs ?? retryMaxDelayMs, fallbackValue: relayConfig.retryMaxDelayMs, min: 0, max: MAX_RETRY_DELAY_MS, allowZero: true }); + const resolvedAllowNonIdempotentRetries = resolveBooleanOverride({ + overrideValue: + relayPolicyConfig.allowNonIdempotentRetries ?? + relayPolicyConfig.allowRetriesForNonIdempotent, + fallbackValue: relayConfig.allowNonIdempotentRetries + }); + const tokenAccessToken = typeof accessToken === "string" && accessToken.length > 0 ? accessToken @@ -246,10 +307,12 @@ export const forwardGetData = async ({ relayRequestId, queryUrl, baseUrl: resolvedBaseUrl, + method: resolvedMethod, timeoutMs: resolvedTimeoutMs, maxRetries: resolvedMaxRetries, retryBaseDelayMs: resolvedRetryBaseDelayMs, - retryMaxDelayMs: resolvedRetryMaxDelayMs + retryMaxDelayMs: resolvedRetryMaxDelayMs, + allowNonIdempotentRetries: resolvedAllowNonIdempotentRetries }); let lastError; @@ -279,7 +342,9 @@ export const forwardGetData = async ({ const retryEligible = shouldRetryRelayError({ error, attempt, - maxRetries: resolvedMaxRetries + maxRetries: resolvedMaxRetries, + method: resolvedMethod, + allowNonIdempotentRetries: resolvedAllowNonIdempotentRetries }); if (!retryEligible) { @@ -288,6 +353,7 @@ export const forwardGetData = async ({ relayRequestId, queryUrl, baseUrl: resolvedBaseUrl, + method: resolvedMethod, attempt: attempt + 1, maxAttempts: resolvedMaxRetries + 1, attemptsMade: attempt + 1, @@ -311,6 +377,7 @@ export const forwardGetData = async ({ relayRequestId, queryUrl, baseUrl: resolvedBaseUrl, + method: resolvedMethod, attempt: attempt + 1, maxAttempts: resolvedMaxRetries + 1, attemptsMade: attempt + 1, diff --git a/tests/phase21/relay-forwarding-hardening.test.cjs b/tests/phase21/relay-forwarding-hardening.test.cjs index 1b0ff9ad..975ec2d4 100644 --- a/tests/phase21/relay-forwarding-hardening.test.cjs +++ b/tests/phase21/relay-forwarding-hardening.test.cjs @@ -369,6 +369,152 @@ test("forwardGetData telemetry request id is stable across lifecycle events", as assert.strictEqual(new Set(relayRequestIds).size, 1); }); +test("forwardGetData relayPolicy override controls retry behavior", async () => { + let callCount = 0; + const infoLogs = []; + + const mod = loadRelayForwardingModule({ + axios: { + get: async () => { + callCount += 1; + if (callCount < 3) { + const error = new Error("temporary outage"); + error.response = { status: 503 }; + throw error; + } + + return { data: { ok: true }, status: 200 }; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => {}, + console: { + log: () => {}, + info: (...args) => infoLogs.push(args), + warn: () => {}, + error: () => {} + } + }); + + const result = await mod.forwardGetData({ + queryUrl: "incidents?$top=1", + maxRetries: 0, + relayPolicy: { + maxRetries: 2, + retryBaseDelayMs: 0, + retryMaxDelayMs: 0, + method: "GET" + } + }); + + assert.strictEqual(callCount, 3); + assert.deepStrictEqual(result.data, { ok: true }); + + const startEvent = infoLogs.find( + (entry) => entry[0] === "relay_request_started" + ); + assert.ok(startEvent); + assert.strictEqual(startEvent[1].maxRetries, 2); + assert.strictEqual(startEvent[1].method, "GET"); +}); + +test("forwardGetData blocks retries for non-idempotent methods by default", async () => { + let callCount = 0; + const infoLogs = []; + + const mod = loadRelayForwardingModule({ + axios: { + get: async () => { + callCount += 1; + const error = new Error("temporary outage"); + error.response = { status: 503 }; + throw error; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => {}, + console: { + log: () => {}, + info: (...args) => infoLogs.push(args), + warn: () => {}, + error: () => {} + } + }); + + let thrown = null; + try { + await mod.forwardGetData({ + queryUrl: "incidents?$top=1", + relayPolicy: { + method: "POST", + maxRetries: 3, + retryBaseDelayMs: 0, + retryMaxDelayMs: 0 + } + }); + } catch (error) { + thrown = error; + } + + assert.ok(thrown); + assert.strictEqual(callCount, 1); + + const retryEvents = infoLogs.filter( + (entry) => entry[0] === "relay_request_retrying" + ); + assert.strictEqual(retryEvents.length, 0); + + const failedEvent = infoLogs.find( + (entry) => entry[0] === "relay_request_failed" + ); + assert.ok(failedEvent); + assert.strictEqual(failedEvent[1].method, "POST"); +}); + +test("forwardGetData can allow retries for non-idempotent methods when explicitly enabled", async () => { + let callCount = 0; + + const mod = loadRelayForwardingModule({ + axios: { + get: async () => { + callCount += 1; + if (callCount === 1) { + const error = new Error("temporary outage"); + error.response = { status: 503 }; + throw error; + } + + return { data: { ok: true }, status: 200 }; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => {} + }); + + const result = await mod.forwardGetData({ + queryUrl: "incidents?$top=1", + relayPolicy: { + method: "PATCH", + maxRetries: 1, + retryBaseDelayMs: 0, + retryMaxDelayMs: 0, + allowNonIdempotentRetries: true + } + }); + + assert.strictEqual(callCount, 2); + assert.deepStrictEqual(result.data, { ok: true }); +}); + const run = async () => { let passed = 0; for (const currentTest of tests) {