From 465f0a0e83a617c4be9047080cb25e36238bc6ec Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 6 Jul 2026 19:18:42 -0300 Subject: [PATCH] fix(combo): advance round-robin pointer past the served model (port from 9router#948) (#6428) combo round-robin advances pointer past served model (port #948). Test combo-rr-fallback-advance-948 (green). Integrated into release/v3.8.46. --- CHANGELOG.md | 1 + open-sse/services/combo.ts | 9 ++ .../combo-rr-fallback-advance-948.test.ts | 107 ++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 tests/unit/combo-rr-fallback-advance-948.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1adda9c8ae..8c6639088c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ ### 🐛 Bug Fixes +- **fix(combo):** round-robin now advances the rotation pointer past the model that **actually served**, not the eagerly-scheduled one. With `stickyLimit: 1` (true round-robin), when the scheduled model failed and a *different* model served via fallback, the counter had already advanced +1 from the scheduled index — so the next request reused the fallback-served model, degrading round-robin into hot-spotting on whichever model was healthy. The pointer now advances to the served index + 1 (mirroring the sticky-limit>1 path). Session-stickiness (#3825) and distribution are preserved. Regression guard: `tests/unit/combo-rr-fallback-advance-948.test.ts`. (thanks @binsarjr) - **fix(sse):** a non-string `model` field is now rejected with a `400` before the resolver, instead of crashing downstream `.toLowerCase()`/`.split()` calls into an empty-body `500` that escapes the error sanitizer ([#6407](https://github.com/diegosouzapw/OmniRoute/issues/6407)). Regression guard: `tests/unit/chat-non-string-model-6407.test.ts`. (thanks @chirag127) - **fix(api):** unknown `/api/*` routes now return a JSON `404` (instead of the dashboard HTML shell) and scalar chat params (`model`/`temperature`/etc.) are validated **before** the provider lookup so malformed requests fail fast with a clear `400` ([#6424](https://github.com/diegosouzapw/OmniRoute/issues/6424), [#6412](https://github.com/diegosouzapw/OmniRoute/issues/6412)). Regression guards: `tests/unit/api/api-catchall-json-404.test.ts`, `tests/unit/chat-early-schema-validation-6412.test.ts`. (thanks @chirag127) - **fix(api):** `/v1/chat/completions` now rejects a non-JSON `Content-Type` with a `400` before parsing the body ([#6414](https://github.com/diegosouzapw/OmniRoute/issues/6414)). Regression guard: `tests/unit/v1-chat-completions-content-type-6414.test.ts`. (thanks @chirag127) diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 78ee27b958..6b17ead328 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -2722,6 +2722,15 @@ async function handleRoundRobinCombo({ if (stickyRoundRobinEnabled) { recordStickyRoundRobinSuccess(combo.name, target, stickyLimit, filteredTargets); + } else { + // #948: true round-robin (stickyLimit <= 1). The counter was advanced + // eagerly (+1 from the scheduled start index) before this loop ran, so + // when the scheduled model failed and a *different* model served via + // fallback, the next request reused the fallback-served model. Advance + // the pointer past the model that ACTUALLY served (modelIndex) instead, + // mirroring recordStickyRoundRobinSuccess's served-index logic. Read + // side applies `% modelCount`, so storing modelIndex + 1 is correct. + rrCounters.set(combo.name, modelIndex + 1); } // #3825: (re)record the sticky binding so the next turn re-pins (prompt-cache). diff --git a/tests/unit/combo-rr-fallback-advance-948.test.ts b/tests/unit/combo-rr-fallback-advance-948.test.ts new file mode 100644 index 0000000000..3ff204c8db --- /dev/null +++ b/tests/unit/combo-rr-fallback-advance-948.test.ts @@ -0,0 +1,107 @@ +/** + * Regression for upstream 9router#948 — round-robin combo pointer must advance + * past the model that ACTUALLY served, not the eagerly-scheduled start index. + * + * With `stickyLimit: 1` ("true round-robin, one request per model"), when the + * scheduled model fails and a *different* model serves via fallback, the counter + * was advanced by +1 from the scheduled start index (eagerly, before the loop), + * so the next request started at — and reused — the fallback-served model. This + * silently degraded round-robin into hot-spotting on whichever model was healthy. + * + * This drives the REAL handleComboChat with session-stickiness disabled (to + * isolate the pure rotation pointer) and asserts two consecutive requests do NOT + * serve the same connection when the scheduled target keeps failing. + */ +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-rr-948-")); +const ORIGINAL_DATA_DIR = process.env.DATA_DIR; +process.env.DATA_DIR = TEST_DATA_DIR; + +const { handleComboChat } = await import("../../open-sse/services/combo.ts"); +const rrState = await import("../../open-sse/services/combo/rrState.ts"); +const dbCore = await import("../../src/lib/db/core.ts"); + +function makeLog() { + return { info() {}, warn() {}, debug() {}, error() {} }; +} + +// Flat round-robin combo. conn-A always fails (fallback-eligible 429); B and C succeed. +function rrCombo(name: string) { + return { + name, + strategy: "round-robin", + // disableSessionStickiness isolates the round-robin pointer from the #3825 + // per-conversation pin; stickyLimit defaults to 1 (true round-robin). + config: { maxRetries: 0, disableSessionStickiness: true }, + models: [ + { kind: "model", provider: "codex", providerId: "codex", model: "m-a", connectionId: "conn-A", id: `${name}-0` }, + { kind: "model", provider: "codex", providerId: "codex", model: "m-b", connectionId: "conn-B", id: `${name}-1` }, + { kind: "model", provider: "glm-cn", providerId: "glm-cn", model: "m-c", connectionId: "conn-C", id: `${name}-2` }, + ], + }; +} + +async function dispatchServedConnection(combo: Record): Promise { + let served = "?"; + await handleComboChat({ + body: { model: combo.name, messages: [{ role: "user", content: "hi" }], stream: false }, + combo, + allCombos: [combo], + isModelAvailable: async () => true, + relayOptions: undefined, + signal: undefined, + settings: {}, + log: makeLog(), + handleSingleModel: async ( + _b: unknown, + modelStr: string, + target?: { connectionId?: string | null } + ) => { + const conn = target?.connectionId ?? "?"; + // conn-A always fails with a fallback-eligible status so rotation must fall through. + if (conn === "conn-A") { + return new Response(JSON.stringify({ error: { message: "rate limited" } }), { + status: 429, + }); + } + served = conn; + return Response.json({ choices: [{ message: { role: "assistant", content: modelStr } }] }); + }, + }); + return served; +} + +test.beforeEach(() => { + rrState.rrCounters.clear(); + rrState.rrStickyTargets.clear(); +}); + +test.after(() => { + try { + dbCore.resetDbInstance?.(); + } catch { + /* ignore */ + } + if (ORIGINAL_DATA_DIR === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = ORIGINAL_DATA_DIR; + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("#948: two consecutive requests do not reuse the fallback-served model", async () => { + const combo = rrCombo("rr948"); + const first = await dispatchServedConnection(combo); + const second = await dispatchServedConnection(combo); + + assert.notEqual(first, "?", "first request must be served by a healthy model"); + assert.notEqual(second, "?", "second request must be served by a healthy model"); + assert.notEqual( + first, + second, + `round-robin must advance past the served model — got ${first} twice (hot-spotting)` + ); +});