feat(relay): add policy overrides and idempotency-aware retry gating
This commit is contained in:
@@ -1263,3 +1263,40 @@ Validation:
|
|||||||
Follow-ups:
|
Follow-ups:
|
||||||
|
|
||||||
- Optional next step: map these lifecycle fields into central dashboards/alerts (retry rate, status-class distribution, p95 elapsedMs).
|
- 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.
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ const DEFAULT_TIMEOUT_MS = 8000;
|
|||||||
const DEFAULT_MAX_RETRIES = 2;
|
const DEFAULT_MAX_RETRIES = 2;
|
||||||
const DEFAULT_RETRY_BASE_DELAY_MS = 200;
|
const DEFAULT_RETRY_BASE_DELAY_MS = 200;
|
||||||
const DEFAULT_RETRY_MAX_DELAY_MS = 1200;
|
const DEFAULT_RETRY_MAX_DELAY_MS = 1200;
|
||||||
|
const DEFAULT_ALLOW_NON_IDEMPOTENT_RETRIES = false;
|
||||||
const MAX_TIMEOUT_MS = 30000;
|
const MAX_TIMEOUT_MS = 30000;
|
||||||
const MAX_RETRIES = 4;
|
const MAX_RETRIES = 4;
|
||||||
const MAX_RETRY_DELAY_MS = 5000;
|
const MAX_RETRY_DELAY_MS = 5000;
|
||||||
@@ -29,11 +30,24 @@ const RETRYABLE_ERROR_CODES = new Set([
|
|||||||
"EPIPE"
|
"EPIPE"
|
||||||
]);
|
]);
|
||||||
|
|
||||||
|
const IDEMPOTENT_RETRY_METHODS = new Set(["GET", "HEAD", "OPTIONS"]);
|
||||||
|
|
||||||
const parsePositiveInt = (value, fallback) => {
|
const parsePositiveInt = (value, fallback) => {
|
||||||
const parsed = Number.parseInt(value, 10);
|
const parsed = Number.parseInt(value, 10);
|
||||||
return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback;
|
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) => {
|
const clamp = (value, min, max) => {
|
||||||
return Math.min(max, Math.max(min, value));
|
return Math.min(max, Math.max(min, value));
|
||||||
};
|
};
|
||||||
@@ -65,6 +79,24 @@ const resolveNumericOverride = ({
|
|||||||
return clamp(Math.floor(overrideValue), lowerBound, max);
|
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 = () => {
|
const getRelayConfig = () => {
|
||||||
return {
|
return {
|
||||||
timeoutMs: sanitizeNumberConfig({
|
timeoutMs: sanitizeNumberConfig({
|
||||||
@@ -93,7 +125,11 @@ const getRelayConfig = () => {
|
|||||||
min: 0,
|
min: 0,
|
||||||
max: MAX_RETRY_DELAY_MS,
|
max: MAX_RETRY_DELAY_MS,
|
||||||
allowZero: true
|
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`;
|
return `${Math.floor(status / 100)}xx`;
|
||||||
};
|
};
|
||||||
|
|
||||||
const shouldRetryRelayError = ({ error, attempt, maxRetries }) => {
|
const shouldRetryRelayError = ({
|
||||||
|
error,
|
||||||
|
attempt,
|
||||||
|
maxRetries,
|
||||||
|
method,
|
||||||
|
allowNonIdempotentRetries
|
||||||
|
}) => {
|
||||||
if (attempt >= maxRetries) return false;
|
if (attempt >= maxRetries) return false;
|
||||||
|
if (!canRetryForMethod({ method, allowNonIdempotentRetries })) return false;
|
||||||
|
|
||||||
const status = error?.response?.status;
|
const status = error?.response?.status;
|
||||||
if (NON_RETRYABLE_STATUS_CODES.has(status)) return false;
|
if (NON_RETRYABLE_STATUS_CODES.has(status)) return false;
|
||||||
@@ -139,12 +182,14 @@ export const relayGet = async ({
|
|||||||
res,
|
res,
|
||||||
errorResponse,
|
errorResponse,
|
||||||
transformData,
|
transformData,
|
||||||
requestOptionsBuilder
|
requestOptionsBuilder,
|
||||||
|
relayPolicy
|
||||||
}) => {
|
}) => {
|
||||||
try {
|
try {
|
||||||
const { data, accessToken } = await relayGetData({
|
const { data, accessToken } = await relayGetData({
|
||||||
queryUrl,
|
queryUrl,
|
||||||
requestOptionsBuilder
|
requestOptionsBuilder,
|
||||||
|
relayPolicy
|
||||||
});
|
});
|
||||||
|
|
||||||
return respondSuccess(
|
return respondSuccess(
|
||||||
@@ -164,12 +209,14 @@ export const relayGet = async ({
|
|||||||
export const relayGetData = async ({
|
export const relayGetData = async ({
|
||||||
queryUrl,
|
queryUrl,
|
||||||
requestOptionsBuilder,
|
requestOptionsBuilder,
|
||||||
accessToken
|
accessToken,
|
||||||
|
relayPolicy
|
||||||
}) => {
|
}) => {
|
||||||
return forwardGetData({
|
return forwardGetData({
|
||||||
queryUrl,
|
queryUrl,
|
||||||
requestOptionsBuilder,
|
requestOptionsBuilder,
|
||||||
accessToken
|
accessToken,
|
||||||
|
relayPolicy
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -182,19 +229,26 @@ export const forwardGetData = async ({
|
|||||||
timeoutMs,
|
timeoutMs,
|
||||||
maxRetries,
|
maxRetries,
|
||||||
retryBaseDelayMs,
|
retryBaseDelayMs,
|
||||||
retryMaxDelayMs
|
retryMaxDelayMs,
|
||||||
|
method,
|
||||||
|
relayPolicy
|
||||||
}) => {
|
}) => {
|
||||||
const relayConfig = getRelayConfig();
|
const relayConfig = getRelayConfig();
|
||||||
|
const relayPolicyConfig = relayPolicy || {};
|
||||||
|
|
||||||
|
const resolvedMethod = resolveRelayMethod(
|
||||||
|
relayPolicyConfig.method || method
|
||||||
|
);
|
||||||
|
|
||||||
const resolvedTimeoutMs = resolveNumericOverride({
|
const resolvedTimeoutMs = resolveNumericOverride({
|
||||||
overrideValue: timeoutMs,
|
overrideValue: relayPolicyConfig.timeoutMs ?? timeoutMs,
|
||||||
fallbackValue: relayConfig.timeoutMs,
|
fallbackValue: relayConfig.timeoutMs,
|
||||||
min: 100,
|
min: 100,
|
||||||
max: MAX_TIMEOUT_MS
|
max: MAX_TIMEOUT_MS
|
||||||
});
|
});
|
||||||
|
|
||||||
const resolvedMaxRetries = resolveNumericOverride({
|
const resolvedMaxRetries = resolveNumericOverride({
|
||||||
overrideValue: maxRetries,
|
overrideValue: relayPolicyConfig.maxRetries ?? maxRetries,
|
||||||
fallbackValue: relayConfig.maxRetries,
|
fallbackValue: relayConfig.maxRetries,
|
||||||
min: 0,
|
min: 0,
|
||||||
max: MAX_RETRIES,
|
max: MAX_RETRIES,
|
||||||
@@ -202,7 +256,7 @@ export const forwardGetData = async ({
|
|||||||
});
|
});
|
||||||
|
|
||||||
const resolvedRetryBaseDelayMs = resolveNumericOverride({
|
const resolvedRetryBaseDelayMs = resolveNumericOverride({
|
||||||
overrideValue: retryBaseDelayMs,
|
overrideValue: relayPolicyConfig.retryBaseDelayMs ?? retryBaseDelayMs,
|
||||||
fallbackValue: relayConfig.retryBaseDelayMs,
|
fallbackValue: relayConfig.retryBaseDelayMs,
|
||||||
min: 0,
|
min: 0,
|
||||||
max: MAX_RETRY_DELAY_MS,
|
max: MAX_RETRY_DELAY_MS,
|
||||||
@@ -210,13 +264,20 @@ export const forwardGetData = async ({
|
|||||||
});
|
});
|
||||||
|
|
||||||
const resolvedRetryMaxDelayMs = resolveNumericOverride({
|
const resolvedRetryMaxDelayMs = resolveNumericOverride({
|
||||||
overrideValue: retryMaxDelayMs,
|
overrideValue: relayPolicyConfig.retryMaxDelayMs ?? retryMaxDelayMs,
|
||||||
fallbackValue: relayConfig.retryMaxDelayMs,
|
fallbackValue: relayConfig.retryMaxDelayMs,
|
||||||
min: 0,
|
min: 0,
|
||||||
max: MAX_RETRY_DELAY_MS,
|
max: MAX_RETRY_DELAY_MS,
|
||||||
allowZero: true
|
allowZero: true
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const resolvedAllowNonIdempotentRetries = resolveBooleanOverride({
|
||||||
|
overrideValue:
|
||||||
|
relayPolicyConfig.allowNonIdempotentRetries ??
|
||||||
|
relayPolicyConfig.allowRetriesForNonIdempotent,
|
||||||
|
fallbackValue: relayConfig.allowNonIdempotentRetries
|
||||||
|
});
|
||||||
|
|
||||||
const tokenAccessToken =
|
const tokenAccessToken =
|
||||||
typeof accessToken === "string" && accessToken.length > 0
|
typeof accessToken === "string" && accessToken.length > 0
|
||||||
? accessToken
|
? accessToken
|
||||||
@@ -246,10 +307,12 @@ export const forwardGetData = async ({
|
|||||||
relayRequestId,
|
relayRequestId,
|
||||||
queryUrl,
|
queryUrl,
|
||||||
baseUrl: resolvedBaseUrl,
|
baseUrl: resolvedBaseUrl,
|
||||||
|
method: resolvedMethod,
|
||||||
timeoutMs: resolvedTimeoutMs,
|
timeoutMs: resolvedTimeoutMs,
|
||||||
maxRetries: resolvedMaxRetries,
|
maxRetries: resolvedMaxRetries,
|
||||||
retryBaseDelayMs: resolvedRetryBaseDelayMs,
|
retryBaseDelayMs: resolvedRetryBaseDelayMs,
|
||||||
retryMaxDelayMs: resolvedRetryMaxDelayMs
|
retryMaxDelayMs: resolvedRetryMaxDelayMs,
|
||||||
|
allowNonIdempotentRetries: resolvedAllowNonIdempotentRetries
|
||||||
});
|
});
|
||||||
|
|
||||||
let lastError;
|
let lastError;
|
||||||
@@ -279,7 +342,9 @@ export const forwardGetData = async ({
|
|||||||
const retryEligible = shouldRetryRelayError({
|
const retryEligible = shouldRetryRelayError({
|
||||||
error,
|
error,
|
||||||
attempt,
|
attempt,
|
||||||
maxRetries: resolvedMaxRetries
|
maxRetries: resolvedMaxRetries,
|
||||||
|
method: resolvedMethod,
|
||||||
|
allowNonIdempotentRetries: resolvedAllowNonIdempotentRetries
|
||||||
});
|
});
|
||||||
|
|
||||||
if (!retryEligible) {
|
if (!retryEligible) {
|
||||||
@@ -288,6 +353,7 @@ export const forwardGetData = async ({
|
|||||||
relayRequestId,
|
relayRequestId,
|
||||||
queryUrl,
|
queryUrl,
|
||||||
baseUrl: resolvedBaseUrl,
|
baseUrl: resolvedBaseUrl,
|
||||||
|
method: resolvedMethod,
|
||||||
attempt: attempt + 1,
|
attempt: attempt + 1,
|
||||||
maxAttempts: resolvedMaxRetries + 1,
|
maxAttempts: resolvedMaxRetries + 1,
|
||||||
attemptsMade: attempt + 1,
|
attemptsMade: attempt + 1,
|
||||||
@@ -311,6 +377,7 @@ export const forwardGetData = async ({
|
|||||||
relayRequestId,
|
relayRequestId,
|
||||||
queryUrl,
|
queryUrl,
|
||||||
baseUrl: resolvedBaseUrl,
|
baseUrl: resolvedBaseUrl,
|
||||||
|
method: resolvedMethod,
|
||||||
attempt: attempt + 1,
|
attempt: attempt + 1,
|
||||||
maxAttempts: resolvedMaxRetries + 1,
|
maxAttempts: resolvedMaxRetries + 1,
|
||||||
attemptsMade: attempt + 1,
|
attemptsMade: attempt + 1,
|
||||||
|
|||||||
@@ -369,6 +369,152 @@ test("forwardGetData telemetry request id is stable across lifecycle events", as
|
|||||||
assert.strictEqual(new Set(relayRequestIds).size, 1);
|
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 () => {
|
const run = async () => {
|
||||||
let passed = 0;
|
let passed = 0;
|
||||||
for (const currentTest of tests) {
|
for (const currentTest of tests) {
|
||||||
|
|||||||
Reference in New Issue
Block a user