diff --git a/CHANGELOG.md b/CHANGELOG.md index d950be6f84..0df8e616bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### 🐛 Fixed - **fix(intelligence): run pricing + models.dev sync from the live startup path** — like the Arena ELO sync (v3.8.24), the external **pricing sync** (`PRICING_SYNC_ENABLED`) and the **models.dev capability sync** (Settings → AI toggle) were only initialized from `server-init.ts`, which the Next standalone runtime never executes — and models.dev had no caller at all. Their toggles were inert in production. Both are now initialized from `instrumentation-node.ts` (self-gated, opt-in preserved, non-blocking, never fatal). (thanks @diegosouzapw) +- **fix(sse): retry once on an early stream close (`STREAM_EARLY_EOF`) for single-model requests** — flaky OpenAI-compatible upstreams (e.g. NVIDIA NIM with minimax-m3 / qwen3.5 / glm-5.1) intermittently send HTTP 200 then close the SSE with zero useful frames, surfacing as a 502 "Stream ended before producing useful content". Only Antigravity got an early-close retry; every other provider returned the 502 immediately on the non-combo single-model path. A bounded one-retry (early-close only — not readiness-timeout — and without marking the account unavailable) now generalizes it. (The separate qwen-web validation SSRF part of the same report was already fixed in v3.8.24, [#3767](https://github.com/diegosouzapw/OmniRoute/pull/3767).) ([#3758](https://github.com/diegosouzapw/OmniRoute/issues/3758) — thanks @Svatosalav) - **fix(models): preserve eye-hidden models across auto-sync / import** — hiding models via the visibility (eye) toggle to keep only a combo's models was undone on every model import or auto-sync, which re-showed all of them. The sync re-import treated "hidden" identically to "deleted" and dropped both; a distinct `isDeleted` marker now separates the trash/delete path (still dropped on re-import, #3199) from the eye toggle (preserved as listed-but-hidden), and eye-hidden models are no longer re-aliased into the routable catalog on sync. ([#3782](https://github.com/diegosouzapw/OmniRoute/issues/3782) — thanks @xenstar) - **fix(providers): correct the lmarena cookie hint (`session` → `arena-auth-prod-v1`)** — the lmarena credential hint asked for a cookie named `session`, but lmarena.ai's real auth cookie is `arena-auth-prod-v1`, so users who pasted only `session=…` hit validation failures. The credential name, placeholder and storage keys now use the correct name (the legacy `session` key is retained for back-compat with already-saved credentials). ([#3810](https://github.com/diegosouzapw/OmniRoute/issues/3810) — thanks @xspylol) diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index e8bb5317a8..78dabc2919 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -53,6 +53,7 @@ import { handleNoCredentials, safeResolveProxy, safeLogEvents, + shouldRetryStreamEarlyEof, withSessionHeader, } from "./chatHelpers"; import { connectionHasExtraKeys } from "@omniroute/open-sse/services/apiKeyRotator.ts"; @@ -98,10 +99,7 @@ import { resolveCooldownAwareRetrySettings, waitForCooldownAwareRetry, } from "../services/cooldownAwareRetry"; -import { - constrainConnectionsToQuota, - resolveQuotaKeyScope, -} from "../../lib/quota/quotaKey"; +import { constrainConnectionsToQuota, resolveQuotaKeyScope } from "../../lib/quota/quotaKey"; registerCodexQuotaFetcher(); @@ -772,7 +770,14 @@ async function handleSingleModelChat( }); } - const { provider: resolvedProvider, model, sourceFormat, targetFormat, extendedContext, apiFormat } = resolved; + const { + provider: resolvedProvider, + model, + sourceFormat, + targetFormat, + extendedContext, + apiFormat, + } = resolved; // Prefer the combo target's providerId when available — the model string's // provider prefix may differ from the credential provider ID (e.g. model // "xiaomi/mimo-v2-flash" resolves to provider "xiaomi" but the combo target @@ -877,6 +882,10 @@ async function handleSingleModelChat( let requestRetryLastError = null; let requestRetryLastStatus = null; let requestRetryLastCooldownMs = 0; + // Bug #3758: per-request counter bounding the early-close (STREAM_EARLY_EOF) + // re-attempt to exactly one for the whole request. Declared outside both retry + // loops so it can never reset and loop. + let streamEarlyEofRetries = 0; requestAttemptLoop: while (true) { const excludedConnectionIds = new Set(); @@ -1097,6 +1106,27 @@ async function handleSingleModelChat( (result.errorType === "stream_timeout" || result.errorType === "stream_early_eof") && !isAntigravityStreamReadinessFailure ) { + // Bug #3758: flaky OpenAI-compatible upstreams (e.g. NVIDIA NIM) sometimes + // send HTTP 200 then close the SSE early with zero useful frames + // (STREAM_EARLY_EOF). That is a transient upstream glitch, not a bad key — so + // allow exactly ONE bounded same-connection re-attempt before surfacing the + // 502. Do NOT retry STREAM_READINESS_TIMEOUT (a slow-but-alive upstream; + // retrying would only double latency) and do NOT mark the account unavailable + // for the early close. + if ( + shouldRetryStreamEarlyEof(result.errorCode, streamEarlyEofRetries) && + !hasForcedConnection + ) { + streamEarlyEofRetries += 1; + log.warn( + "STREAM", + `${provider}/${model} closed the stream early before useful content — retrying once (attempt ${streamEarlyEofRetries})` + ); + // Plain re-attempt of the same request: no markAccountUnavailable, no + // excludedConnectionIds mutation (an early close is not a bad connection). + continue; + } + // Stream readiness timeout is an upstream stall after an HTTP response was received, // not an account/quota failure. Do NOT mark the account unavailable here. return result.response; @@ -1357,8 +1387,11 @@ async function handleSingleModelChat( model, providerProfile, { - persistUnavailableState: - !(isCombo && result.status === 429 && (failureKind === "rate_limit" || failureKind === "transient")), + persistUnavailableState: !( + isCombo && + result.status === 429 && + (failureKind === "rate_limit" || failureKind === "transient") + ), isCombo, } ); diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index 458a2c0079..94030c6694 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -582,6 +582,35 @@ export function handleNoCredentials( ); } +/** + * Bug #3758 (Problem A): NVIDIA NIM (and other flaky OpenAI-compatible upstreams) + * intermittently send HTTP 200, then close the SSE early with zero useful frames. + * The readiness gate surfaces this as `STREAM_EARLY_EOF` / HTTP 502. On the + * single-model (non-combo) path that 502 used to be returned immediately for every + * provider except `antigravity`, so a transient upstream hang-up looked like a hard + * failure to the caller (e.g. the test-chat scenario). + * + * This decides whether a single-model request should re-attempt after an early + * close. It deliberately: + * - retries ONLY on `STREAM_EARLY_EOF` (the strong "upstream hung up after 200" + * signal) — NOT on `STREAM_READINESS_TIMEOUT` / `stream_timeout`, which is a + * slow-but-alive upstream where retrying would only double latency; and + * - is bounded to exactly ONE retry via the per-request `attempt` counter, so it + * can never loop (the second consecutive early close surfaces the 502). + * + * Pure function (no side effects): the caller performs a plain same-connection + * re-attempt and must NOT mark the account unavailable for an early close — it is a + * transient upstream glitch, not a bad key. + */ +export const STREAM_EARLY_EOF_MAX_RETRIES = 1; + +export function shouldRetryStreamEarlyEof( + errorCode: string | null | undefined, + attempt: number +): boolean { + return errorCode === "STREAM_EARLY_EOF" && attempt < STREAM_EARLY_EOF_MAX_RETRIES; +} + /** * Proxy-resolution failure policy. Default: fail-closed (rethrow) so a request * with an assigned-but-unresolvable proxy never silently egresses on the real IP. diff --git a/tests/unit/stream-early-eof-retry-3758.test.ts b/tests/unit/stream-early-eof-retry-3758.test.ts new file mode 100644 index 0000000000..135d1adda2 --- /dev/null +++ b/tests/unit/stream-early-eof-retry-3758.test.ts @@ -0,0 +1,101 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +// Bug #3758 (Problem A): NVIDIA NIM models intermittently send HTTP 200 then close +// the SSE early with zero useful frames (flaky upstream early-close). The readiness +// gate correctly surfaces STREAM_EARLY_EOF / HTTP 502, but on the single-model path +// only `antigravity` got an early-close retry — every other OpenAI-compatible +// provider returned the 502 immediately with no retry. +// +// The fix generalizes the early-close handling: on `STREAM_EARLY_EOF` for ANY +// provider on the single-model path, allow exactly ONE bounded re-attempt before +// surfacing the 502. The decision lives in the pure helper +// `shouldRetryStreamEarlyEof(errorCode, attempt)` so it can be unit-tested in +// isolation and can NEVER loop. + +const { shouldRetryStreamEarlyEof } = await import("../../src/sse/handlers/chatHelpers.ts"); + +test("shouldRetryStreamEarlyEof: retries once on the first STREAM_EARLY_EOF (attempt 0)", () => { + // Attempt 1 returned 200 then closed the SSE early → STREAM_EARLY_EOF. + // A non-antigravity provider (e.g. nvidia) must RETRY ONCE, not return the 502. + assert.equal(shouldRetryStreamEarlyEof("STREAM_EARLY_EOF", 0), true); +}); + +test("shouldRetryStreamEarlyEof: does NOT retry a second consecutive early-close (bounded)", () => { + // Two consecutive early-closes → the second one returns the 502 (exactly one retry, no loop). + assert.equal(shouldRetryStreamEarlyEof("STREAM_EARLY_EOF", 1), false); + assert.equal(shouldRetryStreamEarlyEof("STREAM_EARLY_EOF", 2), false); + assert.equal(shouldRetryStreamEarlyEof("STREAM_EARLY_EOF", 99), false); +}); + +test("shouldRetryStreamEarlyEof: does NOT retry a stream readiness TIMEOUT (preserves latency)", () => { + // A slow-but-alive upstream (STREAM_READINESS_TIMEOUT / stream_timeout) must NOT + // be retried — retrying would double latency for a request that is still warming up. + assert.equal(shouldRetryStreamEarlyEof("STREAM_READINESS_TIMEOUT", 0), false); + assert.equal(shouldRetryStreamEarlyEof("stream_timeout", 0), false); +}); + +test("shouldRetryStreamEarlyEof: ignores unrelated/empty error codes", () => { + assert.equal(shouldRetryStreamEarlyEof("", 0), false); + assert.equal(shouldRetryStreamEarlyEof(null, 0), false); + assert.equal(shouldRetryStreamEarlyEof(undefined, 0), false); + assert.equal(shouldRetryStreamEarlyEof("UPSTREAM_4XX", 0), false); + assert.equal(shouldRetryStreamEarlyEof("account_semaphore_capacity", 0), false); +}); + +// Behavioral end-to-end of the single-model path: stub executeChatWithBreaker so +// attempt 1 returns a STREAM_EARLY_EOF result and attempt 2 returns a successful +// stream. Assert the handler retries ONCE on the same connection, returns the +// successful stream, and never calls markAccountUnavailable for the early-close. +// +// Driving the full handler in a unit test is heavy (DB, auth, breaker), so this +// block mirrors the exact decision wiring the handler uses around the +// STREAM_EARLY_EOF branch, using the pure helper as the single source of truth. +test("single-model early-close: retries once then succeeds; double early-close surfaces 502; no markAccountUnavailable", () => { + // Simulated per-request early-EOF attempt counter, mirroring chat.ts. + function simulate(results: Array<{ errorCode: string; success?: boolean }>) { + let earlyEofAttempts = 0; + let markAccountUnavailableCalls = 0; + let i = 0; + + while (true) { + const result = results[Math.min(i, results.length - 1)]; + if (result.success) { + return { outcome: "success", earlyEofAttempts, markAccountUnavailableCalls }; + } + + // Non-antigravity early-close: never marks the account unavailable. + if (shouldRetryStreamEarlyEof(result.errorCode, earlyEofAttempts)) { + earlyEofAttempts += 1; + i += 1; + continue; + } + + // Falls through to the immediate 502 return. + return { outcome: "502", earlyEofAttempts, markAccountUnavailableCalls }; + } + } + + // attempt 1: early-close → retry; attempt 2: success + const recovered = simulate([ + { errorCode: "STREAM_EARLY_EOF" }, + { errorCode: "STREAM_EARLY_EOF", success: true }, + ]); + assert.equal(recovered.outcome, "success"); + assert.equal(recovered.earlyEofAttempts, 1, "exactly one retry before success"); + assert.equal( + recovered.markAccountUnavailableCalls, + 0, + "early-close must not mark account unavailable" + ); + + // attempt 1 + attempt 2 both early-close → surfaces the 502 (bounded, no loop) + const exhausted = simulate([ + { errorCode: "STREAM_EARLY_EOF" }, + { errorCode: "STREAM_EARLY_EOF" }, + { errorCode: "STREAM_EARLY_EOF" }, + ]); + assert.equal(exhausted.outcome, "502"); + assert.equal(exhausted.earlyEofAttempts, 1, "only one retry attempted before surfacing 502"); + assert.equal(exhausted.markAccountUnavailableCalls, 0); +});