Audit fixes: bounded decision store, strategy budget order, SLO recovery, read-only metrics - #38
Merged
Merged
Conversation
…t (A-H1) The decision store capped the number of decisions (2000) but kept every candidate with its full factor breakdown. Auto combos over the whole catalog consider hundreds of candidates per request, which retained ~1.2 GB of heap at the cap with 300 candidates. Decisions are now stored in a compact form: at most 40 candidates (the selected one always kept), full factors only for the selected candidate and the 10 best, and an additive `omittedCandidates` count on the contract. The store also enforces a 32 MB estimated byte budget, evicting the oldest decisions first. The dashboard lookup card shows the omitted count. Auditor A probe (2000 decisions): 300 candidates 1189.5 MB -> 28.3 MB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
orderTargetsByCostBudget ran after every auto selection, so an explicit
router strategy (cost, latency, lkgp, ...) whose pick was over budgetCap was
moved behind in-budget targets (cheapest) or dropped (strict). That changed
v3.8.53 behavior, and the recorded decision and the "Auto selection" log
named a target that was not the first one attempted.
The budget ordering now applies only when the scoring engine ("rules") made
the selection. Explicit strategies ignore budgetCap again, as in v3.8.53.
Regression test: tests/unit/auto-explicit-strategy-budget-order.test.ts
(fails without the fix; the rules-path control keeps the strict budget).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…idate (A-L5) recordExplicitStrategyDecision took eligibility and identity from the scoring pass it runs only to explain the decision. A pick without a connection id, or one that pass excluded (e.g. every candidate over a strict budget cap, which explicit strategies ignore), gave `selected: undefined` for a served request. The strategy pick now matches on provider/model when it carries no connection id, and a pick without a hard exclusion (model missing, capability missing, quota cutoff, request budget) is reported as the eligible selected candidate. A quota-blocked pick is still not selected. Tests: 3 new cases in auto-routing-decision-recording.test.ts (2 fail without the fix) and the explicit-strategy budget test now asserts the recorded selection is the first target attempted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orever (A-M3) recoveryEpisodes counted any OPEN/HALF_OPEN breaker as an ongoing episode of `now - openedAt`, whatever the window. A breaker leaves HALF_OPEN only when traffic probes it, so a provider that stopped receiving traffic kept provider_recovery (and the overall SLO status) breached indefinitely and hid real regressions behind a constant red status. An ongoing episode now counts only while the breaker has a failure, or an OPEN transition, inside the SLO window. An idle open breaker adds no sample (it is still visible in the circuit-breaker gauges). A breaker that keeps failing still reports its whole open duration and breaches. Test: slo-evaluator "an idle breaker left open or half-open does not breach provider recovery forever". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…A-L1, A-I1) Each /api/metrics scrape and each 60 s SLO alert tick called getAllCircuitBreakerStatuses(), whose getStatus() transitions an elapsed OPEN breaker to HALF_OPEN, persists it and resets its half-open probe. Looking at routing state changed it. CircuitBreaker gains peekStatus() (built on the existing, previously unused peekState()), and getAllCircuitBreakerSnapshots() returns registered breakers plus persisted ones not loaded in this process without transitioning, persisting or registering anything. The metrics snapshot and the SLO alert loop use it; an elapsed OPEN breaker is reported as HALF_OPEN. Test: tests/unit/metrics-scrape-breaker-readonly.test.ts (fails with the old scrape path), added to stryker tap.testFiles since it covers circuitBreaker.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(A-L4, B-09) The SLO alert tracker kept its breached objectives and open providers while `slo.alertsEnabled` was false, so re-enabling alerts replayed a stale `slo.recovered` (or missed a breach). The 60 s timer also started a new async tick even when the previous one was still awaiting slow webhooks, so two ticks could interleave. The tick now lives in SloAlertRunner: a disabled tick resets the tracker, and a tick that finds the previous one still running is skipped. The runner takes its settings, breaker, window, dispatch and clock sources as dependencies, so both behaviors are unit-tested. Tests: slo-evaluator "SloAlertRunner forgets alert state while alerts are off" and "SloAlertRunner skips a tick while the previous one is still dispatching". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s (B-04) The model label dimension admitted the first 100 distinct sanitized values. A client could send ~100 requests with invented model ids that reach an upstream and fail, filling every slot so real models were reported as `other` until restart. The fixed values `redacted`/`unknown` also took slots. BoundedLabelSet.resolve() now lets the fixed values pass through without a slot and takes an `admit` flag; the metrics sink admits a new model label only for a successful response. Totals are unchanged, and an already tracked model still gets its failures counted under its own label. The monitoring guide's label policy says so. Tests: routing-metrics-sink "junk model ids from failed requests do not crowd real models out of the label cap" and "BoundedLabelSet: fixed values take no slot and admit=false never adds" (both fail without the fix). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The contract said the decision "and every failover attempt must respect" the request budget, but planNextAttempt / checkFailoverBudget / classifyAttemptOutcome have no production caller and live traffic has no per-request budget input. ROUTING_CONTRACT.md (types, guarantees, limits), the RoutingBudget JSDoc and the attemptPolicy.ts module comment now say what is live: the same-target retry status set (408/429/500/502/503/504) and, on the auto combo's rules path only, the per-attempt budgetCap ordering of the failover chain. The budget library and RoutingBudget are previews/library only. The limits also describe the compact decision form and the store size budget. No new enforcement was invented without a live input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Monitoring guide: provider_recovery ignores idle open breakers, turning alerts off resets their state, ticks never overlap, and metrics/SLO scrapes read circuit breakers without transitioning them. Follows A-M3, A-L1, A-L4, B-09. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The lookup card rendered "selected", "eligible"/"excluded" and "live"/"preview" as hard-coded English, showed the routing contract enums (quota, circuit, selection mode, exclusion reasons) verbatim, and printed the raw UTC ISO timestamp. Those labels now come from analytics.routeDecision* keys (en, pt-BR, vi; a missing translation falls back to the raw value), and the timestamp is formatted with Intl.DateTimeFormat for the active locale inside a <time> element that keeps the ISO value. Strategy names and ids stay verbatim. Test: routing-decision-lookup "renders badges, enums and the timestamp in the active locale" (pt-BR messages, no English literals). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ults (C-L2)
A 401/403 from the decision endpoint showed the same "Try again" message as a
500, although retrying never works once the session expired. A successful
lookup left the aria-live region empty and focus on the input, so screen
reader users got no announcement.
401 and 403 now show a sign-in-again message; other failures keep the retry
message. A found decision is announced in the live region ("Decision found:
provider/model was chosen.", or that no candidate was selected), and focus
moves to the labelled results region. New strings in en, pt-BR and vi.
Tests: routing-decision-lookup success announcement + focus, 401, 403 and the
500 control case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
POST /api/omniroute/route/preview returned zod's `error.message`, a
pretty-printed JSON issue list inside the `error` string, and an invalid JSON
body was reported as "expected object, received null".
The 400 keeps its status and its string `error` field, but the message now
lists up to five problems as `field: message` ("Invalid route preview
request: candidates: ..."), and a body that is not a JSON object gets
"Invalid route preview request: the body must be a JSON object". The
response shape of successful previews is unchanged. API governance gate:
PASS.
Tests: omniroute-route-preview "a 400 names the invalid fields in a readable
string, not a JSON dump" and "a body that is not a JSON object gets a clear
400" (both fail without the fix).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s (C-I2) ROUTING_CONTRACT.md used `$OMNIROUTE_TOKEN` without defining it, while API_USE_CASES.md uses `$OMNIROUTE_MANAGE_KEY` for the same credential. The examples now use `$OMNIROUTE_MANAGE_KEY`, defined as an API key with the `manage` scope. The preview section also says that the top-level `selected` is only the provider id (`decision.selected` has provider and model) and that an invalid body gets a readable 400 (C-L3). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oo (A-I5) withRoutingRequestContext's comment and ROUTING_CONTRACT.md said streaming responses never carry X-OmniRoute-Decision-Id / X-OmniRoute-Policy-Version. Routing picks the target before the stream starts, so the headers are attached whenever a decision was recorded and the Response headers are mutable. Both texts now say when the headers are present and when to use the request id lookup instead. Comment/doc only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ranch Follow-up to the A-M1 fix: instead of a boolean flag and a ternary around orderTargetsByCostBudget, the rules path sets the failover budget cap and the ordering always runs (a null cap leaves the chain unchanged). Same behavior; resolveAutoStrategyOrder is back to its release-tip cyclomatic (65) and cognitive (46) complexity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to C-L2: the lookup fetch, status and focus handling live in useDecisionLookup(), so the RoutingDecisionLookup component stays within the 80-line function limit. No behavior change; routing-decision-lookup vitest suite 9/9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
LMPrado-DZ23
pushed a commit
that referenced
this pull request
Sep 18, 2026
Resolves the docs/ops/MONITORING_GUIDE.md conflict with PR #38 by keeping both: the dashboard steps to enable SLO alerts and subscribe a webhook, plus #38's notes on alert state reset and read-only breaker scrapes; #38's provider_recovery idle-breaker paragraph plus this branch's accurate telemetry errorRate definition (it is not the SLO error_rate). check:docs-all, API governance, i18n coverage and the webhook/SLO/i18n suites (26/26) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Audit fixes: routing decisions, SLO and metrics
Fixes from the three independent final audits of v3.8.54. One commit per finding; new tests fail on the old code where behavior changed.
omittedCandidatesin the contract, 32 MB byte budget evicting oldest. Auditor probe at 2000 decisions × 300 candidates: 1189.5 MB → 28.3 MBROUTING_CONTRACT.md,RoutingBudgetandattemptPolicy.tsdocs state exactly what live traffic enforces (same-target retry status set; per-attemptbudgetCapon the rules path) vs preview/library-only helpersprovider_recovery: an idle open/half-open breaker reportsinsufficient_datainstead of a permanent breach/api/metricsand the SLO timer read breakers through side-effect-free snapshots (peekStatus,getAllCircuitBreakerSnapshots)SloAlertRunnerresets alert state when alerts are off and skips overlapping ticksfield: messagestring (status anderrortype unchanged)$OMNIROUTE_MANAGE_KEY; decision-header docs accurate for streamingVerification (local, isolated DATA_DIR/HOME)
37 node test files 221/221; vitest lookup card 9/9;
typecheck:coreand open-sse tsc 0; API and dashboard tsc 0 regressions; ESLint clean; complexity no new violations;check:docs-allexit 0; API governance PASS; mutation drift none (checked viafindCoverageDrift);combo.tsuntouched.🤖 Generated with Claude Code