fix(settings,auth): default debugMode to false and skip account rotation on model-unsupported 400 (#10525)

* fix(settings,auth): default debugMode to false and skip account rotation on model-unsupported 400

* fix(auth): disambiguate model-unsupported from auth-credential 400

The model-unsupported guard used MODEL_ACCESS_DENIED_PATTERNS directly,
which also matches auth-credential errors like 'invalid api key for
model X'. Add the AUTH_CREDENTIAL_ERROR_PATTERNS exclusion (same as
checkFallbackError) and use provider_model_unsupported log reason.

Addresses maintainer feedback on PR #10525

Signed-off-by: Minxi Hou <houminxi@gmail.com>

* fix(auth): narrow model-unsupported guard to avoid misclassifying account-scoped entitlement 400s

The #10460 guard reused MODEL_ACCESS_DENIED_PATTERNS directly, which also
matches ambiguous "access"/"permission" phrasing (e.g. "does not have
permission to access this model") that commonly signals an ACCOUNT-scoped
entitlement gap (PRO vs free tier) rather than a genuinely provider-wide
unsupported model — a different account of the same provider may still
have access, so those must keep rotating normally instead of being
short-circuited.

Extract isProviderModelUnsupported400() in accountFallback.ts: reuses the
same AUTH_CREDENTIAL_ERROR_PATTERNS exclusion checkFallbackError's 400
branch already applies, narrowed to a strict subset of unambiguous
"provider does not serve this model at all" phrasings. auth.ts now calls
this shared helper instead of testing the broader patterns in isolation,
and exposes the sanitized reason ("provider_model_unsupported") on the
returned result, not just in the log line.

Also fix DATA_DIR test-isolation ordering in
account-fallback-service.test.ts: it was assigned after the first
dynamic import of accountFallback.ts, which transitively imports
src/lib/db/core.ts (DATA_DIR is captured once at module-load time), so
the intended isolated test directory was silently never used. Move the
assignment before any transitive DB import, and add regression tests for
the 3-account rotation contract: exactly one upstream call for an
unambiguous provider-wide 400 with the combo advancing to the next
target, continued rotation for account-scoped 401/403/429 and for the
permission/entitlement 400 case that motivated this narrowing.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Signed-off-by: Minxi Hou <houminxi@gmail.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
This commit is contained in:
Bob.Hou
2026-08-18 21:51:38 +08:00
committed by GitHub
parent 9392b30575
commit ebf0bf913a
4 changed files with 484 additions and 1 deletions

View File

@@ -302,7 +302,7 @@ export const MODEL_ACCESS_DENIED_PATTERNS = [
// across every target, masking the real "fix your credential" error. When the
// text clearly indicates a bad credential, the regex-based model-access detection
// is suppressed (structured codes/types like model_not_found are unaffected).
const AUTH_CREDENTIAL_ERROR_PATTERNS = [
export const AUTH_CREDENTIAL_ERROR_PATTERNS = [
/\b(?:invalid|incorrect|expired|missing|revoked)\s+api[\s_-]?key\b/i,
/\bapi[\s_-]?key\s+(?:is\s+)?(?:invalid|incorrect|expired|missing|revoked|not\s+valid)\b/i,
/\bauthentication\s+(?:failed|error|required)\b/i,
@@ -311,6 +311,45 @@ const AUTH_CREDENTIAL_ERROR_PATTERNS = [
/\bnot\s+authenticated\b/i,
];
// #10460: strict subset of MODEL_ACCESS_DENIED_PATTERNS that is unambiguously
// PROVIDER-wide — the model does not exist / is not served by this provider at all, so
// no account of that provider could serve it (e.g. "The requested model is not
// supported", "model not found"). Deliberately EXCLUDES the "access"/"permission"
// patterns from MODEL_ACCESS_DENIED_PATTERNS (e.g. "does not have permission to access
// this model", "access denied ... model"): those commonly indicate an ACCOUNT-scoped
// entitlement gap (e.g. PRO vs free tier) where a *different* account of the same
// provider may still have access, so they must keep rotating through the normal
// account-cooldown path — not be treated as provider-wide unsupported.
const PROVIDER_MODEL_UNSUPPORTED_PATTERNS = [
/\binvalid model\b/i,
/\bmodel.*not.*(?:available|found|supported|accessible)\b/i,
/\bmodel.*(?:does not exist|doesn't exist)\b/i,
/\bmodel\b[\s\S]{0,80}?\b(?:does\s+not\s+support|doesn't\s+support|unsupported)\b/i,
/\b(?:does\s+not\s+support|doesn't\s+support|unsupported)\b[\s\S]{0,80}?\bmodel\b/i,
/\bunsupported\s+model\b/i,
/\bplease select a different model\b/i,
];
/**
* #10460: is this 400 an unambiguous, PROVIDER-wide "model not supported" response —
* i.e. would retrying a *different account* of the same provider also fail for the
* same reason? Reuses AUTH_CREDENTIAL_ERROR_PATTERNS (the same bad-credential
* exclusion `checkFallbackError`'s 400 branch applies) so a message like "invalid api
* key for model X" is never misclassified as model-wide. Also excludes the broader,
* ambiguous MODEL_ACCESS_DENIED_PATTERNS access/permission phrasing — those can be
* account-scoped entitlement gaps, not a provider-wide unsupported model — so account
* rotation for those keeps working normally via the regular cooldown path.
*
* Callers that want "should combo keep trying other targets" (not "should this
* specific account keep rotating") should use MODEL_ACCESS_DENIED_PATTERNS /
* isModelScoped400() instead — this helper is deliberately narrower.
*/
export function isProviderModelUnsupported400(status: number, errorText: string): boolean {
if (status !== HTTP_STATUS.BAD_REQUEST) return false;
if (AUTH_CREDENTIAL_ERROR_PATTERNS.some((p) => p.test(errorText))) return false;
return PROVIDER_MODEL_UNSUPPORTED_PATTERNS.some((p) => p.test(errorText));
}
// Malformed request patterns — the model rejected the message format but a different
// provider/model in the combo may accept it.
const MALFORMED_REQUEST_PATTERNS = [

View File

@@ -43,6 +43,7 @@ import {
hasPerModelQuota,
getRuntimeProviderProfile,
recordModelLockoutFailure,
isProviderModelUnsupported400,
} from "@omniroute/open-sse/services/accountFallback.ts";
import { isLocalProvider } from "@omniroute/open-sse/config/providerRegistry.ts";
import { COOLDOWN_MS, RateLimitReason } from "@omniroute/open-sse/config/constants.ts";
@@ -2181,6 +2182,26 @@ export async function markAccountUnavailable(
}
}
// #10460: model-unsupported 400 — the PROVIDER does not serve this model, not
// this account. Cooling down the account and rotating to the next one wastes an
// upstream call because all accounts share the same model catalog. Return
// shouldFallback: false so the error propagates to the combo layer, which already
// has isModelScoped400() (combo.ts:1827) to advance to the next combo target.
// Uses isProviderModelUnsupported400() — the SAME disambiguation
// (AUTH_CREDENTIAL_ERROR_PATTERNS exclusion) checkFallbackError's 400 branch
// applies, narrowed further to exclude the broader/ambiguous
// MODEL_ACCESS_DENIED_PATTERNS access-/permission-phrased matches (e.g. "does not
// have permission to access this model"), which can be an ACCOUNT-scoped
// entitlement gap (PRO vs free tier) rather than a provider-wide unsupported
// model — those must keep rotating to other accounts normally.
if (isProviderModelUnsupported400(status, errorText)) {
log.info(
"AUTH",
`${connectionId.slice(0, 8)} provider_model_unsupported 400 (${provider}/${model ?? "n/a"}) — skipping account cooldown, letting combo advance`
);
return { shouldFallback: false, cooldownMs: 0, reason: "provider_model_unsupported" };
}
const effectiveProviderProfile =
providerProfile || (provider ? await getRuntimeProviderProfile(provider) : null);
// #4530 follow-up: the combo.ts lockout sites forward the admin-configured

View File

@@ -1,11 +1,29 @@
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";
// #10460: DATA_DIR must be assigned BEFORE any transitive DB import. The
// accountFallback.ts import below statically imports `@/lib/db/providers`, which
// imports `src/lib/db/core.ts`, whose `DATA_DIR` is a top-level
// `export const DATA_DIR = resolveWritableDataDir(...)` captured once at module-load
// time. Setting `process.env.DATA_DIR` after that first import is a no-op — the DB
// singleton keeps whatever DATA_DIR it resolved at import time, so an isolated test
// directory assigned later is silently never used and the #10460 tests below would
// actually read/write the shared default DATA_DIR instead.
const TEST_DATA_DIR_10460 = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-10460-"));
process.env.DATA_DIR = TEST_DATA_DIR_10460;
process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "10460-test-secret";
const accountFallback = await import("../../open-sse/services/accountFallback.ts");
const accountSelector = await import("../../open-sse/services/accountSelector.ts");
const { RateLimitReason, COOLDOWN_MS, PROVIDER_PROFILES } =
await import("../../open-sse/config/constants.ts");
const { getCircuitBreaker } = await import("../../src/shared/utils/circuitBreaker.ts");
const core = await import("../../src/lib/db/core.ts");
const providersDb = await import("../../src/lib/db/providers.ts");
const auth = await import("../../src/sse/services/auth.ts");
const {
isOAuthInvalidToken,
@@ -1573,3 +1591,382 @@ test("isAccountDeactivated matches a custom signal after setCustomBannedSignals"
setCustomBannedSignals([]); // cleanup — restore module state for other tests
});
// ─── #10460: model-unsupported 400 skips account rotation ────────────────────
// TEST_DATA_DIR_10460 / DATA_DIR / core / providersDb / auth are set up at the top
// of this file, BEFORE the accountFallback.ts import — see the comment there.
async function resetStorage10460() {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR_10460, { recursive: true, force: true });
fs.mkdirSync(TEST_DATA_DIR_10460, { recursive: true });
}
async function seedConn10460(provider: string): Promise<string> {
const conn = await providersDb.createProviderConnection({
provider,
authType: "apikey",
apiKey: `${provider}-key-10460`,
isActive: true,
testStatus: "active",
});
return (conn as Record<string, unknown>).id as string;
}
test.after(() => {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR_10460, { recursive: true, force: true });
});
test("#10460: model-unsupported 400 returns shouldFallback:false (no account cooldown)", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
const result = await auth.markAccountUnavailable(
connId,
400,
"The requested model is not supported",
"github",
"claude-fable-5"
);
// The guard must prevent account cooldown — the error belongs to the combo layer
assert.strictEqual(result.shouldFallback, false, "must not trigger account rotation");
assert.strictEqual(result.cooldownMs, 0, "must not cool down the account");
// Verify the connection was NOT marked unavailable
const after = await providersDb.getProviderConnectionById(connId);
assert.ok(!after.rateLimitedUntil, "connection must not be rate-limited");
assert.notStrictEqual(after.testStatus, "unavailable", "connection must stay active");
});
test("#10460: model-unsupported 400 handles various phrasings", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
const phrasings = [
"The requested model is not supported",
"model claude-fable-5 is not supported",
"invalid_request_error: model is not supported",
"unsupported model: gpt-9",
];
for (const errorText of phrasings) {
await resetStorage10460();
const id = await seedConn10460("github");
const result = await auth.markAccountUnavailable(id, 400, errorText, "github", "test-model");
assert.strictEqual(result.shouldFallback, false, `phrasing "${errorText}" must not rotate`);
// Verify connection stays healthy after each iteration
const conn = await providersDb.getProviderConnectionById(id);
assert.ok(!conn.rateLimitedUntil, `"${errorText}" must not rate-limit connection`);
assert.notStrictEqual(conn.testStatus, "unavailable", `"${errorText}" must not mark unavailable`);
}
});
test("#10460: body-specific 400 still goes through normal path (not blocked by model guard)", async () => {
await resetStorage10460();
const connId = await seedConn10460("openai");
const result = await auth.markAccountUnavailable(
connId,
400,
"Invalid message format: the request body is malformed",
"openai",
"gpt-4"
);
// Body-specific 400 does NOT match MODEL_ACCESS_DENIED_PATTERNS — normal path applies
assert.strictEqual(result.shouldFallback, true, "body-specific 400 must still allow fallback");
});
test("#10460: non-400 status with model-unsupported text does NOT trigger guard", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
// 429 + model-unsupported text should go through normal path (guard only fires for status===400)
const result = await auth.markAccountUnavailable(
connId,
429,
"The requested model is not supported",
"github",
"test-model"
);
assert.strictEqual(result.shouldFallback, true, "non-400 must not be short-circuited by model guard");
// The key assertion: guard returns shouldFallback:false. If we get here with
// shouldFallback:true, the guard did NOT fire (correct behavior).
});
test("#10460: empty errorText with status 400 does NOT trigger guard", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
const result = await auth.markAccountUnavailable(connId, 400, "", "github", "test-model");
// Empty string matches no patterns — normal path applies
assert.strictEqual(result.shouldFallback, false, "empty errorText is generic 400 → no fallback");
});
test("#10460: auth-credential 400 text does NOT match model-unsupported guard", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
// "Invalid API key provided for model gpt-4o" contains "model" but is NOT a
// model-unsupported error — it's a credential issue. The guard must not fire.
const result = await auth.markAccountUnavailable(
connId,
400,
"Invalid API key provided for model gpt-4o",
"github",
"gpt-4o"
);
// This text does NOT match MODEL_ACCESS_DENIED_PATTERNS (verified by regex test)
// so it falls through to checkFallbackError which returns shouldFallback:false for generic 400
assert.strictEqual(result.shouldFallback, false, "auth-credential 400 must not be caught by model guard");
// The generic 400 path returns cooldownMs:0 — same as the guard, but the
// connection was NOT touched (no rateLimitedUntil set). This distinguishes
// it from the normal fallback path which would set a cooldown.
const conn = await providersDb.getProviderConnectionById(connId);
assert.ok(!conn.rateLimitedUntil, "generic 400 must not rate-limit connection");
});
test("#10460: guard early return does not touch DB (distinguishes from normal path)", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
// Guard path: model-unsupported 400 → shouldFallback:false, cooldownMs:0, no DB change
const guardResult = await auth.markAccountUnavailable(
connId, 400, "The requested model is not supported", "github", "test-model"
);
assert.strictEqual(guardResult.shouldFallback, false);
assert.strictEqual(guardResult.cooldownMs, 0);
const guardConn = await providersDb.getProviderConnectionById(connId);
assert.ok(!guardConn.rateLimitedUntil, "guard path must not touch DB");
assert.strictEqual(guardConn.testStatus, "active", "guard path must keep connection active");
});
test("#10460: returned result exposes a sanitized provider_model_unsupported reason", async () => {
await resetStorage10460();
const connId = await seedConn10460("github");
const result = await auth.markAccountUnavailable(
connId,
400,
"The requested model is not supported",
"github",
"claude-fable-5"
);
assert.strictEqual(result.shouldFallback, false);
assert.strictEqual(
(result as { reason?: string }).reason,
"provider_model_unsupported",
"the canonical sanitized reason must be exposed on the returned result, not only in logs"
);
});
test("#10460: account-scoped permission/entitlement 400 keeps rotating (not misclassified as provider-wide unsupported)", async () => {
await resetStorage10460();
const connIds: string[] = [];
for (let i = 0; i < 3; i++) {
connIds.push(await seedConn10460("github"));
}
let calls = 0;
// Phrased so it matches the broader/ambiguous MODEL_ACCESS_DENIED_PATTERNS
// ("permission" ... "model") but is NOT an unambiguous provider-wide "model not
// supported" response — it reads as an account/key entitlement gap (e.g. this
// key's plan doesn't include this model), where a DIFFERENT account of the same
// provider may still have access. Rotation through all 3 accounts must continue,
// unlike the unambiguous "model is not supported" case above.
for (const connId of connIds) {
calls += 1;
const result = await auth.markAccountUnavailable(
connId,
400,
"Your API key does not have permission to use model gpt-4o",
"github",
"gpt-4o"
);
if (!result.shouldFallback) break;
}
assert.equal(
calls,
3,
"an account-scoped permission/entitlement 400 must keep rotating through all accounts, " +
"not be short-circuited by the provider-wide model-unsupported guard"
);
});
test("#10460: account-scoped 401 keeps rotating through all 3 accounts (not short-circuited)", async () => {
await resetStorage10460();
const connIds: string[] = [];
for (let i = 0; i < 3; i++) {
connIds.push(await seedConn10460("github"));
}
let calls = 0;
for (const connId of connIds) {
calls += 1;
const result = await auth.markAccountUnavailable(
connId,
401,
"Unauthorized: invalid credentials",
"github",
"claude-fable-5"
);
if (!result.shouldFallback) break;
}
assert.equal(
calls,
3,
"401 account-scoped errors must keep rotating through every account, unlike model-unsupported 400"
);
});
test("#10460: account-scoped 403 keeps rotating through all 3 accounts", async () => {
await resetStorage10460();
const connIds: string[] = [];
for (let i = 0; i < 3; i++) {
connIds.push(await seedConn10460("github"));
}
let calls = 0;
for (const connId of connIds) {
calls += 1;
const result = await auth.markAccountUnavailable(
connId,
403,
"Forbidden: access denied for this account",
"github",
"claude-fable-5"
);
if (!result.shouldFallback) break;
}
assert.equal(calls, 3, "403 account-scoped errors must keep rotating through every account");
});
test("#10460: 429 rate limit keeps rotating through all 3 accounts", async () => {
await resetStorage10460();
const connIds: string[] = [];
for (let i = 0; i < 3; i++) {
connIds.push(await seedConn10460("github"));
}
let calls = 0;
for (const connId of connIds) {
calls += 1;
const result = await auth.markAccountUnavailable(
connId,
429,
"Rate limit exceeded",
"github",
"claude-fable-5"
);
if (!result.shouldFallback) break;
}
assert.equal(calls, 3, "429 rate-limit errors must keep rotating through every account");
});
// ─── #10460 acceptance criteria: 3-account rotation + combo target advancement ─
//
// Reproduces the exact regression from issue #10460: a combo with a
// (github/model, 3 accounts) target followed by a sibling target. When GitHub
// returns an unambiguous "model not supported" 400, the account-rotation loop
// must make exactly ONE upstream call (not one per account) and the combo must
// advance to the next target — not just that markAccountUnavailable() in
// isolation returns shouldFallback:false (covered above), but that a realistic
// rotation loop wired to the REAL markAccountUnavailable()/
// isProviderModelUnsupported400() gating actually stops after account 1 and lets
// handleComboChat() move on.
//
// The inner loop below is a faithful, minimal reproduction of the account-rotation
// contract in src/sse/handlers/chat.ts::handleSingleModelChat step 8 ("Fallback to
// next account", ~line 1936): call markAccountUnavailable(); continue to the next
// connection only while shouldFallback is true, otherwise stop immediately and
// surface the failure. Reimplemented at this scope (rather than driving the full
// handleChat()/route stack) so the test can assert on upstream-call counts and
// per-connection DB state directly, while still exercising the real gating logic
// that decides whether rotation continues.
test("#10460 acceptance: unambiguous model-unsupported 400 makes exactly ONE upstream call across 3 accounts, then the combo advances to the next target", async () => {
await resetStorage10460();
const { handleComboChat } = await import("../../open-sse/services/combo.ts");
const githubConnIds: string[] = [];
for (let i = 0; i < 3; i++) {
githubConnIds.push(await seedConn10460("github"));
}
let githubUpstreamCalls = 0;
const triedGithubConnections: string[] = [];
const noopLog = { info: () => {}, warn: () => {}, debug: () => {}, error: () => {} };
const handleSingleModel = async (_body: unknown, modelStr: string) => {
if (modelStr.startsWith("github/")) {
for (const connId of githubConnIds) {
triedGithubConnections.push(connId);
githubUpstreamCalls += 1;
const result = await auth.markAccountUnavailable(
connId,
400,
"The requested model is not supported",
"github",
"claude-fable-5"
);
if (result.shouldFallback) continue;
return new Response(
JSON.stringify({ error: { message: "The requested model is not supported" } }),
{ status: 400, headers: { "Content-Type": "application/json" } }
);
}
// A test-fixture bug (guard not firing) would otherwise silently exhaust
// every account and mask the regression this test exists to catch.
throw new Error("all 3 github accounts were tried — the guard did not fire");
}
// Second combo target (a different provider) — succeeds immediately.
return new Response(JSON.stringify({ id: "ok", choices: [] }), {
status: 200,
headers: { "Content-Type": "application/json" },
});
};
const result = await handleComboChat({
body: { model: "test", messages: [{ role: "user", content: "hi" }] },
combo: {
name: "test-combo-10460",
strategy: "priority",
models: [{ model: "github/claude-fable-5" }, { model: "openai/gpt-4o-mini" }],
},
handleSingleModel,
log: noopLog,
settings: {},
allCombos: [],
});
assert.equal(
githubUpstreamCalls,
1,
`expected exactly ONE upstream call for github/claude-fable-5 across 3 accounts, got ` +
`${githubUpstreamCalls} (tried: ${triedGithubConnections.join(", ")})`
);
assert.equal(triedGithubConnections.length, 1, "only the first account should have been tried");
assert.equal(result.status, 200, "the combo must advance to the next target and succeed");
// The two untouched accounts must remain completely unaffected — proves the
// guard did not just avoid an upstream call but also never cooled them down,
// so they stay immediately eligible for the next unrelated request.
for (const connId of githubConnIds.slice(1)) {
const conn = await providersDb.getProviderConnectionById(connId);
assert.ok(!conn.rateLimitedUntil, `untried account ${connId} must not be rate-limited`);
assert.notStrictEqual(
conn.testStatus,
"unavailable",
`untried account ${connId} must stay active`
);
}
});

View File

@@ -0,0 +1,26 @@
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";
// Isolated DATA_DIR so persisted settings rows don't mask the default.
const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-settings-debugmode-"));
process.env.DATA_DIR = TEST_DATA_DIR;
process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "settings-debugmode-test-secret";
const { getSettings } = await import("../../src/lib/db/settings.ts");
test("debugMode defaults to false for fresh installs (no persisted setting)", async () => {
const settings = await getSettings();
assert.equal(settings.debugMode, false, "debugMode should default to false, not true");
});
test("logToolSources defaults to false", async () => {
const settings = await getSettings();
assert.equal(settings.logToolSources, false, "logToolSources should default to false");
});
test.after(() => {
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
});