fix(routing): 5 correctness bugs surfaced by adversarial code audit - #18
Merged
Merged
Conversation
…s fires Two audit findings, one function: 1. (mirainya#6) chooseExploration compared Price.Multiplier * 2 as the cost ceiling, but Multiplier is a billing scalar that is ~1 across all channels in production, so the guard filtered nothing. A 100x-more-expensive fallback could receive exploration traffic silently. Fix: use the evaluation's EffectiveCost (SelectedTotal / success_rate) so the '2x winner' filter means what the comment says. 2. (mirainya#7) threshold := uint64(rate * float64(^uint64(0))) — float64 rounds ^uint64(0) up to 2^64, and casting 2^64 back to uint64 is implementation- defined (x86 lands on 2^63), degrading rate=1.0 to ~50% fire rate. Fix: short-circuit rate>=1 to always fire. Also added a permissive fallback when winnerCost==0 (zero-token forecast, early warm-up) so the 2x ceiling doesn't collapse to zero and forbid all exploration. Tests: - TestChooseExplorationRejectsExpensiveCandidates: 100x-priced fallback must never win at rate=1.0. - TestChooseExplorationRateOneAlwaysFires: 20 distinct cache_keys must ALL explore at rate=1.0.
getSessionCacheStats stamped stats.ExpiresAt = latestCache + 5min unless the session had run >10min AND had CreateCount>=2. That missed two branches from routing.selectAdaptiveTTL: Gemini (always 1h) and any protocol with sparse avgInterval>4min+CreateCount>=1. In those cases the state machine at cachestate.go:131 flipped CacheHot → CacheExpired up to 55 minutes early: profile.Existing.Valid went false, cacheLifetimes lost the guaranteed initial hit credit, and the candidate over-forecasted a full extra cache write. Fix: extract the TTL policy into store.AssumedCacheTTL and call it from getSessionCacheStats. Same rules, single source of truth, kept in the store package to avoid a store → routing import cycle (routing.selectAdaptiveTTL retains its own copy — see doc-comment cross-reference). Signature change: GetPrefixCacheStats / getSessionCacheStats now take a protocol string. Callers updated (scheduler + tests). Fixes audit finding #1.
…ix cache stats Two related audit findings in the same fallback path: mirainya#4: getSessionCacheStats' in-window query counted all observations while its lifetime widen fallback filtered success=TRUE. A single failed retry inside the 15-min window suppressed the widen path — the same underlying history produced different verdicts depending on where the failure landed relative to the window boundary. Fix: apply success=TRUE to both queries (the widen path is now a strict superset of the window path), matching the comment's intent ('healthy history should survive circuit-breaker recoveries'). mirainya#5: GetPrefixCacheStats's fallback used ':=' inside the error block, shadowing 'stats' and 'fallbackErr'. On fallback failure it returned the primary err (typically ErrNoRows) and the shadowed partial stats, swallowing any real DB error from the fallback. Fix: propagate fallbackErr and return the outer initialised stats on the error path so callers/logs see the true failure and consistent field content.
…ectly attributed route_decisions previously stored only SelectedUpstreamID — the initial pick. When that pick failed and a failover attempt succeeded, the row's selected_upstream_id kept pointing at the failed upstream while actual_cost / actual_input_tokens / actual_cached_tokens described the successful failover attempt's usage. Any join on selected_upstream_id misattributed the request to the upstream that DID NOT serve it. Fix: add actual_upstream_id column (populated from result.FinalUpstreamID) alongside the existing actual_* usage columns. selected_upstream_id retains its meaning (initial pick) so historical decision reasoning stays intact. Downstream analytics that care about 'which upstream served this' should COALESCE(NULLIF(actual_upstream_id, 0), selected_upstream_id). Migration 20260911_030000 adds the column with default 0 (unknown). Fixes audit finding mirainya#8.
…ched semantics Anthropic responses report usage.input_tokens as EXCLUSIVE of cache_read / cache_creation. OpenAI (prompt_tokens) and Gemini (promptTokenCount) report their input as INCLUSIVE of cached. Previously parseUsageObject stored whichever was largest — mixing conventions in the same column. For any downstream consumer that sums the three fields to reconstruct the billed prompt total (e.g. usage attribution, cache-coverage analytics), this double-counts cached for OpenAI/Gemini rows by roughly cached/total. Fix at the ingest layer: parseUsageObject subtracts cached from the inclusive prompt_tokens/promptTokenCount, matching Anthropic semantics. Every column downstream (routing_observations.input_tokens, request_attempts.input_tokens) now has a single well-defined meaning. Tests: - TestUsageAuditNormalizesOpenAIPromptTokensToUncached - TestUsageAuditNormalizesGeminiPromptTokensToUncached - TestUsageAuditClampsNegativeUncached - Updated TestRelayResponseCapturesUsageBytesAndRequestID for new contract.
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.
Summary
Follow-up to #17. An adversarial 12-lens code audit surfaced 5 additional correctness bugs in the routing/store layers. This PR bundles the upstream-relevant fixes.
Bugs fixed (each is a separate commit for reviewability)
fix(forward): normalize OpenAI/Gemini prompt_tokens to uncached semantics (
d3de0ce)input_tokensexcludes cached; OpenAIprompt_tokensand GeminipromptTokenCountinclude cached. parseUsageObject stored whichever was max, mixing conventions in the same column and causing any downstream SQL that sums the three token fields to double-count cached for OpenAI/Gemini.fix(routing): exploration ceiling uses EffectiveCost + rate=1.0 always fires (
79760b9)chooseExplorationcomparedPrice.Multiplier * 2as the cost ceiling, but Multiplier is a billing scalar that's ~1 across all channels in production. A 100x-more-expensive fallback could silently receive exploration traffic. Fix: useevaluations[winner].EffectiveCost.uint64(rate * float64(^uint64(0)))on x86 rounds 2^64-1 up to 2^64 and casts back to 2^63, silently degrading rate=1.0 to ~50% fire rate. Fix: short-circuit rate>=1 to^uint64(0).fix(store): align session-cache TTL fallback with selectAdaptiveTTL (
1ca15d8)getSessionCacheStatsstampedExpiresAt = latestCache + 5minunless the session had run >10min AND CreateCount>=2. That missed two branches fromrouting.selectAdaptiveTTL: Gemini (always 1h) and sparse conversations (avgInterval>4min + any rebuild). In those cases the state machine flipped CacheHot → CacheExpired up to 55 minutes early.store.AssumedCacheTTL(single source of truth, avoids store→routing import cycle).fix(store): symmetric success filter + surface fallback error (
8582603)fix(routing): record actual_upstream_id for failover attempts (
c06cede)selected_upstream_idkept pointing at the failed upstream whileactual_cost/actual_*_tokensdescribed the successful attempt. Any join onselected_upstream_idmisattributed the failover request.actual_upstream_idcolumn (default 0 for unknown), populated fromresult.FinalUpstreamIDinRouteDecisionOutcome. Migration20260911_030000_add_route_decisions_actual_upstream.sql.Testing
go vet ./...clean,go test ./...all pass. Each commit adds its own regression tests:TestUsageAuditNormalizesOpenAIPromptTokensToUncachedTestUsageAuditNormalizesGeminiPromptTokensToUncachedTestUsageAuditClampsNegativeUncachedTestChooseExplorationRejectsExpensiveCandidatesTestChooseExplorationRateOneAlwaysFiresTestAssumedCacheTTLContext
Two audit findings (#4 in-window/lifetime filter asymmetry, #5 shadowed error) share commit
8582603because they touch the same function.Two audit findings (#6 exploration ceiling, #7 rate=1.0 saturation) share commit
79760b9because they're in the same function and fix together.This builds on #17. #17 must land first (or be co-merged) since the TokenInflationFactor issue it fixes shares the same root symptom.