diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 3f76fc956b..badcf451c3 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -218,27 +218,58 @@ export async function getProviderCredentials( if (strategy === "round-robin") { const stickyLimit = toNumber((settings as Record).stickyRoundRobinLimit, 3); - // Sort by lastUsed (most recent first) to find current candidate - const byRecency = [...orderedConnections].sort((a: any, b: any) => { - if (!a.lastUsedAt && !b.lastUsedAt) return (a.priority || 999) - (b.priority || 999); - if (!a.lastUsedAt) return 1; - if (!b.lastUsedAt) return -1; - return new Date(b.lastUsedAt).getTime() - new Date(a.lastUsedAt).getTime(); - }); + // If excluding an account (fallback scenario), skip sticky logic and go straight to LRU + // This prevents the system from getting stuck on a failed account + const isFallbackScenario = excludeConnectionId !== null; - const current = byRecency[0]; - const currentCount = current?.consecutiveUseCount || 0; - - if (current && current.lastUsedAt && currentCount < stickyLimit) { - // Stay with current account - connection = current; - // Update lastUsedAt and increment count (await to ensure persistence) - await updateProviderConnection(connection.id, { - lastUsedAt: new Date().toISOString(), - consecutiveUseCount: (connection.consecutiveUseCount || 0) + 1, + if (!isFallbackScenario) { + // Sort by lastUsed (most recent first) to find current candidate + const byRecency = [...orderedConnections].sort((a: any, b: any) => { + if (!a.lastUsedAt && !b.lastUsedAt) return (a.priority || 999) - (b.priority || 999); + if (!a.lastUsedAt) return 1; + if (!b.lastUsedAt) return -1; + return new Date(b.lastUsedAt).getTime() - new Date(a.lastUsedAt).getTime(); }); + + const current = byRecency[0]; + const currentCount = current?.consecutiveUseCount || 0; + + if (current && current.lastUsedAt && currentCount < stickyLimit) { + // Stay with current account + connection = current; + log.debug( + "AUTH", + `${provider} round-robin: staying with ${current.id?.slice(0, 8)}... (count=${currentCount}/${stickyLimit})` + ); + // Update lastUsedAt and increment count (await to ensure persistence) + await updateProviderConnection(connection.id, { + lastUsedAt: new Date().toISOString(), + consecutiveUseCount: (connection.consecutiveUseCount || 0) + 1, + }); + } else { + // Pick the least recently used (excluding current if possible) + const sortedByOldest = [...orderedConnections].sort((a: any, b: any) => { + if (!a.lastUsedAt && !b.lastUsedAt) return (a.priority || 999) - (b.priority || 999); + if (!a.lastUsedAt) return -1; + if (!b.lastUsedAt) return 1; + return new Date(a.lastUsedAt).getTime() - new Date(b.lastUsedAt).getTime(); + }); + + connection = sortedByOldest[0]; + log.debug( + "AUTH", + `${provider} round-robin: switching to LRU ${connection.id?.slice(0, 8)}... (current count=${currentCount} >= limit=${stickyLimit} or no lastUsedAt)` + ); + + // Update lastUsedAt and reset count to 1 (await to ensure persistence) + await updateProviderConnection(connection.id, { + lastUsedAt: new Date().toISOString(), + consecutiveUseCount: 1, + }); + } } else { - // Pick the least recently used (excluding current if possible) + // Fallback scenario: excluded an account due to failure + // Always pick the least recently used to ensure proper cycling const sortedByOldest = [...orderedConnections].sort((a: any, b: any) => { if (!a.lastUsedAt && !b.lastUsedAt) return (a.priority || 999) - (b.priority || 999); if (!a.lastUsedAt) return -1; @@ -247,6 +278,10 @@ export async function getProviderCredentials( }); connection = sortedByOldest[0]; + log.info( + "AUTH", + `${provider} round-robin: FALLBACK MODE - excluded ${excludeConnectionId?.slice(0, 8)}..., picked LRU ${connection.id?.slice(0, 8)}...` + ); // Update lastUsedAt and reset count to 1 (await to ensure persistence) await updateProviderConnection(connection.id, { diff --git a/tests/unit/account-selector.test.mjs b/tests/unit/account-selector.test.mjs index 10168c6a6c..2be1a805b6 100644 --- a/tests/unit/account-selector.test.mjs +++ b/tests/unit/account-selector.test.mjs @@ -1,10 +1,8 @@ import test from "node:test"; import assert from "node:assert/strict"; -const { - selectAccountP2C, - selectAccount, -} = await import("../../open-sse/services/accountSelector.ts"); +const { selectAccountP2C, selectAccount } = + await import("../../open-sse/services/accountSelector.ts"); // ─── selectAccountP2C ─────────────────────────────────────────────────────── @@ -19,11 +17,7 @@ test("selectAccountP2C: returns single account", () => { }); test("selectAccountP2C: returns one of the candidates", () => { - const accounts = [ - { id: "a1" }, - { id: "a2" }, - { id: "a3" }, - ]; + const accounts = [{ id: "a1" }, { id: "a2" }, { id: "a3" }]; const selected = selectAccountP2C(accounts); assert.ok(accounts.includes(selected)); }); @@ -79,3 +73,52 @@ test("selectAccount: empty accounts returns null", () => { const { account } = selectAccount([], "fill-first"); assert.equal(account, null); }); + +// ─── Round-robin fallback scenario (Issue #340) ───────────────────────────── + +test("selectAccount: round-robin with excludeConnectionId skips excluded", () => { + const accounts = [{ id: "a" }, { id: "b" }, { id: "c" }]; + + // Simulate: first request picks 'a', then it fails + // Second request should exclude 'a' and pick 'b' + let state = {}; + + // First request - no exclusion + const { account: acc1 } = selectAccount(accounts, "round-robin", state); + state = { lastIndex: 0 }; // Simulate 'a' was picked + + // 'a' fails, exclude it + const { account: acc2 } = selectAccount( + accounts.filter((a) => a.id !== "a"), + "round-robin", + state + ); + + // Should pick 'b' or 'c', not 'a' + assert.notEqual(acc2.id, "a", "Should not pick excluded account"); + assert.ok(["b", "c"].includes(acc2.id), "Should pick from remaining accounts"); +}); + +test("selectAccount: round-robin respects state across calls", () => { + const accounts = [{ id: "a" }, { id: "b" }, { id: "c" }]; + let state = { lastIndex: -1 }; + + // First call should pick index 0 + const { account: acc1, state: state1 } = selectAccount(accounts, "round-robin", state); + assert.equal(acc1.id, "a"); + assert.equal(state1.lastIndex, 0); + + // Second call should pick index 1 + const { account: acc2, state: state2 } = selectAccount(accounts, "round-robin", state1); + assert.equal(acc2.id, "b"); + assert.equal(state2.lastIndex, 1); + + // Third call should pick index 2 + const { account: acc3, state: state3 } = selectAccount(accounts, "round-robin", state2); + assert.equal(acc3.id, "c"); + assert.equal(state3.lastIndex, 2); + + // Fourth call should wrap to index 0 + const { account: acc4 } = selectAccount(accounts, "round-robin", state3); + assert.equal(acc4.id, "a"); +});