diff --git a/.env.example b/.env.example index 3131df30cf..3acce9cdfd 100644 --- a/.env.example +++ b/.env.example @@ -2244,8 +2244,7 @@ QUOTA_STORE_DRIVER=sqlite # sqlite | redis # or lifecycle tuning. # ───────────────────────────────────────────────────────────────────────────── # OMNIROUTE_VNC_IMAGE=omniroute-vnc-chromium:local -# OMNIROUTE_DOCKER_BIN=docker -# OMNIROUTE_VNC_CONTAINER_VNC_PORT=3000 +# OMNIROUTE_DOCKER_BIN=docker# OMNIROUTE_VNC_CONTAINER_VNC_PORT=3000 # OMNIROUTE_VNC_CONTAINER_CDP_PORT=9223 # OMNIROUTE_VNC_CONTAINER_PROFILE_DIR=/config # OMNIROUTE_VNC_PROFILE_DIR= @@ -2259,6 +2258,5 @@ QUOTA_STORE_DRIVER=sqlite # sqlite | redis # ───────────────────────────────────────────────────────────────────────────── # Data-dir alias (optional — open-sse/services/notionThreadSessions.ts) # Legacy fallback for DATA_DIR, checked only after DATA_DIR and -# OMNIROUTE_DATA_DIR are both unset. Locates the Notion web-thread session cache. -# ───────────────────────────────────────────────────────────────────────────── +# OMNIROUTE_DATA_DIR are both unset. Locates the Notion web-thread session cache.# ───────────────────────────────────────────────────────────────────────────── # VIBEPROXY_DATA_DIR= diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index 2a83b341c0..de60a4e8ec 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -1266,3 +1266,23 @@ Not required for normal operation — developer tooling only. | Variable | Default | Source File | Description | | ---------------------------- | ------------ | ----------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | | `OMNIROUTE_EVAL_CREDENTIALS` | `{}` (empty) | `scripts/compression-eval/index.ts` | Operator-supplied JSON credentials for the provider exercised by the offline compression-eval CLI (parsed with `JSON.parse`). Leave unset for a dry run. | + +### VNC Browser Sessions + +Used by `src/lib/vncSession/manifest.ts` to configure Docker-based headless Chromium sessions for browser-automation providers. All optional — defaults shown below. + +| Variable | Default | Source File | Description | +| ------------------------------------- | ----------------------------- | --------------------------------- | ------------------------------------------------------------------------------ | +| `OMNIROUTE_DOCKER_BIN` | `docker` | `src/lib/vncSession/manifest.ts` | Path to the Docker binary used to spawn VNC containers. | +| `OMNIROUTE_VNC_IMAGE` | `omniroute-vnc-chromium:local`| `src/lib/vncSession/manifest.ts` | Docker image for the VNC Chromium container. | +| `OMNIROUTE_VNC_CHROMIUM_ARGS` | _(built-in flags)_ | `src/lib/vncSession/manifest.ts` | Extra Chromium CLI args passed to the browser inside the container. | +| `OMNIROUTE_VNC_CONTAINER_VNC_PORT` | `3000` | `src/lib/vncSession/manifest.ts` | VNC port inside the container. | +| `OMNIROUTE_VNC_CONTAINER_CDP_PORT` | `9223` | `src/lib/vncSession/manifest.ts` | Chrome DevTools Protocol port inside the container. | +| `OMNIROUTE_VNC_CONTAINER_PROFILE_DIR` | `/config` | `src/lib/vncSession/manifest.ts` | Profile directory inside the container. | +| `OMNIROUTE_VNC_PROFILE_DIR` | _(unset)_ | `src/lib/vncSession/manifest.ts` | Host-side directory for persistent browser profiles. | +| `OMNIROUTE_VNC_IDLE_MS` | `600000` | `src/lib/vncSession/manifest.ts` | Idle timeout (ms) before a VNC session is harvested. | +| `OMNIROUTE_VNC_MAX_MS` | `1800000` | `src/lib/vncSession/manifest.ts` | Maximum session duration (ms). | +| `OMNIROUTE_VNC_MAX_SESSIONS` | `4` | `src/lib/vncSession/manifest.ts` | Maximum concurrent VNC sessions. | +| `OMNIROUTE_VNC_READY_MS` | `45000` | `src/lib/vncSession/manifest.ts` | Browser readiness timeout (ms). | +| `OMNIROUTE_VNC_HARVEST_MS` | `20000` | `src/lib/vncSession/manifest.ts` | Harvest/cleanup timeout (ms). | +| `VIBEPROXY_DATA_DIR` | _(unset)_ | `open-sse/services/notionThreadSessions.ts` | Directory for Notion thread session persistence. | diff --git a/open-sse/services/combo/targetExhaustion.ts b/open-sse/services/combo/targetExhaustion.ts index 66f7ef5cd2..dc8bfb015f 100644 --- a/open-sse/services/combo/targetExhaustion.ts +++ b/open-sse/services/combo/targetExhaustion.ts @@ -28,6 +28,15 @@ 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]; +// Auth-level failure statuses: the provider's credentials are invalid/expired (401) or +// forbidden (403). When the failing target carries a connectionId, only that connection's +// credentials are bad — sibling connections on the same provider may still be healthy, so +// mark connection-level exhaustion (mirrors markConnectionLevelExhaustion). Only fall back to +// whole-provider exhaustion when no connectionId is available (#8133: combo engine wastes +// attempts on dead connections; #8137: whole-provider exhaustion wrongly skipped healthy +// sibling connections on the same provider). +const AUTH_LEVEL_ERROR_STATUSES = [401, 403]; + // #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 @@ -68,56 +77,107 @@ export function applyComboTargetExhaustion( target: ResolvedComboTarget, opts: ApplyComboTargetExhaustionOptions ): boolean { - const { - result, - fallbackResult, - errorText, - rawModel, - isTokenLimitBreach, - allAccountsRateLimited, - sets, - log, - tag, - exhaustedLogLevel, - structuredError, - } = opts; - const { exhaustedProviders, exhaustedConnections, transientRateLimitedProviders } = sets; + const { result, sets, log, tag } = opts; const provider = target.provider; + // #8133/#8137: auth-level failures (401/403) mean that connection's credentials are bad. + // Split out to keep applyComboTargetExhaustion under the complexity ceiling. + if (AUTH_LEVEL_ERROR_STATUSES.includes(result.status) && provider && provider !== "unknown") { + markAuthLevelExhaustion(target, { result, sets, log, tag }); + return true; + } + // #1731: full provider quota exhausted → skip remaining same-provider targets this request. - // Passthrough/per-model-quota providers multiplex models behind one connection, so a quota - // 429 for one model must NOT skip fallback targets for another model on the same provider. - const providerExhausted = - Boolean(provider && provider !== "unknown") && - !hasPerModelQuota(provider, rawModel) && - (isProviderExhaustedReason(fallbackResult) || - classifyErrorText(structuredError?.code || errorText) === RateLimitReason.QUOTA_EXHAUSTED || - allAccountsRateLimited); + const providerExhausted = isProviderQuotaExhausted(provider, opts); if (providerExhausted) { - exhaustedProviders.add(provider); - const emit = exhaustedLogLevel === "debug" ? log.debug : log.info; - emit?.( - tag, - `Provider ${provider} quota exhausted — marking for skip on remaining targets (#1731)` - ); + markProviderQuotaExhaustion(provider as string, opts); } else { - if (result.status === 429 && !isTokenLimitBreach && provider && provider !== "unknown") { - transientRateLimitedProviders.add(provider); - } - markConnectionLevelExhaustion(target, { - result, - errorText, - sets, - log, - tag, - rawModel, - structuredError, - }); + markTransientOrConnectionLevel(target, opts); } return providerExhausted; } +/** + * #1731: full-provider quota-exhaustion classification, split out of applyComboTargetExhaustion + * to keep it under the complexity ceiling. Passthrough/per-model-quota providers multiplex + * models behind one connection, so a quota 429 for one model must NOT skip fallback targets for + * another model on the same provider. + */ +function isProviderQuotaExhausted( + provider: string | null | undefined, + opts: Pick< + ApplyComboTargetExhaustionOptions, + "rawModel" | "fallbackResult" | "structuredError" | "errorText" | "allAccountsRateLimited" + > +): boolean { + const { rawModel, fallbackResult, structuredError, errorText, allAccountsRateLimited } = opts; + return ( + Boolean(provider && provider !== "unknown") && + !hasPerModelQuota(provider as string, rawModel) && + (isProviderExhaustedReason(fallbackResult) || + classifyErrorText(structuredError?.code || errorText) === RateLimitReason.QUOTA_EXHAUSTED || + allAccountsRateLimited) + ); +} + +function markProviderQuotaExhaustion( + provider: string, + opts: Pick +): void { + const { sets, log, tag, exhaustedLogLevel } = opts; + sets.exhaustedProviders.add(provider); + const emit = exhaustedLogLevel === "debug" ? log.debug : log.info; + emit?.(tag, `Provider ${provider} quota exhausted — marking for skip on remaining targets (#1731)`); +} + +/** + * Not-quota-exhausted path: track transient 429 rate-limiting, then delegate to the + * connection-level (408/5xx) classification. Split out of applyComboTargetExhaustion to keep + * it under the complexity ceiling. + */ +function markTransientOrConnectionLevel( + target: ResolvedComboTarget, + opts: ApplyComboTargetExhaustionOptions +): void { + const { result, errorText, rawModel, isTokenLimitBreach, sets, log, tag, structuredError } = + opts; + const provider = target.provider; + if (result.status === 429 && !isTokenLimitBreach && provider && provider !== "unknown") { + sets.transientRateLimitedProviders.add(provider); + } + markConnectionLevelExhaustion(target, { result, errorText, sets, log, tag, rawModel, structuredError }); +} + +/** + * #8133/#8137: mark an auth-level (401/403) failure. When the target carries a connectionId, + * only that connection's credentials are bad — sibling connections on the same provider may + * still be healthy, so mark connection-level exhaustion (mirrors markConnectionLevelExhaustion). + * Falls back to whole-provider exhaustion only when no connectionId is available, since every + * model behind an unscoped provider will fail identically. + */ +function markAuthLevelExhaustion( + target: ResolvedComboTarget, + opts: Pick +): void { + const { result, sets, log, tag } = opts; + const provider = target.provider; + const connId = target.connectionId ?? undefined; + if (connId) { + sets.exhaustedConnections.add(`${provider}:${connId}`); + log.info( + tag, + `Provider ${provider} connection ${connId} auth failure (${result.status}) — marking for skip on remaining targets (#8133)` + ); + } else { + sets.exhaustedProviders.add(provider); + log.info( + tag, + `Provider ${provider} auth failure (${result.status}) — marking for skip on remaining targets (#8133)` + ); + } +} + /** * #1731v2: connection-level errors (408/5xx, excluding the OmniRoute circuit-open signal) suggest * the provider connection itself is bad → skip remaining same-connection (or same-provider, when diff --git a/tests/unit/combo/combo-target-exhaustion.test.ts b/tests/unit/combo/combo-target-exhaustion.test.ts index 4ac95bcbd3..3e776d6d23 100644 --- a/tests/unit/combo/combo-target-exhaustion.test.ts +++ b/tests/unit/combo/combo-target-exhaustion.test.ts @@ -382,3 +382,125 @@ test("gemini 524 DOES exhaust connection (cloudflare timeout)", () => { }); assert.equal(s.exhaustedConnections.has("gemini:gemini-key-abc"), true); }); + +// #8133/#8137: auth-level failures (401/403) mean THAT connection's credentials are bad. +// When the target carries a connectionId, only that connection is marked exhausted — sibling +// connections on the same provider must stay eligible (#8137: whole-provider exhaustion wrongly +// skipped healthy sibling connections). Only fall back to whole-provider exhaustion when no +// connectionId is available. +test("401 auth failure marks only that connection exhausted, not the whole provider (#8137)", () => { + const s = sets(); + const exhausted = applyComboTargetExhaustion(target(), { + ...baseOpts, + result: { status: 401 }, + fallbackResult: {}, + errorText: "Missing API key.", + sets: s, + }); + assert.equal(exhausted, true); + assert.equal( + s.exhaustedProviders.size, + 0, + "must NOT exhaust the whole provider when a connectionId is present" + ); + assert.ok( + s.exhaustedConnections.has("test-dedup-provider:conn-1"), + "must exhaust the specific connection" + ); + assert.equal(s.transientRateLimitedProviders.size, 0); +}); + +test("403 forbidden marks only that connection exhausted, not the whole provider (#8137)", () => { + const s = sets(); + const exhausted = applyComboTargetExhaustion(target(), { + ...baseOpts, + result: { status: 403 }, + fallbackResult: {}, + errorText: "Forbidden.", + sets: s, + }); + assert.equal(exhausted, true); + assert.equal(s.exhaustedProviders.size, 0); + assert.ok(s.exhaustedConnections.has("test-dedup-provider:conn-1")); +}); + +test("401 without a connectionId falls back to whole-provider exhaustion (#8133)", () => { + const s = sets(); + const exhausted = applyComboTargetExhaustion(target({ connectionId: null }), { + ...baseOpts, + result: { status: 401 }, + fallbackResult: {}, + errorText: "Missing API key.", + sets: s, + }); + assert.equal(exhausted, true); + assert.ok( + s.exhaustedProviders.has("test-dedup-provider"), + "no connectionId to scope to — must fall back to whole-provider" + ); + assert.equal(s.exhaustedConnections.size, 0); +}); + +test("401 on unknown provider does NOT mark anything (guard)", () => { + const s = sets(); + const exhausted = applyComboTargetExhaustion(target({ provider: "unknown" }), { + ...baseOpts, + result: { status: 401 }, + fallbackResult: {}, + errorText: "Missing API key.", + sets: s, + }); + assert.equal(exhausted, false, "unknown provider must not be marked exhausted"); + assert.equal(s.exhaustedProviders.size, 0); + assert.equal(s.exhaustedConnections.size, 0); +}); + +test("401 on per-model-quota provider marks only the failing connection (auth is connection-scoped, not model-specific)", () => { + const s = sets(); + const exhausted = applyComboTargetExhaustion( + target({ provider: "gemini", connectionId: "gemini-conn-1" }), + { + ...baseOpts, + result: { status: 401 }, + fallbackResult: {}, + errorText: "Missing API key.", + rawModel: "gemini-2.0-flash", + sets: s, + } + ); + assert.equal(exhausted, true); + assert.equal(s.exhaustedProviders.size, 0, "must not exhaust the whole provider"); + assert.ok( + s.exhaustedConnections.has("gemini:gemini-conn-1"), + "auth failure is scoped to the failing connection" + ); +}); + +// #8137 regression: a SIBLING connection on the SAME provider must NOT be skipped when a +// DIFFERENT connection on that provider returned 401/403 — proves the fix at the call-site +// level (getExhaustedTargetSkipReason-style check), not just the raw Set contents above. +test("sibling connection on the same provider is NOT skipped after a different connection's 401 (#8137)", () => { + const s = sets(); + const failingTarget = target({ provider: "test-dedup-provider", connectionId: "conn-1" }); + const siblingTarget = target({ provider: "test-dedup-provider", connectionId: "conn-2" }); + + applyComboTargetExhaustion(failingTarget, { + ...baseOpts, + result: { status: 401 }, + fallbackResult: {}, + errorText: "Missing API key.", + sets: s, + }); + + // Whole-provider check (what the OLD buggy code would have used) must be false. + assert.equal(s.exhaustedProviders.has(siblingTarget.provider), false); + // Connection-level check for the sibling's OWN connection must also be false — it was never + // marked, so combo routing must still consider it eligible. + assert.equal( + s.exhaustedConnections.has(`${siblingTarget.provider}:${siblingTarget.connectionId}`), + false, + "sibling connection must remain eligible after a different connection's auth failure" + ); + // The failing connection itself IS marked. + assert.ok(s.exhaustedConnections.has(`${failingTarget.provider}:${failingTarget.connectionId}`)); +});