fix(account-selector): enhance round-robin logic to handle excluded accounts and maintain state (#349)

Co-authored-by: nyatoru <nyarutoru0002@outlook.co.th>

src/sse/services/auth.ts

@@ -218,27 +218,58 @@ export async function getProviderCredentials(
     if (strategy === "round-robin") {
       const stickyLimit = toNumber((settings as Record<string, unknown>).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, {

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");
+});