From 5315e73be53fce885760dc7bb64a2ff615cd041f Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:08:17 -0300 Subject: [PATCH] fix(combo): fail over on empty-content 502 instead of exhausting the provider (#5085) (#5104) --- CHANGELOG.md | 1 + open-sse/services/combo/targetExhaustion.ts | 16 +- .../combo-empty-content-failover-5085.test.ts | 162 ++++++++++++++++++ 3 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 tests/unit/combo-empty-content-failover-5085.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 211ecc02fd..84359c8f5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ _In development — bullets added per PR; finalized at release._ ### 🔧 Bug Fixes +- **fix(combo): empty-content 502 now fails over within the same request instead of exhausting the provider** — a leg that answers HTTP 200 with no usable completion is rewritten to `502 "Provider returned empty content"`, but the combo exhaustion classifier treated that synthetic 502 as a connection-level failure (`#1731v2`) and marked the whole provider/connection exhausted, skipping every remaining **same-provider** leg in that request. The connection is actually healthy (it just returned an empty body), so empty-content 502s are now classified as model-level transient failures: the request advances to the next leg and the rest of that provider's legs stay eligible. Genuine gateway 502s still trip connection exhaustion. ([#5085](https://github.com/diegosouzapw/OmniRoute/issues/5085), thanks @andrea-kingautomation) - **fix(executors): strip `client_metadata` from forwarded body for Cerebras and Mistral** — Cerebras returns 400 (`wrong_api_format`) and Mistral returns 422 (`extra_forbidden`) when the passthrough body carries `client_metadata` (an OpenAI Codex / Claude CLI field with no equivalent on these upstreams). The default executor now drops it for these two providers before sending downstream; other providers (notably `openai`/`codex`) keep it. (thanks @saurabh321gupta) - **fix(codebuddy):** only send reasoning params when the client requests reasoning. (thanks @anki1kr) - **fix(sse):** keep streaming for forceStream providers when a JSON client requests it. Providers marked `forceStream:true` reject `stream:false` upstream (HTTP 400); `resolveStreamFlag` now guards against this so stream-only providers keep streaming even when the client sends `Accept: application/json` or `stream:false`. (thanks @anki1kr) diff --git a/open-sse/services/combo/targetExhaustion.ts b/open-sse/services/combo/targetExhaustion.ts index fb4a371b3c..bf3d69dc57 100644 --- a/open-sse/services/combo/targetExhaustion.ts +++ b/open-sse/services/combo/targetExhaustion.ts @@ -24,6 +24,16 @@ import type { ComboLogger, ResolvedComboTarget } from "./types.ts"; // unreachable, proxy/gateway error), so remaining same-connection targets are skipped. const CONNECTION_LEVEL_ERROR_STATUSES = [408, 500, 502, 503, 504, 524]; +// #5085: an "empty content" 502 is the synthetic status chatCore assigns to a provider that +// answered HTTP 200 with no usable completion (isEmptyContentResponse). The connection is +// HEALTHY — it just returned an empty body — so this must NOT be classified as a connection +// failure (which would exhaust the whole provider/connection and skip every remaining +// same-provider leg via #1731v2). It is a model-level transient failure: advance to the next +// leg, leaving the rest of that provider's legs eligible. +function isEmptyContentFailure(status: number, errorText: string): boolean { + return status === 502 && /empty content/i.test(errorText); +} + export type ComboExhaustionSets = { exhaustedProviders: Set; exhaustedConnections: Set; @@ -106,7 +116,11 @@ function markConnectionLevelExhaustion( !provider || provider === "unknown" || !CONNECTION_LEVEL_ERROR_STATUSES.includes(result.status) || - isProviderCircuitOpenResult(result, errorText) + isProviderCircuitOpenResult(result, errorText) || + // #5085: empty-content 502 is a healthy connection returning no body — model-level, not + // connection-level. Don't exhaust the provider; let the remaining legs (incl. same-provider) + // be tried in-request. + isEmptyContentFailure(result.status, errorText) ) { return; } diff --git a/tests/unit/combo-empty-content-failover-5085.test.ts b/tests/unit/combo-empty-content-failover-5085.test.ts new file mode 100644 index 0000000000..e8188c7a0e --- /dev/null +++ b/tests/unit/combo-empty-content-failover-5085.test.ts @@ -0,0 +1,162 @@ +/** + * #5085 — A multi-leg combo whose first leg returns a 502 "Provider returned + * empty content" must FAIL OVER to the next leg within the same request, not + * surface the 502 to the caller. An empty completion is a fake-success failure + * (HTTP 200 with no content → rewritten to 502 in chatCore), and for a combo + * whose whole purpose is resilience it should behave like any other transient + * leg failure and advance. + * + * Reproduction: a `priority` combo with two legs on DIFFERENT providers. Leg 1 + * returns the empty-content 502 (exactly the body buildErrorBody produces in + * chatCore's isEmptyContentResponse branch); leg 2 returns a healthy 200. The + * combo must try leg 2 and surface its 200 — never return the leg-1 502. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-combo-5085-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "combo-5085-test-secret"; + +const { handleComboChat } = await import("../../open-sse/services/combo.ts"); + +const noop = () => {}; +const log = { info: noop, warn: noop, debug: noop, error: noop }; + +// Mirrors chatCore's empty-content branch: buildErrorBody(502, "Provider returned empty content"). +function emptyContent502() { + return new Response( + JSON.stringify({ error: { message: "Provider returned empty content", type: "bad_gateway" } }), + { status: 502, headers: { "Content-Type": "application/json" } } + ); +} + +function healthy200(model: string) { + return new Response( + JSON.stringify({ + id: "ok", + object: "chat.completion", + model, + choices: [{ index: 0, message: { role: "assistant", content: "hello from " + model }, finish_reason: "stop" }], + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); +} + +function makeCombo(models: string[]) { + return { + name: "test-combo-5085", + strategy: "priority", + models: models.map((m) => ({ model: m })), + }; +} + +test("#5085 combo fails over to the next leg when leg 1 returns empty-content 502", async () => { + const modelsCalled: string[] = []; + const handleSingleModel = async (_body: unknown, modelStr: string) => { + modelsCalled.push(modelStr); + // First leg (different provider) returns the empty-content 502; second leg is healthy. + if (modelsCalled.length === 1) return emptyContent502(); + return healthy200(modelStr); + }; + + const result = await handleComboChat({ + body: { model: "test", messages: [{ role: "user", content: "hi" }] }, + combo: makeCombo(["nvidia/minimaxai/minimax-m3", "openai/gpt-4o-mini"]), + handleSingleModel, + log, + settings: {}, + allCombos: [], + }); + + assert.equal( + modelsCalled.length, + 2, + `empty-content 502 on leg 1 must advance to leg 2, but tried: ${modelsCalled.join(", ")}` + ); + assert.equal( + result.status, + 200, + "the combo must surface the healthy second leg's 200, not the leg-1 empty-content 502" + ); +}); + +// ── Precise unit test of the exhaustion classifier ────────────────────────── +// The integration test above already passes because the empty-content leg and +// the healthy leg are on different providers. The real defect is provider-level: +// an empty-content 502 is currently classified as a CONNECTION-level failure +// (502 ∈ CONNECTION_LEVEL_ERROR_STATUSES), which marks the whole provider/ +// connection exhausted and skips every REMAINING SAME-PROVIDER leg (#1731v2). +// An empty completion arrived on a HEALTHY connection (HTTP 200, no content) and +// must not be treated as a bad connection. +const { applyComboTargetExhaustion } = await import("../../open-sse/services/combo/targetExhaustion.ts"); + +function makeTarget(provider: string, modelStr: string, connectionId: string | null = null) { + return { + kind: "model" as const, + stepId: "s", + executionKey: "e", + modelStr, + provider, + providerId: provider, + connectionId, + weight: 1, + label: null, + }; +} + +function freshSets() { + return { + exhaustedProviders: new Set(), + exhaustedConnections: new Set(), + transientRateLimitedProviders: new Set(), + }; +} + +test("#5085 empty-content 502 must NOT mark the provider/connection exhausted (model-level, not connection-level)", () => { + const sets = freshSets(); + const providerExhausted = applyComboTargetExhaustion(makeTarget("nvidia", "nvidia/minimaxai/minimax-m3"), { + result: { status: 502, headers: new Headers() }, + fallbackResult: { reason: "server_error" }, + errorText: "Provider returned empty content", + rawModel: "minimaxai/minimax-m3", + isTokenLimitBreach: false, + allAccountsRateLimited: false, + sets, + log, + tag: "COMBO", + exhaustedLogLevel: "info", + }); + + assert.equal(providerExhausted, false, "empty-content is not a quota exhaustion"); + assert.equal( + sets.exhaustedProviders.has("nvidia"), + false, + "empty-content 502 must NOT mark the whole provider exhausted — remaining same-provider legs must still be tried" + ); +}); + +test("#5085 a real connection-level 502 (gateway error) STILL marks the provider exhausted", () => { + const sets = freshSets(); + applyComboTargetExhaustion(makeTarget("nvidia", "nvidia/minimaxai/minimax-m3"), { + result: { status: 502, headers: new Headers() }, + fallbackResult: { reason: "server_error" }, + errorText: "Bad gateway: upstream connection reset", + rawModel: "minimaxai/minimax-m3", + isTokenLimitBreach: false, + allAccountsRateLimited: false, + sets, + log, + tag: "COMBO", + exhaustedLogLevel: "info", + }); + + assert.equal( + sets.exhaustedProviders.has("nvidia"), + true, + "a genuine gateway 502 must still mark the provider connection-exhausted (#1731v2 preserved)" + ); +});