Repository navigation
fix(routing): the latency budget must not refuse a candidate on a guess - #68
Merged
Merged
Conversation
Found by the architecture audit. `latencyBudget.ts` promised that "candidates and targets whose latency is unknown are treated as within budget", but on the candidate path that state was unreachable: `resolveP95LatencyMs` always returns a number, falling back to `getBootstrapLatencyMs` — a hardcoded per-model table where anything unlisted is 1500 and `claude-opus-4.6` is 6000. So an unmeasured model was indistinguishable from a measured one. On a fresh install `X-OmniRoute-Latency-Budget: 1000` permanently excluded every model, on no evidence at all — and it is self-sealing, because the traffic that would replace the guess with a real p95 can only happen if the candidate is allowed through. `resolveLatencyProfile` now marks the bootstrap fallback with `latencyIsEstimated`. The selection filter and the failover filter both honour it, and so does `collectExclusionReasons` — otherwise a recorded decision would report `latency_over_budget` for a candidate selection actually kept, and the explanation would disagree with what happened. A candidate that does not declare the flag is still treated as measured, so every caller that was already measuring is unchanged: the flag only ever widens what is allowed through. tests/unit/latency-budget-unmeasured-latency.test.ts covers the case the existing suite structurally could not — it builds candidates with explicit `p95LatencyMs`, so it never reaches the bootstrap path. Proven failable: with the flag check turned into a no-op, 2 of 5 fail. new file 5 pass / 0 fail (3 pass / 2 fail without the fix) existing auto-routing-latency-budget.test.ts 7 pass / 0 fail typecheck:core exit 0, 0 errors; eslint exit 0; prettier clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the architecture audit of this tree, in the feature #53 just shipped.
The defect
latencyBudget.tssays, in its own docstring:On the candidate path that state is unreachable.
resolveP95LatencyMsalways returns a number — with fewer thanMIN_HISTORY_SAMPLES(10) and no live average it falls back togetBootstrapLatencyMs, a hardcoded table (claude-opus-4.6: 6000,claude-sonnet-4.6: 4000, anything unlisted:1500).So an unmeasured model is indistinguishable from a measured one, and the budget refuses it on a constant.
Why it matters
On a fresh install,
X-OmniRoute-Latency-Budget: 1000excludes every model that has not accumulated 10 samples — then answers503 No auto strategy candidate fits the request latency budget of Nms. A request that could have been served fails, on no evidence.And it is self-sealing: the traffic that would replace the guess with a real p95 can only happen if the candidate is allowed through.
The fix
resolveLatencyProfilemarks the bootstrap fallback withlatencyIsEstimated, mirroringresolveP95LatencyMs's own fallbacks. Three places honour it:candidatesWithinLatencyBudgetdropTargetsOverLatencyBudgetcollectExclusionReasonslatency_over_budgetThat third one is not cosmetic.
latencyBudget.tsstates that the filter and the recorded reason are the same test "so what the decision says was excluded is exactly what selection and failover refused to use" — fixing only the filter would have made the explanation lie.Scoring still uses the bootstrap value. A guess is fine for ranking; it is not evidence for a refusal.
Backwards compatible by construction: a candidate that does not declare the flag is treated as measured, so the flag only ever widens what is allowed through, never narrows it. A test pins that.
Tests
The existing 219-line
auto-routing-latency-budget.test.tsstructurally could not catch this — it constructs candidates with explicitp95LatencyMs, so it never reaches the bootstrap path at all.tests/unit/latency-budget-unmeasured-latency.test.tscovers it, and I checked it has teeth rather than assuming: with the flag check turned into a no-op, 2 of 5 fail.auto-routing-latency-budget.test.tstypecheck:coreeslint --max-warnings=0prettier --check🤖 Generated with Claude Code