Repository navigation
fix(sse): stop combo at the first body-specific 400 (#4279) - #4316
Merged
Merged
Conversation
The #2101 guard that detects a body-specific 400 (context overflow / malformed / model-access-denied) logged "stopping combo" but executed a bare `break`, which only exits the inner retry loop. executeTarget then returns null, and the outer target loop treats null as "this target produced nothing" and advances to the next model — so the guard never actually stopped fallback, and a combo of N targets that all reject the same request body tried all N (the report shows a 143-model Codex combo marching through every target). Surface the 400 via the {ok,response} contract (mirrors the 499 client-disconnect path) so the outer loop resolves the combo and stops. Regression test: a 3-target priority combo whose targets all return a body-specific 400 must stop after target 1 (RED before, GREEN after). Closes #4279
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…ombo-stop-body-400
…ombo-stop-body-400
5 tasks done
diegosouzapw
pushed a commit
that referenced
this pull request
Jun 21, 2026
When a combo target (e.g. `antigravity/opus → github/opus`) hit a leg whose only configured connections were disabled, `handleNoCredentials` in `src/sse/handlers/chatHelpers.ts` returned 400 BAD_REQUEST. The combo target loop treats 400 as a hard stop — added by PR #4316 / issue #4279 to prevent infinite fallback loops on body-specific 4xx errors — so the combo died on the first leg even when the next target was perfectly healthy. The "no usable credentials, no connections tried yet" branch now returns 404 NOT_FOUND with the message "No active credentials for provider: <p>" instead. 404 flows through `checkFallbackError` as `shouldFallback: true` via the generic-error catch-all path in `open-sse/services/accountFallback.ts` (no explicit 404 special-case + transient cooldown), so the combo loop tries the next target. The log level for this branch also drops from `error` to `warn` because zero active credentials is an expected operator-driven state, not a server fault. The sibling branches in `handleNoCredentials` were left untouched: - `allRateLimited` already returns the upstream status (combo-fallbackable). - `allExpired` already returns 401 with a reconnect hint (combo-fallbackable via `checkFallbackError`). - The post-exhaustion branch (after at least one connection tried) already preserves `lastStatus`. Tests updated to encode the new contract (TDD: failing-then-passing per Hard Rule #18): `chat-helpers`, `chat-route-coverage`, `vscode-token-routes`, `chat-pipeline`, `llama-cpp-provider`, `combo-routing-e2e`. The `vscode-token-routes` assertions also flip `error.code` from `bad_request` to `model_not_found` per the OpenAI-compatible status mapping in `open-sse/config/errorConfig.ts:29`. Routes that emit `400 No credentials for provider` directly (rerank, audio, moderations, image edits, embeddings) are untouched — they don't flow through combo, so the 400 hard-stop semantics don't apply. Inspired-by upstream decolua/9router#336 (Ibrahim Ryan). Co-authored-by: Ibrahim Ryan <ryan@nuevanext.com>
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
… (diegosouzapw#4316) The diegosouzapw#2101 guard that detects a body-specific 400 (context overflow / malformed / model-access-denied) logged "stopping combo" but executed a bare `break`, which only exits the inner retry loop. executeTarget then returns null, and the outer target loop treats null as "this target produced nothing" and advances to the next model — so the guard never actually stopped fallback, and a combo of N targets that all reject the same request body tried all N (the report shows a 143-model Codex combo marching through every target). Surface the 400 via the {ok,response} contract (mirrors the 499 client-disconnect path) so the outer loop resolves the combo and stops. Regression test: a 3-target priority combo whose targets all return a body-specific 400 must stop after target 1 (RED before, GREEN after). Closes diegosouzapw#4279
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4279
Problem
A Docker user's combo (143 Codex models against a single ChatGPT-account Codex connection) produced a flood of
[400] "The 'gpt-5.X' model is not supported when using Codex with a ChatGPT account."— the gateway iterated every target despite loggingskipping fallback to other targets to prevent infinite loop/stopping comboon each.Root cause (
open-sse/services/combo.ts): the#2101body-specific-400 guard executes a barebreak, but thatbreakis inside the inner retry loop, not the outer target loop.executeTargetthen falls through toreturn null, and the outer loop (const res = await executeTarget(i); if (res && !anySuccess) { … }) treatsnullas "this target produced nothing" and advances to the next model. So the guard never stopped fallback — a combo of N targets that all reject the same body marched through all N (wasting upstream calls + per-attempt work; contributes to the user's container churn).Fix
Replace the
breakwithreturn { ok: false, response: result }— the same{ ok, response }contract the 499 client-disconnect path already uses (a few lines up). The outer loop then hits itselse if (res.response)branch, resolves the combo with the 400, and stops immediately.The "service shutting down" part of the report is not an OmniRoute crash in the provided log (the request ultimately succeeded on
gpt-5.4-mini; the string isn't in the codebase) — that is tracked separately as needs-info (likely Docker OOM/healthcheck). This PR fixes the confirmed fallback-loop bug.Validation (Hard Rule #18 — TDD)
tests/unit/combo-body-specific-400-stop-4279.test.ts— a 3-target priority combo whosehandleSingleModelreturns a body-specific 400 ("model is not supported", which classifies asMODEL_CAPACITY). Asserts the combo callshandleSingleModelonce and returns 400 (RED before: tried all 3 —gpt-5.2, gpt-5.3-codex, gpt-5.4; GREEN after: stops at 1).Regression:
combo-499-abort,combo-strategy-fallbacks,combo-provider-cooldowngreen (20/20). Lint +typecheck:coreclean.combo.tsfile-size baseline bumped 2605 → 2611.