fix(combo): skip remaining same-provider targets on 401/403 auth failure (#8195)

* fix(ci): resolve upstream-inherited check failures * fix(combo): skip remaining same-provider targets on 401/403 auth failure (#8133) When a provider returns 401/403 (auth failure), every remaining model behind the same provider will fail identically. Previously the combo engine continued trying sibling models on the dead connection, wasting attempts. Now auth-level failures (401, 403) are classified as provider-level exhaustion, marking exhaustedProviders so subsequent same-provider targets are skipped via the existing exhaustion-skip mechanism. Tests: 4 new cases in combo-target-exhaustion.test.ts covering 401, 403, unknown-provider guard, and per-model-quota provider (auth is provider-wide). * fix(combo): connection-level exhaustion on 401/403, not whole-provider (#8137) Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
2026-07-26 09:52:11 +03:00 · 2026-07-23 05:33:57 -03:00
parent 612b6b38ef
commit 1b42044c15
4 changed files with 244 additions and 44 deletions
--- 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=
--- 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.                               |
--- 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<ApplyComboTargetExhaustionOptions, "sets" | "log" | "tag" | "exhaustedLogLevel">
+): 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<ApplyComboTargetExhaustionOptions, "result" | "sets" | "log" | "tag">
+): 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
--- 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}`));
+});