fix(sse): configurable round-robin combo queue depth for faster failover (#3872) - #4390
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 a configurable pre-cascade semaphore queue depth (queueDepth) for round-robin combos to allow faster failover under concurrency saturation. The changes span backend configuration resolution, validation schemas, rate-limiting semaphores, frontend settings components, and unit tests. The review feedback identifies a coercion bug in resolveComboQueueDepth where empty or null values incorrectly resolve to 0 instead of the default fallback, a UI issue where clearing the queue depth input field prevents restoring the default value, and a recommendation to add unit tests covering these edge cases.
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.
| export function resolveComboQueueDepth(config: Record<string, unknown> | null | undefined): number { | ||
| const raw = isRecord(config) ? Number(config.queueDepth) : Number.NaN; | ||
| if (!Number.isFinite(raw) || raw < 0) return DEFAULT_COMBO_QUEUE_DEPTH; | ||
| return Math.min(Math.floor(raw), MAX_COMBO_QUEUE_DEPTH); | ||
| } |
There was a problem hiding this comment.
In JavaScript, Number(null) and Number("") both evaluate to 0. Because 0 is a valid and meaningful value for queueDepth (meaning "never queue, fail over immediately"), passing null or "" will bypass the fallback check raw < 0 and incorrectly resolve the queue depth to 0 instead of falling back to DEFAULT_COMBO_QUEUE_DEPTH (20). Explicitly checking for null and "" ensures robust fallback behavior.
References
- Ensure robust input handling and fallback logic for configuration parameters. (link)
| onChange={(e) => | ||
| setComboDefaults((prev) => ({ | ||
| ...prev, | ||
| queueDepth: parseInt(e.target.value) || 0, | ||
| })) | ||
| } |
There was a problem hiding this comment.
Using parseInt(e.target.value) || 0 means that if the user clears the input field (resulting in an empty string ""), parseInt("") evaluates to NaN, which falls back to 0. Since 0 is a valid and active setting ("never queue"), the user can never clear the field to restore the default behavior (which would show the placeholder 20 and fall back to DEFAULT_COMBO_QUEUE_DEPTH on the backend). Setting it to undefined when empty allows the placeholder to be displayed and the backend to correctly fall back.
References
- Allow clearing optional numeric fields to restore default values. (link)
| test("resolveComboQueueDepth defaults to 20, honors configured values, and clamps the range", () => { | ||
| assert.equal(resolveComboQueueDepth(null), 20); | ||
| assert.equal(resolveComboQueueDepth({}), 20); | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: 5 }), 5); | ||
| // 0 is a valid, meaningful value: queue nothing → fail over to the next member immediately. | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: 0 }), 0); | ||
| // Invalid / negative inputs fall back to the safe default. | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: -3 }), 20); | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: Number.NaN }), 20); | ||
| // Out-of-range high values are clamped, not trusted. | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: 99999 }), 100); | ||
| // Fractional values floor to an integer queue slot count. | ||
| assert.equal(resolveComboQueueDepth({ queueDepth: 3.9 }), 3); | ||
| }); |
There was a problem hiding this comment.
Add assertions to verify that null and "" (empty string) values for queueDepth correctly fall back to the default value of 20 instead of being coerced to 0.
References
- Always include comprehensive unit tests covering edge cases and boundary conditions. (link)
e2f3a09 to
cb5bfe5
Compare
851926b to
38d47d4
Compare
…ver (#3872) Round-robin combo members deep-queued under concurrency saturation: the per-model rate-limit semaphore had an unbounded queue and only emitted SEMAPHORE_TIMEOUT after the full queueTimeoutMs (default 30s), so failover to the next combo member happened far too late (or the client died first). The per-model semaphore now accepts a bounded queue depth and rejects with SEMAPHORE_QUEUE_FULL once the queue is full — the round-robin loop already cascades to the next member on that code, so a low depth fails over immediately. A new `queueDepth` combo-config knob (global default / provider override / per-combo; default 20 for backward compatibility, 0 = never queue) is plumbed via a resolveComboQueueDepth helper and surfaced in Settings → Combo Defaults. TDD: rateLimitSemaphore.test.ts (bounded queue + SEMAPHORE_QUEUE_FULL, RED before the maxQueueSize cap) and combo-config.test.ts (queueDepth cascade, helper clamps, schema range). Co-authored-by: KooshaPari <KooshaPari@users.noreply.github.com>
38d47d4 to
5e2ed34
Compare
…ver (diegosouzapw#3872) (diegosouzapw#4390) Round-robin combo members deep-queued under concurrency saturation: the per-model rate-limit semaphore had an unbounded queue and only emitted SEMAPHORE_TIMEOUT after the full queueTimeoutMs (default 30s), so failover to the next combo member happened far too late (or the client died first). The per-model semaphore now accepts a bounded queue depth and rejects with SEMAPHORE_QUEUE_FULL once the queue is full — the round-robin loop already cascades to the next member on that code, so a low depth fails over immediately. A new `queueDepth` combo-config knob (global default / provider override / per-combo; default 20 for backward compatibility, 0 = never queue) is plumbed via a resolveComboQueueDepth helper and surfaced in Settings → Combo Defaults. TDD: rateLimitSemaphore.test.ts (bounded queue + SEMAPHORE_QUEUE_FULL, RED before the maxQueueSize cap) and combo-config.test.ts (queueDepth cascade, helper clamps, schema range). Co-authored-by: KooshaPari <KooshaPari@users.noreply.github.com>
1 participant
Closes #3872
Problem
When a round-robin combo member is saturated, requests sit in the per-model rate-limit semaphore's unbounded queue and only fail over to the next member after the full
queueTimeoutMs(default 30s) elapses. A burst of agentic requests therefore deep-queues one hot member (e.g.minimaxat concurrency 3) instead of spilling to healthy members — the client often dies mid-task before failover.The combo already cascades on
SEMAPHORE_QUEUE_FULL/SEMAPHORE_TIMEOUT(combo.tsround-robin loop), but the combo's own semaphore (rateLimitSemaphore.ts) never emittedSEMAPHORE_QUEUE_FULL— its queue was unbounded, so that fast-cascade path was unreachable.Fix
rateLimitSemaphore.acquireaccepts an optionalmaxQueueSize; once the queue is that deep it rejects immediately withSEMAPHORE_QUEUE_FULLinstead of waiting. Omitted/negative keeps the historical unbounded behavior (backward-compatible).queueDepthcombo-config knob, resolved through the full 3-layer cascade (global default → provider override → per-combo) via a pureresolveComboQueueDepthhelper (mirrors the existingresolveComboTargetTimeoutMspattern). Default 20 (backward-compatible; matches the number reporters saw); 0 = never queue → fail over to the next member immediately.semaphore.acquire(...)call, surfaced in Settings → Combo Defaults, and validated by the combo Zod schema (min(0).max(100), accepted on create + global defaults + provider overrides).Why this is the right lever (investigation note)
The user's exact
Semaphore queue full (20) for minimax:<connId>429 comes from the per-accountaccountSemaphoredeeper inchatCore, but the combo already cascades on a 429 result. The actionable lever for faster combo failover is bounding the combo's own per-model queue so it cascades before the 30s timeout — which also relieves pressure on the per-account semaphore (fewer requests pile in concurrently).Validation (Hard Rule #18 — TDD)
tests/unit/rateLimitSemaphore.test.ts(new): unbounded default unchanged; bounded queue emitsSEMAPHORE_QUEUE_FULL;maxQueueSize: 0fails over immediately. RED before themaxQueueSizecap, GREEN after.tests/unit/combo-config.test.ts:queueDepthdefault 20, 3-layer cascade,resolveComboQueueDepthclamps (default/0/negative/NaN/>100/fractional), schema accepts0+ rejects out-of-range. RED before the config field.typecheck:coreclean, lint clean,check:test-discovery/check:file-size/check:any-budget:t11/ i18n-ui-coverage (--threshold=65) green.Default is unchanged behavior for existing installs (20-deep then cascade vs. previously unbounded-then-30s-timeout — strictly better under burst, no-op under light load).