fix(autoCombo): rotate across all connections, never waste provider capacity - #3078
diegosouzapw merged 3 commits into
Conversation
…apacity Per-connection expansion in buildAutoCandidates turns 43 Cerebras keys into 43 candidates instead of 1. ScoreTierRotator uses per-combo round-robin state (was a global counter) so all connections in a tier are visited across requests. New connectionDensity factor (weight 0.05) rewards multi-connection providers so they surface in the top tier. Budget cap degradation now respects rotation — filters to budget-compliant candidates and uses the rotator instead of always picking the single cheapest. Falls back to cheapest only if no candidate meets the cap. Verification: 43/43 Cerebras connections hit in 500 requests, balanced 10-12 picks each. All 146 vitest + 83 Node.js combo tests pass.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tiered rotation and connection density factors to the auto-combo routing engine, allowing traffic to be distributed across tiers and active provider connections. The review identified several critical improvements: resolving a mathematical bias in the weighted tier selection, moving the newly added test file to the tests/ directory to comply with the repository style guide, optimizing the budget cap enforcement's performance from O(N^2 log N) to O(N log N) using a cost map, and adding defensive checks to prevent potential runtime crashes when resolving connection IDs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function chooseTierWeighted( | ||
| tiers: Record<TierName, ScoredProvider[]>, | ||
| prefs: Record<TierName, number>, | ||
| pickFromPool: (pool: ScoredProvider[]) => ScoredProvider, | ||
| fallback: () => ScoredProvider | ||
| ): ScoredProvider { | ||
| const total = prefs.top + prefs.mid + prefs.rest; | ||
| if (total <= 0) return fallback(); | ||
| const r = Math.random() * total; | ||
| let acc = 0; | ||
| if ((acc += prefs.top) >= r && tiers.top.length > 0) return pickFromPool(tiers.top); | ||
| if ((acc += prefs.mid) >= r && tiers.mid.length > 0) return pickFromPool(tiers.mid); | ||
| if (tiers.rest.length > 0) return pickFromPool(tiers.rest); | ||
| return fallback(); | ||
| } |
There was a problem hiding this comment.
When some tiers are empty, the current implementation of chooseTierWeighted introduces a mathematical bias. The weight of the empty tier is entirely absorbed by the next non-empty tier in the sequence instead of being distributed proportionally among all active tiers. Furthermore, this causes uneven round-robin rotation because it splits the rotation state between rrCounter and tierCounters unnecessarily when falling back. Normalizing the weights of only the non-empty tiers resolves both issues cleanly.
function chooseTierWeighted(
tiers: Record<TierName, ScoredProvider[]>,
prefs: Record<TierName, number>,
pickFromPool: (pool: ScoredProvider[]) => ScoredProvider,
fallback: () => ScoredProvider
): ScoredProvider {
const activePrefs = {
top: tiers.top.length > 0 ? prefs.top : 0,
mid: tiers.mid.length > 0 ? prefs.mid : 0,
rest: tiers.rest.length > 0 ? prefs.rest : 0,
};
const total = activePrefs.top + activePrefs.mid + activePrefs.rest;
if (total <= 0) return fallback();
const r = Math.random() * total;
let acc = 0;
if (activePrefs.top > 0 && (acc += activePrefs.top) >= r) return pickFromPool(tiers.top);
if (activePrefs.mid > 0 && (acc += activePrefs.mid) >= r) return pickFromPool(tiers.mid);
if (activePrefs.rest > 0) return pickFromPool(tiers.rest);
return fallback();
}| /** | ||
| * Tests for ScoreTierRotator and connectionDensity factor. | ||
| * Verifies that multi-connection providers surface in ranked candidates | ||
| * and that tiered rotation distributes traffic fairly. | ||
| */ |
There was a problem hiding this comment.
According to the Repository Style Guide (Rule 1), all unit tests, integration tests, ecosystem tests, or Vitest files must strictly be placed within the tests/ directory (e.g., tests/unit/, tests/integration/). Creating test files inside the open-sse/ directory violates this rule. Please move this file to tests/unit/autoCombo/tieredRotation.test.ts or similar.
References
- ALL unit tests, integration tests, ecosystem tests, or Vitest files MUST strictly be placed within the tests/ directory. (link)
| const estimatedCostFor = (s: ScoredProvider) => { | ||
| const c = candidates.find( | ||
| (cand) => cand.provider === s.provider && cand.model === s.model | ||
| ); | ||
| const cost = c?.costPer1MTokens ?? 0; | ||
| return (cost / 1_000_000) * 1000; | ||
| }; |
There was a problem hiding this comment.
Calling candidates.find inside filter and sort results in
const costMap = new Map<string, number>();
for (const c of candidates) {
costMap.set(`${c.provider}\\0${c.model}`, c.costPer1MTokens);
}
const estimatedCostFor = (s: ScoredProvider) => {
const cost = costMap.get(`${s.provider}\\0${s.model}`) ?? 0;
return (cost / 1_000_000) * 1000;
};| const connectionIds = providerConnections | ||
| .map((c) => (typeof c.id === "string" ? c.id : null)) | ||
| .filter((id): id is string => id !== null); |
There was a problem hiding this comment.
Accessing c.id directly without verifying that c is a non-null object poses a runtime crash risk if the database query returns unexpected null or undefined elements. Adding a defensive check ensures robustness.
| const connectionIds = providerConnections | |
| .map((c) => (typeof c.id === "string" ? c.id : null)) | |
| .filter((id): id is string => id !== null); | |
| const connectionIds = providerConnections | |
| .map((c) => (c && typeof c === "object" && typeof c.id === "string" ? c.id : null)) | |
| .filter((id): id is string => id !== null); |
- chooseTierWeighted: normalize active tier weights only, fix bias where empty tier weight was absorbed by next non-empty tier - Add CLEAR_WINNER_THRESHOLD fast path: when score gap between best and worst >= 0.1, always pick from top tier (preserves deterministic best-candidate behavior for clear winners) - Budget cap: pre-calculate cost Map, reduce O(N^2 log N) to O(N log N) - combo.ts: add null check on connection object before accessing .id - Move tieredRotation.test.ts from open-sse/services/autoCombo/__tests__/ to tests/unit/autoCombo/ per repository style guide - Add tests/unit/autoCombo/ to vitest.mcp.config.ts include list
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · 1,776,783 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| expandedTargets.push({ | ||
| ...target, | ||
| connectionId, | ||
| executionKey: `${target.executionKey}@${connectionId}`, |
There was a problem hiding this comment.
SUGGESTION: Execution key delimiter collision risk
Using @ as a separator in the execution key could cause collisions if connection IDs contain @. Consider using a safer delimiter like | or # which is less likely to appear in UUIDs/connection identifiers.
| tierAffinity: 0, | ||
| specificityMatch: 0, | ||
| contextAffinity: 0.05, | ||
| contextAffinity: 0.0, |
There was a problem hiding this comment.
CRITICAL: Mode pack weight sum validation failure
The cost-saver mode pack sums to 0.96 (0.14+0.19+0.37+0.05+0.1+0.05+0.05+0+0+0+0+0.05), which fails validateWeights() check (Math.abs(sum - 1.0) < 0.01). This would cause the pack to be rejected and fall back to DEFAULT_WEIGHTS. Similar issues exist in quality-first and offline-friendly packs.
…ombo-rotation-v3.8.8
|
Thank you @oyi77 for this excellent improvement! The per-connection candidate expansion (43 Cerebras keys -> 43 candidates) combined with ScoreTierRotator and the connectionDensity factor correctly solves the provider capacity underutilization problem. All 146 vitest + 83 combo tests pass. Merged into release/v3.8.9 — will ship in v3.8.9. |
…apacity (diegosouzapw#3078) Integrated into release/v3.8.9. Clean merge, all 146 vitest tests pass.
…apacity (diegosouzapw#3078) Integrated into release/v3.8.9. Clean merge, all 146 vitest tests pass.
…apacity (diegosouzapw#3078) Integrated into release/v3.8.9. Clean merge, all 146 vitest tests pass.
2 participants
Problem
Auto combos on release/v3.8.8 silently underutilize connected providers. 43 Cerebras connections collapse to 1 candidate because buildAutoCandidates maps targets 1:1 to candidates — rotation never reaches individual keys. Selection is greedy (candidates_[0]) so identical-score candidates always pick the first in array order.
Impact: hundreds of active connections across free-tier providers in the autoCombo candidate pool, but routing only ever touches one at a time per provider.
Fix
Verification
Backward compatibility