diff --git a/changelog.d/fixes/13575-proxy-pool-chat-path-rotation.md b/changelog.d/fixes/13575-proxy-pool-chat-path-rotation.md new file mode 100644 index 0000000000..32539342e2 --- /dev/null +++ b/changelog.d/fixes/13575-proxy-pool-chat-path-rotation.md @@ -0,0 +1 @@ +- fix(db): make the chat-path proxy resolver (`resolveProxyForConnection`) rotate a multi-member pool the same way the registry resolver already does — its per-connection cache was freezing on the first pool member forever instead of re-running the scope's round-robin/sticky/random strategy on each request, unless the connection needs a stable egress (opencode's egress-bucketed quota, grok-web's IP-pinned `cf_clearance`) (#13575) diff --git a/src/lib/db/settings.ts b/src/lib/db/settings.ts index cb4f410579..448c38e6d7 100644 --- a/src/lib/db/settings.ts +++ b/src/lib/db/settings.ts @@ -15,6 +15,7 @@ import { isProxySkipRecentlyFailedEnabled } from "@/shared/utils/featureFlags"; import { invalidateDbCache } from "./readCache"; import { encrypt, decrypt } from "./encryption"; import { getProxyRegistryGeneration, resolveProxyForScopeFromRegistry } from "./proxies"; +import { isEgressBucketedLockScope } from "@omniroute/open-sse/config/providerErrorRules.ts"; import { getComboModelProvider as getComboEntryProvider } from "@/lib/combos/steps"; import { requestBodyLimitMbFromEnv } from "@/shared/constants/bodySize"; import { DEFAULT_RESPONSES_PREVIOUS_RESPONSE_ID_MODE } from "@/shared/constants/responsesPreviousResponseId"; @@ -527,6 +528,41 @@ function isCachedPoolMemberSetAside(entry: ProxyResolutionCacheEntry): boolean { return isProxySkipRecentlyFailedEnabled(); } +// Providers that need a STABLE egress across requests, never rotated under them by this +// cache-invalidation path (#13575): opencode's free-tier quota is bucketed by egress IP +// (EGRESS_BUCKETED_LOCK_PROVIDERS — rotating would fragment one connection's quota across +// several IPs), and grok-web's cf_clearance cookie is pinned to the IP/User-Agent/TLS +// fingerprint that earned it (src/shared/providers/webSessionCredentials.ts "grok-web" — +// rotating the egress would turn every subsequent request into a Cloudflare 403). +function requiresStableEgress(provider: string | null): boolean { + if (!provider) return false; + return isEgressBucketedLockScope(provider) || provider.toLowerCase() === "grok-web"; +} + +// The chat-path cache (below) exists so a hot connection does not pay the full resolution +// cascade on every request, but it must not FREEZE a rotating pool's choice: the registry +// resolver (resolveProxyForScopeFromRegistry, called directly by every #6365 rotation test) +// re-runs its strategy on every call and rotates correctly, while the cache here returned +// the same first-resolved member forever (#13575). A cached member is stale whenever it came +// from a live scope pool (source: "registry") and the connection is not in the two populations +// above that need a pinned egress instead: the caller then falls through to the full cascade, +// which re-invokes resolveProxyForScopeFromRegistry and applies the pool's own selection +// strategy (round-robin advances, sticky holds until its window elapses, random reshuffles) — +// no new strategy is introduced here. +function isCachedPoolMemberDue( + entry: ProxyResolutionCacheEntry, + db: ReturnType, + connectionId: string +): boolean { + const { result } = entry; + if (result.source !== "registry" || result.proxy == null) return false; + const row = db + .prepare("SELECT provider FROM provider_connections WHERE id = ?") + .get(connectionId) as { provider?: string } | undefined; + const provider = typeof row?.provider === "string" ? row.provider : null; + return !requiresStableEgress(provider); +} + export async function resolveProxyForConnection( connectionId: string, apiKeyId?: string, @@ -542,18 +578,18 @@ export async function resolveProxyForConnection( registryGeneration: getProxyRegistryGeneration(), refusalSeq: getProxyRefusalSeq(), }; + const db = getDbInstance(); const cached = proxyResolutionCache.get(cacheKey); if ( cached && cached.generation === stamp.generation && cached.registryGeneration === stamp.registryGeneration && - !isCachedPoolMemberSetAside(cached) + !isCachedPoolMemberSetAside(cached) && + !isCachedPoolMemberDue(cached, db, connectionId) ) { return cached.result; } - const db = getDbInstance(); - // Step 1: Check global proxyEnabled setting // Read only the proxyEnabled key for performance instead of loading all settings. let globalProxyEnabled = true; diff --git a/tests/unit/proxy-pool-chat-path-rotation-13575.test.ts b/tests/unit/proxy-pool-chat-path-rotation-13575.test.ts new file mode 100644 index 0000000000..a11f79668c --- /dev/null +++ b/tests/unit/proxy-pool-chat-path-rotation-13575.test.ts @@ -0,0 +1,120 @@ +/** + * Regression test for issue #13575. + * + * The chat path (`resolveProxyForConnection`) must rotate across a multi-member + * pool the same way the registry-level resolver does (`resolveProxyForScopeFromRegistry`), + * instead of freezing on the first pick for the life of the per-connection cache. + * Providers with a stable-egress requirement (egress-bucketed quota, IP-pinned web + * session credentials) must stay pinned to a single member. + */ +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-proxy-13575-rotation-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "test-secret"; + +const core = await import("../../src/lib/db/core.ts"); +const proxiesDb = await import("../../src/lib/db/proxies.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); + +let proxySeq = 0; +async function makeProxy() { + proxySeq++; + const proxy = await proxiesDb.createProxy({ + name: `Pool proxy ${proxySeq}`, + type: "http", + host: `10.9.1.${proxySeq}`, + port: 9100 + proxySeq, + status: "active", + }); + return proxy!; +} + +async function makeConnection(provider = "openai"): Promise { + const conn = await providersDb.createProviderConnection({ + provider, + authType: "apiKey", + name: `Conn ${Date.now()} ${Math.random()}`, + apiKey: "sk-test", + }); + return (conn as { id: string }).id; +} + +test.after(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +test("#13575: resolveProxyForConnection rotates round-robin across a multi-member pool", async () => { + const a = await makeProxy(); + const b = await makeProxy(); + const c = await makeProxy(); + const connId = await makeConnection("openai"); + await proxiesDb.addProxyToScopePool("account", connId, a.id); + await proxiesDb.addProxyToScopePool("account", connId, b.id); + await proxiesDb.addProxyToScopePool("account", connId, c.id); + + const chatPathHosts: string[] = []; + for (let i = 0; i < 6; i++) { + const r = (await settingsDb.resolveProxyForConnection(connId)) as { + proxy?: { host?: string } | null; + }; + chatPathHosts.push(r.proxy?.host ?? "null"); + } + + const distinctChatHosts = new Set(chatPathHosts); + assert.ok( + distinctChatHosts.size > 1, + `expected resolveProxyForConnection to rotate across the pool, but got: ${JSON.stringify(chatPathHosts)}` + ); + assert.deepEqual(distinctChatHosts, new Set([a.host, b.host, c.host])); +}); + +test("#13575: egress-bucketed-quota providers (opencode) stay pinned on the chat path", async () => { + const a = await makeProxy(); + const b = await makeProxy(); + const connId = await makeConnection("opencode"); + await proxiesDb.addProxyToScopePool("account", connId, a.id); + await proxiesDb.addProxyToScopePool("account", connId, b.id); + + const hosts: string[] = []; + for (let i = 0; i < 6; i++) { + const r = (await settingsDb.resolveProxyForConnection(connId)) as { + proxy?: { host?: string } | null; + }; + hosts.push(r.proxy?.host ?? "null"); + } + + assert.deepEqual( + new Set(hosts), + new Set([hosts[0]]), + `expected an egress-bucketed-quota connection to stay pinned to one member, got: ${JSON.stringify(hosts)}` + ); +}); + +test("#13575: IP-pinned web-session providers (grok-web) stay pinned on the chat path", async () => { + const a = await makeProxy(); + const b = await makeProxy(); + const connId = await makeConnection("grok-web"); + await proxiesDb.addProxyToScopePool("account", connId, a.id); + await proxiesDb.addProxyToScopePool("account", connId, b.id); + + const hosts: string[] = []; + for (let i = 0; i < 6; i++) { + const r = (await settingsDb.resolveProxyForConnection(connId)) as { + proxy?: { host?: string } | null; + }; + hosts.push(r.proxy?.host ?? "null"); + } + + assert.deepEqual( + new Set(hosts), + new Set([hosts[0]]), + `expected an IP-pinned web-session connection to stay pinned to one member, got: ${JSON.stringify(hosts)}` + ); +}); diff --git a/tests/unit/proxy-pool-skips-refused-member.test.ts b/tests/unit/proxy-pool-skips-refused-member.test.ts index c2dc6c293c..56e2b6ec4e 100644 --- a/tests/unit/proxy-pool-skips-refused-member.test.ts +++ b/tests/unit/proxy-pool-skips-refused-member.test.ts @@ -5,9 +5,10 @@ import os from "node:os"; import path from "node:path"; // With PROXY_SKIP_RECENTLY_FAILED on, pool selection skips members that just failed, for -// every rotation strategy, and the per-connection resolution cache stops re-serving such a -// member (once per set-aside event, never a DB cascade per request). With every member set -// aside, or the flag off (the default), selection is exactly what it was. +// every rotation strategy. The per-connection resolution used by the chat path +// (resolveProxyForConnection) re-runs the registry cascade on every call for a multi-member +// pool (#13575) and skips a set-aside member on top of that. With every member set aside, or +// the flag off (the default), set-aside skipping does not apply but rotation continues. const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-pool-skip-refused-")); process.env.DATA_DIR = TEST_DATA_DIR; @@ -167,34 +168,39 @@ test("a DB override turning the flag off wins over the environment", async () => assert.deepEqual(await picks(2), [members[0].host, members[2].host]); }); -test("a connection's cached pool member is not re-served once set aside", async () => { +// #13575: the chat-path resolveProxyForConnection() no longer freezes a multi-member +// pool's choice for the connection's lifetime — it re-runs the registry cascade on every +// call (like every other pool consumer) so round-robin/sticky/random keep working exactly +// as resolveProxyForScopeFromRegistry() already does when called directly. A member set +// aside is still skipped on top of that rotation, same as before. +test("a connection's chat-path resolution skips a member set aside", async () => { const [a, b, c] = await pool(3, "account", "conn-pool"); const first = await settingsDb.resolveProxyForConnection("conn-pool"); assert.equal((first as { proxy: { host: string } }).proxy.host, a.host); - assert.strictEqual(await settingsDb.resolveProxyForConnection("conn-pool"), first); - setAside(a); + setAside(b); const next = await settingsDb.resolveProxyForConnection("conn-pool"); - assert.equal((next as { proxy: { host: string } }).proxy.host, b.host); + assert.equal((next as { proxy: { host: string } }).proxy.host, c.host); - memory.noteProxyRecovered(keyOf(a), "proxy_unreachable"); - assert.equal(memory.isProxyAvoided(keyOf(a)), false); + memory.noteProxyRecovered(keyOf(b), "proxy_unreachable"); + assert.equal(memory.isProxyAvoided(keyOf(b)), false); assert.deepEqual( [await pick("account", "conn-pool"), await pick("account", "conn-pool")], - [c.host, a.host] + [a.host, b.host] ); }); -test("with the flag off a connection keeps its cached pool member even once set aside", async () => { - const [a] = await pool(3, "account", "conn-off"); +test("with the flag off a connection's chat-path resolution still rotates normally", async () => { + const [a, b] = await pool(3, "account", "conn-off"); delete process.env.PROXY_SKIP_RECENTLY_FAILED; const first = await settingsDb.resolveProxyForConnection("conn-off"); assert.equal((first as { proxy: { host: string } }).proxy.host, a.host); setAside(a); - assert.strictEqual(await settingsDb.resolveProxyForConnection("conn-off"), first); + const next = await settingsDb.resolveProxyForConnection("conn-off"); + assert.equal((next as { proxy: { host: string } }).proxy.host, b.host); }); -test("with every member set aside the cascade re-runs once, not on every request", async () => { +test("with every member set aside, chat-path resolution keeps rotating like the registry", async () => { // Round-robin advances its persisted cursor on each cascade run, so the cursor counts // how many times the registry pool was actually queried for this connection. const [a, b] = await pool(2, "account", "conn-all"); @@ -218,10 +224,9 @@ test("with every member set aside the cascade re-runs once, not on every request assert.equal((second as { proxy: { host: string } }).proxy.host, b.host); assert.equal(cursor(), 2); - for (let i = 0; i < 5; i++) { - assert.strictEqual(await settingsDb.resolveProxyForConnection("conn-all"), second); - } - assert.equal(cursor(), 2, "a member set aside before the entry was cached must not bypass it"); + const third = await settingsDb.resolveProxyForConnection("conn-all"); + assert.equal((third as { proxy: { host: string } }).proxy.host, a.host); + assert.equal(cursor(), 3); }); test("a legacy single-proxy level stays cached even when its proxy is set aside", async () => {