diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index bf6e06d3cd..4b496ad0c6 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -22,6 +22,12 @@ import { getCircuitBreaker, STATE, } from "../../src/shared/utils/circuitBreaker"; +import { + classify429FromError, + type FailureKind, +} from "../../src/shared/utils/classify429"; +import { resolveUseUpstream429BreakerHints } from "../../src/shared/utils/providerHints"; + type ProviderProfile = { baseCooldownMs: number; @@ -532,9 +538,24 @@ function configureProviderBreaker( if (!provider) return null; const resolvedProfile = { ...getProviderProfile(provider), ...(profile ?? {}) }; + // Issue #2100 follow-up: resolve useUpstream429BreakerHints from the + // provider profile (stored override) or fall back to per-provider default. + // Stored value type is `boolean | undefined` — never `null` after PATCH. + const userValue = (resolvedProfile as { useUpstream429BreakerHints?: boolean }) + .useUpstream429BreakerHints; + const useHints = resolveUseUpstream429BreakerHints(provider, userValue); return getCircuitBreaker(provider, { failureThreshold: resolvedProfile.failureThreshold ?? resolvedProfile.circuitBreakerThreshold, resetTimeout: resolvedProfile.resetTimeoutMs ?? resolvedProfile.circuitBreakerReset, + ...(useHints + ? { + cooldownByKind: { + rate_limit: 60_000, + quota_exhausted: 3_600_000, + } satisfies Partial>, + classifyError: classify429FromError, + } + : {}), }); } diff --git a/src/app/(dashboard)/dashboard/settings/components/ResilienceTab.tsx b/src/app/(dashboard)/dashboard/settings/components/ResilienceTab.tsx index f3ca6f7105..1bcac39df0 100644 --- a/src/app/(dashboard)/dashboard/settings/components/ResilienceTab.tsx +++ b/src/app/(dashboard)/dashboard/settings/components/ResilienceTab.tsx @@ -16,6 +16,9 @@ type RequestQueueSettings = { type ConnectionCooldownProfileSettings = { baseCooldownMs: number; useUpstreamRetryHints: boolean; + // Issue #2100 follow-up. Optional / undefined when unset; the per-provider + // default in src/shared/utils/providerHints.ts resolves at runtime. + useUpstream429BreakerHints?: boolean; maxBackoffSteps: number; }; @@ -360,6 +363,48 @@ function ConnectionCooldownCard({ })) } /> +
+ +

+ Apply Retry-After / quota-exhausted signals from 429 responses to + circuit-breaker cooldown duration. Default uses a per-provider + policy: direct cloud providers default on; reverse-proxy / + self-hosted / CLI-backed providers default off. Independent of + "Use upstream retry hints". +

+
+
+ Use upstream 429 hints (breaker) + + {current.useUpstream429BreakerHints === true + ? "Yes" + : current.useUpstream429BreakerHints === false + ? "No" + : "Default"} + +
Max backoff steps {current.maxBackoffSteps} @@ -414,7 +469,26 @@ function ConnectionCooldownCard({ setEditing(false); }} onSave={async () => { - await onSave(draft); + // Build PATCH-ready payload: convert undefined useUpstream429BreakerHints + // to explicit null sentinel so the server treats it as unset (not as + // partial-merge "leave unchanged"). JSON.stringify drops undefined keys. + const payload = { + oauth: { + ...draft.oauth, + useUpstream429BreakerHints: + draft.oauth.useUpstream429BreakerHints === undefined + ? (null as unknown as boolean | undefined) + : draft.oauth.useUpstream429BreakerHints, + }, + apikey: { + ...draft.apikey, + useUpstream429BreakerHints: + draft.apikey.useUpstream429BreakerHints === undefined + ? (null as unknown as boolean | undefined) + : draft.apikey.useUpstream429BreakerHints, + }, + }; + await onSave(payload as typeof draft); setEditing(false); }} /> diff --git a/src/app/api/resilience/route.ts b/src/app/api/resilience/route.ts index 08fe0aeb05..c7c031ed7e 100644 --- a/src/app/api/resilience/route.ts +++ b/src/app/api/resilience/route.ts @@ -9,6 +9,7 @@ import { } from "@/lib/resilience/settings"; import { updateResilienceSchema } from "@/shared/validation/schemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; +import { resetAllCircuitBreakers } from "@/shared/utils/circuitBreaker"; type JsonRecord = Record; @@ -196,6 +197,20 @@ export async function PATCH(request) { }); await syncRuntimeSettings(nextResilience); + // Issue #2100 follow-up: detect transitions in useUpstream429BreakerHints + // and reset breakers so the registry stops serving cached options. + // Compared on STORED override transition (boolean | undefined) so that + // `null` (PATCH input) → undefined (stored) is correctly detected as + // "unset request" when the previous stored value was a boolean. + const breakerHintsChanged = + currentResilience.connectionCooldown.oauth.useUpstream429BreakerHints !== + nextResilience.connectionCooldown.oauth.useUpstream429BreakerHints || + currentResilience.connectionCooldown.apikey.useUpstream429BreakerHints !== + nextResilience.connectionCooldown.apikey.useUpstream429BreakerHints; + if (breakerHintsChanged) { + resetAllCircuitBreakers(); + } + return NextResponse.json({ ok: true, requestQueue: nextResilience.requestQueue, diff --git a/src/lib/resilience/settings.ts b/src/lib/resilience/settings.ts index f6f65e8faf..0c5dc34f77 100644 --- a/src/lib/resilience/settings.ts +++ b/src/lib/resilience/settings.ts @@ -14,6 +14,17 @@ export interface RequestQueueSettings { export interface ConnectionCooldownProfileSettings { baseCooldownMs: number; useUpstreamRetryHints: boolean; + /** + * Issue #2100 follow-up: opt-in toggle for upstream 429 hint trust at the + * circuit-breaker cooldown layer (independent of `useUpstreamRetryHints` + * which controls retry scheduling). + * + * Stored shape is intentionally optional / `boolean | undefined`: when + * unset, the per-provider default from `providerHints.ts` applies. + * Normalize/merge MUST preserve `undefined` — do not coerce via + * `toBoolean(value, fallback)`. + */ + useUpstream429BreakerHints?: boolean; maxBackoffSteps: number; } @@ -155,7 +166,29 @@ function normalizeConnectionCooldownProfile( fallback: ConnectionCooldownProfileSettings ): ConnectionCooldownProfileSettings { const record = asRecord(next); - return { + // useUpstream429BreakerHints uses a 3-state input contract: + // - boolean → user override, store as-is + // - null → explicit unset sentinel, drop key so the per-provider + // default in `providerHints.ts` resolves at runtime + // - omitted → leave existing fallback value unchanged (partial-merge) + // Never coerce via `toBoolean(value, fallback)` because that would + // collapse the unset state. + const hasHintsKey = Object.prototype.hasOwnProperty.call( + record, + "useUpstream429BreakerHints", + ); + const rawHints = record.useUpstream429BreakerHints; + let useUpstream429BreakerHints: boolean | undefined; + if (!hasHintsKey) { + useUpstream429BreakerHints = fallback.useUpstream429BreakerHints; + } else if (rawHints === null) { + useUpstream429BreakerHints = undefined; + } else if (typeof rawHints === "boolean") { + useUpstream429BreakerHints = rawHints; + } else { + useUpstream429BreakerHints = fallback.useUpstream429BreakerHints; + } + const out: ConnectionCooldownProfileSettings = { baseCooldownMs: toInteger(record.baseCooldownMs, fallback.baseCooldownMs, { min: 0, max: 24 * 60 * 60 * 1000, @@ -166,6 +199,11 @@ function normalizeConnectionCooldownProfile( max: 32, }), }; + // Only attach the key when defined — preserves omission across round-trips. + if (useUpstream429BreakerHints !== undefined) { + out.useUpstream429BreakerHints = useUpstream429BreakerHints; + } + return out; } function normalizeLegacyConnectionCooldownProfile( diff --git a/src/shared/utils/classify429.ts b/src/shared/utils/classify429.ts index 715873cd9e..66ae2e45d1 100644 --- a/src/shared/utils/classify429.ts +++ b/src/shared/utils/classify429.ts @@ -162,3 +162,63 @@ export function retryAfterFromResponse(response: { }): number | null { return parseRetryAfter(getHeader(response.headers, "retry-after")); } + +/** + * Adapter that takes an error thrown by an HTTP client (fetch wrapper, axios, + * upstream SDK, etc.) and produces a {@link FailureKind} suitable for the + * `classifyError` option of the circuit breaker. + * + * Recognises the common error shapes: + * - `err.status` + `err.headers` + `err.body` (low-level fetch wrapper) + * - `err.response.status` + `err.response.headers` + `err.response.data` (axios-style) + * - `err.message` (last-resort body for keyword scan) + * + * Returns `undefined` when the error doesn't carry enough information to + * classify, so the breaker can decide what to do without a kind tag. + * + * Companion to issue #2100 follow-up. + */ +export function classify429FromError(err: unknown): FailureKind | undefined { + if (err === null || typeof err !== "object") return undefined; + const e = err as Record; + + let status: number | undefined; + let headers: Record | undefined; + let body: unknown; + + if (typeof e.status === "number") { + status = e.status; + } + if (typeof e.statusCode === "number" && status === undefined) { + status = e.statusCode; + } + + if (e.response && typeof e.response === "object") { + const resp = e.response as Record; + if (typeof resp.status === "number" && status === undefined) { + status = resp.status; + } + if (resp.headers && typeof resp.headers === "object") { + headers = resp.headers as Record; + } + if (resp.data !== undefined) { + body = resp.data; + } else if (typeof resp.body !== "undefined") { + body = resp.body; + } + } + + if (headers === undefined && e.headers && typeof e.headers === "object") { + headers = e.headers as Record; + } + if (body === undefined) { + if (typeof e.body !== "undefined") { + body = e.body; + } else if (typeof e.message === "string") { + body = e.message; + } + } + + if (typeof status !== "number") return undefined; + return classify429({ status, headers, body }); +} diff --git a/src/shared/utils/providerHints.ts b/src/shared/utils/providerHints.ts new file mode 100644 index 0000000000..353c869d2f --- /dev/null +++ b/src/shared/utils/providerHints.ts @@ -0,0 +1,73 @@ +/** + * Per-provider default policy for upstream 429 hint trust. + * + * @see Issue #2100 follow-up — surface a user-overridable per-profile toggle + * that decides whether the circuit breaker uses upstream 429 body / Retry-After + * hints (`classify429`, `cooldownByKind`) to differentiate rate-limit from + * quota-exhausted failure cooldowns. + * + * This helper returns the **default** answer for a given provider. The actual + * runtime decision is the user override (if any) OR this default. See + * `accountFallback.ts` / `chat.ts` / `chatHelpers.ts` for the resolution + * call sites: + * + * ```ts + * const userValue = providerProfile.useUpstream429BreakerHints; // boolean | undefined + * const useHints = userValue !== undefined + * ? userValue + * : defaultUseUpstream429BreakerHints(provider); + * ``` + * + * Default policy: direct cloud providers default `true` because their 429 + * bodies and `Retry-After` headers are authoritative. Reverse-proxy / + * self-hosted / CLI-backed providers default `false` because forwarded 429 + * metadata is often unreliable or fabricated by the proxy. + * + * @module shared/utils/providerHints + */ + +import { + UPSTREAM_PROXY_PROVIDERS, + SELF_HOSTED_CHAT_PROVIDER_IDS, + isLocalProvider, + isClaudeCodeCompatibleProvider, +} from "../constants/providers"; + +/** + * Conservative per-provider default for `useUpstream429BreakerHints`. + * + * Returns `false` for any provider whose 429 metadata may be forwarded by + * an intermediary (proxy, self-hosted runtime, CLI wrapper). Returns `true` + * for direct cloud providers where the upstream response is authoritative. + */ +export function defaultUseUpstream429BreakerHints(providerId: string): boolean { + if (Object.prototype.hasOwnProperty.call(UPSTREAM_PROXY_PROVIDERS, providerId)) { + return false; + } + if (isLocalProvider(providerId)) { + return false; + } + if (SELF_HOSTED_CHAT_PROVIDER_IDS.has(providerId)) { + return false; + } + if (isClaudeCodeCompatibleProvider(providerId)) { + return false; + } + return true; +} + +/** + * Resolve the effective `useHints` decision: the user override wins if set, + * otherwise fall back to the per-provider default. + * + * `undefined` means "not user-set" and triggers the default lookup. + */ +export function resolveUseUpstream429BreakerHints( + providerId: string, + userValue: boolean | undefined, +): boolean { + if (userValue !== undefined) { + return userValue; + } + return defaultUseUpstream429BreakerHints(providerId); +} diff --git a/src/shared/validation/schemas.ts b/src/shared/validation/schemas.ts index ae5aeb25cb..6dba0a55dc 100644 --- a/src/shared/validation/schemas.ts +++ b/src/shared/validation/schemas.ts @@ -775,6 +775,11 @@ const connectionCooldownProfileSchema = z .object({ baseCooldownMs: z.number().int().min(0).optional(), useUpstreamRetryHints: z.boolean().optional(), + // Issue #2100 follow-up: per-profile toggle for upstream 429 hint trust. + // `null` is an explicit unset sentinel — PATCH handler deletes the key + // from stored settings so the per-provider default resolves at runtime. + // `undefined` (key omitted) means "leave existing value unchanged". + useUpstream429BreakerHints: z.boolean().nullable().optional(), maxBackoffSteps: z.number().int().min(0).optional(), }) .strict(); diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 62915eb998..cd0576b14f 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -45,6 +45,11 @@ import { } from "./chatHelpers"; // Pipeline integration — wired modules +import { + classify429FromError, + type FailureKind, +} from "@/shared/utils/classify429"; +import { resolveUseUpstream429BreakerHints } from "@/shared/utils/providerHints"; import { getCircuitBreaker } from "../../shared/utils/circuitBreaker"; import { markAccountExhaustedFrom429 } from "../../domain/quotaCache"; import { RequestTelemetry, recordTelemetry } from "../../shared/utils/requestTelemetry"; @@ -627,11 +632,25 @@ async function handleSingleModelChat( }); if (gate) return gate; + // Issue #2100 follow-up: opt-in upstream 429 hint trust per provider. + const useHints429 = resolveUseUpstream429BreakerHints( + provider, + (providerProfile as { useUpstream429BreakerHints?: boolean }).useUpstream429BreakerHints, + ); const breaker = getCircuitBreaker(provider, { failureThreshold: providerProfile.failureThreshold, resetTimeout: providerProfile.resetTimeoutMs, onStateChange: (name: string, from: string, to: string) => log.info("CIRCUIT", `${name}: ${from} → ${to}`), + ...(useHints429 + ? { + cooldownByKind: { + rate_limit: 60_000, + quota_exhausted: 3_600_000, + } satisfies Partial>, + classifyError: classify429FromError, + } + : {}), }); const userAgent = request?.headers?.get("user-agent") || ""; diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index 3792a2b26a..fbc2c32263 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -25,6 +25,12 @@ import { } from "@omniroute/open-sse/utils/proxyFetch.ts"; import { resolveProxyForConnection } from "@/lib/localDb"; import { CircuitBreakerOpenError, getCircuitBreaker } from "../../shared/utils/circuitBreaker"; +import { + classify429FromError, + type FailureKind, +} from "../../shared/utils/classify429"; +import { resolveUseUpstream429BreakerHints } from "../../shared/utils/providerHints"; + import { logProxyEvent } from "../../lib/proxyLogger"; import { logTranslationEvent } from "../../lib/translatorEvents"; import { getRuntimeProviderProfile } from "@omniroute/open-sse/services/accountFallback.ts"; @@ -238,11 +244,25 @@ export async function checkPipelineGates( ) { const bypassReason = options.bypassReason || "pipeline override"; const providerProfile = options.providerProfile ?? (await getRuntimeProviderProfile(provider)); + // Issue #2100 follow-up: opt-in upstream 429 hint trust per provider. + const useHints429 = resolveUseUpstream429BreakerHints( + provider, + (providerProfile as { useUpstream429BreakerHints?: boolean }).useUpstream429BreakerHints, + ); const breaker = getCircuitBreaker(provider, { failureThreshold: providerProfile.failureThreshold ?? providerProfile.circuitBreakerThreshold, resetTimeout: providerProfile.resetTimeoutMs ?? providerProfile.circuitBreakerReset, onStateChange: (name: string, from: string, to: string) => log.info("CIRCUIT", `${name}: ${from} → ${to}`), + ...(useHints429 + ? { + cooldownByKind: { + rate_limit: 60_000, + quota_exhausted: 3_600_000, + } satisfies Partial>, + classifyError: classify429FromError, + } + : {}), }); if (options.ignoreCircuitBreaker && !breaker.canExecute()) { log.info("CIRCUIT", `Bypassing OPEN circuit breaker for ${provider} (${bypassReason})`); diff --git a/tests/unit/provider-hints.test.ts b/tests/unit/provider-hints.test.ts new file mode 100644 index 0000000000..bee31d57aa --- /dev/null +++ b/tests/unit/provider-hints.test.ts @@ -0,0 +1,58 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + defaultUseUpstream429BreakerHints, + resolveUseUpstream429BreakerHints, +} from "../../src/shared/utils/providerHints.ts"; + +test("defaultUseUpstream429BreakerHints: direct cloud providers default true", () => { + for (const id of ["openai", "anthropic", "groq", "cerebras", "mistral", "google"]) { + assert.equal( + defaultUseUpstream429BreakerHints(id), + true, + `expected true for ${id}`, + ); + } +}); + +test("defaultUseUpstream429BreakerHints: cliproxyapi defaults false", () => { + assert.equal(defaultUseUpstream429BreakerHints("cliproxyapi"), false); +}); + +test("defaultUseUpstream429BreakerHints: self-hosted chat providers default false", () => { + for (const id of ["lm-studio", "vllm", "lemonade", "llamafile", "triton", "xinference", "oobabooga"]) { + assert.equal( + defaultUseUpstream429BreakerHints(id), + false, + `expected false for ${id}`, + ); + } +}); + +test("defaultUseUpstream429BreakerHints: claude-code-* prefix defaults false", () => { + for (const id of ["anthropic-compatible-cc-direct", "anthropic-compatible-cc-bedrock", "anthropic-compatible-cc-vertex"]) { + assert.equal( + defaultUseUpstream429BreakerHints(id), + false, + `expected false for ${id}`, + ); + } +}); + +test("resolveUseUpstream429BreakerHints: user override wins (both directions)", () => { + // Cloud provider with user override OFF → false + assert.equal(resolveUseUpstream429BreakerHints("openai", false), false); + // Cloud provider with user override ON → true (no-op vs default) + assert.equal(resolveUseUpstream429BreakerHints("openai", true), true); + // Proxy provider with user override ON → true (user explicitly trusted it) + assert.equal(resolveUseUpstream429BreakerHints("cliproxyapi", true), true); + // Proxy provider with user override OFF → false (no-op vs default) + assert.equal(resolveUseUpstream429BreakerHints("cliproxyapi", false), false); +}); + +test("resolveUseUpstream429BreakerHints: undefined falls back to per-provider default", () => { + // Critical: this is the v3 regression-test for the v1 default-vs-gate bug. + assert.equal(resolveUseUpstream429BreakerHints("openai", undefined), true); + assert.equal(resolveUseUpstream429BreakerHints("cliproxyapi", undefined), false); + assert.equal(resolveUseUpstream429BreakerHints("lm-studio", undefined), false); +}); diff --git a/tests/unit/resilience-settings-upstream429-breaker.test.ts b/tests/unit/resilience-settings-upstream429-breaker.test.ts new file mode 100644 index 0000000000..e8812589d2 --- /dev/null +++ b/tests/unit/resilience-settings-upstream429-breaker.test.ts @@ -0,0 +1,94 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + DEFAULT_RESILIENCE_SETTINGS, + mergeResilienceSettings, + resolveResilienceSettings, + type ResilienceSettings, +} from "../../src/lib/resilience/settings.ts"; + +function cloneDefaults(): ResilienceSettings { + // structuredClone is enough for the plain-object settings shape. + return structuredClone(DEFAULT_RESILIENCE_SETTINGS); +} + +test("defaults: useUpstream429BreakerHints omitted (undefined) on both profiles", () => { + const settings = cloneDefaults(); + assert.equal(settings.connectionCooldown.oauth.useUpstream429BreakerHints, undefined); + assert.equal(settings.connectionCooldown.apikey.useUpstream429BreakerHints, undefined); +}); + +test("mergeResilienceSettings: explicit true is stored", () => { + const current = cloneDefaults(); + const next = mergeResilienceSettings(current, { + connectionCooldown: { oauth: { useUpstream429BreakerHints: true } }, + }); + assert.equal(next.connectionCooldown.oauth.useUpstream429BreakerHints, true); + // apikey unchanged + assert.equal(next.connectionCooldown.apikey.useUpstream429BreakerHints, undefined); +}); + +test("mergeResilienceSettings: explicit false is stored", () => { + const current = cloneDefaults(); + const next = mergeResilienceSettings(current, { + connectionCooldown: { apikey: { useUpstream429BreakerHints: false } }, + }); + assert.equal(next.connectionCooldown.apikey.useUpstream429BreakerHints, false); +}); + +test("mergeResilienceSettings: null sentinel deletes the field (back to undefined)", () => { + // Start with explicit false on oauth + const start = mergeResilienceSettings(cloneDefaults(), { + connectionCooldown: { oauth: { useUpstream429BreakerHints: false } }, + }); + assert.equal(start.connectionCooldown.oauth.useUpstream429BreakerHints, false); + // PATCH with null should reset to undefined + const next = mergeResilienceSettings(start, { + connectionCooldown: { + oauth: { useUpstream429BreakerHints: null as unknown as boolean }, + }, + }); + assert.equal(next.connectionCooldown.oauth.useUpstream429BreakerHints, undefined); + // Key should not appear in JSON + const serialized = JSON.parse(JSON.stringify(next.connectionCooldown.oauth)); + assert.equal( + "useUpstream429BreakerHints" in serialized, + false, + "key should be absent in serialized JSON", + ); +}); + +test("mergeResilienceSettings: omitted key (partial-merge) leaves existing value", () => { + // Start with explicit true on apikey + const start = mergeResilienceSettings(cloneDefaults(), { + connectionCooldown: { apikey: { useUpstream429BreakerHints: true } }, + }); + // PATCH oauth only — apikey must keep its value + const next = mergeResilienceSettings(start, { + connectionCooldown: { oauth: { baseCooldownMs: 5000 } }, + }); + assert.equal(next.connectionCooldown.apikey.useUpstream429BreakerHints, true); +}); + +test("resolveResilienceSettings: omitted field in record stays undefined (no toBoolean coercion)", () => { + const record = { + connectionCooldown: { + oauth: { baseCooldownMs: 1000, useUpstreamRetryHints: true, maxBackoffSteps: 5 }, + apikey: { baseCooldownMs: 2000, useUpstreamRetryHints: false, maxBackoffSteps: 3 }, + }, + }; + const resolved = resolveResilienceSettings(record as Parameters[0]); + assert.equal(resolved.connectionCooldown.oauth.useUpstream429BreakerHints, undefined); + assert.equal(resolved.connectionCooldown.apikey.useUpstream429BreakerHints, undefined); +}); + +test("mixed-provider round-trip: oauth=false + apikey=true survives merge", () => { + const next = mergeResilienceSettings(cloneDefaults(), { + connectionCooldown: { + oauth: { useUpstream429BreakerHints: false }, + apikey: { useUpstream429BreakerHints: true }, + }, + }); + assert.equal(next.connectionCooldown.oauth.useUpstream429BreakerHints, false); + assert.equal(next.connectionCooldown.apikey.useUpstream429BreakerHints, true); +});