Repository navigation
fix(routing): count the first abandoned combo target as a fallback - #16250
diegosouzapw merged 3 commits into
Conversation
`fallbackCount` is the number of ordered targets abandoned before the one that serves a combo request. It drives `totalFallbacks` / `fallbackRate` in /api/combos/metrics and must equal the index of the target that wins — the value stamped on that target as `fallbackAttempts` and returned to clients as `X-OmniRoute-Fallback-Attempts`. Every increment site was gated on `i > 0` / `offset > 0`, which conflated "not the first target" with "is a fallback": abandoning the primary for the secondary — i.e. the first fallback — was never counted. A 2-target combo that fell back once recorded `fallbackCount: 0` (0% fallback rate) while the response advertised `X-OmniRoute-Fallback-Attempts: 1`. Drop the guards: one abandoned ordered target is one fallback, whatever its index. Sites: executeTargetAttempt.ts (8 abandon paths), the `bumpFallback` helper in executeTargetGates.ts and roundRobinCombo.ts (9 skip paths). Retrying the *same* target is not an abandon path and still counts nothing. Tests (failing against the unfixed sources, passing after): - execute-target-attempt.test.ts: a retryable failure of target 0 counts one fallback and reaches `totalFallbacks` in the metrics record; a same-target retry that succeeds keeps the counter at 0. - execute-target-gates.test.ts: a gate skip of target 0 counts one fallback, a skip of a later target still counts one. - round-robin-combo.test.ts: source guard so neither condition returns.
Review follow-up on cheaper-inference#16250. Three branches of executeTargetAttempt abort the combo instead of advancing to the next target — local queue capacity, input-bound failure, and a body-specific 400 that must surface as-is. They still bumped state.fallbackCount, so a request that simply failed was recorded as having failed over, and a first-target stop was indistinguishable from a real fallback in totalFallbacks. The counter tracks the number of ordered targets abandoned before the one that serves the request, so a branch that tries no further target counts nothing. roundRobinCombo.ts already behaves that way on its own queue-capacity branch, whose log line reads "returning without upstream fallback" — the priority path was the outlier. Adds behavioural non-regression coverage for the three stops (each one turns red when the increment is re-added), and replaces the source-regex guard on the round-robin fallback with a test that drives handleComboChat and asserts the observable totalFallbacks metric.
Review follow-up — every stop that ends the combo no longer counts as a fallbackThanks for the review. Point 1 was right, and it was one branch wider than reported. What changed (commit
The third one was not in the review but behaves the same way; it is corrected too. Point 2 — the round-robin test The source-regex guard is replaced by a test that drives the real Non-regression proof
Gates on this head, run locally: Point 3 — the workflows in Expected for a PR from a fork: the GitHub Actions jobs stay queued until a maintainer approves them. Nothing on our side can start them, and it is not a test failure. Semgrep ran and passed independently. Diff is now 8 files: 4 engine + 3 tests + 1 changelog fragment. No routing condition is touched — the counter only. |
|
Hi @M3stryX — closing this pull request to clear the open queue. The branch |
|
Hi @M3stryX — reopening this pull request. The close was a mistake, and it is back in the merge queue. The branch |
639fb13
into
cheaper-inference:release/v3.8.52
Summary
fallbackCount++in the attempt loop was gated byif (i > 0)/if (offset > 0). A failover that abandons target [WIP] Add FastAPI OpenAI-compatible LLM gateway with routing #1 and succeeds on target build(deps): bump actions/github-script from 7 to 8 #2 therefore left the counter at0.X-OmniRoute-Fallback-Attemptsis documented indocs/reference/API_REFERENCE.mdas part of the public cost-telemetry header set and is emitted only when > 0, so the most common failover shape (primary fails, the next target answers) reported no header at all and was indistinguishable from "no fallback happened".recordComboRequest({ fallbackCount })and the aggregatedtotalFallbacksmetric, so combo telemetry and the Control Center under-report failovers by one.Related Issues
#12339 (closed) reported
X-OmniRoute-Fallback-Attemptsas always0on chat completions and was fixed by threadingfallbackAttemptsthrough the streaming header path. That was a different root cause (the value was not forwarded at all). The expected behaviour stated in that issue — "X-OmniRoute-Fallback-Attemptsshould be >= 1 on the streaming response whenever a failover occurred" — is exactly what the guard prevents for a first-target failover, so this PR closes the remaining half.Validation
npm run lintCommands run, on this exact head commit (base
release/v3.8.52@048d9e2f2a):Behavioural evidence (same test files, sources flipped):
release/v3.8.52sourcesThe four tests that go red without the change are precisely the ones asserting a first-target abandonment is counted (
a retryable failure of the first ordered target counts one fallback,a first-target fallback reaches totalFallbacks, not just the counter,a gate skip of the first ordered target counts one fallback,counts an abandoned first model as a fallback).Tests Added Or Updated
tests/unit/combo/execute-target-attempt.test.ts— first-target retryable failure, same-target retry that succeeds (must not count), and the metric aggregation path.tests/unit/combo/execute-target-gates.test.ts— first-target and later-target gate skips.tests/unit/combo/round-robin-combo.test.ts— the round-robin extraction guard test.Coverage Notes
open-sse/services/combo/{comboAttemptLoop,executeTargetAttempt,executeTargetGates,roundRobinCombo}.ts. All four are exercised by the three test files above, which previously had no assertion on this counter (the onlyfallbackCountmentions in the existing suite wereemptyStateinitialisers and the aggregation assertions intests/unit/services/combo-metrics-memory.test.tsandtests/unit/services-branch-hardening.test.ts, both of which keep passing unchanged).Reviewer Notes
recordComboRequest()and the combo metrics memory; nothing gates dispatch on it.totalFallbacksnaming and docs are what should change — worth a maintainer call, and I am happy to rework the patch either way.roundRobinCombo.tsrefusal branch,if (offset > 0)) and is covered by the same reasoning; it was added with the current cycle.