refactor(api): tighten relay retry policy and config safety bounds

memory-bank/change-log.md

@@ -1148,3 +1148,44 @@ Validation:
 Follow-ups:
 
 - Optional next hardening increment: introduce endpoint-specific retry overrides for write paths (if future non-GET use is introduced) to keep retry policy conservative by operation type.
+
+---
+
+### CL-031: TASK22236 P2-S3 Batch 2A policy tightening (retry classification, config clamping, log de-dup)
+
+date: 2026-03-24
+author: Cline
+scope: `pages/api/middleware/relayForwarding.js`, `tests/phase21/relay-forwarding-hardening.test.cjs`
+type: change
+rationale: Execute P2-S3 Batch 2A by tightening relay retry policy and operational safety bounds while preventing duplicate error noise between relay-layer and endpoint-layer logging.
+impact: Stronger resilience and cleaner observability with no endpoint contract changes; retry behavior is now explicitly conservative for deterministic client/auth failures.
+status: completed
+
+Summary:
+
+- Updated relay retry policy:
+    - explicitly non-retryable statuses: `400`, `401`, `403`, `404`, `422`
+    - retries still allowed for transient classes (`408`, `429`, `5xx`) and selected transport error codes
+    - any other explicit numeric HTTP status now treated as non-retryable by default
+- Added runtime-safe config clamping for relay knobs:
+    - timeout clamped to `100..30000ms`
+    - retries clamped to `0..4`
+    - retry delays clamped to `0..5000ms`
+    - both env-derived and per-call numeric overrides are sanitized
+- Reduced duplicate logging noise:
+    - non-retry terminal relay failures are marked as already logged in middleware
+    - `relayGet` catch now skips `consoleLogger` when relay layer has already emitted a structured log
+- Expanded relay hardening tests:
+    - no retry on `401`
+    - duplicate logging suppression path through `relayGet`
+    - env-value clamping behavior for timeout bounds
+
+Validation:
+
+- `node tests/phase21/relay-forwarding-hardening.test.cjs` -> pass (6/6)
+- `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:
+
+- Batch 2B: update memory/context docs with canonical relay hardening policy, env knobs, and rollback/tuning guidance.

pages/api/middleware/relayForwarding.js

@@ -13,6 +13,11 @@ 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 MAX_TIMEOUT_MS = 30000;
+const MAX_RETRIES = 4;
+const MAX_RETRY_DELAY_MS = 5000;
+
+const NON_RETRYABLE_STATUS_CODES = new Set([400, 401, 403, 404, 422]);
 
 const RETRYABLE_STATUS_CODES = new Set([408, 429, 500, 502, 503, 504]);
 const RETRYABLE_ERROR_CODES = new Set([
@@ -29,24 +34,66 @@ const parsePositiveInt = (value, fallback) => {
     return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback;
 };
 
+const clamp = (value, min, max) => {
+    return Math.min(max, Math.max(min, value));
+};
+
+const sanitizeNumberConfig = ({
+    value,
+    fallback,
+    min,
+    max,
+    allowZero = false
+}) => {
+    const parsed = parsePositiveInt(value, fallback);
+    const lowerBound = allowZero ? 0 : min;
+    return clamp(parsed, lowerBound, max);
+};
+
+const resolveNumericOverride = ({
+    overrideValue,
+    fallbackValue,
+    min,
+    max,
+    allowZero = false
+}) => {
+    if (typeof overrideValue !== "number" || !Number.isFinite(overrideValue)) {
+        return fallbackValue;
+    }
+
+    const lowerBound = allowZero ? 0 : min;
+    return clamp(Math.floor(overrideValue), lowerBound, max);
+};
+
 const getRelayConfig = () => {
     return {
-        timeoutMs: parsePositiveInt(
+        timeoutMs: sanitizeNumberConfig({
-            process.env.RELAY_TIMEOUT_MS,
+            value: process.env.RELAY_TIMEOUT_MS,
-            DEFAULT_TIMEOUT_MS
+            fallback: DEFAULT_TIMEOUT_MS,
-        ),
+            min: 100,
-        maxRetries: parsePositiveInt(
+            max: MAX_TIMEOUT_MS
-            process.env.RELAY_RETRY_MAX,
+        }),
-            DEFAULT_MAX_RETRIES
+        maxRetries: sanitizeNumberConfig({
-        ),
+            value: process.env.RELAY_RETRY_MAX,
-        retryBaseDelayMs: parsePositiveInt(
+            fallback: DEFAULT_MAX_RETRIES,
-            process.env.RELAY_RETRY_BASE_DELAY_MS,
+            min: 0,
-            DEFAULT_RETRY_BASE_DELAY_MS
+            max: MAX_RETRIES,
-        ),
+            allowZero: true
-        retryMaxDelayMs: parsePositiveInt(
+        }),
-            process.env.RELAY_RETRY_MAX_DELAY_MS,
+        retryBaseDelayMs: sanitizeNumberConfig({
-            DEFAULT_RETRY_MAX_DELAY_MS
+            value: process.env.RELAY_RETRY_BASE_DELAY_MS,
-        )
+            fallback: DEFAULT_RETRY_BASE_DELAY_MS,
+            min: 0,
+            max: MAX_RETRY_DELAY_MS,
+            allowZero: true
+        }),
+        retryMaxDelayMs: sanitizeNumberConfig({
+            value: process.env.RELAY_RETRY_MAX_DELAY_MS,
+            fallback: DEFAULT_RETRY_MAX_DELAY_MS,
+            min: 0,
+            max: MAX_RETRY_DELAY_MS,
+            allowZero: true
+        })
     };
 };
 
@@ -66,7 +113,9 @@ const shouldRetryRelayError = ({ error, attempt, maxRetries }) => {
     if (attempt >= maxRetries) return false;
 
     const status = error?.response?.status;
+    if (NON_RETRYABLE_STATUS_CODES.has(status)) return false;
     if (RETRYABLE_STATUS_CODES.has(status)) return true;
+    if (typeof status === "number") return false;
 
     const code = error?.code;
     return RETRYABLE_ERROR_CODES.has(code);
@@ -92,7 +141,9 @@ export const relayGet = async ({
                 : data
         );
     } catch (error) {
-        consoleLogger(error);
+        if (!error?.__relayAlreadyLogged) {
+            consoleLogger(error);
+        }
         return respondError(res, errorResponse);
     }
 };
@@ -122,25 +173,36 @@ export const forwardGetData = async ({
 }) => {
     const relayConfig = getRelayConfig();
 
-    const resolvedTimeoutMs =
+    const resolvedTimeoutMs = resolveNumericOverride({
-        typeof timeoutMs === "number" && timeoutMs >= 0
+        overrideValue: timeoutMs,
-            ? timeoutMs
+        fallbackValue: relayConfig.timeoutMs,
-            : relayConfig.timeoutMs;
+        min: 100,
+        max: MAX_TIMEOUT_MS
+    });
 
-    const resolvedMaxRetries =
+    const resolvedMaxRetries = resolveNumericOverride({
-        typeof maxRetries === "number" && maxRetries >= 0
+        overrideValue: maxRetries,
-            ? maxRetries
+        fallbackValue: relayConfig.maxRetries,
-            : relayConfig.maxRetries;
+        min: 0,
+        max: MAX_RETRIES,
+        allowZero: true
+    });
 
-    const resolvedRetryBaseDelayMs =
+    const resolvedRetryBaseDelayMs = resolveNumericOverride({
-        typeof retryBaseDelayMs === "number" && retryBaseDelayMs >= 0
+        overrideValue: retryBaseDelayMs,
-            ? retryBaseDelayMs
+        fallbackValue: relayConfig.retryBaseDelayMs,
-            : relayConfig.retryBaseDelayMs;
+        min: 0,
+        max: MAX_RETRY_DELAY_MS,
+        allowZero: true
+    });
 
-    const resolvedRetryMaxDelayMs =
+    const resolvedRetryMaxDelayMs = resolveNumericOverride({
-        typeof retryMaxDelayMs === "number" && retryMaxDelayMs >= 0
+        overrideValue: retryMaxDelayMs,
-            ? retryMaxDelayMs
+        fallbackValue: relayConfig.retryMaxDelayMs,
-            : relayConfig.retryMaxDelayMs;
+        min: 0,
+        max: MAX_RETRY_DELAY_MS,
+        allowZero: true
+    });
 
     const tokenAccessToken =
         typeof accessToken === "string" && accessToken.length > 0
@@ -184,6 +246,7 @@ export const forwardGetData = async ({
             });
 
             if (!retryEligible) {
+                error.__relayAlreadyLogged = true;
                 structuredRelayLog("relay_request_failed", {
                     queryUrl,
                     baseUrl: resolvedBaseUrl,

tests/phase21/relay-forwarding-hardening.test.cjs

@@ -111,6 +111,118 @@ test("forwardGetData does not retry non-retryable HTTP status", async () => {
     assert.strictEqual(callCount, 1);
 });
 
+test("forwardGetData does not retry unauthorized status", async () => {
+    let callCount = 0;
+
+    const mod = loadRelayForwardingModule({
+        axios: {
+            get: async () => {
+                callCount += 1;
+                const error = new Error("unauthorized");
+                error.response = { status: 401 };
+                throw error;
+            }
+        },
+        getToken: async () => ({ access_token: "token" }),
+        hashAPIPath: () => "&hash=abc",
+        azureHeaders: () => ({ headers: {} }),
+        redactSensitive: (value) => value,
+        consoleLogger: () => {}
+    });
+
+    let thrown = null;
+    try {
+        await mod.forwardGetData({
+            queryUrl: "incidents?$top=1",
+            maxRetries: 3,
+            retryBaseDelayMs: 0,
+            retryMaxDelayMs: 0
+        });
+    } catch (error) {
+        thrown = error;
+    }
+
+    assert.ok(thrown);
+    assert.strictEqual(callCount, 1);
+});
+
+test("relayGet avoids duplicate consoleLogger when relay layer already logged", async () => {
+    let consoleLoggerCalls = 0;
+    let respondErrorCalls = 0;
+
+    const mod = loadRelayForwardingModule({
+        axios: {
+            get: async () => {
+                const error = new Error("bad request");
+                error.response = { status: 400 };
+                throw error;
+            }
+        },
+        getToken: async () => ({ access_token: "token" }),
+        hashAPIPath: () => "&hash=abc",
+        azureHeaders: () => ({ headers: {} }),
+        redactSensitive: (value) => value,
+        consoleLogger: () => {
+            consoleLoggerCalls += 1;
+        },
+        respondSuccess: () => {},
+        respondError: () => {
+            respondErrorCalls += 1;
+        }
+    });
+
+    await mod.relayGet({
+        queryUrl: "incidents?$top=1",
+        res: {},
+        errorResponse: { code: "FAILED" }
+    });
+
+    assert.strictEqual(consoleLoggerCalls, 0);
+    assert.strictEqual(respondErrorCalls, 1);
+});
+
+test("forwardGetData clamps invalid env config values to safe bounds", async () => {
+    const previousEnv = {
+        RELAY_TIMEOUT_MS: process.env.RELAY_TIMEOUT_MS,
+        RELAY_RETRY_MAX: process.env.RELAY_RETRY_MAX,
+        RELAY_RETRY_BASE_DELAY_MS: process.env.RELAY_RETRY_BASE_DELAY_MS,
+        RELAY_RETRY_MAX_DELAY_MS: process.env.RELAY_RETRY_MAX_DELAY_MS
+    };
+
+    process.env.RELAY_TIMEOUT_MS = "999999";
+    process.env.RELAY_RETRY_MAX = "999";
+    process.env.RELAY_RETRY_BASE_DELAY_MS = "-1";
+    process.env.RELAY_RETRY_MAX_DELAY_MS = "999999";
+
+    let capturedOptions = null;
+
+    const mod = loadRelayForwardingModule({
+        axios: {
+            get: async (url, options) => {
+                capturedOptions = options;
+                return { data: { ok: true } };
+            }
+        },
+        getToken: async () => ({ access_token: "token" }),
+        hashAPIPath: () => "&hash=abc",
+        azureHeaders: () => ({ headers: {} }),
+        redactSensitive: (value) => value,
+        consoleLogger: () => {}
+    });
+
+    await mod.forwardGetData({
+        queryUrl: "incidents?$top=1"
+    });
+
+    assert.strictEqual(capturedOptions.timeout, 30000);
+
+    process.env.RELAY_TIMEOUT_MS = previousEnv.RELAY_TIMEOUT_MS;
+    process.env.RELAY_RETRY_MAX = previousEnv.RELAY_RETRY_MAX;
+    process.env.RELAY_RETRY_BASE_DELAY_MS =
+        previousEnv.RELAY_RETRY_BASE_DELAY_MS;
+    process.env.RELAY_RETRY_MAX_DELAY_MS = previousEnv.RELAY_RETRY_MAX_DELAY_MS;
+});
+
 test("forwardGetData applies timeout and appendHash=false behavior", async () => {
     let capturedUrl = null;
     let capturedOptions = null;