Repository navigation
feat(combo): add sticky weighted selection limit with exhaustion-aware renormalization - #4489
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces sticky routing for the weighted selection strategy, allowing consecutive successful requests to stick to a selected target, and adds support for per-combo overrides of the sticky round-robin limit. Feedback on these changes highlights a potential runtime TypeError when target.provider is falsy, a prefix-matching bug with execution keys using startsWith, redundant recursive calls to resolveWeightedStepGroups that can be optimized, and a potential memory leak where stale sticky targets are not cleared when the sticky limit is disabled.
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.
| const isTargetSelectableForWeighted = async (target: ResolvedComboTarget): Promise<boolean> => { | ||
| const rawModel = parseModel(target.modelStr).model || target.modelStr; | ||
| if (getCircuitBreaker(target.provider).getStatus().state === "OPEN") return false; |
There was a problem hiding this comment.
The target.provider property can be falsy (e.g., null, undefined, or empty string) for certain targets or unresolved steps. Calling getCircuitBreaker(target.provider) directly without a guard will cause a runtime TypeError when trying to access .getStatus() on undefined. Add a defensive check for target.provider before calling getCircuitBreaker.
| const isTargetSelectableForWeighted = async (target: ResolvedComboTarget): Promise<boolean> => { | |
| const rawModel = parseModel(target.modelStr).model || target.modelStr; | |
| if (getCircuitBreaker(target.provider).getStatus().state === "OPEN") return false; | |
| const isTargetSelectableForWeighted = async (target: ResolvedComboTarget): Promise<boolean> => { | |
| const rawModel = parseModel(target.modelStr).model || target.modelStr; | |
| if (target.provider && getCircuitBreaker(target.provider).getStatus().state === "OPEN") return false; |
| const step = weightedResolution.orderedSteps.find((entry) => | ||
| target.executionKey.startsWith(entry.executionKey) | ||
| ); |
There was a problem hiding this comment.
Using startsWith to match execution keys can lead to incorrect matches when one step ID is a prefix of another (e.g., step1 and step10). For instance, "step10>step2".startsWith("step1") evaluates to true, which would incorrectly associate the success of step10 with step1. To prevent this prefix-matching bug, ensure the key matches exactly or is followed by the > separator.
| const step = weightedResolution.orderedSteps.find((entry) => | |
| target.executionKey.startsWith(entry.executionKey) | |
| ); | |
| const step = weightedResolution.orderedSteps.find((entry) => | |
| target.executionKey === entry.executionKey || | |
| target.executionKey.startsWith(entry.executionKey + ">") | |
| ); |
| const weightedEligibleKeys = new Set<string>(); | ||
| if (strategy === "weighted") { | ||
| const stepGroups = resolveWeightedStepGroups(combo, allCombos); | ||
| for (const group of stepGroups) { | ||
| const availability = await Promise.all(group.targets.map(isTargetSelectableForWeighted)); | ||
| if (availability.some(Boolean)) weightedEligibleKeys.add(group.step.executionKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
resolveWeightedStepGroups is called twice per request for weighted combos: once on line 587 to check target availability, and again inside resolveWeightedTargets on line 604. Since this function performs recursive DAG expansion and array allocations, calling it twice is redundant. We can optimize this by declaring stepGroups outside the if block and passing it to resolveWeightedTargets.
| const weightedEligibleKeys = new Set<string>(); | |
| if (strategy === "weighted") { | |
| const stepGroups = resolveWeightedStepGroups(combo, allCombos); | |
| for (const group of stepGroups) { | |
| const availability = await Promise.all(group.targets.map(isTargetSelectableForWeighted)); | |
| if (availability.some(Boolean)) weightedEligibleKeys.add(group.step.executionKey); | |
| } | |
| } | |
| let stepGroups: Array<{ step: ComboRuntimeStep; targets: ResolvedComboTarget[] }> | undefined; | |
| const weightedEligibleKeys = new Set<string>(); | |
| if (strategy === "weighted") { | |
| stepGroups = resolveWeightedStepGroups(combo, allCombos); | |
| for (const group of stepGroups) { | |
| const availability = await Promise.all(group.targets.map(isTargetSelectableForWeighted)); | |
| if (availability.some(Boolean)) weightedEligibleKeys.add(group.step.executionKey); | |
| } | |
| } |
| if (rawStickyWeightedKey && !stickyWeightedKey) { | ||
| weightedStickyTargets.delete(combo.name); | ||
| } |
There was a problem hiding this comment.
If stickyWeightedLimit is dynamically changed to 1 or unset (disabled), getStickyWeightedExecutionKey returns null, meaning rawStickyWeightedKey is falsy. Consequently, the if (rawStickyWeightedKey && !stickyWeightedKey) block is skipped, leaving the stale sticky target in weightedStickyTargets indefinitely. Explicitly clear the sticky target when the strategy is not weighted or the limit is disabled to prevent memory leaks and stale routing state.
if (strategy !== "weighted" || stickyWeightedLimit <= 1) {
weightedStickyTargets.delete(combo.name);
} else if (rawStickyWeightedKey && !stickyWeightedKey) {
weightedStickyTargets.delete(combo.name);
}| const weightedResolution = | ||
| strategy === "weighted" | ||
| ? resolveWeightedTargets(combo, allCombos, stickyWeightedKey, weightedEligibleKeys) | ||
| : null; |
There was a problem hiding this comment.
Pass the pre-resolved stepGroups to resolveWeightedTargets to avoid redundant calculations.
| const weightedResolution = | |
| strategy === "weighted" | |
| ? resolveWeightedTargets(combo, allCombos, stickyWeightedKey, weightedEligibleKeys) | |
| : null; | |
| const weightedResolution = | |
| strategy === "weighted" | |
| ? resolveWeightedTargets(combo, allCombos, stickyWeightedKey, weightedEligibleKeys, stepGroups) | |
| : null; |
| export function resolveWeightedTargets( | ||
| combo: ComboLike, | ||
| allCombos: ComboCollectionLike, | ||
| preferredExecutionKey: string | null = null, | ||
| eligibleExecutionKeys: ReadonlySet<string> | null = null | ||
| ): { | ||
| orderedTargets: ResolvedComboTarget[]; | ||
| selectedStep: ComboRuntimeStep | null; | ||
| orderedSteps: ComboRuntimeStep[]; | ||
| } { |
There was a problem hiding this comment.
Add an optional stepGroups parameter to resolveWeightedTargets to allow reusing the pre-resolved step groups and avoid redundant recursive expansions.
export function resolveWeightedTargets(
combo: ComboLike,
allCombos: ComboCollectionLike,
preferredExecutionKey: string | null = null,
eligibleExecutionKeys: ReadonlySet<string> | null = null,
stepGroups?: Array<{ step: ComboRuntimeStep; targets: ResolvedComboTarget[] }>
): {
orderedTargets: ResolvedComboTarget[];
selectedStep: ComboRuntimeStep | null;
orderedSteps: ComboRuntimeStep[];
} {| const targetsByStep = new Map( | ||
| resolveWeightedStepGroups(combo, allCombos).map((group) => [ | ||
| group.step.executionKey, | ||
| group.targets, | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
Use the optional stepGroups parameter if provided, falling back to resolveWeightedStepGroups only when it is not passed.
| const targetsByStep = new Map( | |
| resolveWeightedStepGroups(combo, allCombos).map((group) => [ | |
| group.step.executionKey, | |
| group.targets, | |
| ]) | |
| ); | |
| const targetsByStep = new Map( | |
| (stepGroups || resolveWeightedStepGroups(combo, allCombos)).map((group) => [ | |
| group.step.executionKey, | |
| group.targets, | |
| ]) | |
| ); |
…, stale cleanup Fixes four issues from gemini-code-assist review on diegosouzapw#4489: 1. Guard falsy target.provider before getCircuitBreaker call in both the weighted availability pre-filter and the RR sticky pre-filter to prevent a TypeError on unresolved steps. 2. Fix prefix-matching bug in getWeightedStepKeyForTarget: use exact match or startsWith with ">" separator so step-1 no longer matches step-10's expanded targets. 3. Eliminate redundant resolveWeightedStepGroups call: hoist stepGroups outside the if block and pass it to resolveWeightedTargets via a new optional parameter, avoiding a second recursive DAG expansion. 4. Clear stale weightedStickyTargets entries when the strategy is not weighted or stickyWeightedLimit is disabled (<=1), preventing unbounded memory growth from orphaned sticky records. Co-authored-by: Cursor <cursoragent@cursor.com>
…, stale cleanup Fixes four issues from gemini-code-assist review on diegosouzapw#4489: 1. Guard falsy target.provider before getCircuitBreaker call in both the weighted availability pre-filter and the RR sticky pre-filter to prevent a TypeError on unresolved steps. 2. Fix prefix-matching bug in getWeightedStepKeyForTarget: use exact match or startsWith with ">" separator so step-1 no longer matches step-10's expanded targets. 3. Eliminate redundant resolveWeightedStepGroups call: hoist stepGroups outside the if block and pass it to resolveWeightedTargets via a new optional parameter, avoiding a second recursive DAG expansion. 4. Clear stale weightedStickyTargets entries when the strategy is not weighted or stickyWeightedLimit is disabled (<=1), preventing unbounded memory growth from orphaned sticky records. Co-authored-by: Cursor <cursoragent@cursor.com>
4225406 to
5eed395
Compare
Modeled on the existing per-combo stickyRoundRobinLimit (diegosouzapw#3846). Adds a stickyWeightedLimit config field (0–1000, clamps to 1) that makes weighted combos pin their random selection for N consecutive successes before re-drawing. Fallback successes migrate sticky to the new healthy step. Exhaustion-aware: if the currently-sticky top-level weighted step becomes unavailable (all its expanded leaves are missing), sticky is cleared and the next weighted draw renormalizes over the remaining available steps. A top-level step is still considered available if at least one of its nested leaves reports healthy. Runtime logic: - resolveWeightedStepGroups() — new pure resolver exposing top-level weighted step → leaf expansion before sampling - resolveWeightedTargets() — optional preferredExecutionKey + optional eligibleExecutionKeys set for availability filtering - handleComboChat() — availability pre-filter checks circuit breaker, provider cooldown, model lockout, and isModelAvailable State lives in a new weightedStickyTargets map (same eviction ceiling as rrStickyTargets). getStickyWeightedExecutionKey / recordStickyWeightedSuccess are symmetrical to the round-robin helpers. Schema: comboRuntimeConfigSchema.stickyWeightedLimit (0–1000, coerce). UI: "Sticky Weighted Limit" input in the weighted advanced section, stickyWeightedLimit field description in ADVANCED_FIELD_HELP_FALLBACK, and save wiring in handleSave. Applying recommendations for weighted now opens advanced settings (consistent with round-robin). Tests: schema accept/reject, sticky batch sequencing (controlled Math.random), sticky migration on fallback success, stale sticky clearing when the preselected step is unavailable, nested combo-ref availability when one leaf is down. Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror the weighted sticky availability pre-filter in handleRoundRobinCombo: before computing the start index, check whether the currently sticky round-robin target is still available (circuit breaker, provider cooldown, model lockout, isModelAvailable). If not, clear the stale sticky record so the rotation starts at the counter position instead of probing a dead target. Round-robin already migrated sticky on fallback success (Option B), so this only saves the one wasted probe on the first request after exhaustion. Added for consistency with the weighted sticky behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
…, stale cleanup Fixes four issues from gemini-code-assist review on diegosouzapw#4489: 1. Guard falsy target.provider before getCircuitBreaker call in both the weighted availability pre-filter and the RR sticky pre-filter to prevent a TypeError on unresolved steps. 2. Fix prefix-matching bug in getWeightedStepKeyForTarget: use exact match or startsWith with ">" separator so step-1 no longer matches step-10's expanded targets. 3. Eliminate redundant resolveWeightedStepGroups call: hoist stepGroups outside the if block and pass it to resolveWeightedTargets via a new optional parameter, avoiding a second recursive DAG expansion. 4. Clear stale weightedStickyTargets entries when the strategy is not weighted or stickyWeightedLimit is disabled (<=1), preventing unbounded memory growth from orphaned sticky records. Co-authored-by: Cursor <cursoragent@cursor.com>
5eed395 to
85ab546
Compare
|
Hi Diego — pushing the latest force-push with the 4 review fixes from the Gemini bot. Two notes for when you rebase onto release/v3.8.33:
|
|
Hey Diego, force-pushed to drop my failed test-fix attempts. Two tests ( and ) mock The fix needs to mock
All 4 review fixes are still in. Just the 2 weighted test cases need reworking once you rebase. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
7 similar comments
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
|
Hi Diego, The 3 commits in this PR depend on your merged #4472 ( One thing worth noting: in The fix is to mock Everything else (production code, schema, UI, the other 3 tests including the RR pre-filter test) is clean and verified locally. |
The two weighted-sticky tests (`batches successful weighted selections` and `follows fallback success`) were mocking Math.random, but selectWeightedTarget uses secureRandomFloat (via the secure-random helper that replaced Math.random for routing selection). The mocks were dead code, so the tests asserted traces driven by the real crypto float and only passed by coincidence before upstream changed how weighted picks are sampled. Switch both tests to use the test-only _setSecureRandomFloatSource hook, which the secureRandom module exports specifically for deterministic selection assertions. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Obrigado pela contribuição, @adivekar-utexas! 🙏 Comecei a integração na No primeiro, com O round-robin sticky (commits relacionados) passa; o problema é específico do weighted sticky. Você poderia ajustar a lógica de seleção weighted+sticky para que os seus próprios testes fiquem verdes? Assim que estiver verde eu reconstruo sobre a release e faço o merge preservando seu crédito. 🙌 (Obs.: o commit 1 |
…mit field PR diegosouzapw#4489 adds the Sticky Weighted Limit input (1 line) in the weighted strategy advanced config section, analogous to the existing stickyRoundRobinLimit field for round-robin. Co-authored-by: Cursor <cursoragent@cursor.com>
CI measures the merged tree (PR base release/v3.8.33), where combo.ts is 2761 lines: the release base (2649) plus this PR's sticky-weighted routing (state wiring, availability pre-filter, exhaustion-aware sticky clearing/migration, RR sticky pre-filter, and the 4 gemini-review fixes). The v3.8.32-based branch tip measures smaller locally, so this bump covers the merged measurement. Structural shrink of combo.ts tracked in diegosouzapw#3501. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi Diego — quick status update after the latest pushes. I re-ran the PR-owned combo suite locally on DISABLE_SQLITE_AUTO_BACKUP=true ./node_modules/.bin/cross-env node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit tests/unit/combo-config.test.ts tests/unit/combo-strategy-fallbacks.test.tsResult:
The earlier test failures were fixed by switching the deterministic tests from ineffective I also verified the local file-size ratchet no longer reports the PR-owned files ( One current GitHub-side note: the PR now reports |
Resolves conflicts with diegosouzapw#4545 (provider-wildcard) and diegosouzapw#4558 (isHidden) which both rewrote the same combo.ts target-resolution region: - wildcard expansion (expandProviderWildcardsInCombo/Collection) now runs first, and the sticky-weighted eligibility pass (resolveWeightedStepGroups / resolveWeightedTargets / resolveComboTargets) consumes expandedCombo/expandedAllCombos so wildcard-originated steps are treated identically. - config.stickyWeightedLimit read via the existing (config as Record<string, unknown>) cast pattern (mirrors stickyRoundRobinLimit at combo.ts), since the resolved combo config type tightened on the release base after this branch forked. - file-size baselines reconciled: combo.ts 2701->2818, combos/page.tsx 4386->4406 (Hard Rule diegosouzapw#9; fast-gate PR->release does not run check:file-size). 52/52 sticky-weighted tests pass on the merged tip; typecheck:core clean. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
|
Thanks @adivekar-utexas! The exhaustion-aware renormalization is a nice touch — the sticky-weighted batch only sticks to a step group that still has at least one selectable target (circuit-breaker / cooldown / model-lockout / availability filtered), and falls back cleanly otherwise. I merged the current release into your branch and resolved the conflicts with two PRs that landed in the same combo.ts region this cycle: #4545 (provider-wildcard expansion now runs first, and your eligibility pass consumes the expanded combo) and the tightened combo-config type (read |
…w#4537) Rebases the nestedComboMode feature onto release now that diegosouzapw#4489 (sticky-weighted) is merged. Conflict resolution: - combo.ts / comboStructure.ts / combo-strategy-fallbacks.test.ts: kept release's reconciled sticky-weighted + provider-wildcard (diegosouzapw#4545) versions (the branch carried the pre-diegosouzapw#4489 sticky-weighted code + the old Math.random test mocks); the nestedComboMode dispatch (resolveComboRuntimeUnits / normalizeNestedComboMode / execute-mode branch with depth/cycle/budget caps) is preserved and layered on top. - config.stickyWeightedLimit read via the (config as Record<string, unknown>) cast at the two nested-execute sites (mirrors stickyRoundRobinLimit), since the combo config type tightened on the release base. - page.tsx: kept the new Nested Combo Behavior selector. - baselines reconciled: combo.ts 2818->2955, combos/page.tsx 4406->4434, chat.ts 1513->1515, combo-strategy-fallbacks.test.ts frozen at 880, complexity 1913->1915 (Hard Rule diegosouzapw#9). 59/59 combo tests pass on the merged tip; typecheck:core clean. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
…e drift) Pre-existing drift on origin/release/v3.8.33 surfaced by the release full CI (cognitive-complexity doesn't run on PR->release fast-gates). My release-finalize prod changes add 0 new violations to the count (verified: geminiHelper merge is intra-function; auth.ts only gained args). +4 from this cycle's combo logic (#4537/#4489/#4581). Structural shrink tracked in #3501.
…e renormalization (diegosouzapw#4489) Integrated into release/v3.8.33
…e drift) Pre-existing drift on origin/release/v3.8.33 surfaced by the release full CI (cognitive-complexity doesn't run on PR->release fast-gates). My release-finalize prod changes add 0 new violations to the count (verified: geminiHelper merge is intra-function; auth.ts only gained args). +4 from this cycle's combo logic (diegosouzapw#4537/diegosouzapw#4489/diegosouzapw#4581). Structural shrink tracked in diegosouzapw#3501.
Summary
Adds a
stickyWeightedLimitconfig field (0-1000, clamps to 1) to the weighted combo strategy. When set, consecutive successful requests pin the selected top-level weighted step for up to N successes before the next weighted draw. This provides cache locality - traffic stays on one model/provider family long enough for upstream prompt caches to stay warm.Exhaustion-Aware Behavior
The sticky choice is exhaustion-aware: before each request the router checks whether the currently sticky top-level step still has at least one healthy expanded leaf (circuit breaker, provider cooldown, model lockout,
isModelAvailable). If none of its leaves are available, sticky is cleared and a fresh weighted draw renormalizes over the remaining available steps.If a fallback from a different top-level step succeeds, sticky migrates to that successful step - matching the round-robin sticky behavior.
A top-level step is considered available if at least one of its expanded nested leaves passes all pre-flight availability checks.
Files Changed
open-sse/services/combo/rrState.tsweightedStickyTargetsmap,getStickyWeightedExecutionKey,recordStickyWeightedSuccessopen-sse/services/combo/comboStructure.tsresolveWeightedStepGroups()- exposes top-level step to leaf expansion;resolveWeightedTargets()- optionalpreferredExecutionKeyandeligibleExecutionKeyssetopen-sse/services/combo.tssrc/shared/validation/schemas/combo.tsstickyWeightedLimit: z.coerce.number().int().min(0).max(1000)src/app/(dashboard)/dashboard/combos/page.tsxstickyWeightedLimit; weighted strategy now opens advanced settings on recommendationstests/unit/combo-config.test.tstests/unit/combo-strategy-fallbacks.test.tsRationale
Without sticky weighted, each request draws a fresh weighted random selection, so cache locality is limited to a single request. With sticky weighted and a reasonable limit (e.g. 50), each draw runs for 50 consecutive successes, giving upstream prompt caches time to warm.
The exhaustion-aware behavior prevents the router from repeatedly probing an unavailable sticky target. When a stuck target recovers, it re-enters the eligible set on the next weighted draw.