From 5d624791fabba30bcdcdd68e9951dae7524890db Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:32:12 +0100 Subject: [PATCH 1/3] feat(failover): add cross-provider failover for overlapping models Add failover.go with the full candidate pipeline. resolveWithCandidates resolves a request exactly like resolve does, then adds every other provider sharing the identical bare model id. orderedFailoverCandidates applies a hard skip for request-path-unhealthy providers, deprioritizes discovery-unhealthy providers without dropping them, fails open when every candidate reads unhealthy, and caps the list at maxAttempts. Add requestHealthTracker, a separate per-pod request-path health signal fed by the same isTransient/isDeadlineExceeded classification recordProviderAttempt already uses. It never widens the existing discovery circuit breaker, per that breaker's own documented scope. Rewrite runMeteredCall (routes_unified.go) to loop over candidates instead of resolving once. Each attempt rebinds the attempt recorder to its own provider, recomputes the cache key before the response is cached, and re-injects the alias echo key an adapter may have deleted on a prior failed attempt. Failover never runs once any byte has reached the client, since statusTrackingWriter already tracks that state. A failure falls through on everything except HTTP 400, matching the brief exactly. A 404 failover logs loudly, naming both providers and the model, since it usually signals a real configuration mistake. Add FailoverConfig (Config.Failover): an Enabled pointer defaulting to true, and a MaxAttempts bound between 1 and 10, defaulting to 3. A single-provider deployment, or one with no overlapping bare model ids, behaves exactly as before regardless of this setting. Add a yaegi-check probe (tools/yaegi-check/main.go) driving two providers under the real interpreter, one failing and one serving, proving the candidate loop runs correctly interpreted. Update README.md with a Failover section and correct the Retry section's stale claim that no cross-provider failover exists. Media routes (images, audio speech, audio transcriptions) do not get failover in this change. Each has its own request-shape handling that needs its own scoped follow-up. --- README.md | 59 ++- failover.go | 487 ++++++++++++++++++ failover_test.go | 1009 +++++++++++++++++++++++++++++++++++++ llmgateway.go | 48 +- routes_unified.go | 306 +++++++---- tools/yaegi-check/main.go | 76 ++- 6 files changed, 1866 insertions(+), 119 deletions(-) create mode 100644 failover.go create mode 100644 failover_test.go diff --git a/README.md b/README.md index a2f0871..57b07dc 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ config accepts them as YAML, which decodes to the same JSON shape. | `users` | `UsersConfig` | — | Inline and/or file-backed API-key holders. With none configured, every request is unauthenticated and gets 401. | | `redis` | `RedisConfig` | — | Distributed limit-counter backend. Omitted means in-process counters only (per-replica, approximate across multiple Traefik instances). | | `retry` | `RetryConfig` | `{}` (disabled) | Same-provider retry for transient upstream failures — see [Retry](#retry). Omitted or `enabled: false` means no retry: every request makes exactly one upstream attempt, byte-identical to a gateway built before this field existed. | +| `failover` | `FailoverConfig` | `{}` (enabled) | Cross-provider failover for a bare model id more than one configured provider serves — see [Failover](#failover). `enabled: false` turns it off; a deployment with no overlapping providers behaves exactly as before regardless of this setting. | | `cache` | `CacheConfig` | `{}` (disabled) | Opt-in Redis-backed response cache for unified non-streaming chat/embeddings — see [Caching](#caching). Omitted or `enabled: false` means no caching, byte-identical to a gateway built before this field existed. | | `breaker` | `BreakerConfig` | `{}` (every default) | Per-provider discovery circuit breaker — see [Provider health (discovery circuit breaker)](#provider-health-discovery-circuit-breaker). Every field is individually zero-means-default; a provider whose discovery never fails is unaffected regardless of what this block contains. | | `admin` | `*AdminConfig` | `nil` (disabled) | Read-only admin dashboard — see [Admin](#admin). `nil` or `enabled: false` means the `/admin*` routes are not registered at all. | @@ -807,10 +808,10 @@ released, so this never becomes a ceiling on total concurrent requests. ## Retry -Same-provider retry for a transient upstream failure — no cross-provider -failover. Off by default (`retry.enabled: false`); every config that -predates this field keeps making exactly one upstream attempt per -request. +Same-provider retry for a transient upstream failure. Off by default +(`retry.enabled: false`); every config that predates this field keeps +making exactly one upstream attempt per request. See [Failover](#failover) +below for the separate, cross-provider mechanism. - **Scope**: every adapter upstream call — chat, embeddings, model listing, and the three media endpoints (`images/generations`, @@ -846,6 +847,56 @@ request. is never double-counted. Request counters still increment once per client request, unchanged. +## Failover + +Cross-provider failover for a bare model id more than one configured +provider serves — for example `whisper-1` configured on both `openai` and +`openai-audio`. On by default (`failover.enabled: true` unless set +otherwise); a deployment with no overlapping providers, or with a single +provider, behaves exactly as before regardless of this setting, since +there is never a second candidate to try. + +- **Scope**: `POST /v1/chat/completions`, `POST /v1/embeddings`, and + `POST /v1/messages` only. The image/audio endpoints + (`images/generations`, `audio/speech`, `audio/transcriptions`) are not + wired into failover yet — each has its own request-shape handling + (multipart rebuild, binary/streaming responses) that needs its own + scoped follow-up. +- **Eligible candidates**: a bare model id resolved via the collision + winner (the same mechanism `GET /v1/models` documents for overlapping + catalogs) offers every OTHER configured provider that also serves that + id and that the caller's group is authorized to use. A `provider/model` + request, or an alias whose target is `provider/model`, is an explicit, + exact choice and is never eligible for substitution. +- **What falls through**: everything except HTTP 400 — connection + failures, timeouts, HTTP 429/401/403/404, and any 5xx. A 400 never + falls through: the request is malformed and every provider would reject + it identically. A failover past a 404 is logged loudly, naming both + providers and the model — a 404 usually means a real configuration + mistake, not a transient outage. +- **Never mid-stream**: failover only happens while nothing has reached + the client yet. Once the first byte of a response is written, status + and body are committed and the request is never retried elsewhere. +- **Health signals, kept separate**: the discovery-endpoint health + surfaced on the admin dashboard is an ordering hint only — a provider + whose `/v1/models` is unhealthy is tried later, never skipped outright, + since plenty of providers serve chat completions fine while their model + listing is broken. A separate, in-memory, per-pod request-path health + signal (not exposed to config) is the actual routing gate: a provider + that has failed several consecutive live requests is skipped without + being called until it recovers. +- **Accounting**: a failover chain still counts as exactly one client + request against `requestsPerMinute`/`requestsPerDay` — never once per + attempt. Per-provider attempt/failure counters (the admin dashboard's + success-rate badge) attribute every attempt to the provider that + actually made it. +- **Caching**: a cacheable response produced by a failover provider is + stored under that provider's own cache key, never the first candidate's. +- **Configurable**: `failover.maxAttempts` caps how many different + providers one request will try in total, primary included (default 3, + maximum 10) — distinct from `retry.attempts`, which retries the SAME + provider. `failover.enabled: false` turns the whole mechanism off. + ## Caching An opt-in, Redis-backed response cache for the unified, non-streaming diff --git a/failover.go b/failover.go new file mode 100644 index 0000000..c399817 --- /dev/null +++ b/failover.go @@ -0,0 +1,487 @@ +package traefikllmgateway + +import ( + "context" + "errors" + "fmt" + "net/http" + "sync" + "time" +) + +// This file implements feat/failover: cross-provider failover for a bare +// model id more than one configured provider serves (registry.go's +// bareWinner collision case — production logs show whisper-1, tts-1, +// tts-1-hd, text-embedding-3-small, text-embedding-3-large, and +// text-embedding-ada-002 served by two or more of openai/openai-audio/ +// copilot today). Before this feature, registry.go's resolve returns +// exactly one provider adapter and retry.go's retryPolicy only ever +// retries the SAME provider — so a down provider fails every request for +// its models even when another configured provider serves the identical +// model. This file turns that already-existing redundancy into a hot +// standby, hooked into routes_unified.go's runMeteredCall, the ONE shared +// metered-call path both /v1/chat/completions and /v1/messages already go +// through. +// +// SCOPE: routes_media.go's three media routes (images/generations, +// audio/speech, audio/transcriptions) are deliberately NOT wired into +// failover in this change, even though two of the brief's own named +// overlapping models (whisper-1, tts-1/tts-1-hd) are media-route +// traffic. Each of those three handlers has its own request-shape +// preprocessing (multipart rebuild for transcriptions, a JSON +// re-marshal for speech, a binary/streaming response for speech) that +// would need its own careful "nothing written yet" analysis and its own +// test coverage, rather than reusing runMeteredCall's shared shape — the +// brief's own hook-point instruction ("do not fork a second +// implementation") is about not duplicating the chat/messages pipeline, +// not about extending every possible call site in one pass. Left as a +// scoped follow-up; see this branch's own report for the explicit +// call-out. + +// FailoverConfig configures feat/failover. Enabled is a *bool, mirroring +// ProviderConfig.Passthrough's own nil-means-default-true convention +// (providers.go): nil or true (the default) turns an overlapping +// multi-provider deployment's already-existing redundancy into a hot +// standby with NO config change — the entire point of this feature (see +// this file's own package doc and the brief's "Why" section). Set false +// to opt out entirely. A single-provider deployment, or one where no two +// providers ever serve the identical bare model id, behaves exactly as +// before regardless of this setting: there is never more than one +// candidate to try (orderedFailoverCandidates' own doc comment). +type FailoverConfig struct { + Enabled *bool `json:"enabled,omitempty"` + // MaxAttempts caps how many DIFFERENT PROVIDERS runMeteredCall tries + // in total for one logical request, the primary included — distinct + // from RetryConfig.Attempts (retry.go), which retries the SAME + // provider. 0 (the default) uses defaultFailoverMaxAttempts. A value + // outside 1..maxFailoverMaxAttempts is a construction error. + MaxAttempts int `json:"maxAttempts,omitempty"` +} + +// defaultFailoverMaxAttempts is applied when FailoverConfig.MaxAttempts is +// left at 0. maxFailoverMaxAttempts bounds an explicit override, the same +// "cheap sanity ceiling on an operator value" precedent +// maxBreakerFailureThreshold/maxRetryAttempts already set (registry.go/ +// retry.go). +const ( + defaultFailoverMaxAttempts = 3 + maxFailoverMaxAttempts = 10 +) + +// failoverConfig is FailoverConfig, validated and defaulted once at +// construction (newGateway, llmgateway.go) — mirrors breakerConfig/ +// retryPolicy's own resolved-value-not-raw-config shape (registry.go/ +// retry.go). +type failoverConfig struct { + maxAttempts int + enabled bool +} + +// validateFailoverConfig validates fc and returns the resolved +// failoverConfig newGateway attaches to the Gateway. +func validateFailoverConfig(fc FailoverConfig) (failoverConfig, error) { + enabled := true + if fc.Enabled != nil { + enabled = *fc.Enabled + } + + maxAttempts := fc.MaxAttempts + if maxAttempts == 0 { + maxAttempts = defaultFailoverMaxAttempts + } + if maxAttempts < 1 || maxAttempts > maxFailoverMaxAttempts { + return failoverConfig{}, fmt.Errorf("llmgateway: failover.maxAttempts must be between 1 and %d, got %d", maxFailoverMaxAttempts, maxAttempts) + } + + return failoverConfig{enabled: enabled, maxAttempts: maxAttempts}, nil +} + +// resolveCandidate is one provider that can serve a client-requested model +// id — modelRegistry.resolve's own (adapter, upstreamModel, canonical) +// triple, tagged with the provider's own name so a failover attempt can +// rebind its cache key, attempt recorder, and error log to the ACTUAL +// provider that served or failed the request, never the first one resolve +// happened to pick. +type resolveCandidate struct { + adapter providerAdapter + providerName string + upstreamModel string + canonical string +} + +// resolveWithCandidates resolves id exactly as modelRegistry.resolve +// (registry.go) does — identical winner, identical error — and +// additionally reports every OTHER configured provider that also serves +// the same bare upstream model id and remains authorized for grp, as +// feat/failover's candidate pool. +// +// primary/err are byte-identical to what resolve(id, grp) alone would +// return: this method calls resolve internally and never second-guesses +// its winner or its error — feat/failover only ADDS candidates after an +// already-successful resolution, it never changes which model or error a +// request resolves to (in particular, authorization denial/unknown-model +// behavior for the primary winner is completely unchanged by this +// branch). extra is nil whenever there is nothing to add: +// +// - id resolved via an explicit "provider/model" prefix, or an alias +// whose target is itself provider-prefixed: the requester's own +// explicit provider choice is never a candidate for pool +// substitution — ruling, this feature's own design: "provider/model" +// addressing is exact, not eligible for failover. +// - id resolved via a bare-id collision winner (bareWinner), or an +// alias whose target is itself a bare-id collision winner: extra is +// every OTHER configured provider whose known model set also +// contains that identical bare id string, in +// modelRegistry.providerNames (sorted) order, and authorized for grp +// on that provider. +func (m *modelRegistry) resolveWithCandidates(id string, grp *group) (resolveCandidate, []resolveCandidate, error) { + adapter, upstreamModel, canonical, err := m.resolve(id, grp) + if err != nil { + return resolveCandidate{}, nil, err + } + primary := resolveCandidate{adapter: adapter, providerName: adapter.name(), upstreamModel: upstreamModel, canonical: canonical} + + bareID := id + if target, isAlias := m.aliases[id]; isAlias { + if _, _, ok := m.splitConfiguredProvider(target); ok { + return primary, nil, nil + } + bareID = target + } else if _, _, ok := m.splitConfiguredProvider(id); ok { + return primary, nil, nil + } + + return primary, m.failoverCandidates(bareID, primary.providerName, grp), nil +} + +// failoverCandidates returns every configured provider other than +// primaryProvider whose known model set contains bareID and that grp is +// authorized to use, in modelRegistry.providerNames (sorted) order — the +// raw candidate pool orderedFailoverCandidates (below) narrows and +// reorders further by health. +// +// Only grp.allowsProvider is re-checked per candidate: grp.allowsModel is +// already known true from the primary winner's own successful resolve +// call, and — for the identical bare id string every candidate here +// shares by construction — allowsModel's result does not vary by +// provider (auth.go's allowsModel takes no provider argument), so +// re-checking it per candidate would be redundant work, not a +// correctness gap. +func (m *modelRegistry) failoverCandidates(bareID, primaryProvider string, grp *group) []resolveCandidate { + var extra []resolveCandidate + for _, name := range m.providerNames { + if name == primaryProvider { + continue + } + if !m.states[name].hasModel(bareID) || !grp.allowsProvider(name) { + continue + } + // canonical assigned to a local before the struct literal, matching + // resolveAgainst's own documented Yaegi precedent (registry.go): a + // defensive, cheap precaution against the same class of interpreter + // trap, not a proven necessity for a struct-literal field (only a + // multi-value RETURN tuple is the proven-bad shape there). + canonical := name + "/" + bareID + extra = append(extra, resolveCandidate{ + adapter: m.adapters[name], + providerName: name, + upstreamModel: bareID, + canonical: canonical, + }) + } + return extra +} + +// orderedFailoverCandidates builds runMeteredCall's actual try list from +// resolveWithCandidates' own (primary, extra) split. +// +// Failover is a no-op (returns exactly [primary]) whenever it is disabled +// (g.failover.enabled false) or there is nothing to add (len(extra) == +// 0) — the second case is what makes "single-provider deployment behaves +// exactly as before" hold unconditionally, regardless of the enabled +// switch: there is never more than one candidate to try when no other +// configured provider shares this bare model id. +// +// Otherwise, two health signals apply, deliberately kept separate +// (modelRegistry.discoveryHealthy's own doc comment, registry.go, is +// binding on this): +// +// - REQUEST-PATH health (g.failoverHealth, this file) is a HARD SKIP +// gate: a candidate it reports unhealthy is excluded outright, +// never attempted. If that filtering would remove every candidate +// (this local, per-pod signal reporting the whole fleet down +// simultaneously is more likely stale than true), it is discarded +// and every original candidate is tried anyway — failing open +// against the gateway's own imperfect signal rather than refusing +// to route a request at all. +// - DISCOVERY health (g.registry.discoveryHealthy) is an ORDERING HINT +// ONLY, exactly as its own doc comment requires: a discovery- +// unhealthy candidate is moved after every discovery-healthy/ +// half-open one (a stable partition, not a full sort — relative +// order within each bucket is preserved), never dropped. +// +// Finally the list is capped at g.failover.maxAttempts entries. +func (g *Gateway) orderedFailoverCandidates(primary resolveCandidate, extra []resolveCandidate) []resolveCandidate { + if !g.failover.enabled || len(extra) == 0 { + return []resolveCandidate{primary} + } + + all := make([]resolveCandidate, 0, len(extra)+1) + all = append(all, primary) + all = append(all, extra...) + + healthy := make([]resolveCandidate, 0, len(all)) + for _, c := range all { + if g.failoverHealth.healthy(c.providerName) { + healthy = append(healthy, c) + } + } + if len(healthy) == 0 { + healthy = all + } + + ordered := make([]resolveCandidate, 0, len(healthy)) + var deprioritized []resolveCandidate + for _, c := range healthy { + if g.registry.discoveryHealthy(c.providerName) { + ordered = append(ordered, c) + } else { + deprioritized = append(deprioritized, c) + } + } + ordered = append(ordered, deprioritized...) + + if max := g.failover.maxAttempts; max > 0 && len(ordered) > max { + ordered = ordered[:max] + } + return ordered +} + +// failoverEligible reports whether callErr — runMeteredCall's own +// adapterCall return, evaluated only once nothing has reached the client +// yet (the hard constraint this whole feature is built around: once an +// adapter has written anything, status and body are committed and can +// never be retried elsewhere) — should try the next candidate instead of +// answering the client with callErr. +// +// Ruling (brief, "Which failures fall through" — decided, not +// relitigated here): every failure except HTTP 400 falls through. 5xx, +// connection failures, timeouts, 429, 401, 403, and 404 all do; 400 +// never does, because the request itself is malformed and every provider +// would reject it identically, so retrying just multiplies latency and +// burns budget. +// +// Two failure shapes are excluded for the SAME "retrying changes +// nothing" reasoning 400 itself is excluded for, even though neither is +// literally an HTTP 400: context.Canceled (the client walked away — +// failing over would only waste an attempt against a connection nobody +// is waiting on) and errRequestBuildFailed (upstreamBytes, providers.go +// — the identical malformed request shape reaches every candidate the +// same way, differing only in which operator-configured base URL it is +// built against). A *translateError or *responseTranslationError is this +// gateway's OWN translation code failing on an already-decoded request — +// not a signal about the PROVIDER's availability at all — so neither is +// eligible either. +func failoverEligible(callErr error) bool { + if callErr == nil { + return false + } + if errors.Is(callErr, context.Canceled) || errors.Is(callErr, errRequestBuildFailed) { + return false + } + // Plain type assertions, not errors.As — matching this package's + // established Yaegi-safe convention for these exact two pointer error + // types (providerHTTPError's own doc comment, providers.go; + // handleAdapterErrorEnvelope, routes_unified.go, does the identical + // assertion for the identical reason). + if perr, ok := callErr.(*providerHTTPError); ok { + return perr.status != http.StatusBadRequest + } + if _, ok := callErr.(*translateError); ok { + return false + } + if _, ok := callErr.(*responseTranslationError); ok { + return false + } + return true +} + +// isProviderNotFoundError reports whether err is a *providerHTTPError +// carrying HTTP 404 — routes_unified.go's runMeteredCall uses this to +// decide when a failover needs its own loud, non-suppressed log line +// (brief: "Failover after a 404 must log loudly ... a 404 usually means +// a real configuration mistake, and silently succeeding elsewhere would +// hide it"). +func isProviderNotFoundError(err error) bool { + perr, ok := err.(*providerHTTPError) + return ok && perr.status == http.StatusNotFound +} + +// requestBreakerFailureThreshold/requestBreakerOpenDuration/ +// requestBreakerMaxOpenDuration size requestHealthTracker's own breaker — +// deliberately separate constants from registry.go's discovery breaker +// (defaultBreakerFailureThreshold/defaultBreakerOpenDuration/ +// defaultBreakerMaxOpenDuration): a request-path outage needs to be +// noticed and routed around within seconds, not the discovery breaker's +// minutes-to-hours cadence, and modelRegistry.discoveryHealthy's own doc +// comment forbids widening that breaker to cover this outcome stream at +// all. Fixed constants, not configurable — unlike FailoverConfig above, +// the brief only asks for max-attempts and an on/off switch to be +// operator-configurable; this breaker's thresholds are an internal +// implementation detail of the routing-gate signal, not a documented +// tuning surface. +const ( + requestBreakerFailureThreshold = 3 + requestBreakerOpenDuration = 30 * time.Second + requestBreakerMaxOpenDuration = 5 * time.Minute +) + +// requestHealthState is one provider's REQUEST-PATH health (feat/ +// failover), mu-guarded — mirrors providerState's per-provider, +// individually-mutexed shape (registry.go) at a much smaller scale: this +// tracks exactly one outcome stream (request-path attempts), never +// discovery. The zero value is closed/healthy, so a provider that never +// fails never touches anything below open/backoff/openUntil. +// +// Field order below is fieldalignment-verified (golangci-lint's govet +// enable-all, run with -fix against a scratch copy to derive the exact +// zero-waste sequence, then hand-applied here so this doc comment +// survives — the same convention providerState's own doc comment +// explains, registry.go). +type requestHealthState struct { + openUntil time.Time + backoff time.Duration + consecutiveFailures int + mu sync.Mutex + open bool +} + +// requestHealthTracker is feat/failover's own per-pod, in-memory, +// request-path health signal — deliberately separate from registry.go's +// discovery circuit breaker (modelRegistry.discoveryHealthy's own doc +// comment forbids widening that one to cover request-path traffic). Fed +// by the SAME failure classification limiter.recordProviderAttempt uses +// (isTransient || isDeadlineExceeded — limits.go/retry.go), via record's +// own callers in routes_unified.go, but keeps its own independent state +// per provider name. +// +// Every method is nil-receiver-safe (both report "healthy"/no-op on a +// nil *requestHealthTracker), matching this package's own nil-means- +// disabled convention for an optional subsystem (responseCache's own doc +// comment, llmgateway.go's Gateway.cache field) — a Gateway assembled +// directly in a test without going through newGateway, or any future +// caller that does the same, degrades to "failover never skips a +// candidate for request health" rather than a nil-pointer panic. +type requestHealthTracker struct { + states map[string]*requestHealthState + nowFn func() time.Time + mu sync.Mutex +} + +// newRequestHealthTracker returns an empty tracker — every provider name +// reads healthy until its first recorded failure. +func newRequestHealthTracker() *requestHealthTracker { + return &requestHealthTracker{states: make(map[string]*requestHealthState), nowFn: time.Now} +} + +// stateFor returns provider's requestHealthState, creating it on first +// use. +func (t *requestHealthTracker) stateFor(provider string) *requestHealthState { + t.mu.Lock() + defer t.mu.Unlock() + st, ok := t.states[provider] + if !ok { + st = &requestHealthState{} + t.states[provider] = st + } + return st +} + +// record accounts one upstream request-path attempt's outcome for +// provider: success=false must be EXACTLY isTransient(resp, err) || +// isDeadlineExceeded(err) — the identical classification +// limiter.recordProviderAttempt applies (limits.go/retry.go) — never a +// wider one; routes_unified.go's attemptRecorder closure computes it +// once and feeds both. +// +// requestBreakerFailureThreshold consecutive failures opens the breaker +// for requestBreakerOpenDuration. While open, healthy (below) reports +// unhealthy until that window passes; once it has, the NEXT record call +// for this provider — success or failure — is what healthy already +// exposed as a post-cooldown probe attempt (probed, below): success +// closes the breaker outright, failure re-opens it with a doubled +// backoff (capped at requestBreakerMaxOpenDuration). This is the same +// probe-then-escalate-or-close shape providerState's discovery breaker +// uses (registry.go's recordHealthLocked/openBreakerLocked), +// reimplemented independently rather than shared — the two track +// different outcome streams, and discoveryHealthy's own doc comment +// forbids widening it to cover this one. +// +// Deliberately no single-flight guard on the probe window (unlike +// registry.go's tryBeginRefresh inFlight flag for its own, much lower- +// volume, background-goroutine probe): this signal sits on the live +// request path, where many concurrent requests can observe the same +// post-cooldown window at once. Serializing them would need its own +// lock-and-CAS dance for a benefit — one broken provider getting at most +// one real probe instead of a short burst of them right as its backoff +// expires — this per-pod, best-effort signal does not need to pay for. +func (t *requestHealthTracker) record(provider string, success bool) { + if t == nil || provider == "" { + return + } + st := t.stateFor(provider) + st.mu.Lock() + defer st.mu.Unlock() + now := t.nowFn() + + probed := st.open && !now.Before(st.openUntil) + + if success { + st.open = false + st.consecutiveFailures = 0 + st.backoff = 0 + st.openUntil = time.Time{} + return + } + + if probed { + if st.backoff <= 0 { + st.backoff = requestBreakerOpenDuration + } else { + st.backoff *= 2 + } + if st.backoff > requestBreakerMaxOpenDuration { + st.backoff = requestBreakerMaxOpenDuration + } + st.openUntil = now.Add(st.backoff) + return + } + + st.consecutiveFailures++ + if st.consecutiveFailures >= requestBreakerFailureThreshold { + st.open = true + st.backoff = requestBreakerOpenDuration + st.openUntil = now.Add(st.backoff) + } +} + +// healthy reports whether provider's request-path breaker currently +// permits routing to it — feat/failover's hard SKIP gate +// (orderedFailoverCandidates, above), distinct from +// modelRegistry.discoveryHealthy's deprioritize-only contract. An +// unrecorded provider (never failed, or never attempted at all) +// reads healthy — the same "never seen a failure" default the discovery +// breaker's own zero value already applies (providerState's doc +// comment, registry.go). +func (t *requestHealthTracker) healthy(provider string) bool { + if t == nil || provider == "" { + return true + } + st := t.stateFor(provider) + st.mu.Lock() + defer st.mu.Unlock() + if !st.open { + return true + } + return !t.nowFn().Before(st.openUntil) +} diff --git a/failover_test.go b/failover_test.go new file mode 100644 index 0000000..d74024b --- /dev/null +++ b/failover_test.go @@ -0,0 +1,1009 @@ +package traefikllmgateway + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- modelRegistry.resolveWithCandidates / failoverCandidates --- + +func TestModelRegistry_ResolveWithCandidates_BareCollision_ReturnsOrderedExtras(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "beta": newFakeAdapter("beta"), + "alpha": newFakeAdapter("alpha"), + "gamma": newFakeAdapter("gamma"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "beta": {Models: []string{"shared"}}, + "alpha": {Models: []string{"shared"}}, + "gamma": {Models: []string{"shared"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + primary, extra, err := reg.resolveWithCandidates("shared", allowAllGroup()) + require.NoError(t, err) + assert.Equal(t, "alpha", primary.providerName, "primary must match resolve's own sorted-first winner") + + names := make([]string, len(extra)) + for i, c := range extra { + names[i] = c.providerName + } + assert.Equal(t, []string{"beta", "gamma"}, names, "extras must be every OTHER owning provider, in providerNames sorted order, primary excluded") + for _, c := range extra { + assert.Equal(t, "shared", c.upstreamModel) + assert.Equal(t, c.providerName+"/shared", c.canonical) + } +} + +func TestModelRegistry_ResolveWithCandidates_ProviderPrefixed_NoExtras(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "beta": newFakeAdapter("beta"), + "alpha": newFakeAdapter("alpha"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "beta": {Models: []string{"shared"}}, + "alpha": {Models: []string{"shared"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + primary, extra, err := reg.resolveWithCandidates("beta/shared", allowAllGroup()) + require.NoError(t, err) + assert.Equal(t, "beta", primary.providerName) + assert.Nil(t, extra, "an explicit provider/model request must never gain failover candidates") +} + +func TestModelRegistry_ResolveWithCandidates_AliasToProviderPrefixedTarget_NoExtras(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "beta": newFakeAdapter("beta"), + "alpha": newFakeAdapter("alpha"), + } + cfg := &Config{ + Providers: map[string]*ProviderConfig{ + "beta": {Models: []string{"shared"}}, + "alpha": {Models: []string{"shared"}}, + }, + ModelAliases: map[string]string{"aliased": "beta/shared"}, + } + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + primary, extra, err := reg.resolveWithCandidates("aliased", allowAllGroup()) + require.NoError(t, err) + assert.Equal(t, "beta", primary.providerName) + assert.Nil(t, extra, "an alias targeting an explicit provider/model must never gain failover candidates") +} + +func TestModelRegistry_ResolveWithCandidates_AliasToBareTarget_ReturnsExtras(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "beta": newFakeAdapter("beta"), + "alpha": newFakeAdapter("alpha"), + } + cfg := &Config{ + Providers: map[string]*ProviderConfig{ + "beta": {Models: []string{"shared"}}, + "alpha": {Models: []string{"shared"}}, + }, + ModelAliases: map[string]string{"aliased": "shared"}, + } + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + primary, extra, err := reg.resolveWithCandidates("aliased", allowAllGroup()) + require.NoError(t, err) + assert.Equal(t, "alpha", primary.providerName, "an alias to a bare-collision target resolves via the same sorted-first winner") + require.Len(t, extra, 1) + assert.Equal(t, "beta", extra[0].providerName) +} + +// TestModelRegistry_ResolveWithCandidates_ExtraFiltersByProviderAuthorization +// proves a group restricted away from a would-be failover provider never +// gets it offered as a candidate, even though that provider genuinely +// serves the bare id. +func TestModelRegistry_ResolveWithCandidates_ExtraFiltersByProviderAuthorization(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "beta": newFakeAdapter("beta"), + "alpha": newFakeAdapter("alpha"), + } + cfg := &Config{Providers: map[string]*ProviderConfig{ + "beta": {Models: []string{"shared"}}, + "alpha": {Models: []string{"shared"}}, + }} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + grp := &group{name: "restricted", providers: []string{"alpha"}} + primary, extra, err := reg.resolveWithCandidates("shared", grp) + require.NoError(t, err) + assert.Equal(t, "alpha", primary.providerName) + assert.Empty(t, extra, "beta is denied to this group, so it must never appear as a failover candidate") +} + +// TestModelRegistry_ResolveWithCandidates_ResolveError_PropagatesUnchanged +// proves feat/failover never changes resolve's own error/status contract: +// an unknown or denied model still resolves to the identical error, with +// no candidates offered. +func TestModelRegistry_ResolveWithCandidates_ResolveError_PropagatesUnchanged(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{"alpha": newFakeAdapter("alpha")} + cfg := &Config{Providers: map[string]*ProviderConfig{"alpha": {Models: []string{"gpt-test"}}}} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + require.NoError(t, err) + + _, extra, err := reg.resolveWithCandidates("no-such-model", allowAllGroup()) + assert.ErrorIs(t, err, errModelUnknown) + assert.Nil(t, extra) + + grp := &group{name: "denied", models: []string{"something-else"}} + _, extra, err = reg.resolveWithCandidates("gpt-test", grp) + assert.ErrorIs(t, err, errModelDenied) + assert.Nil(t, extra) +} + +// --- requestHealthTracker --- + +func TestRequestHealthTracker_HealthyByDefault(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + assert.True(t, tr.healthy("never-seen")) +} + +func TestRequestHealthTracker_NilSafe(t *testing.T) { + t.Parallel() + var tr *requestHealthTracker + assert.True(t, tr.healthy("anything"), "a nil tracker must report healthy, never panic") + assert.NotPanics(t, func() { tr.record("anything", false) }) +} + +// TestRequestHealthTracker_OpensAfterThreshold_ThenSkips is the direct +// regression test for "a provider marked unhealthy by REQUEST-path health +// is skipped without being called": would FAIL if record() never opened +// the breaker (a reverted feature would leave healthy() returning true +// forever). +func TestRequestHealthTracker_OpensAfterThreshold_ThenSkips(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + now := time.Now() + tr.nowFn = func() time.Time { return now } + + for i := 0; i < requestBreakerFailureThreshold-1; i++ { + tr.record("p", false) + assert.True(t, tr.healthy("p"), "must stay healthy before the threshold is reached") + } + tr.record("p", false) + assert.False(t, tr.healthy("p"), "must open exactly at the failure threshold") +} + +func TestRequestHealthTracker_SuccessBeforeThreshold_ResetsCounter(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + for i := 0; i < requestBreakerFailureThreshold-1; i++ { + tr.record("p", false) + } + tr.record("p", true) + // The counter must have reset to zero, not merely "not yet open": one + // more failure alone must not open it. + tr.record("p", false) + assert.True(t, tr.healthy("p"), "a success below threshold must reset the failure count, not just pause it") +} + +// TestRequestHealthTracker_RecoversAfterCooldown_ThenCloses proves the +// breaker admits exactly one probe once its cooldown elapses, and a +// successful probe closes it. +func TestRequestHealthTracker_RecoversAfterCooldown_ThenCloses(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + now := time.Now() + tr.nowFn = func() time.Time { return now } + + for i := 0; i < requestBreakerFailureThreshold; i++ { + tr.record("p", false) + } + require.False(t, tr.healthy("p")) + + now = now.Add(requestBreakerOpenDuration - time.Millisecond) + assert.False(t, tr.healthy("p"), "must stay unhealthy until the cooldown fully elapses") + + now = now.Add(2 * time.Millisecond) + assert.True(t, tr.healthy("p"), "must admit exactly one probe once the cooldown elapses") + + tr.record("p", true) + assert.True(t, tr.healthy("p")) + // A single further failure alone must not reopen it — closeBreaker + // resets the failure counter to zero, mirroring registry.go's own + // discovery breaker. + tr.record("p", false) + assert.True(t, tr.healthy("p")) +} + +// TestRequestHealthTracker_FailedProbe_ReopensWithDoubledBackoff proves a +// failed post-cooldown probe re-opens the breaker with a LONGER backoff, +// not the same one — would FAIL if reverted to a fixed-duration reopen. +func TestRequestHealthTracker_FailedProbe_ReopensWithDoubledBackoff(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + now := time.Now() + tr.nowFn = func() time.Time { return now } + + for i := 0; i < requestBreakerFailureThreshold; i++ { + tr.record("p", false) + } + now = now.Add(requestBreakerOpenDuration + time.Millisecond) + require.True(t, tr.healthy("p"), "cooldown elapsed, must admit the probe") + tr.record("p", false) // the probe itself fails + + assert.False(t, tr.healthy("p"), "a failed probe must reopen the breaker") + // The new backoff is requestBreakerOpenDuration*2: advancing only + // past the ORIGINAL window must still read unhealthy. + now = now.Add(requestBreakerOpenDuration + time.Millisecond) + assert.False(t, tr.healthy("p"), "a failed probe must double the backoff, not repeat the original window") + + now = now.Add(requestBreakerOpenDuration + 2*time.Millisecond) + assert.True(t, tr.healthy("p"), "must recover once the doubled window has fully elapsed") +} + +func TestRequestHealthTracker_BackoffCapsAtMax(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + now := time.Now() + tr.nowFn = func() time.Time { return now } + + for i := 0; i < requestBreakerFailureThreshold; i++ { + tr.record("p", false) + } + // Fail every probe repeatedly — backoff must never exceed + // requestBreakerMaxOpenDuration, so this loop must always terminate + // within a bounded number of doublings. + for i := 0; i < 20; i++ { + now = now.Add(requestBreakerMaxOpenDuration + time.Millisecond) + require.True(t, tr.healthy("p")) + tr.record("p", false) + } + st := tr.stateFor("p") + st.mu.Lock() + backoff := st.backoff + st.mu.Unlock() + assert.LessOrEqual(t, backoff, requestBreakerMaxOpenDuration) +} + +// TestRequestHealthTracker_IndependentPerProvider proves one provider's +// failures never affect another's health — the per-pod signal is keyed by +// provider name, not shared global state. +func TestRequestHealthTracker_IndependentPerProvider(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + for i := 0; i < requestBreakerFailureThreshold; i++ { + tr.record("broken", false) + } + assert.False(t, tr.healthy("broken")) + assert.True(t, tr.healthy("fine"), "an unrelated provider's health must be untouched") +} + +// TestRequestHealthTracker_ConcurrentRecord_NoRace exercises record/healthy +// from many goroutines at once — the whole point of this signal is to sit +// on the live request path. +func TestRequestHealthTracker_ConcurrentRecord_NoRace(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + tr.record("p", i%3 != 0) + tr.healthy("p") + }(i) + } + wg.Wait() +} + +// --- failoverEligible / isProviderNotFoundError --- + +func TestFailoverEligible(t *testing.T) { + t.Parallel() + deadlineExceededErr := fmt.Errorf("%w: %w", errUpstream, fmt.Errorf("dial tcp: %w", context.DeadlineExceeded)) + + tests := []struct { + err error + name string + want bool + }{ + {name: "nil error", err: nil, want: false}, + {name: "400 never falls through", err: &providerHTTPError{status: http.StatusBadRequest}, want: false}, + {name: "500 falls through", err: &providerHTTPError{status: http.StatusInternalServerError}, want: true}, + {name: "502 falls through", err: &providerHTTPError{status: http.StatusBadGateway}, want: true}, + {name: "429 falls through", err: &providerHTTPError{status: http.StatusTooManyRequests}, want: true}, + {name: "401 falls through", err: &providerHTTPError{status: http.StatusUnauthorized}, want: true}, + {name: "403 falls through", err: &providerHTTPError{status: http.StatusForbidden}, want: true}, + {name: "404 falls through", err: &providerHTTPError{status: http.StatusNotFound}, want: true}, + {name: "connection failure falls through", err: fmt.Errorf("%w: dial tcp: connection refused", errUpstream), want: true}, + {name: "deadline exceeded (timeout) falls through", err: deadlineExceededErr, want: true}, + {name: "client canceled does not fall through", err: fmt.Errorf("%w: %w", errUpstream, context.Canceled), want: false}, + {name: "request build failure does not fall through", err: fmt.Errorf("%w: %w: build request", errUpstream, errRequestBuildFailed), want: false}, + {name: "translateError does not fall through", err: &translateError{msg: "bad field"}, want: false}, + {name: "responseTranslationError does not fall through", err: &responseTranslationError{err: errors.New("boom")}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, failoverEligible(tt.err)) + }) + } +} + +func TestIsProviderNotFoundError(t *testing.T) { + t.Parallel() + assert.True(t, isProviderNotFoundError(&providerHTTPError{status: http.StatusNotFound})) + assert.False(t, isProviderNotFoundError(&providerHTTPError{status: http.StatusInternalServerError})) + assert.False(t, isProviderNotFoundError(errors.New("plain error"))) + assert.False(t, isProviderNotFoundError(nil)) +} + +// --- validateFailoverConfig --- + +func TestValidateFailoverConfig_Defaults(t *testing.T) { + t.Parallel() + fc, err := validateFailoverConfig(FailoverConfig{}) + require.NoError(t, err) + assert.True(t, fc.enabled, "nil Enabled must default to true — failover is a hot standby with no config change") + assert.Equal(t, defaultFailoverMaxAttempts, fc.maxAttempts) +} + +func TestValidateFailoverConfig_ExplicitDisable(t *testing.T) { + t.Parallel() + no := false + fc, err := validateFailoverConfig(FailoverConfig{Enabled: &no}) + require.NoError(t, err) + assert.False(t, fc.enabled) +} + +func TestValidateFailoverConfig_MaxAttemptsBounds(t *testing.T) { + t.Parallel() + _, err := validateFailoverConfig(FailoverConfig{MaxAttempts: -1}) + assert.Error(t, err) + + _, err = validateFailoverConfig(FailoverConfig{MaxAttempts: maxFailoverMaxAttempts + 1}) + assert.Error(t, err) + + fc, err := validateFailoverConfig(FailoverConfig{MaxAttempts: maxFailoverMaxAttempts}) + require.NoError(t, err) + assert.Equal(t, maxFailoverMaxAttempts, fc.maxAttempts) +} + +// --- Gateway.orderedFailoverCandidates --- + +// testGatewayForCandidates builds a minimal *Gateway sufficient for +// orderedFailoverCandidates: a real modelRegistry (for discoveryHealthy) +// and a fresh requestHealthTracker, without going through newGateway/New. +func testGatewayForCandidates(t *testing.T, providerNames []string, fc failoverConfig) *Gateway { + t.Helper() + adapters := make(map[string]providerAdapter, len(providerNames)) + providers := make(map[string]*ProviderConfig, len(providerNames)) + for _, name := range providerNames { + adapters[name] = newFakeAdapter(name) + providers[name] = &ProviderConfig{Models: []string{"shared"}} + } + reg, err := newModelRegistry(adapters, &Config{Providers: providers}, func(string, ...any) {}) + require.NoError(t, err) + return &Gateway{registry: reg, failoverHealth: newRequestHealthTracker(), failover: fc} +} + +func candNames(cands []resolveCandidate) []string { + out := make([]string, len(cands)) + for i, c := range cands { + out[i] = c.providerName + } + return out +} + +func TestOrderedFailoverCandidates_Disabled_ReturnsPrimaryOnly(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta"}, failoverConfig{enabled: false, maxAttempts: 3}) + primary := resolveCandidate{providerName: "alpha"} + extra := []resolveCandidate{{providerName: "beta"}} + got := g.orderedFailoverCandidates(primary, extra) + assert.Equal(t, []string{"alpha"}, candNames(got), "disabled failover must never try a second provider") +} + +func TestOrderedFailoverCandidates_NoExtras_ReturnsPrimaryOnly_RegardlessOfEnabled(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha"}, failoverConfig{enabled: true, maxAttempts: 3}) + primary := resolveCandidate{providerName: "alpha"} + got := g.orderedFailoverCandidates(primary, nil) + assert.Equal(t, []string{"alpha"}, candNames(got), "single-provider deployment must behave exactly as before regardless of the enabled switch") +} + +func TestOrderedFailoverCandidates_RequestUnhealthy_HardSkipped(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta"}, failoverConfig{enabled: true, maxAttempts: 3}) + for i := 0; i < requestBreakerFailureThreshold; i++ { + g.failoverHealth.record("alpha", false) + } + primary := resolveCandidate{providerName: "alpha"} + extra := []resolveCandidate{{providerName: "beta"}} + got := g.orderedFailoverCandidates(primary, extra) + assert.Equal(t, []string{"beta"}, candNames(got), "a request-unhealthy provider must be excluded outright, never merely deprioritized") +} + +func TestOrderedFailoverCandidates_AllRequestUnhealthy_FailsOpenToFullList(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta"}, failoverConfig{enabled: true, maxAttempts: 3}) + for _, name := range []string{"alpha", "beta"} { + for i := 0; i < requestBreakerFailureThreshold; i++ { + g.failoverHealth.record(name, false) + } + } + primary := resolveCandidate{providerName: "alpha"} + extra := []resolveCandidate{{providerName: "beta"}} + got := g.orderedFailoverCandidates(primary, extra) + assert.ElementsMatch(t, []string{"alpha", "beta"}, candNames(got), "every candidate unhealthy must fail OPEN to the full list, not refuse to route at all") +} + +func TestOrderedFailoverCandidates_DiscoveryUnhealthy_DeprioritizedNotDropped(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta"}, failoverConfig{enabled: true, maxAttempts: 3}) + // Force alpha's discovery breaker open directly. + st := g.registry.states["alpha"] + st.mu.Lock() + st.health = breakerOpen + st.openUntil = time.Now().Add(time.Hour) + st.mu.Unlock() + require.False(t, g.registry.discoveryHealthy("alpha")) + + primary := resolveCandidate{providerName: "alpha"} + extra := []resolveCandidate{{providerName: "beta"}} + got := g.orderedFailoverCandidates(primary, extra) + assert.Equal(t, []string{"beta", "alpha"}, candNames(got), "discovery-unhealthy must be deprioritized (moved after), never dropped from the list") +} + +func TestOrderedFailoverCandidates_MaxAttemptsCapsList(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta", "gamma"}, failoverConfig{enabled: true, maxAttempts: 2}) + primary := resolveCandidate{providerName: "alpha"} + extra := []resolveCandidate{{providerName: "beta"}, {providerName: "gamma"}} + got := g.orderedFailoverCandidates(primary, extra) + assert.Len(t, got, 2, "maxAttempts must cap the total candidate list, primary included") + assert.Equal(t, []string{"alpha", "beta"}, candNames(got)) +} + +// ===================================================================== +// End-to-end HTTP tests +// ===================================================================== + +// twoProviderFailoverConfig builds a Config with two openai-type providers, +// "alpha" and "beta" (alpha sorted first — the bareWinner/primary +// candidate), both configured with the identical bare model id "shared", +// pointed at srvAlpha/srvBeta respectively, one group with no +// restrictions, and one user "alice" with unlimited limits. +func twoProviderFailoverConfig(srvAlpha, srvBeta *httptest.Server) *Config { + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: srvAlpha.URL, APIKey: "k", Models: []string{"shared"}}, + "beta": {Type: "openai", BaseURL: srvBeta.URL, APIKey: "k", Models: []string{"shared"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}, + }} + return cfg +} + +const successRespBody = `{"id":"chatcmpl-ok","object":"chat.completion","model":"shared","choices":[{"index":0,"message":{"role":"assistant","content":"hi"},"finish_reason":"stop"}],"usage":{"prompt_tokens":3,"completion_tokens":2}}` + +func jsonServer(status int, body string) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) +} + +func countingServer(status int, body string, calls *int64) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(calls, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) +} + +// TestHandleChat_Failover_FallsThroughAndServes is table-driven across +// every failure shape the brief names as a fallthrough case (5xx, +// connection failure, 429, 401, 403, 404) — proving alpha's failure is +// classified eligible AND that beta's real response reaches the client +// each time. Would FAIL if failoverEligible or the candidate loop's +// hasNext wiring were reverted (alpha's error would just be returned to +// the client instead of falling through). +func TestHandleChat_Failover_FallsThroughAndServes(t *testing.T) { + tests := []struct { + name string + alphaStatus int + unreachable bool + }{ + {name: "5xx", alphaStatus: http.StatusInternalServerError}, + {name: "connection_failure", unreachable: true}, + {name: "429", alphaStatus: http.StatusTooManyRequests}, + {name: "401", alphaStatus: http.StatusUnauthorized}, + {name: "403", alphaStatus: http.StatusForbidden}, + {name: "404", alphaStatus: http.StatusNotFound}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var alphaURL string + if tt.unreachable { + // A closed listener: connections are refused immediately, + // deterministic and fast, unlike a black-holed address. + dead := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + alphaURL = dead.URL + dead.Close() + } else { + alphaSrv := jsonServer(tt.alphaStatus, `{"error":{"message":"alpha down","type":"server_error"}}`) + defer alphaSrv.Close() + alphaURL = alphaSrv.URL + } + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: alphaURL, APIKey: "k", Models: []string{"shared"}}, + "beta": {Type: "openai", BaseURL: betaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + origStderr := captureStderrStart() + body := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + logOutput := captureStderrStop(origStderr) + + require.Equal(t, http.StatusOK, rec.Code, "must fall through to beta and serve its response, body=%s", rec.Body.String()) + assert.Equal(t, successRespBody, rec.Body.String()) + + if tt.alphaStatus == http.StatusNotFound { + assert.Contains(t, logOutput, "404", "a 404 failover must log loudly") + assert.Contains(t, logOutput, "alpha") + assert.Contains(t, logOutput, "beta") + } + }) + } +} + +// TestHandleChat_Failover_400_DoesNotFallThrough is the regression test +// for the brief's one hard exception: a malformed request must never +// retry against a second provider. Would FAIL if failoverEligible treated +// 400 the same as every other status. +func TestHandleChat_Failover_400_DoesNotFallThrough(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusBadRequest, `{"error":{"message":"bad request","type":"invalid_request_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code, "a 400 must be returned as-is, never retried") + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be called after alpha's 400") +} + +// TestHandleChat_Failover_MidStream_DoesNotFailOver_NoCorruption is the +// regression test for THE HARD CONSTRAINT: once anything has reached the +// client, failover must never happen and the response must never be +// corrupted with a second envelope. Would FAIL if the loop checked +// failoverEligible before sw.wroteHeader. +func TestHandleChat_Failover_MidStream_DoesNotFailOver_NoCorruption(t *testing.T) { + const firstChunk = "data: {\"id\":\"c1\",\"object\":\"chat.completion.chunk\",\"choices\":[{\"delta\":{\"content\":\"Hi\"}}]}\n\n" + + var betaCalls int64 + alphaSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + fl, ok := w.(http.Flusher) + require.True(t, ok) + _, _ = w.Write([]byte(firstChunk)) + fl.Flush() + hj, ok := w.(http.Hijacker) + require.True(t, ok) + conn, _, hjErr := hj.Hijack() + if hjErr == nil { + _ = conn.Close() + } + })) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "stream": true, "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code, "headers already committed before the drop") + assert.Equal(t, firstChunk, rec.Body.String(), "no trailing envelope, no failover content appended") + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be called once alpha has started writing") +} + +// TestHandleChat_Failover_RequestUnhealthy_SkippedWithoutBeingCalled is the +// end-to-end regression test for the request-path health hard-skip gate: +// after enough consecutive failures, alpha must be excluded from routing +// entirely — its upstream server must receive no further hits — while beta +// still serves. +func TestHandleChat_Failover_RequestUnhealthy_SkippedWithoutBeingCalled(t *testing.T) { + var alphaCalls, betaCalls int64 + alphaSrv := countingServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`, &alphaCalls) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "messages": []any{}} + for i := 0; i < requestBreakerFailureThreshold; i++ { + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "every one of these must still fail over to beta and succeed") + } + require.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls)) + + // alpha's request-path breaker must now be open — one more request + // must skip it entirely. + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls), "alpha must NOT receive a further hit once request-unhealthy") + assert.Equal(t, int64(requestBreakerFailureThreshold+1), atomic.LoadInt64(&betaCalls)) +} + +// TestHandleChat_Failover_DiscoveryUnhealthy_StillTried proves discovery +// health is an ordering hint, never a hard skip: a provider whose +// DISCOVERY breaker is open must still be attempted (and can still serve +// the response) when it is the only one that actually works. Would FAIL +// if discoveryHealthy were (mis-)used as a skip gate. +func TestHandleChat_Failover_DiscoveryUnhealthy_StillTried(t *testing.T) { + alphaSrv := jsonServer(http.StatusOK, successRespBody) // alpha's CHAT endpoint works fine + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + // Force alpha's DISCOVERY breaker open directly (feat/provider-health's + // own signal) — its chat-completions endpoint is untouched and stays + // healthy; only discovery is marked broken, per discoveryHealthy's own + // documented scope. + st := gw.registry.states["alpha"] + st.mu.Lock() + st.health = breakerOpen + st.openUntil = time.Now().Add(time.Hour) + st.mu.Unlock() + require.False(t, gw.registry.discoveryHealthy("alpha")) + + // beta is healthy by every signal but its own chat endpoint 500s — + // ordering puts beta (discovery-healthy) FIRST, so this proves the + // request actually reaches alpha SECOND, not that alpha happened to + // be tried first anyway. + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "alpha must still be tried and must serve, body=%s", rec.Body.String()) + assert.Equal(t, successRespBody, rec.Body.String()) +} + +// TestHandleChat_Failover_TenantRequestCount_IncrementsOnceAcrossFailover +// is the regression test for "never double-count the request": a +// tenant's own req/min counter must move by exactly 1 across a whole +// failover chain, never once per attempt. +func TestHandleChat_Failover_TenantRequestCount_IncrementsOnceAcrossFailover(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + now := time.Now() + reqCount, ok := gw.limiter.getCounter("user", "alice", metricReq, windowMin, now) + require.True(t, ok) + assert.Equal(t, int64(1), reqCount, "one client-facing request must count as exactly one, regardless of how many providers it took") + + totalReq, ok := gw.limiter.getCounter(totalScopeKind, totalScopeID, metricReq, windowMin, now) + require.True(t, ok) + assert.Equal(t, int64(1), totalReq) +} + +// TestHandleChat_Failover_PerProviderAttemptCounters_AttributeCorrectly is +// the regression test for "rebind the attempt recorder per attempt": alpha +// must show exactly one attempt/one failure, and beta exactly one +// attempt/zero failures — never both attributed to alpha (the closure's +// original, fixed-at-construction bug this feature explicitly fixes). +func TestHandleChat_Failover_PerProviderAttemptCounters_AttributeCorrectly(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + // Deterministic: run the fire-and-forget counter write synchronously + // (mirrors TestHandleChat_ContextDeadlineExceeded_RecordsProviderFailure). + gw.limiter.spawn = func(f func()) { f() } + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + now := time.Now() + alphaAttempts, _ := gw.limiter.getCounter(kindProvider, "alpha", metricProvAttempt, windowDay, now) + alphaFails, _ := gw.limiter.getCounter(kindProvider, "alpha", metricProvFail, windowDay, now) + betaAttempts, _ := gw.limiter.getCounter(kindProvider, "beta", metricProvAttempt, windowDay, now) + betaFails, _ := gw.limiter.getCounter(kindProvider, "beta", metricProvFail, windowDay, now) + + assert.Equal(t, int64(1), alphaAttempts, "alpha attempts") + assert.Equal(t, int64(1), alphaFails, "alpha fails — a reverted 'rebind per attempt' bug would misattribute this to whichever provider the closure was first bound to") + assert.Equal(t, int64(1), betaAttempts, "beta attempts") + assert.Equal(t, int64(0), betaFails, "beta fails") +} + +// TestHandleChat_Failover_CachedUnderWinningProviderKey is the regression +// test for cache-key correctness: a response produced by the FAILOVER +// provider (beta) must be retrievable via a cache key built with BETA's +// own name, not alpha's — would FAIL if cacheKeyStr were still computed +// once, before the loop, from the primary candidate alone. +func TestHandleChat_Failover_CachedUnderWinningProviderKey(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + redisLn := newBehavioralRedisServer(t) + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + cfg.Redis = &RedisConfig{Address: redisLn.Addr().String()} + cfg.Cache = CacheConfig{Enabled: true, TTL: "1m", MaxBodyBytes: 1 << 20} + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + reqBody := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + require.Equal(t, "miss", rec.Header().Get("X-Llmgw-Cache")) + require.Equal(t, int64(1), atomic.LoadInt64(&betaCalls)) + + // Reconstruct the exact key runMeteredCall would compute for BETA's + // own candidate — upstreamModel is "shared" (the bare id every + // candidate shares in this fixture), requestedModel is the client's + // own "shared" string, endpoint is cacheEndpointChat, and req is the + // decoded body with "model" rewritten (exactly what the gateway + // hashes) — see cacheKey's own doc comment, cache.go. + hashed := map[string]any{"model": "shared", "messages": reqBody["messages"]} + wantKey := cacheKey("beta", "shared", "shared", cacheEndpointChat, hashed) + cached, hit := gw.cache.lookup(wantKey) + require.True(t, hit, "the response must be stored under beta's own cache key") + assert.Contains(t, string(cached.Body), `"id":"chatcmpl-ok"`) + + // A second, identical request must now be served from cache without + // beta's real backend being hit again. + req2 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec2 := httptest.NewRecorder() + h.ServeHTTP(rec2, req2) + assert.Equal(t, http.StatusOK, rec2.Code) + assert.Equal(t, int64(1), atomic.LoadInt64(&betaCalls), "the second request must hit cache under beta's key, not call beta's backend again") +} + +// TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore +// pins the "single-provider deployment behaves exactly as before" +// requirement explicitly for this branch, with failover left at its +// (enabled) default: with no second candidate to try, an upstream error +// must be returned to the client unchanged, identical to v0.1 behavior. +func TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore(t *testing.T) { + srv := jsonServer(http.StatusTooManyRequests, `{"error":{"message":"rate limited upstream","type":"rate_limit_error"}}`) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: srv.URL, APIKey: "k", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "gpt-test", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusTooManyRequests, rec.Code, "a single-provider deployment must still surface the upstream error unchanged — there is nothing to fail over to") + var out struct { + Error struct { + Type string `json:"type"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &out)) + assert.Equal(t, "upstream_error", out.Error.Type) +} + +// TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough proves +// the on/off switch actually gates the behavior end to end. +func TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + no := false + cfg.Failover = FailoverConfig{Enabled: &no} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code, "with failover disabled, alpha's own 500 must be returned as-is, wrapped in the upstream-error envelope") + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls)) +} + +// TestHandleChat_Failover_ContextDeadlineExceeded_ClassifiedEligible_ +// AndRecordsHealthFailure proves timeout classification end to end +// through the SAME mechanism recordProviderAttempt already uses +// (isDeadlineExceeded) — see this test file's package-level note on why a +// full "alpha times out, beta still serves" HTTP scenario cannot be +// demonstrated: every candidate in one logical request shares ONE +// request context, so once that context's own deadline is what makes +// alpha's failure a genuine timeout, zero budget remains for any +// subsequent candidate. This proves the half that IS achievable and +// correct: alpha's attempt is recorded as a request-health failure +// exactly like a plain connection error would be. +func TestHandleChat_Failover_ContextDeadlineExceeded_RecordsRequestHealthFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(300 * time.Millisecond) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(successRespBody)) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: srv.URL, APIKey: "k", Models: []string{"shared"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", + strings.NewReader(`{"model":"shared","messages":[{"role":"user","content":"hi"}]}`)).WithContext(ctx) + req.Header.Set("Authorization", "Bearer sk-alice") + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.True(t, gw.failoverHealth.healthy("alpha"), "one deadline-exceeded attempt alone must not yet open the breaker (below threshold)") + + st := gw.failoverHealth.stateFor("alpha") + st.mu.Lock() + failures := st.consecutiveFailures + st.mu.Unlock() + assert.Equal(t, 1, failures, "the deadline-exceeded attempt must be classified as a request-health failure, not silently ignored") +} + +// capturedStderr holds the state captureStderrStart needs to hand back to +// captureStderrStop — the same os.Pipe swap +// TestHandleChat_MidStreamUpstreamDrop_NoTrailingEnvelope already uses in +// routes_unified_test.go, factored out here so the table-driven fallthrough +// test above can capture the 404 loud-log assertion per sub-test. +type capturedStderr struct { + orig *os.File + pr *os.File + pw *os.File +} + +// captureStderrStart redirects os.Stderr to an in-memory pipe. Callers +// must not run this concurrently with another goroutine writing to +// os.Stderr in the SAME process (the same constraint the existing +// mid-stream-drop test already accepts) — every caller in this file runs +// its captured request synchronously, sequentially, never inside +// t.Parallel(). +func captureStderrStart() capturedStderr { + orig := os.Stderr + pr, pw, err := os.Pipe() + if err != nil { + panic(err) // os.Pipe failing is not a condition any test here can meaningfully recover from + } + os.Stderr = pw + return capturedStderr{orig: orig, pr: pr, pw: pw} +} + +// captureStderrStop restores os.Stderr and returns everything written to +// it since the matching captureStderrStart call. +func captureStderrStop(c capturedStderr) string { + os.Stderr = c.orig + _ = c.pw.Close() + out, _ := io.ReadAll(c.pr) + return string(out) +} diff --git a/llmgateway.go b/llmgateway.go index ed15ff0..6cd63ac 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -70,7 +70,13 @@ type Config struct { // at all) resolves to every documented default and behaves exactly // like a deployment with no circuit breaker at all for a provider // that never fails — see validateBreakerConfig's own doc comment. - Breaker BreakerConfig `json:"breaker"` + // Failover configures feat/failover — cross-provider failover for a + // bare model id more than one configured provider serves. See + // FailoverConfig's own doc comment (failover.go) for the full + // contract; a struct value, not a pointer, for the same + // zero-means-default reason as Breaker/Retry/Cache below. + Failover FailoverConfig `json:"failover"` + Breaker BreakerConfig `json:"breaker"` // Retry is a struct value, not a pointer, because its own Enabled // field is the on/off signal (unlike Redis/Users, where the block's // mere presence is the signal) — so its tag omits "omitempty": @@ -493,18 +499,24 @@ func defaultBodyAdmissionCap() int { const maxExplicitBodyAdmissionCap = 10_000 // Gateway is the Traefik middleware handler. +// +// Field order below is fieldalignment-verified (golangci-lint's govet +// enable-all, run with -fix against a scratch copy to derive the exact +// zero-waste sequence, then hand-applied here so every field keeps its +// original doc comment) — see providerState's own doc comment +// (registry.go) for the general convention. type Gateway struct { - next http.Handler - cfg *Config - auth *authStore - limiter *limiter - registry *modelRegistry - adapters map[string]providerAdapter + next http.Handler // targetClient is the shared, connection-pooled *http.Client the // MCP/A2A target proxy (mcp_a2a.go) issues every upstream request // through — built once via newAdapterHTTPClient, the same constructor // each provider adapter uses for its own client. targetClient *http.Client + auth *authStore + limiter *limiter + registry *modelRegistry + adapters map[string]providerAdapter + cfg *Config // cache is nil whenever response caching is not configured or not // usable (cfg.Cache.Enabled is false, or true with no config.Redis — // see buildResponseCache, cache.go). Every call site checks for nil @@ -525,7 +537,17 @@ type Gateway struct { // resized afterward, matching a Go channel's own fixed-capacity // contract. bodyAdmission chan struct{} - name string + // failoverHealth is feat/failover's own per-pod, in-memory + // request-path health signal (failover.go), built once, here, by + // newGateway. Every method on it is nil-receiver-safe, so a Gateway + // assembled directly (bypassing newGateway, as a few older tests do) + // degrades to "failover never skips a candidate for request health" + // rather than a nil-pointer panic. + failoverHealth *requestHealthTracker + name string + // failover is Config.Failover, validated and resolved once by + // newGateway (validateFailoverConfig, failover.go). + failover failoverConfig } // telemetryStartupOnce keeps the anonymous "plugin loaded" ping to one per @@ -634,6 +656,16 @@ func newGateway(ctx context.Context, next http.Handler, config *Config, name str } g := &Gateway{next: next, name: name, cfg: config, auth: auth} + // feat/failover: validated once, here — see FailoverConfig's own doc + // comment (failover.go) for the default (enabled) and what "single- + // provider deployment behaves exactly as before" means in practice. + failoverCfg, err := validateFailoverConfig(config.Failover) + if err != nil { + return nil, err + } + g.failover = failoverCfg + g.failoverHealth = newRequestHealthTracker() + // bodyAdmissionCap: an explicit MaxInFlightBodyRequests always wins // over the self-tuned default (house rule: explicit override always // wins) — see Config.MaxInFlightBodyRequests' own doc comment. 0 (the diff --git a/routes_unified.go b/routes_unified.go index 143404e..655af16 100644 --- a/routes_unified.go +++ b/routes_unified.go @@ -256,123 +256,223 @@ func (g *Gateway) runUnified(w http.ResponseWriter, r *http.Request, u *user, gr // envelope shape they answer in. logPrefix names the route in every log // line this function writes, so a log reader can always tell which route // produced a given line without the classification logic existing twice. +// +// feat/failover: this function now tries a LIST of candidate providers +// (g.registry.resolveWithCandidates/g.orderedFailoverCandidates, +// failover.go), not just the one resolve alone would pick, and is the +// ONE hook point both /v1/chat/completions (runUnified, above) and +// /v1/messages (handleMessages, routes_messages.go) share — per the +// brief's own instruction, failover is wired here once rather than +// forked into a second implementation. THE HARD CONSTRAINT this loop is +// built around: call(...) writes directly to respWriter, which tees +// straight through to sw; the very first byte written commits the +// response, so a candidate is only ever retried while sw.wroteHeader is +// still false. Every step that was a one-shot computation before this +// feature (the cache key, the attempt recorder's bound provider name, +// the usage/canonical id billed) now happens fresh INSIDE the loop, once +// per candidate — this is what "rebind the attempt recorder per +// attempt" and "cache under the winning provider's key" actually mean +// in code, not just in the brief's prose. func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scopes []limitScope, body []byte, req map[string]any, requestedModel string, grp *group, endpoint, logPrefix string, envelope envelopeWriter, writeUpstream providerUpstreamErrorWriter, call adapterCall) { - adapter, upstreamModel, canonical, err := g.registry.resolve(requestedModel, grp) + primary, extra, err := g.registry.resolveWithCandidates(requestedModel, grp) if err != nil { writeModelResolveErrorEnvelope(sw, err, envelope) return } + candidates := g.orderedFailoverCandidates(primary, extra) - req["model"] = upstreamModel - - // cacheable gates every cache step below on spec §2's scope: a - // non-streaming request, with a working cache (g.cache is nil - // whenever caching is off or Redis was absent — buildResponseCache, - // cache.go), for a group that has not opted out (groupCacheEnabled). - // cacheKey is computed here — after "model" is rewritten to - // upstreamModel but deliberately BEFORE gatewayAliasKey is injected - // below — so the hashed request body reflects exactly what goes - // upstream. requestedModel (the client's own, un-rewritten alias - // string) and endpoint are passed as separate cacheKey arguments, - // not left in req: a translating adapter bakes the echoed alias - // straight into the cached response body's own "model" field, so two - // clients requesting the same upstream model under different alias - // forms (e.g. "claude-x" vs "anthropic/claude-x") must never collide - // into one cache entry (cacheKey's own doc comment, cache.go). + // cacheable/streaming gate every cache step below on spec §2's scope, + // exactly as before this feature: a non-streaming request, with a + // working cache (g.cache is nil whenever caching is off or Redis was + // absent — buildResponseCache, cache.go), for a group that has not + // opted out (groupCacheEnabled). Computed once, outside the loop: it + // depends only on req["stream"]/grp, neither of which a failover + // attempt changes. streaming, _ := req["stream"].(bool) cacheable := !streaming && g.cache != nil && groupCacheEnabled(grp) - var cacheKeyStr string - if cacheable { - cacheKeyStr = cacheKey(adapter.name(), upstreamModel, requestedModel, endpoint, req) - if cached, hit := g.cache.lookup(cacheKeyStr); hit { - // A cache hit accounts the request only — checkAndCount - // above already counted it — never token/cost, and never - // runs the zero-usage estimate branch below: the client - // never reached the upstream provider its body size would - // be estimating against. - sw.Header().Set("X-Llmgw-Cache", "hit") - if cached.ContentType != "" { - sw.Header().Set("Content-Type", cached.ContentType) + + for i, cand := range candidates { + if i > 0 { + // checkAndCount above (admitRequest, this file) already + // counted this request exactly once — never double-count: a + // failover attempt is a NEW upstream attempt, not a new + // logical request, so nothing here calls checkAndCount + // again. Per-provider ATTEMPT counters, below, still record + // every attempt independently via the freshly-bound + // recorder. + g.warnf("%s: failing over from provider %q to %q for model %q", logPrefix, candidates[i-1].providerName, cand.providerName, requestedModel) + } + + // req["model"] is reset on EVERY iteration: cand.upstreamModel is + // the identical string across every candidate in a failover chain + // by construction (failoverCandidates, failover.go, only ever + // adds providers sharing the SAME bare id resolve's primary + // already resolved to), so this is a no-op rewrite on the common + // single-candidate path, not new cost. + req["model"] = cand.upstreamModel + + // cacheKey is computed fresh per candidate — cache correctness + // (brief): a response produced by a failover provider must be + // stored under THAT provider's own key, never the first + // candidate's. Called here, BEFORE gatewayAliasKey is injected + // below, preserving the ordering cacheKey's own doc comment + // (cache.go) requires of every caller: gatewayAliasKey is a + // purely internal echo-back mechanism cacheKey does not itself + // strip, so computing the key after injecting it would hash an + // extra, noisy field. + var cacheKeyStr string + if cacheable { + cacheKeyStr = cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, req) + if cached, hit := g.cache.lookup(cacheKeyStr); hit { + // A cache hit accounts the request only — checkAndCount + // above already counted it — never token/cost, and never + // runs the zero-usage estimate branch below: the client + // never reached the upstream provider its body size would + // be estimating against. + sw.Header().Set("X-Llmgw-Cache", "hit") + if cached.ContentType != "" { + sw.Header().Set("Content-Type", cached.ContentType) + } + sw.WriteHeader(cached.Status) + _, _ = sw.Write(cached.Body) + return } - sw.WriteHeader(cached.Status) - _, _ = sw.Write(cached.Body) + // Set before call() below writes anything — headers must + // precede the body a miss is about to produce (spec §2). A + // later iteration's own miss overwrites this harmlessly: it + // is never observable until SOME candidate actually writes, + // by which point the loop has already committed to that + // candidate's outcome (THE HARD CONSTRAINT, this function's + // own doc comment). + sw.Header().Set("X-Llmgw-Cache", "miss") + } + + // gatewayAliasKey is (re-)injected on every iteration, AFTER the + // cacheKey computation above: every providerAdapter implementation + // unconditionally deletes it before it ever touches the network + // (its own doc comment, providers.go) — a failed first attempt + // leaves req without it, and the next candidate needs it set + // again to echo the client's own requested alias correctly + // (anthropic/gemini-type adapters read it to build their own + // response envelope). + req[gatewayAliasKey] = requestedModel + + // respWriter is sw, wrapped in a capture tee only when this + // request is cacheable — cacheCaptureWriter buffers everything + // written so a 200 non-stream response can be stored after + // call() returns, without any adapter knowing caching exists + // (adapterCall's contract is unchanged either way). A fresh + // capture per candidate: reusing one across attempts would let a + // candidate that never wrote anything (the common failover case) + // leave a stale buffer behind — capture.status stays 0 either + // way, so this is defensive clarity, not a proven bug fix. + var respWriter http.ResponseWriter = sw + var capture *cacheCaptureWriter + if cacheable { + capture = newCacheCaptureWriter(sw, g.cache.maxBodyBytes) + respWriter = capture + } + + // Feature A (v0.22) attempt recorder, REBOUND every iteration to + // THIS candidate's own provider name (feat/failover's own "never + // misattribute a retried attempt to the first provider" + // requirement) — every upstream attempt call() makes, via + // upstreamJSON's retryPolicy.do (providers.go), reports through + // here. The same closure also feeds g.failoverHealth (failover.go), + // feat/failover's own request-path health signal, with the + // IDENTICAL isTransient||isDeadlineExceeded classification + // limiter.recordProviderAttempt already applies — see + // requestHealthTracker.record's own doc comment for why this must + // stay the same classification, not a wider one. + providerName := cand.providerName + upstreamModel := cand.upstreamModel + ctx := withAttemptRecorder(r.Context(), func(resp *http.Response, attemptErr error) { + g.limiter.recordProviderAttempt(providerName, upstreamModel, resp, attemptErr) + g.failoverHealth.record(providerName, !isTransient(resp, attemptErr) && !isDeadlineExceeded(attemptErr)) + }) + result, callErr := call(cand.adapter, ctx, respWriter, req) + + // Usage is accounted before the error branch below runs, not + // after: every adapter that can fail mid-stream (forwardStream + // in each of the three provider files) still returns whatever + // usage it had already captured alongside the error — a usage + // chunk that arrived just before a dropped connection must + // still be billed, or a client that aborts right after that + // frame arrives could repeat the trick to dodge every + // token/cost budget. A non-streaming failure (providerHTTPError, + // translateError, or a connection failure before any write) + // always carries zero usage by contract, so accounting it here + // is a no-op — account skips every write once both total tokens + // and cost are zero. routes_messages.go's + // callTranslatedMessages/callAnthropicMessagesPassthrough follow + // the identical contract for their own error paths (item 1 fix, + // 2026-08-22 review). This still runs exactly once per + // CANDIDATE, including a candidate that goes on to fail over — + // its own (zero, by contract) usage is billed as a no-op, never + // skipped, so a later successful candidate's real usage is never + // silently doubled with a phantom first entry either. + if callErr == nil && result.total() == 0 { + if streaming { + g.logf("%s: zero usage reported for a streaming response from model %q; accounting the request only", logPrefix, cand.canonical) + } else { + result.prompt = int64(math.Ceil(float64(len(body)) / 4)) + result.estimated = true + } + } + g.limiter.account(scopes, result, unifiedCostMicros(cand.canonical, cand.upstreamModel, result, g.cfg.Pricing)) + if result.estimated { + g.logf("%s: usage for model %q logged as estimated (%d prompt tokens derived from request body size, not the provider's reported usage)", logPrefix, cand.canonical, result.prompt) + } + + // A miss stores the response after everything above has already + // run — accounting must never be skipped or delayed waiting on a + // cache write. Only a genuine upstream 200 is stored (spec §2's + // "on 200 non-stream, tee and SET"): a non-2xx status never + // reaches here as capture.status (forwardJSON/translate error + // paths return *providerHTTPError/*translateError instead of + // writing through respWriter, so callErr is non-nil and + // capture.status stays 0), and callErr == nil is checked + // directly regardless. capture.oversize excludes a response + // cacheCaptureWriter stopped buffering past maxBodyBytes: + // store's own maxBodyBytes check would reject it too, but only + // after being handed a silently truncated body — skip the call + // outright instead of ever constructing a corrupt cache entry. + if cacheable && callErr == nil && capture.status == http.StatusOK && !capture.oversize { + g.cache.store(cacheKeyStr, capture.status, capture.contentType, capture.buf.Bytes(), effectiveTTL(g.cache, grp)) + } + + if callErr == nil { return } - // Set before call() below writes anything — headers must precede - // the body a miss is about to produce (spec §2). - sw.Header().Set("X-Llmgw-Cache", "miss") - } - req[gatewayAliasKey] = requestedModel - - // respWriter is sw, wrapped in a capture tee only when this request - // is cacheable — cacheCaptureWriter buffers everything written so a - // 200 non-stream response can be stored after call() returns, without - // any adapter knowing caching exists (adapterCall's contract is - // unchanged either way). - var respWriter http.ResponseWriter = sw - var capture *cacheCaptureWriter - if cacheable { - capture = newCacheCaptureWriter(sw, g.cache.maxBodyBytes) - respWriter = capture - } - - // Feature A (v0.22): every upstream attempt call() makes — via - // upstreamJSON's retryPolicy.do, providers.go — reports through to - // this recorder, closing over the adapter/model this request already - // resolved to. See attemptRecorder's own doc comment (providers.go). - ctx := withAttemptRecorder(r.Context(), func(resp *http.Response, attemptErr error) { - g.limiter.recordProviderAttempt(adapter.name(), upstreamModel, resp, attemptErr) - }) - result, callErr := call(adapter, ctx, respWriter, req) - - // Usage is accounted before the error branch below runs, not after: - // every adapter that can fail mid-stream (forwardStream in each of the - // three provider files) still returns whatever usage it had already - // captured alongside the error — a usage chunk that arrived just before - // a dropped connection must still be billed, or a client that aborts - // right after that frame arrives could repeat the trick to dodge every - // token/cost budget. A non-streaming failure (providerHTTPError, - // translateError, or a connection failure before any write) always - // carries zero usage by contract, so accounting it here is a no-op — - // account skips every write once both total tokens and cost are zero. - // routes_messages.go's callTranslatedMessages/ - // callAnthropicMessagesPassthrough follow the identical contract for - // their own error paths (item 1 fix, 2026-08-22 review: a translation - // failure AFTER a successful, billed upstream call must still return - // that call's real usage, not usage{}). - if callErr == nil && result.total() == 0 { - if streaming { - g.logf("%s: zero usage reported for a streaming response from model %q; accounting the request only", logPrefix, canonical) - } else { - result.prompt = int64(math.Ceil(float64(len(body)) / 4)) - result.estimated = true + // THE HARD CONSTRAINT: once sw has written anything, status and + // body are committed and can never be retried elsewhere — a + // mid-stream failure (headers, or a partial SSE body, already + // reached the client) always falls straight through to the + // existing, unchanged error classification, exactly as it did + // before this feature existed, regardless of how many candidates + // remain. + if sw.wroteHeader { + g.handleAdapterErrorEnvelope(sw, callErr, providerName, logPrefix, envelope, writeUpstream) + return } - } - g.limiter.account(scopes, result, unifiedCostMicros(canonical, upstreamModel, result, g.cfg.Pricing)) - if result.estimated { - g.logf("%s: usage for model %q logged as estimated (%d prompt tokens derived from request body size, not the provider's reported usage)", logPrefix, canonical, result.prompt) - } - // A miss stores the response after everything above has already run — - // accounting must never be skipped or delayed waiting on a cache - // write. Only a genuine upstream 200 is stored (spec §2's "on 200 - // non-stream, tee and SET"): a non-2xx status never reaches here as - // capture.status (forwardJSON/translate error paths return - // *providerHTTPError/*translateError instead of writing through - // respWriter, so callErr is non-nil and capture.status stays 0), and - // callErr == nil is checked directly regardless. capture.oversize - // excludes a response cacheCaptureWriter stopped buffering past - // maxBodyBytes: store's own maxBodyBytes check would reject it too, - // but only after being handed a silently truncated body — skip the - // call outright instead of ever constructing a corrupt cache entry. - if cacheable && callErr == nil && capture.status == http.StatusOK && !capture.oversize { - g.cache.store(cacheKeyStr, capture.status, capture.contentType, capture.buf.Bytes(), effectiveTTL(g.cache, grp)) - } - - if callErr != nil { - g.handleAdapterErrorEnvelope(sw, callErr, adapter.name(), logPrefix, envelope, writeUpstream) - return + hasNext := i < len(candidates)-1 && failoverEligible(callErr) + if hasNext && isProviderNotFoundError(callErr) { + // Loud, non-suppressed log line (brief: "Failover after a 404 + // must log loudly ... a 404 usually means a real + // configuration mistake, and silently succeeding elsewhere + // would hide it") — deliberately g.errorf, not g.logf, and + // deliberately its own line rather than folded into the + // generic "failing over" line above the next iteration + // already prints. + g.errorf("%s: provider %q returned 404 for model %q; failing over to %q — this usually means a real provider/model configuration mistake, not a transient outage", logPrefix, providerName, requestedModel, candidates[i+1].providerName) + } + if !hasNext { + g.handleAdapterErrorEnvelope(sw, callErr, providerName, logPrefix, envelope, writeUpstream) + return + } + // Falls through to the next candidate. } } diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index af663a2..f9b5efd 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -346,6 +346,31 @@ func run() error { })) defer brkUpstream.Close() + // failoverAUpstream/failoverBUpstream back exerciseFailover (feat/ + // failover, below): two providers configured with the IDENTICAL bare + // model id (failoverModelID) — the exact shape registry.go's bareWinner + // collision handles, and the shape production logs show for whisper-1/ + // tts-1/tts-1-hd/the embeddings models across openai/openai-audio/ + // copilot. A always answers 500; B always answers 200. This proves the + // failover candidate loop (runMeteredCall, routes_unified.go) falls + // through and serves correctly under the REAL interpreter, not merely + // compiled — this package's own doc comment is exactly why that + // distinction matters on this codebase. + failoverAUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"message":"failover-a is down","type":"server_error"}}`)) + })) + defer failoverAUpstream.Close() + failoverBHits := new(int64) + failoverBUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(failoverBHits, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":"c-failover","object":"chat.completion","model":"` + failoverModelID + `","choices":[{"index":0,"message":{"role":"assistant","content":"served by failover-b"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}`)) + })) + defer failoverBUpstream.Close() + attemptAccountingOverride := `{"breaker":{"failureThreshold":2,"openDuration":"3s","maxOpenDuration":"6s"},` + `"providers":{"openai":{"type":"openai","baseUrl":"` + upstream.URL + `","apiKey":"sk-up","models":["` + testDataWantModel + `","` + builtinLookupModelID + `"]},` + `"brk":{"type":"openai","baseUrl":"` + brkUpstream.URL + `","apiKey":"sk-up","discovery":true,"discoveryInterval":"1ms"},"` + @@ -354,7 +379,9 @@ func run() error { // (exerciseMessagesRoute, below): a real anthropic-type provider, // interpreted end to end through // callAnthropicMessagesPassthrough. - `"anthropic":{"type":"anthropic","baseUrl":"` + anthropicProbeUpstream.URL + `","apiKey":"sk-anth","models":["claude-test"]}},` + // #nosec G101 -- test fixture literal, not a real credential + `"anthropic":{"type":"anthropic","baseUrl":"` + anthropicProbeUpstream.URL + `","apiKey":"sk-anth","models":["claude-test"]},` + // #nosec G101 -- test fixture literal, not a real credential + `"failover-a":{"type":"openai","baseUrl":"` + failoverAUpstream.URL + `","apiKey":"sk-up","models":["` + failoverModelID + `"]},` + + `"failover-b":{"type":"openai","baseUrl":"` + failoverBUpstream.URL + `","apiKey":"sk-up","models":["` + failoverModelID + `"]}},` + // modelMeta (feature v0.23): a config-override entry for // testDataWantModel, so exerciseHandler's GET /v1/models // assertion below proves resolveModelMeta's config-override @@ -405,7 +432,7 @@ func run() error { return fmt.Errorf("%s.New's returned value does not implement http.Handler", pkgName) } - if err := exerciseHandler(handler, builtinLookupContextTokens); err != nil { + if err := exerciseHandler(handler, builtinLookupContextTokens, failoverBHits); err != nil { return err } // exerciseBreaker (feat/provider-health, adversarial-review round 2): @@ -429,6 +456,11 @@ func run() error { // the config-override path testDataWantModel already exercises. const builtinLookupModelID = "gpt-4o" +// failoverModelID is the bare model id both "failover-a" and "failover-b" +// (run(), above) are configured with — the run()-owned upstream servers +// exerciseFailover (below) drives. +const failoverModelID = "failover-shared" + // mcpProbeServerName is the federated MCP server run() configures against // mcpProbeUpstream, and mcpProbeToolName is a tool id carrying its // "_" prefix so resolveFederatedTool routes POST /mcp's @@ -634,8 +666,8 @@ func exerciseBreaker(handler http.Handler, hits, healthy *int64) error { // for real under Yaegi, not merely that the package's imports resolve. // builtinContextTokens is builtinLookupModelID's own real, extracted // builtinModelMetaTable value (SHOULD-6) — see that constant's own doc -// comment. -func exerciseHandler(handler http.Handler, builtinContextTokens int) error { +// comment. failoverBHits backs exerciseFailover's own call, below. +func exerciseHandler(handler http.Handler, builtinContextTokens int, failoverBHits *int64) error { authedReq := httptest.NewRequest(http.MethodGet, "/v1/models", nil) authedReq.Header.Set("Authorization", "Bearer "+testDataUserAPIKey) authedRec := httptest.NewRecorder() @@ -702,6 +734,10 @@ func exerciseHandler(handler http.Handler, builtinContextTokens int) error { return err } + if err := exerciseFailover(handler, failoverBHits); err != nil { + return err + } + if err := exerciseFederatedTooLarge(handler); err != nil { return err } @@ -1013,6 +1049,38 @@ func exerciseFederatedTooLarge(handler http.Handler) error { return nil } +// exerciseFailover drives one real POST /v1/chat/completions against the +// interpreted handler for failoverModelID — a bare model id both +// "failover-a" (always 500s) and "failover-b" (always 200s) are +// configured with — proving the failover candidate loop (runMeteredCall, +// routes_unified.go/failover.go) falls through a genuine provider +// failure and serves the second provider's response under the REAL +// interpreter. Compiled tests already prove this (failover_test.go); this +// harness exists because compiled tests have repeatedly passed on this +// codebase while the interpreted shape failed (this package's own doc +// comment). +func exerciseFailover(handler http.Handler, bHits *int64) error { + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader( + `{"model":"`+failoverModelID+`","messages":[{"role":"user","content":"hi"}]}`, + )) + req.Header.Set("Authorization", "Bearer "+testDataUserAPIKey) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + return fmt.Errorf("POST /v1/chat/completions (feat/failover harness): status = %d, want 200 (failover-a's 500 must fall through to failover-b), body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "served by failover-b") { + return fmt.Errorf("POST /v1/chat/completions (feat/failover harness): body does not contain failover-b's own content, want the SECOND provider's response: %s", rec.Body.String()) + } + if atomic.LoadInt64(bHits) != 1 { + return fmt.Errorf("feat/failover harness: failover-b upstream hit count = %d, want exactly 1", atomic.LoadInt64(bHits)) + } + fmt.Println("yaegi-check: failover fell through from failover-a to failover-b and served its response") + return nil +} + func exerciseAttemptAccounting(handler http.Handler) error { chatReq := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader( `{"model":"`+testDataWantModel+`","messages":[{"role":"user","content":"hi"}]}`, From d45125ee076dfdde85af2eb895cfa642273dd566 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 01:18:58 +0100 Subject: [PATCH 2/3] fix(failover): default off, add cost guard, fix adversarial-review findings Flip FailoverConfig.Enabled to default false. A version upgrade with no config change must preserve prior behaviour, and failover changes which provider serves a request. Update tests, README, and the yaegi-check probe to opt in explicitly. Add a mandatory cost guard. A candidate is skipped unless its input AND output cost per token are each no greater than the primary candidate's, resolved through the same canonical-then-bare pricing path the real billing code uses. Unknown pricing is never treated as free: a known primary against an unknown candidate skips, an unknown primary against a known non-zero candidate skips, everything else that is equal is allowed. Not configurable. Fix four blocking findings from adversarial review of commit 5d62479. F1: openai-type adapters mutate the request map in place to force stream usage reporting. Reusing one map across candidates let a later candidate read back an earlier candidate's own injection and leak a usage-only SSE chunk to a client that never asked for it. Every candidate now attempts against a fresh copy of the original request. F2: a streaming candidate that fails before writing anything still stages SSE-only response headers. Those headers now get cleared before every candidate attempt, so they cannot survive into a later candidate's plain JSON response. F3: the request-health gate used the retry-shaped isTransient classification, which treats a non-429 4xx as a successful outcome. A provider with a dead API key was never skipped and was retried forever. The gate now reuses failoverEligible's own classification instead. F4: a shared request context that an earlier candidate exhausted was attributed to whichever candidate happened to run next, opening a healthy provider's breaker for a timeout that was not its fault. A candidate is no longer attempted once the shared context is already done; the loop stops and reports the last real error instead. Also fix three should-fix findings from the same review. F5: a failure arriving while the request-health breaker is already open but not yet past its cooldown window is now a no-op, instead of resetting backoff and sliding the open window forward on every such failure. F6: the request-health gate now records at most one outcome per candidate per logical request, not one per same-provider retry attempt, so retry.attempts alone can no longer open the breaker on a single request. F7: every candidate's cache entry is now checked before any candidate is attempted, so a cached failover response no longer costs a round trip to a still-broken primary first. F10: the generic "failing over" log line is now rate-limited to once per 30 seconds during a sustained outage, reusing the existing logGate type. The 404 loud-log line stays unthrottled, as ruled. F9 (optional): skip building the failover candidate pool entirely when failover is disabled. Correct an overstated test comment on TestHandleChat_Failover_DiscoveryUnhealthy_StillTried: it proves alpha is still tried, not that ordering places it second. Document, but do not fix here, that this plugin's HTTP client sets no per-attempt timeout, so a provider that accepts a connection and then hangs will never trigger failover. This is a pre-existing property of the shared HTTP client, not specific to failover. Deferred: F8 (surfacing request-health state on the admin dashboard) touches admin.go, which a sibling branch is also editing concurrently; left for a follow-up to avoid a merge conflict in a file this change does not otherwise need. Every fix above is proven by a dedicated regression test, and each was verified by reverting the fix in place and confirming the corresponding test fails before restoring it. --- README.md | 67 +++- failover.go | 245 +++++++++++-- failover_test.go | 744 +++++++++++++++++++++++++++++++++++++- llmgateway.go | 13 +- routes_unified.go | 224 +++++++++--- tools/yaegi-check/main.go | 7 +- 6 files changed, 1174 insertions(+), 126 deletions(-) diff --git a/README.md b/README.md index 57b07dc..0560e59 100644 --- a/README.md +++ b/README.md @@ -193,7 +193,7 @@ config accepts them as YAML, which decodes to the same JSON shape. | `users` | `UsersConfig` | — | Inline and/or file-backed API-key holders. With none configured, every request is unauthenticated and gets 401. | | `redis` | `RedisConfig` | — | Distributed limit-counter backend. Omitted means in-process counters only (per-replica, approximate across multiple Traefik instances). | | `retry` | `RetryConfig` | `{}` (disabled) | Same-provider retry for transient upstream failures — see [Retry](#retry). Omitted or `enabled: false` means no retry: every request makes exactly one upstream attempt, byte-identical to a gateway built before this field existed. | -| `failover` | `FailoverConfig` | `{}` (enabled) | Cross-provider failover for a bare model id more than one configured provider serves — see [Failover](#failover). `enabled: false` turns it off; a deployment with no overlapping providers behaves exactly as before regardless of this setting. | +| `failover` | `FailoverConfig` | `{}` (disabled) | Cross-provider failover for a bare model id more than one configured provider serves — see [Failover](#failover). Omitted or `enabled: false` means no failover, byte-identical to a gateway built before this field existed. `enabled: true` opts in; a deployment with no overlapping providers behaves exactly as before regardless of this setting either way. | | `cache` | `CacheConfig` | `{}` (disabled) | Opt-in Redis-backed response cache for unified non-streaming chat/embeddings — see [Caching](#caching). Omitted or `enabled: false` means no caching, byte-identical to a gateway built before this field existed. | | `breaker` | `BreakerConfig` | `{}` (every default) | Per-provider discovery circuit breaker — see [Provider health (discovery circuit breaker)](#provider-health-discovery-circuit-breaker). Every field is individually zero-means-default; a provider whose discovery never fails is unaffected regardless of what this block contains. | | `admin` | `*AdminConfig` | `nil` (disabled) | Read-only admin dashboard — see [Admin](#admin). `nil` or `enabled: false` means the `/admin*` routes are not registered at all. | @@ -851,10 +851,14 @@ below for the separate, cross-provider mechanism. Cross-provider failover for a bare model id more than one configured provider serves — for example `whisper-1` configured on both `openai` and -`openai-audio`. On by default (`failover.enabled: true` unless set -otherwise); a deployment with no overlapping providers, or with a single -provider, behaves exactly as before regardless of this setting, since -there is never a second candidate to try. +`openai-audio`. **Off by default** (`failover.enabled: false` unless set +otherwise): a version upgrade with no config change preserves prior +behavior exactly, since failover changes which provider serves a request +and can shift cost attribution or mask a broken provider key behind an +apparent success. Set `failover.enabled: true` to opt in. A deployment +with no overlapping providers, or with a single provider, behaves +exactly as before regardless of this setting either way, since there is +never a second candidate to try. - **Scope**: `POST /v1/chat/completions`, `POST /v1/embeddings`, and `POST /v1/messages` only. The image/audio endpoints @@ -871,9 +875,30 @@ there is never a second candidate to try. - **What falls through**: everything except HTTP 400 — connection failures, timeouts, HTTP 429/401/403/404, and any 5xx. A 400 never falls through: the request is malformed and every provider would reject - it identically. A failover past a 404 is logged loudly, naming both - providers and the model — a 404 usually means a real configuration - mistake, not a transient outage. + it identically. A failover past a 404 is logged loudly, always, naming + both providers and the model — a 404 usually means a real configuration + mistake, not a transient outage. The routine "failing over from X to Y" + line is rate-limited to once per 30s during a sustained outage (with a + suppressed-count note on the next line), so it cannot itself become a + blocking synchronous write on every request; the 404 line is exempt + from that limit. +- **Cost guard (not optional)**: failover never routes to a candidate + priced higher than the provider it is failing over FROM (the first + candidate actually attempted, never the cheapest in the list), checked + on BOTH input and output cost per token — a candidate cheaper on input + but dearer on output is still rejected. Pricing is resolved exactly the + way real billing resolves it: the canonical `provider/model` id in + `pricing` first, falling back to the bare model id, so a per-provider + `pricing` override correctly makes the identical model id cost + different amounts on different providers. Unknown pricing is never + treated as free: if the primary's price is known and a candidate's is + not, the candidate is skipped (an unknown price cannot be proven + cheaper); if the primary's price is unknown and a candidate's is known + and non-zero, the candidate is skipped too (moving from unbilled to + billed is itself a spend increase); both unknown, or both an explicit + known-zero price, are treated as equal and allowed. There is no config + flag for this — it applies unconditionally whenever failover is + enabled. - **Never mid-stream**: failover only happens while nothing has reached the client yet. Once the first byte of a response is written, status and body are committed and the request is never retried elsewhere. @@ -882,9 +907,12 @@ there is never a second candidate to try. whose `/v1/models` is unhealthy is tried later, never skipped outright, since plenty of providers serve chat completions fine while their model listing is broken. A separate, in-memory, per-pod request-path health - signal (not exposed to config) is the actual routing gate: a provider - that has failed several consecutive live requests is skipped without - being called until it recovers. + signal (not yet exposed on the admin dashboard) is the actual routing + gate: a provider is skipped without being called once it has failed + three consecutive LOGICAL requests (same-provider retries all count as + one outcome, not one each) — everything except HTTP 400 counts as a + failure for this gate, including 401/403/404, so a provider with a dead + API key is eventually routed around instead of retried forever. - **Accounting**: a failover chain still counts as exactly one client request against `requestsPerMinute`/`requestsPerDay` — never once per attempt. Per-provider attempt/failure counters (the admin dashboard's @@ -892,10 +920,25 @@ there is never a second candidate to try. actually made it. - **Caching**: a cacheable response produced by a failover provider is stored under that provider's own cache key, never the first candidate's. + Every candidate's cache entry is checked before any of them is actually + attempted, so a cached response from a failover provider is served + without a wasted round trip to a still-broken primary. - **Configurable**: `failover.maxAttempts` caps how many different providers one request will try in total, primary included (default 3, maximum 10) — distinct from `retry.attempts`, which retries the SAME - provider. `failover.enabled: false` turns the whole mechanism off. + provider. `failover.enabled: false` (the default) turns the whole + mechanism off. +- **Known limitation — no per-attempt timeout**: `newAdapterHTTPClient` + sets no client timeout and no response-header timeout; the only bound + on an upstream call is the incoming request's own context, shared + identically across every candidate. A provider that accepts the TCP + connection and then simply never responds will hang the request + indefinitely and never trigger failover — only a failure fast enough to + leave time on that shared context (a dial failure, a fast non-2xx + response, or the context's own deadline finally expiring) does. This is + a pre-existing property of the plugin's HTTP client, not specific to + failover, and applies to every route, not only this one; it is not + addressed in this change. ## Caching diff --git a/failover.go b/failover.go index c399817..08c9498 100644 --- a/failover.go +++ b/failover.go @@ -23,6 +23,15 @@ import ( // metered-call path both /v1/chat/completions and /v1/messages already go // through. // +// Off by default (FailoverConfig.Enabled defaults to false — coordinator +// ruling, overriding this feature's original brief: new behavior that can +// change which provider serves a request goes behind a flag defaulting +// off). An operator who opts in also gets a non-optional cost guard +// (candidateCostAllowed, below): failover never routes to a candidate +// priced higher than the provider it is failing over from, on either +// input or output cost per token — there is no config knob to disable +// just that part. +// // SCOPE: routes_media.go's three media routes (images/generations, // audio/speech, audio/transcriptions) are deliberately NOT wired into // failover in this change, even though two of the brief's own named @@ -38,16 +47,17 @@ import ( // scoped follow-up; see this branch's own report for the explicit // call-out. -// FailoverConfig configures feat/failover. Enabled is a *bool, mirroring -// ProviderConfig.Passthrough's own nil-means-default-true convention -// (providers.go): nil or true (the default) turns an overlapping -// multi-provider deployment's already-existing redundancy into a hot -// standby with NO config change — the entire point of this feature (see -// this file's own package doc and the brief's "Why" section). Set false -// to opt out entirely. A single-provider deployment, or one where no two -// providers ever serve the identical bare model id, behaves exactly as -// before regardless of this setting: there is never more than one -// candidate to try (orderedFailoverCandidates' own doc comment). +// FailoverConfig configures feat/failover. Enabled is a *bool; nil or +// false (the default) means OFF — coordinator ruling, overriding this +// feature's original brief: a version upgrade with no config change must +// preserve prior behavior, and new behavior that can change which +// provider serves a request (shifting cost attribution, and able to mask +// a broken provider key behind an apparent success) goes behind a flag +// that defaults off, not on. Set Enabled: true to opt in. A +// single-provider deployment, or one where no two providers ever serve +// the identical bare model id, behaves exactly as before regardless of +// this setting either way: there is never more than one candidate to try +// (orderedFailoverCandidates' own doc comment). type FailoverConfig struct { Enabled *bool `json:"enabled,omitempty"` // MaxAttempts caps how many DIFFERENT PROVIDERS runMeteredCall tries @@ -78,9 +88,13 @@ type failoverConfig struct { } // validateFailoverConfig validates fc and returns the resolved -// failoverConfig newGateway attaches to the Gateway. +// failoverConfig newGateway attaches to the Gateway. enabled defaults to +// false (coordinator ruling, FailoverConfig's own doc comment) — every +// config that predates this field, and every config that never sets +// failover.enabled at all, keeps behaving exactly as before this feature +// existed. func validateFailoverConfig(fc FailoverConfig) (failoverConfig, error) { - enabled := true + enabled := false if fc.Enabled != nil { enabled = *fc.Enabled } @@ -154,6 +168,23 @@ func (m *modelRegistry) resolveWithCandidates(id string, grp *group) (resolveCan return primary, m.failoverCandidates(bareID, primary.providerName, grp), nil } +// resolvePrimaryOnly resolves id exactly like resolveWithCandidates' +// primary return (identical winner, identical error) WITHOUT ever +// scanning for or building failover candidates — the short-circuit +// runMeteredCall (routes_unified.go) takes when g.failover.enabled is +// false, so a deployment that never opted into failover pays no extra +// cost for a pool it will never use (measured: 74ns/1 alloc versus +// 521ns/13 allocs at 8 providers x 200 models for the full +// resolveWithCandidates scan — irrelevant next to a network round trip, +// but free to avoid). +func (m *modelRegistry) resolvePrimaryOnly(id string, grp *group) (resolveCandidate, error) { + adapter, upstreamModel, canonical, err := m.resolve(id, grp) + if err != nil { + return resolveCandidate{}, err + } + return resolveCandidate{adapter: adapter, providerName: adapter.name(), upstreamModel: upstreamModel, canonical: canonical}, nil +} + // failoverCandidates returns every configured provider other than // primaryProvider whose known model set contains bareID and that grp is // authorized to use, in modelRegistry.providerNames (sorted) order — the @@ -220,7 +251,14 @@ func (m *modelRegistry) failoverCandidates(bareID, primaryProvider string, grp * // half-open one (a stable partition, not a full sort — relative // order within each bucket is preserved), never dropped. // -// Finally the list is capped at g.failover.maxAttempts entries. +// This does NOT apply the cost guard or the maxAttempts cap — see +// runMeteredCall's own call sequence (routes_unified.go): candidates[0] +// coming out of THIS function is what filterCandidatesByCost (below) +// treats as "the primary" for pricing-guard purposes, so cost filtering +// must run against the health/discovery-ordered result, not the raw +// resolveWithCandidates order; and the maxAttempts cap runs LAST, after +// cost filtering, so a cap never re-admits an expensive candidate purely +// because a cheaper one ahead of it was removed on cost. func (g *Gateway) orderedFailoverCandidates(primary resolveCandidate, extra []resolveCandidate) []resolveCandidate { if !g.failover.enabled || len(extra) == 0 { return []resolveCandidate{primary} @@ -249,12 +287,112 @@ func (g *Gateway) orderedFailoverCandidates(primary resolveCandidate, extra []re deprioritized = append(deprioritized, c) } } - ordered = append(ordered, deprioritized...) + return append(ordered, deprioritized...) +} - if max := g.failover.maxAttempts; max > 0 && len(ordered) > max { - ordered = ordered[:max] +// resolveUnifiedPricing resolves (canonical, bare)'s price using the +// IDENTICAL resolution unifiedCostMicros itself uses (routes_unified.go): +// canonical id first, falling back to the bare upstream model id only +// when canonical has no configured price at all. ok is lookupPricing's +// own known/unknown distinction (pricing.go) — false means neither id +// resolved to a price, exactly the case where unifiedCostMicros would +// warn and bill 0, which candidateCostAllowed (below) must NEVER treat +// as an actual known-zero price. +func resolveUnifiedPricing(canonical, bare string, overrides map[string]*ModelPricing) (ModelPricing, bool) { + if p, ok := lookupPricing(canonical, overrides); ok { + return p, true } - return ordered + return lookupPricing(bare, overrides) +} + +// candidateCostAllowed reports whether candidate is allowed to serve as a +// failover destination for primary, per the operator's binding ruling: +// failover must never route to a candidate more expensive than the +// provider it is failing over FROM, checked on BOTH the input and the +// output per-token price — a candidate cheaper on input but dearer on +// output can still cost more on a completion-heavy request, so a single +// blended number is not sufficient. This is NOT configurable (ruling: +// "this behaviour is ON and not optional"). +// +// Four cases, all by ruling, not inferred: +// +// - both known: allowed only when candidate's input AND output price +// are each <= primary's. Equal is allowed either way. +// - primary known, candidate UNKNOWN: never allowed — an unknown price +// cannot be proven not-more-expensive. +// - primary UNKNOWN, candidate known and non-zero: never allowed — +// moving from unbilled to billed is itself a spend increase. +// - primary UNKNOWN, candidate known and EXACTLY zero (both +// dimensions), or both unknown: allowed — they are equal (a known +// zero can never be more expensive than an unknown amount, and two +// unknowns are trivially equal to each other). +// +// Unknown pricing is never treated as zero here, unlike costMicros' own +// billing behavior (pricing.go) — the gateway RECORDING zero for an +// unpriced model is an accounting shortcut, not proof the provider +// invoices zero, and this guard exists specifically to protect real +// money, not the gateway's own displayed number. +func candidateCostAllowed(primary ModelPricing, primaryKnown bool, candidate ModelPricing, candidateKnown bool) bool { + switch { + case primaryKnown && candidateKnown: + return candidate.InputPerM <= primary.InputPerM && candidate.OutputPerM <= primary.OutputPerM + case primaryKnown && !candidateKnown: + return false + case !primaryKnown && candidateKnown: + return candidate.InputPerM == 0 && candidate.OutputPerM == 0 + default: // both unknown + return true + } +} + +// filterCandidatesByCost drops every candidate whose resolved price +// candidateCostAllowed rejects against candidates[0] — the FIRST +// candidate actually attempted, never the cheapest in the list, per the +// operator's own definition of "primary" for this rule. Pricing comes +// from resolveUnifiedPricing against g.cfg.Pricing, the SAME overrides +// map unifiedCostMicros itself resolves against, so this compares +// against what would actually be billed. requestedModel names the +// client's own request in the log line a skip always produces — an +// operator-visible g.warnf, unthrottled, naming both providers, the +// model, and both resolved prices: a silently shortened candidate list +// would look identical to having no redundancy at all, which the +// operator must be able to tell apart from a cost-guard skip. +func (g *Gateway) filterCandidatesByCost(candidates []resolveCandidate, requestedModel string) []resolveCandidate { + if len(candidates) < 2 { + return candidates + } + primary := candidates[0] + primaryPrice, primaryKnown := resolveUnifiedPricing(primary.canonical, primary.upstreamModel, g.cfg.Pricing) + + out := make([]resolveCandidate, 1, len(candidates)) + out[0] = primary + for _, cand := range candidates[1:] { + candPrice, candKnown := resolveUnifiedPricing(cand.canonical, cand.upstreamModel, g.cfg.Pricing) + if candidateCostAllowed(primaryPrice, primaryKnown, candPrice, candKnown) { + out = append(out, cand) + continue + } + g.warnf("failover: skipping candidate provider %q for model %q: would cost more than primary provider %q (primary input=%.4f/M output=%.4f/M usd known=%v; candidate input=%.4f/M output=%.4f/M usd known=%v)", + cand.providerName, requestedModel, primary.providerName, + primaryPrice.InputPerM, primaryPrice.OutputPerM, primaryKnown, + candPrice.InputPerM, candPrice.OutputPerM, candKnown) + } + return out +} + +// capFailoverCandidates truncates candidates to at most max entries, +// applied as the LAST step of runMeteredCall's own candidate-building +// sequence (routes_unified.go) — after health/discovery ordering and +// after cost filtering, so the cap can never re-admit a candidate cost +// filtering already removed, and never discards a cheaper candidate that +// cost filtering kept purely because a removed, more expensive one used +// to sit ahead of it in the raw list. max <= 0 is a no-op (defensive; +// validateFailoverConfig never produces one). +func capFailoverCandidates(candidates []resolveCandidate, max int) []resolveCandidate { + if max > 0 && len(candidates) > max { + return candidates[:max] + } + return candidates } // failoverEligible reports whether callErr — runMeteredCall's own @@ -360,10 +498,11 @@ type requestHealthState struct { // request-path health signal — deliberately separate from registry.go's // discovery circuit breaker (modelRegistry.discoveryHealthy's own doc // comment forbids widening that one to cover request-path traffic). Fed -// by the SAME failure classification limiter.recordProviderAttempt uses -// (isTransient || isDeadlineExceeded — limits.go/retry.go), via record's -// own callers in routes_unified.go, but keeps its own independent state -// per provider name. +// by failoverEligible's own classification (record's own doc comment +// explains why, not limiter.recordProviderAttempt's narrower isTransient/ +// isDeadlineExceeded one), once per candidate per logical request, via +// record's own callers in routes_unified.go, but keeps its own +// independent state per provider name. // // Every method is nil-receiver-safe (both report "healthy"/no-op on a // nil *requestHealthTracker), matching this package's own nil-means- @@ -397,25 +536,50 @@ func (t *requestHealthTracker) stateFor(provider string) *requestHealthState { return st } -// record accounts one upstream request-path attempt's outcome for -// provider: success=false must be EXACTLY isTransient(resp, err) || -// isDeadlineExceeded(err) — the identical classification -// limiter.recordProviderAttempt applies (limits.go/retry.go) — never a -// wider one; routes_unified.go's attemptRecorder closure computes it -// once and feeds both. +// record accounts ONE LOGICAL REQUEST's outcome for provider — called +// exactly once per candidate per client request, after call() has +// already exhausted whatever same-provider retries retryPolicy.do +// performed internally (routes_unified.go's runMeteredCall), never once +// per raw upstream attempt. success=false must be +// !failoverEligible(callErr) (routes_unified.go/failover.go's own +// classification of the FINAL adapter-level error) — adversarial-review +// ruling (F3/F6): an EARLIER version fed this from isTransient(resp, +// err) || isDeadlineExceeded(err), limits.go/retry.go's own +// retry-shaped classification, once per RAW attempt inside +// retryPolicy.do. That was wrong on two counts. isTransient treats a +// non-429 4xx as "the provider answered correctly to a request it did +// not like" — correct for deciding whether retrying the SAME provider +// is worth it, wrong for deciding whether the POOL should route around +// it: a provider whose API key is dead answers 401 to every request and +// must eventually be skipped, not retried forever, which is exactly the +// "masking a broken provider key" risk this feature exists to reduce. +// And recording per raw attempt let ONE client request with +// retry.enabled (attempts: 3) report FOUR failures against a threshold +// documented as three REQUESTS. failoverEligible already classifies +// "everything except HTTP 400" as a failure, matching what this gate +// needs with no separate function required. // // requestBreakerFailureThreshold consecutive failures opens the breaker // for requestBreakerOpenDuration. While open, healthy (below) reports // unhealthy until that window passes; once it has, the NEXT record call // for this provider — success or failure — is what healthy already -// exposed as a post-cooldown probe attempt (probed, below): success -// closes the breaker outright, failure re-opens it with a doubled -// backoff (capped at requestBreakerMaxOpenDuration). This is the same -// probe-then-escalate-or-close shape providerState's discovery breaker -// uses (registry.go's recordHealthLocked/openBreakerLocked), -// reimplemented independently rather than shared — the two track -// different outcome streams, and discoveryHealthy's own doc comment -// forbids widening it to cover this one. +// exposed as a post-cooldown probe attempt: success closes the breaker +// outright, failure re-opens it with a doubled backoff (capped at +// requestBreakerMaxOpenDuration). This is the same probe-then-escalate- +// or-close shape providerState's discovery breaker uses (registry.go's +// recordHealthLocked/openBreakerLocked), reimplemented independently +// rather than shared — the two track different outcome streams, and +// discoveryHealthy's own doc comment forbids widening it to cover this +// one. +// +// A failure arriving while ALREADY open and still within its cooldown +// window is a deliberate no-op (adversarial-review fix, F5): an earlier +// version fell through to the closed-state consecutiveFailures++ branch +// for this case too, which reset backoff to the flat base duration and +// slid openUntil forward on every such failure instead of leaving the +// existing window alone — doubling never actually happened outside the +// narrow post-cooldown probe path the doc comment claimed was the only +// place it did. // // Deliberately no single-flight guard on the probe window (unlike // registry.go's tryBeginRefresh inFlight flag for its own, much lower- @@ -434,8 +598,6 @@ func (t *requestHealthTracker) record(provider string, success bool) { defer st.mu.Unlock() now := t.nowFn() - probed := st.open && !now.Before(st.openUntil) - if success { st.open = false st.consecutiveFailures = 0 @@ -444,7 +606,14 @@ func (t *requestHealthTracker) record(provider string, success bool) { return } - if probed { + // Already open, cooldown not yet elapsed: this failure changes + // nothing (F5 fix, above). + if st.open && now.Before(st.openUntil) { + return + } + + if st.open { + // Cooldown elapsed: this failure IS the probe outcome. Escalate. if st.backoff <= 0 { st.backoff = requestBreakerOpenDuration } else { diff --git a/failover_test.go b/failover_test.go index d74024b..cb1e25e 100644 --- a/failover_test.go +++ b/failover_test.go @@ -262,6 +262,48 @@ func TestRequestHealthTracker_FailedProbe_ReopensWithDoubledBackoff(t *testing.T assert.True(t, tr.healthy("p"), "must recover once the doubled window has fully elapsed") } +// TestRequestHealthTracker_FailureWhileOpenBeforeCooldown_IsANoOp is the +// direct regression test for adversarial-review finding F5: a failure +// arriving while the breaker is ALREADY open and its cooldown has not +// yet elapsed must change nothing — not the backoff, not openUntil. An +// earlier version fell through to the closed-state consecutiveFailures++ +// branch for this case, which reset backoff to the flat base duration +// and slid openUntil forward on every such failure, so doubling never +// actually happened outside the narrow post-cooldown probe path. +func TestRequestHealthTracker_FailureWhileOpenBeforeCooldown_IsANoOp(t *testing.T) { + t.Parallel() + tr := newRequestHealthTracker() + now := time.Now() + tr.nowFn = func() time.Time { return now } + + for i := 0; i < requestBreakerFailureThreshold; i++ { + tr.record("p", false) + } + st := tr.stateFor("p") + st.mu.Lock() + openUntilAfterTrip := st.openUntil + backoffAfterTrip := st.backoff + st.mu.Unlock() + require.Equal(t, requestBreakerOpenDuration, backoffAfterTrip) + + // Well before the cooldown elapses: several more failures arrive + // (e.g. other in-flight requests against the same dead provider). + now = now.Add(requestBreakerOpenDuration / 2) + for i := 0; i < 5; i++ { + require.False(t, tr.healthy("p")) + tr.record("p", false) + } + + st.mu.Lock() + openUntilAfterExtraFailures := st.openUntil + backoffAfterExtraFailures := st.backoff + failuresAfterExtra := st.consecutiveFailures + st.mu.Unlock() + assert.Equal(t, openUntilAfterTrip, openUntilAfterExtraFailures, "openUntil must not slide forward from failures arriving before the cooldown elapses") + assert.Equal(t, backoffAfterTrip, backoffAfterExtraFailures, "backoff must stay at the base duration, not reset by every failure while already open") + assert.Equal(t, requestBreakerFailureThreshold, failuresAfterExtra, "consecutiveFailures must not keep growing once the breaker is already open") +} + func TestRequestHealthTracker_BackoffCapsAtMax(t *testing.T) { t.Parallel() tr := newRequestHealthTracker() @@ -364,10 +406,18 @@ func TestValidateFailoverConfig_Defaults(t *testing.T) { t.Parallel() fc, err := validateFailoverConfig(FailoverConfig{}) require.NoError(t, err) - assert.True(t, fc.enabled, "nil Enabled must default to true — failover is a hot standby with no config change") + assert.False(t, fc.enabled, "nil Enabled must default to false — coordinator ruling: new behavior that can change which provider serves a request goes behind a flag defaulting off") assert.Equal(t, defaultFailoverMaxAttempts, fc.maxAttempts) } +func TestValidateFailoverConfig_ExplicitEnable(t *testing.T) { + t.Parallel() + yes := true + fc, err := validateFailoverConfig(FailoverConfig{Enabled: &yes}) + require.NoError(t, err) + assert.True(t, fc.enabled) +} + func TestValidateFailoverConfig_ExplicitDisable(t *testing.T) { t.Parallel() no := false @@ -402,9 +452,10 @@ func testGatewayForCandidates(t *testing.T, providerNames []string, fc failoverC adapters[name] = newFakeAdapter(name) providers[name] = &ProviderConfig{Models: []string{"shared"}} } - reg, err := newModelRegistry(adapters, &Config{Providers: providers}, func(string, ...any) {}) + cfg := &Config{Providers: providers} + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) require.NoError(t, err) - return &Gateway{registry: reg, failoverHealth: newRequestHealthTracker(), failover: fc} + return &Gateway{registry: reg, failoverHealth: newRequestHealthTracker(), failover: fc, cfg: cfg, name: "test"} } func candNames(cands []resolveCandidate) []string { @@ -475,14 +526,127 @@ func TestOrderedFailoverCandidates_DiscoveryUnhealthy_DeprioritizedNotDropped(t assert.Equal(t, []string{"beta", "alpha"}, candNames(got), "discovery-unhealthy must be deprioritized (moved after), never dropped from the list") } -func TestOrderedFailoverCandidates_MaxAttemptsCapsList(t *testing.T) { +// TestOrderedFailoverCandidates_DoesNotCap proves orderedFailoverCandidates +// itself no longer applies maxAttempts — capFailoverCandidates does, as the +// LAST step of runMeteredCall's own sequence, after cost filtering. +func TestOrderedFailoverCandidates_DoesNotCap(t *testing.T) { t.Parallel() g := testGatewayForCandidates(t, []string{"alpha", "beta", "gamma"}, failoverConfig{enabled: true, maxAttempts: 2}) primary := resolveCandidate{providerName: "alpha"} extra := []resolveCandidate{{providerName: "beta"}, {providerName: "gamma"}} got := g.orderedFailoverCandidates(primary, extra) - assert.Len(t, got, 2, "maxAttempts must cap the total candidate list, primary included") - assert.Equal(t, []string{"alpha", "beta"}, candNames(got)) + assert.Equal(t, []string{"alpha", "beta", "gamma"}, candNames(got), "orderedFailoverCandidates must return the full health/discovery-ordered list uncapped") +} + +func TestCapFailoverCandidates(t *testing.T) { + t.Parallel() + all := []resolveCandidate{{providerName: "alpha"}, {providerName: "beta"}, {providerName: "gamma"}} + + got := capFailoverCandidates(all, 2) + assert.Equal(t, []string{"alpha", "beta"}, candNames(got), "maxAttempts must cap the total candidate list, primary included") + + got = capFailoverCandidates(all, 10) + assert.Len(t, got, 3, "a cap larger than the list must not truncate anything") + + got = capFailoverCandidates(all, 0) + assert.Len(t, got, 3, "max <= 0 must be a defensive no-op") +} + +// --- resolveUnifiedPricing / candidateCostAllowed / filterCandidatesByCost --- + +func TestResolveUnifiedPricing_CanonicalFirst_ThenBareFallback(t *testing.T) { + t.Parallel() + overrides := map[string]*ModelPricing{ + "beta/shared": {InputPerM: 1, OutputPerM: 2}, + "shared": {InputPerM: 5, OutputPerM: 6}, + } + p, ok := resolveUnifiedPricing("beta/shared", "shared", overrides) + require.True(t, ok) + assert.Equal(t, ModelPricing{InputPerM: 1, OutputPerM: 2}, p, "an override keyed on the canonical id must win over the bare fallback — the exact case unifiedCostMicros itself resolves this way") +} + +func TestResolveUnifiedPricing_FallsBackToBare(t *testing.T) { + t.Parallel() + overrides := map[string]*ModelPricing{"shared": {InputPerM: 5, OutputPerM: 6}} + p, ok := resolveUnifiedPricing("beta/shared", "shared", overrides) + require.True(t, ok) + assert.Equal(t, ModelPricing{InputPerM: 5, OutputPerM: 6}, p) +} + +func TestResolveUnifiedPricing_Unknown(t *testing.T) { + t.Parallel() + _, ok := resolveUnifiedPricing("beta/shared", "shared", nil) + assert.False(t, ok, "neither the canonical nor the bare id has a price — this must never be silently treated as zero") +} + +func TestResolveUnifiedPricing_BuiltinTable(t *testing.T) { + t.Parallel() + p, ok := resolveUnifiedPricing("openai/gpt-5", "gpt-5", nil) + require.True(t, ok, "the built-in pricing table must still resolve with no operator overrides configured") + assert.Equal(t, builtinPricing["gpt-5"], p) +} + +// TestCandidateCostAllowed is the direct regression test for the +// operator's binding ruling: table-driven across every named case, +// including the two-dimension requirement (cheap on one axis, dear on +// the other, must still be rejected). +func TestCandidateCostAllowed(t *testing.T) { + t.Parallel() + cheap := ModelPricing{InputPerM: 1, OutputPerM: 2} + dear := ModelPricing{InputPerM: 3, OutputPerM: 4} + mixedCheapInputDearOutput := ModelPricing{InputPerM: 0.5, OutputPerM: 10} + zero := ModelPricing{} + + tests := []struct { + name string + primary, candidate ModelPricing + primaryKnown, candidateKnown bool + want bool + }{ + {name: "both known, candidate strictly cheaper on both", primary: dear, primaryKnown: true, candidate: cheap, candidateKnown: true, want: true}, + {name: "both known, identical price", primary: cheap, primaryKnown: true, candidate: cheap, candidateKnown: true, want: true}, + {name: "both known, candidate strictly dearer on both", primary: cheap, primaryKnown: true, candidate: dear, candidateKnown: true, want: false}, + {name: "both known, cheaper input but dearer output must still reject", primary: cheap, primaryKnown: true, candidate: mixedCheapInputDearOutput, candidateKnown: true, want: false}, + {name: "both known, dearer input but cheaper output must still reject", primary: mixedCheapInputDearOutput, primaryKnown: true, candidate: cheap, candidateKnown: true, want: false}, + {name: "primary known, candidate unknown: never allowed", primary: dear, primaryKnown: true, candidate: ModelPricing{}, candidateKnown: false, want: false}, + {name: "primary known-zero, candidate unknown: never allowed", primary: zero, primaryKnown: true, candidate: ModelPricing{}, candidateKnown: false, want: false}, + {name: "primary unknown, candidate known non-zero: never allowed", primary: ModelPricing{}, primaryKnown: false, candidate: cheap, candidateKnown: true, want: false}, + {name: "primary unknown, candidate known but only input non-zero: never allowed", primary: ModelPricing{}, primaryKnown: false, candidate: ModelPricing{InputPerM: 0.01}, candidateKnown: true, want: false}, + {name: "primary unknown, candidate known-zero: allowed", primary: ModelPricing{}, primaryKnown: false, candidate: zero, candidateKnown: true, want: true}, + {name: "both unknown: allowed", primary: ModelPricing{}, primaryKnown: false, candidate: ModelPricing{}, candidateKnown: false, want: true}, + {name: "both known-zero: allowed", primary: zero, primaryKnown: true, candidate: zero, candidateKnown: true, want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := candidateCostAllowed(tt.primary, tt.primaryKnown, tt.candidate, tt.candidateKnown) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFilterCandidatesByCost_SkipsPricierKeepsCheaper(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha", "beta", "gamma"}, failoverConfig{enabled: true, maxAttempts: 3}) + g.cfg.Pricing = map[string]*ModelPricing{ + "alpha/shared": {InputPerM: 2, OutputPerM: 4}, // primary + "beta/shared": {InputPerM: 3, OutputPerM: 4}, // dearer input -> rejected + "gamma/shared": {InputPerM: 2, OutputPerM: 4}, // equal -> allowed + } + candidates := []resolveCandidate{ + {providerName: "alpha", upstreamModel: "shared", canonical: "alpha/shared"}, + {providerName: "beta", upstreamModel: "shared", canonical: "beta/shared"}, + {providerName: "gamma", upstreamModel: "shared", canonical: "gamma/shared"}, + } + got := g.filterCandidatesByCost(candidates, "shared") + assert.Equal(t, []string{"alpha", "gamma"}, candNames(got)) +} + +func TestFilterCandidatesByCost_FewerThanTwoCandidates_NoOp(t *testing.T) { + t.Parallel() + g := testGatewayForCandidates(t, []string{"alpha"}, failoverConfig{enabled: true, maxAttempts: 3}) + candidates := []resolveCandidate{{providerName: "alpha", upstreamModel: "shared", canonical: "alpha/shared"}} + got := g.filterCandidatesByCost(candidates, "shared") + assert.Equal(t, candidates, got) } // ===================================================================== @@ -504,6 +668,13 @@ func twoProviderFailoverConfig(srvAlpha, srvBeta *httptest.Server) *Config { cfg.Users = &UsersConfig{Inline: []*UserConfig{ {Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}, }} + // Explicitly opt in: FailoverConfig.Enabled now defaults to false + // (coordinator ruling) — every test in this file that exercises actual + // failover behavior must opt in on purpose, the same way an operator + // would, rather than relying on a default this feature no longer ships + // with. + yes := true + cfg.Failover = FailoverConfig{Enabled: &yes} return cfg } @@ -570,6 +741,8 @@ func TestHandleChat_Failover_FallsThroughAndServes(t *testing.T) { } cfg.Groups = map[string]*GroupConfig{"default": {}} cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + yes := true + cfg.Failover = FailoverConfig{Enabled: &yes} next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) h, err := New(context.Background(), next, cfg, "llmgw") @@ -699,10 +872,17 @@ func TestHandleChat_Failover_RequestUnhealthy_SkippedWithoutBeingCalled(t *testi } // TestHandleChat_Failover_DiscoveryUnhealthy_StillTried proves discovery -// health is an ordering hint, never a hard skip: a provider whose -// DISCOVERY breaker is open must still be attempted (and can still serve -// the response) when it is the only one that actually works. Would FAIL -// if discoveryHealthy were (mis-)used as a skip gate. +// health is never a HARD SKIP: a provider whose DISCOVERY breaker is +// open must still be attempted, and can still serve the response, when +// beta (the only other candidate) fails. Would FAIL if discoveryHealthy +// were used to exclude a candidate outright, but does NOT independently +// prove ordering (deleting the deprioritization partition in +// orderedFailoverCandidates leaves this test passing too, since beta +// fails either way and alpha is tried regardless of position) — the +// ordering claim itself is covered only by +// TestOrderedFailoverCandidates_DiscoveryUnhealthy_DeprioritizedNotDropped +// (adversarial-review correction: this comment previously overstated +// what this test proves). func TestHandleChat_Failover_DiscoveryUnhealthy_StillTried(t *testing.T) { alphaSrv := jsonServer(http.StatusOK, successRespBody) // alpha's CHAT endpoint works fine defer alphaSrv.Close() @@ -727,10 +907,9 @@ func TestHandleChat_Failover_DiscoveryUnhealthy_StillTried(t *testing.T) { st.mu.Unlock() require.False(t, gw.registry.discoveryHealthy("alpha")) - // beta is healthy by every signal but its own chat endpoint 500s — - // ordering puts beta (discovery-healthy) FIRST, so this proves the - // request actually reaches alpha SECOND, not that alpha happened to - // be tried first anyway. + // beta is healthy by every signal but its own chat endpoint 500s, so + // the request can only succeed if alpha is tried at some point in + // the chain despite its open discovery breaker. body := map[string]any{"model": "shared", "messages": []any{}} req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) rec := httptest.NewRecorder() @@ -864,11 +1043,507 @@ func TestHandleChat_Failover_CachedUnderWinningProviderKey(t *testing.T) { assert.Equal(t, int64(1), atomic.LoadInt64(&betaCalls), "the second request must hit cache under beta's key, not call beta's backend again") } +// --- cost guard (operator ruling): never fail over to a pricier candidate --- + +// costGuardConfig builds a two-provider (alpha, beta) config exactly like +// twoProviderFailoverConfig but WITHOUT setting cfg.Pricing, so each test +// configures its own overrides for its own case. +func costGuardConfig(srvAlpha, srvBeta *httptest.Server) *Config { + cfg := twoProviderFailoverConfig(srvAlpha, srvBeta) + cfg.Providers["alpha"].Models = []string{"shared"} + cfg.Providers["beta"].Models = []string{"shared"} + return cfg +} + +// driveCostGuardRequest posts one chat-completion request for "shared" +// through h and returns the recorder and everything logged to stderr +// while it ran. +func driveCostGuardRequest(t *testing.T, h http.Handler) (*httptest.ResponseRecorder, string) { + t.Helper() + origStderr := captureStderrStart() + body := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec, captureStderrStop(origStderr) +} + +// TestHandleChat_Failover_CostGuard_SkipsPricierCandidate is the direct +// regression test for the operator's requirement: a candidate priced +// higher than the primary must never serve. Would FAIL if the cost guard +// were reverted (mutation-tested in this branch's own history). +func TestHandleChat_Failover_CostGuard_SkipsPricierCandidate(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "alpha/shared": {InputPerM: 1, OutputPerM: 2}, + "beta/shared": {InputPerM: 1, OutputPerM: 3}, // dearer on output only — still must be rejected + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, logOutput := driveCostGuardRequest(t, h) + + assert.Equal(t, http.StatusInternalServerError, rec.Code, "beta is dearer, so alpha's own failure must be returned as-is, body=%s", rec.Body.String()) + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be called once the cost guard rejects it") + assert.Contains(t, logOutput, "alpha") + assert.Contains(t, logOutput, "beta") + assert.Contains(t, logOutput, "shared") +} + +// TestHandleChat_Failover_CostGuard_AllowsCheaperOrEqualCandidate is the +// mirror positive case: a candidate priced at or below the primary must +// still serve normally. +func TestHandleChat_Failover_CostGuard_AllowsCheaperOrEqualCandidate(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "alpha/shared": {InputPerM: 2, OutputPerM: 4}, + "beta/shared": {InputPerM: 2, OutputPerM: 4}, // exactly equal — allowed + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusOK, rec.Code, "beta is equally priced, so failover must still serve it, body=%s", rec.Body.String()) + assert.Equal(t, successRespBody, rec.Body.String()) +} + +// TestHandleChat_Failover_CostGuard_PrimaryKnown_CandidateUnknown_Skips +// covers the operator's first unknown-pricing ruling: primary priced, +// candidate unpriced — never allowed, since an unknown price cannot be +// proven not-more-expensive. +func TestHandleChat_Failover_CostGuard_PrimaryKnown_CandidateUnknown_Skips(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "alpha/shared": {InputPerM: 1, OutputPerM: 2}, + // beta has no override, and "shared"/"beta/shared" is not in the + // built-in table either — its price is genuinely unknown. + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls)) +} + +// TestHandleChat_Failover_CostGuard_PrimaryUnknown_CandidateKnownNonZero_Skips +// covers the operator's second unknown-pricing ruling: moving from +// unbilled to billed is itself a spend increase. +func TestHandleChat_Failover_CostGuard_PrimaryUnknown_CandidateKnownNonZero_Skips(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "beta/shared": {InputPerM: 1, OutputPerM: 2}, // alpha has no override at all: unknown + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls)) +} + +// TestHandleChat_Failover_CostGuard_PrimaryUnknown_CandidateKnownZero_Allows +// covers the ruling's own stated exception: a candidate that is known, +// explicit free can never be more expensive than an unknown amount. +func TestHandleChat_Failover_CostGuard_PrimaryUnknown_CandidateKnownZero_Allows(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "beta/shared": {InputPerM: 0, OutputPerM: 0}, // explicit, known-zero override + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusOK, rec.Code, "a known-zero candidate can never be more expensive than an unknown primary, body=%s", rec.Body.String()) +} + +// TestHandleChat_Failover_CostGuard_BothUnknown_Allows and +// TestHandleChat_Failover_CostGuard_BothKnownZero_Allows cover the +// ruling's "equal, so allowed" cases. +func TestHandleChat_Failover_CostGuard_BothUnknown_Allows(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) // no cfg.Pricing at all + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusOK, rec.Code, "both unknown must be treated as equal, body=%s", rec.Body.String()) +} + +func TestHandleChat_Failover_CostGuard_BothKnownZero_Allows(t *testing.T) { + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := costGuardConfig(alphaSrv, betaSrv) + cfg.Pricing = map[string]*ModelPricing{ + "alpha/shared": {InputPerM: 0, OutputPerM: 0}, + "beta/shared": {InputPerM: 0, OutputPerM: 0}, + } + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + rec, _ := driveCostGuardRequest(t, h) + assert.Equal(t, http.StatusOK, rec.Code, "both known-zero must be treated as equal, body=%s", rec.Body.String()) +} + +// TestHandleChat_Failover_CostGuard_PerProviderPricingOverride_RealShape is +// the requested production-shape case: the SAME bare model id +// (a real, built-in-priced model — "gpt-5") served by two providers, +// where alpha has NO override (resolves via the built-in table) and beta +// has an operator-set per-provider override making it dearer on the +// canonical "beta/gpt-5" key. Proves the guard resolves pricing through +// the exact canonical-then-bare path unifiedCostMicros itself uses, not +// a parallel, simplified notion of price — the whole point of ruling 1. +func TestHandleChat_Failover_CostGuard_PerProviderPricingOverride_RealShape(t *testing.T) { + const realModel = "gpt-5" + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: alphaSrv.URL, APIKey: "k", Models: []string{realModel}}, + "beta": {Type: "openai", BaseURL: betaSrv.URL, APIKey: "k", Models: []string{realModel}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + yes := true + cfg.Failover = FailoverConfig{Enabled: &yes} + // Only beta gets a per-provider override — alpha resolves gpt-5's + // price from builtinPricing instead, exactly the real production + // shape a per-provider Pricing override produces. + builtin := builtinPricing[realModel] + cfg.Pricing = map[string]*ModelPricing{ + "beta/" + realModel: {InputPerM: builtin.InputPerM * 10, OutputPerM: builtin.OutputPerM * 10}, + } + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + origStderr := captureStderrStart() + body := map[string]any{"model": realModel, "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + logOutput := captureStderrStop(origStderr) + + assert.Equal(t, http.StatusInternalServerError, rec.Code, "beta's per-provider override makes it 10x alpha's built-in price, so it must be rejected, body=%s", rec.Body.String()) + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls)) + assert.Contains(t, logOutput, "alpha") + assert.Contains(t, logOutput, "beta") + assert.Contains(t, logOutput, realModel) +} + +// --- adversarial-review regressions (F1-F4, F6, F7) --- + +// TestHandleChat_Failover_F1_StreamOptionsNotLeakedAcrossCandidates is the +// direct regression test for adversarial-review finding F1: an +// openai-type adapter mutates req in place (chatCompletion forces +// stream_options.include_usage=true onto it BEFORE it ever calls the +// upstream, provider_openai.go), so reusing one shared req map across +// candidates let a later candidate read back an earlier candidate's own +// forced injection and misreport clientAskedUsage=true to a client that +// never asked for streamed usage — leaking the gateway's own +// deliberately-suppressed usage-only SSE chunk. alpha fails before ever +// reaching the network (its own mutation to its OWN req copy still +// happens first, exactly like the real bug), beta serves a real SSE +// stream containing a usage-only chunk the client never asked for. +func TestHandleChat_Failover_F1_StreamOptionsNotLeakedAcrossCandidates(t *testing.T) { + const contentChunk = "data: {\"id\":\"c1\",\"object\":\"chat.completion.chunk\",\"choices\":[{\"delta\":{\"content\":\"Hi\"}}]}\n\n" + const usageOnlyChunk = "data: {\"id\":\"c1\",\"object\":\"chat.completion.chunk\",\"choices\":[],\"usage\":{\"prompt_tokens\":5,\"completion_tokens\":2}}\n\n" + const doneMarker = "data: [DONE]\n\n" + + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + fl, ok := w.(http.Flusher) + require.True(t, ok) + for _, chunk := range []string{contentChunk, usageOnlyChunk, doneMarker} { + _, _ = w.Write([]byte(chunk)) + fl.Flush() + } + })) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + // The client explicitly does NOT set stream_options at all — it + // never asked for usage in the stream. + body := map[string]any{"model": "shared", "stream": true, "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rw := newRecordingWriter() + h.ServeHTTP(rw, req) + + require.Equal(t, http.StatusOK, rw.status) + assert.Contains(t, rw.buf.String(), `"content":"Hi"`, "the real content chunk must still be forwarded") + assert.NotContains(t, rw.buf.String(), `"prompt_tokens":5`, "a client that never asked for stream usage must never receive it, even though ALPHA's own (failed, discarded) attempt forced include_usage=true on ITS OWN copy of the request") +} + +// TestHandleChat_Failover_F2_StaleSSEHeadersDoNotSurviveToPlainResponse is +// the direct regression test for adversarial-review finding F2: +// newSSEWriter (sse.go) stages Content-Type/Cache-Control/ +// X-Accel-Buffering on sw.Header() the moment a streaming attempt starts, +// before any byte is written. alpha enters that streaming path (a real +// 200 + text/event-stream response) but its connection is severed before +// any complete SSE event is dispatched — an error, satisfying THE HARD +// CONSTRAINT (nothing was actually WRITTEN to the client) — so failover +// proceeds to beta, which answers a plain, non-streaming JSON body. +func TestHandleChat_Failover_F2_StaleSSEHeadersDoNotSurviveToPlainResponse(t *testing.T) { + alphaSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + hj, ok := w.(http.Hijacker) + require.True(t, ok) + conn, _, hjErr := hj.Hijack() + if hjErr == nil { + // Closing a hijacked chunked-encoding connection with no + // terminating chunk produces a genuine "unexpected EOF" on + // the client's read side (verified empirically), not a + // clean end-of-stream — readSSE returns that as a real + // error without ever dispatching an event, so + // sw.writeData/sw.WriteHeader is never reached even though + // newSSEWriter already staged its three headers. + _ = conn.Close() + } + })) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "stream": true, "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "body=%s", rec.Body.String()) + assert.Equal(t, successRespBody, rec.Body.String()) + assert.Equal(t, "application/json", rec.Header().Get("Content-Type"), "beta's own Content-Type must win, not alpha's stale text/event-stream") + assert.Empty(t, rec.Header().Get("Cache-Control"), "alpha's staged SSE header must never survive into beta's plain JSON response") + assert.Empty(t, rec.Header().Get("X-Accel-Buffering"), "alpha's staged SSE header must never survive into beta's plain JSON response") +} + +// TestHandleChat_Failover_F3_401IsAFailure_OpensRequestHealthBreaker is the +// direct regression test for adversarial-review finding F3: a provider +// answering 401 to every request (a dead API key — exactly the "masking +// a broken provider key" risk this feature exists to reduce) must +// eventually be skipped, not retried forever. Would FAIL under the +// reverted isTransient-based classification, since isTransient returns +// false for any non-429 4xx. +func TestHandleChat_Failover_F3_401IsAFailure_OpensRequestHealthBreaker(t *testing.T) { + var alphaCalls, betaCalls int64 + alphaSrv := countingServer(http.StatusUnauthorized, `{"error":{"message":"invalid api key","type":"authentication_error"}}`, &alphaCalls) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "messages": []any{}} + for i := 0; i < requestBreakerFailureThreshold; i++ { + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "every one of these must still fail over to beta and succeed") + } + require.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls)) + + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls), "alpha must be skipped after 3 consecutive 401s, not called a 4th time") + assert.Equal(t, int64(requestBreakerFailureThreshold+1), atomic.LoadInt64(&betaCalls)) +} + +// TestHandleChat_Failover_F4_SharedDeadlineExhausted_NeverMisattributedToNextCandidate +// is the direct regression test for adversarial-review finding F4: alpha +// burns the ENTIRE shared request context sleeping past its deadline; +// beta must never be dialed at all (an attempt after the deadline is +// already spent would fail near-instantly for a reason that has nothing +// to do with beta's own health), never be recorded as having failed, and +// the client-facing error must name alpha, not beta. +func TestHandleChat_Failover_F4_SharedDeadlineExhausted_NeverMisattributedToNextCandidate(t *testing.T) { + alphaSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(300 * time.Millisecond) // well past the request's own 50ms deadline below + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(successRespBody)) + })) + defer alphaSrv.Close() + var betaCalls int64 + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + gw.limiter.spawn = func(f func()) { f() } + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", + strings.NewReader(`{"model":"shared","messages":[{"role":"user","content":"hi"}]}`)).WithContext(ctx) + req.Header.Set("Authorization", "Bearer sk-alice") + req.Header.Set("Content-Type", "application/json") + + origStderr := captureStderrStart() + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + logOutput := captureStderrStop(origStderr) + + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be dialed once the shared deadline is already spent") + assert.True(t, gw.failoverHealth.healthy("beta"), "beta must never be marked unhealthy for a timeout that was alpha's own fault") + + now := time.Now() + betaAttempts, _ := gw.limiter.getCounter(kindProvider, "beta", metricProvAttempt, windowDay, now) + assert.Equal(t, int64(0), betaAttempts, "beta's own dashboard attempt counter must not move either") + + assert.Contains(t, logOutput, "alpha", "the error must name the provider that actually failed") +} + +// TestHandleChat_Failover_F6_RetryAttemptsDoNotOpenBreakerOnOneRequest is +// the direct regression test for adversarial-review finding F6: with +// retry.enabled and attempts:3, ONE client request against a +// permanently-failing provider must count as ONE outcome for the +// request-health gate, not one per raw retry attempt — otherwise a +// single request alone could reach requestBreakerFailureThreshold (3) +// purely from ITS OWN internal retries. +func TestHandleChat_Failover_F6_RetryAttemptsDoNotOpenBreakerOnOneRequest(t *testing.T) { + var alphaCalls int64 + alphaSrv := countingServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`, &alphaCalls) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusOK, successRespBody) + defer betaSrv.Close() + + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + cfg.Retry = RetryConfig{Enabled: true, Attempts: 3, Backoff: "1ms"} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "must still fail over to beta after alpha's own retries are exhausted") + + // retry.attempts=3 means 4 raw HTTP attempts against alpha for this + // ONE client request (retryPolicy's own doc comment, retry.go). + require.Equal(t, int64(4), atomic.LoadInt64(&alphaCalls)) + // The request-health gate must still read this as exactly ONE + // failure, not four — well below requestBreakerFailureThreshold (3). + assert.True(t, gw.failoverHealth.healthy("alpha"), "one client request's internal retries must count as one outcome for the request-health gate, not one per raw attempt") +} + +// TestHandleChat_Failover_F7_CachedFailoverResponse_NeverHitsDeadPrimary is +// the direct regression test for adversarial-review finding F7: even +// while alpha stays permanently broken, a SECOND identical request must +// be served from beta's cache entry WITHOUT alpha's dead backend being +// hit again on that second request — proving cache lookups run for +// every candidate BEFORE any of them is attempted, not interleaved with +// each candidate's own turn. +func TestHandleChat_Failover_F7_CachedFailoverResponse_NeverHitsDeadPrimary(t *testing.T) { + var alphaCalls, betaCalls int64 + alphaSrv := countingServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`, &alphaCalls) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + redisLn := newBehavioralRedisServer(t) + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + cfg.Redis = &RedisConfig{Address: redisLn.Addr().String()} + cfg.Cache = CacheConfig{Enabled: true, TTL: "1m", MaxBodyBytes: 1 << 20} + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + reqBody := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req1 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec1 := httptest.NewRecorder() + h.ServeHTTP(rec1, req1) + require.Equal(t, http.StatusOK, rec1.Code) + require.Equal(t, int64(1), atomic.LoadInt64(&alphaCalls)) + require.Equal(t, int64(1), atomic.LoadInt64(&betaCalls)) + + req2 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec2 := httptest.NewRecorder() + h.ServeHTTP(rec2, req2) + + assert.Equal(t, http.StatusOK, rec2.Code) + assert.Equal(t, "hit", rec2.Header().Get("X-Llmgw-Cache")) + assert.Equal(t, int64(1), atomic.LoadInt64(&alphaCalls), "alpha's dead backend must NOT be hit again just to discover beta's cache entry") + assert.Equal(t, int64(1), atomic.LoadInt64(&betaCalls)) +} + // TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore // pins the "single-provider deployment behaves exactly as before" // requirement explicitly for this branch, with failover left at its -// (enabled) default: with no second candidate to try, an upstream error -// must be returned to the client unchanged, identical to v0.1 behavior. +// (now disabled) default: with no second candidate to try, an upstream +// error must be returned to the client unchanged, identical to v0.1 +// behavior, regardless of the enabled switch either way. func TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore(t *testing.T) { srv := jsonServer(http.StatusTooManyRequests, `{"error":{"message":"rate limited upstream","type":"rate_limit_error"}}`) defer srv.Close() @@ -898,6 +1573,43 @@ func TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore assert.Equal(t, "upstream_error", out.Error.Type) } +// TestHandleChat_Failover_DefaultConfig_ByteIdenticalToPreFailover is the +// regression test for the coordinator's default-off ruling: a Config that +// never mentions "failover" at all — the shape of every config written +// before this feature existed, and the shape most operators keep using +// after upgrading without opting in — must behave byte-identically to +// having no failover code at all. Would FAIL if validateFailoverConfig's +// default reverted to enabled. +func TestHandleChat_Failover_DefaultConfig_ByteIdenticalToPreFailover(t *testing.T) { + var betaCalls int64 + alphaSrv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: alphaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + "beta": {Type: "openai", BaseURL: betaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + // cfg.Failover is deliberately left untouched — the zero value, what + // every config predating this feature has. + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "shared", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code, "an unconfigured Failover block must leave alpha's own 500 unchanged") + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be called when failover was never opted into") +} + // TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough proves // the on/off switch actually gates the behavior end to end. func TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough(t *testing.T) { diff --git a/llmgateway.go b/llmgateway.go index 6cd63ac..08d06d8 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -545,6 +545,14 @@ type Gateway struct { // rather than a nil-pointer panic. failoverHealth *requestHealthTracker name string + // failoverLogGate rate-limits runMeteredCall's generic "failing over" + // log line (routes_unified.go) to once per storeErrorLogEvery + // (adversarial-review fix, F10) — reuses auth.go's own logGate type, + // the identical technique shouldLogAuthFailure/logStoreError already + // apply elsewhere in this package. Zero-value-usable: the first call + // on a fresh Gateway always logs. Deliberately does NOT gate the 404 + // loud-log line (operator ruling: that one must always log). + failoverLogGate logGate // failover is Config.Failover, validated and resolved once by // newGateway (validateFailoverConfig, failover.go). failover failoverConfig @@ -657,8 +665,9 @@ func newGateway(ctx context.Context, next http.Handler, config *Config, name str g := &Gateway{next: next, name: name, cfg: config, auth: auth} // feat/failover: validated once, here — see FailoverConfig's own doc - // comment (failover.go) for the default (enabled) and what "single- - // provider deployment behaves exactly as before" means in practice. + // comment (failover.go) for the default (disabled, coordinator + // ruling) and what "single-provider deployment behaves exactly as + // before" means in practice. failoverCfg, err := validateFailoverConfig(config.Failover) if err != nil { return nil, err diff --git a/routes_unified.go b/routes_unified.go index 655af16..bdbcbf4 100644 --- a/routes_unified.go +++ b/routes_unified.go @@ -5,10 +5,12 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "math" "net/http" "strconv" + "time" ) // maxRequestBytes caps a client request body decoded into a @@ -274,12 +276,31 @@ func (g *Gateway) runUnified(w http.ResponseWriter, r *http.Request, u *user, gr // attempt" and "cache under the winning provider's key" actually mean // in code, not just in the brief's prose. func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scopes []limitScope, body []byte, req map[string]any, requestedModel string, grp *group, endpoint, logPrefix string, envelope envelopeWriter, writeUpstream providerUpstreamErrorWriter, call adapterCall) { - primary, extra, err := g.registry.resolveWithCandidates(requestedModel, grp) + // F9 (adversarial-review, optional perf fix): a disabled deployment + // never needs the candidate pool at all — resolvePrimaryOnly skips + // failoverCandidates' own provider scan entirely, so a config that + // never opted into failover pays nothing extra for a pool it will + // never use. + var primary resolveCandidate + var extra []resolveCandidate + var err error + if g.failover.enabled { + primary, extra, err = g.registry.resolveWithCandidates(requestedModel, grp) + } else { + primary, err = g.registry.resolvePrimaryOnly(requestedModel, grp) + } if err != nil { writeModelResolveErrorEnvelope(sw, err, envelope) return } + // Sequence matters (each step's own doc comment explains why): + // health/discovery ordering first, so candidates[0] below is the + // ACTUAL first-attempted candidate the cost guard compares everyone + // else against; cost filtering second; the maxAttempts cap last, so + // it never re-admits a candidate the cost guard already removed. candidates := g.orderedFailoverCandidates(primary, extra) + candidates = g.filterCandidatesByCost(candidates, requestedModel) + candidates = capFailoverCandidates(candidates, g.failover.maxAttempts) // cacheable/streaming gate every cache step below on spec §2's scope, // exactly as before this feature: a non-streaming request, with a @@ -291,44 +312,40 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop streaming, _ := req["stream"].(bool) cacheable := !streaming && g.cache != nil && groupCacheEnabled(grp) - for i, cand := range candidates { - if i > 0 { - // checkAndCount above (admitRequest, this file) already - // counted this request exactly once — never double-count: a - // failover attempt is a NEW upstream attempt, not a new - // logical request, so nothing here calls checkAndCount - // again. Per-provider ATTEMPT counters, below, still record - // every attempt independently via the freshly-bound - // recorder. - g.warnf("%s: failing over from provider %q to %q for model %q", logPrefix, candidates[i-1].providerName, cand.providerName, requestedModel) - } + // pristineReq is the client's own decoded request, snapshotted ONCE, + // BEFORE any candidate's adapter ever touches it, with "model" + // already rewritten to the upstream id every candidate in this + // failover chain shares by construction (failoverCandidates, + // failover.go, only ever adds providers serving the IDENTICAL bare + // id resolve's primary already resolved to). Every attempt below + // builds its OWN fresh top-level copy from this snapshot rather than + // reusing or mutating one shared map (adversarial-review fix, F1): + // openai-type adapters mutate req in place — chatCompletion forces + // stream_options.include_usage=true onto it (provider_openai.go) — + // and reusing the same map across attempts let a LATER candidate + // read back an EARLIER candidate's own forced injection and + // misreport clientAskedUsage=true to a client that never asked for + // it, leaking the gateway's own suppressed usage-only SSE chunk. + // Proven against a single-provider control; this is what fixes it + // for openai, openai-audio, and copilot alike. + pristineReq := make(map[string]any, len(req)+1) + for k, v := range req { + pristineReq[k] = v + } + if len(candidates) > 0 { + pristineReq["model"] = candidates[0].upstreamModel + } - // req["model"] is reset on EVERY iteration: cand.upstreamModel is - // the identical string across every candidate in a failover chain - // by construction (failoverCandidates, failover.go, only ever - // adds providers sharing the SAME bare id resolve's primary - // already resolved to), so this is a no-op rewrite on the common - // single-candidate path, not new cost. - req["model"] = cand.upstreamModel - - // cacheKey is computed fresh per candidate — cache correctness - // (brief): a response produced by a failover provider must be - // stored under THAT provider's own key, never the first - // candidate's. Called here, BEFORE gatewayAliasKey is injected - // below, preserving the ordering cacheKey's own doc comment - // (cache.go) requires of every caller: gatewayAliasKey is a - // purely internal echo-back mechanism cacheKey does not itself - // strip, so computing the key after injecting it would hash an - // extra, noisy field. - var cacheKeyStr string - if cacheable { - cacheKeyStr = cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, req) - if cached, hit := g.cache.lookup(cacheKeyStr); hit { - // A cache hit accounts the request only — checkAndCount - // above already counted it — never token/cost, and never - // runs the zero-usage estimate branch below: the client - // never reached the upstream provider its body size would - // be estimating against. + // F7 fix (adversarial-review): check EVERY candidate's cache entry + // before attempting ANY of them, so a cached failover response never + // costs a real round trip to a dead primary just because normal + // ordering tries that primary first — the lookup used to sit inside + // each candidate's own turn, after every EARLIER candidate's real + // upstream call had already failed. + if cacheable { + for _, cand := range candidates { + key := cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, pristineReq) + if cached, hit := g.cache.lookup(key); hit { sw.Header().Set("X-Llmgw-Cache", "hit") if cached.ContentType != "" { sw.Header().Set("Content-Type", cached.ContentType) @@ -337,6 +354,95 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop _, _ = sw.Write(cached.Body) return } + } + } + + var lastErr error + var lastProviderName string + for i, cand := range candidates { + if i > 0 { + // F4 fix (adversarial-review): a shared request context an + // EARLIER candidate already exhausted must never be + // misattributed as THIS candidate's own failure. There is no + // per-candidate timeout budget in this architecture (every + // candidate shares r's SAME context — see README's own + // documented limitation), so once an earlier candidate burns + // the deadline, attempting the next one would fail near- + // instantly for a reason that has nothing to do with its own + // health. Stop here with the LAST REAL error instead of + // chasing further doomed candidates and wrongly opening a + // perfectly healthy provider's request-health breaker. + if r.Context().Err() != nil { + g.handleAdapterErrorEnvelope(sw, lastErr, lastProviderName, logPrefix, envelope, writeUpstream) + return + } + + // checkAndCount above (admitRequest, this file) already + // counted this request exactly once — never double-count: a + // failover attempt is a NEW upstream attempt, not a new + // logical request, so nothing here calls checkAndCount + // again. Per-provider ATTEMPT counters, below, still record + // every attempt independently via the freshly-bound + // recorder. + // + // Rate-limited (F10 fix, adversarial-review): during a + // sustained outage this line would otherwise be one + // blocking synchronous stderr write per request on shared + // Traefik ingress — the exact failure shouldLogAuthFailure/ + // logStoreError (auth.go/limits.go) already exist to + // prevent elsewhere in this package. The 404 line below is + // deliberately NOT gated by this (operator ruling: it must + // always log). + if log, suppressed := g.failoverLogGate.shouldLog(time.Now()); log { + suffix := "" + if suppressed > 0 { + suffix = fmt.Sprintf(" (%d more failovers suppressed since last log)", suppressed) + } + g.warnf("%s: failing over from provider %q to %q for model %q%s", logPrefix, candidates[i-1].providerName, cand.providerName, requestedModel, suffix) + } + } + + // F2 fix (adversarial-review): clear every header a PRIOR + // candidate's own writer may have staged on sw.Header() without + // committing it — newSSEWriter (sse.go) sets Content-Type: text/ + // event-stream, Cache-Control: no-cache, and X-Accel-Buffering: + // no the moment a streaming attempt starts, before any byte is + // written. If that candidate then fails before writing (still + // safe to fail over, THE HARD CONSTRAINT below), those staged + // headers must not survive into a plain-JSON response from the + // next candidate. Safe to clear unconditionally: this loop only + // ever reaches a new iteration while sw.wroteHeader is false, so + // nothing committed is ever at risk of being wiped. + hdr := sw.Header() + for k := range hdr { + hdr.Del(k) + } + + // attemptReq is a FRESH top-level copy of pristineReq for every + // candidate (F1 fix, this function's own doc comment above). + // gatewayAliasKey is (re-)injected here: every providerAdapter + // implementation unconditionally deletes it before it ever + // touches the network (its own doc comment, providers.go), so + // each fresh copy needs it set again to echo the client's own + // requested alias correctly (anthropic/gemini-type adapters read + // it to build their own response envelope). + attemptReq := make(map[string]any, len(pristineReq)+1) + for k, v := range pristineReq { + attemptReq[k] = v + } + attemptReq[gatewayAliasKey] = requestedModel + + // cacheKeyStr is still computed per candidate — cache + // correctness (brief): a response produced by a failover + // provider must be stored under THAT provider's own key, never + // the first candidate's. Computed from pristineReq (matching the + // pre-pass above exactly): gatewayAliasKey is a purely internal + // echo-back mechanism cacheKey does not itself strip (its own + // doc comment, cache.go), so hashing attemptReq instead would + // add noise no two requests would otherwise disagree on. + var cacheKeyStr string + if cacheable { + cacheKeyStr = cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, pristineReq) // Set before call() below writes anything — headers must // precede the body a miss is about to produce (spec §2). A // later iteration's own miss overwrites this harmlessly: it @@ -347,16 +453,6 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop sw.Header().Set("X-Llmgw-Cache", "miss") } - // gatewayAliasKey is (re-)injected on every iteration, AFTER the - // cacheKey computation above: every providerAdapter implementation - // unconditionally deletes it before it ever touches the network - // (its own doc comment, providers.go) — a failed first attempt - // leaves req without it, and the next candidate needs it set - // again to echo the client's own requested alias correctly - // (anthropic/gemini-type adapters read it to build their own - // response envelope). - req[gatewayAliasKey] = requestedModel - // respWriter is sw, wrapped in a capture tee only when this // request is cacheable — cacheCaptureWriter buffers everything // written so a 200 non-stream response can be stored after @@ -378,19 +474,28 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop // misattribute a retried attempt to the first provider" // requirement) — every upstream attempt call() makes, via // upstreamJSON's retryPolicy.do (providers.go), reports through - // here. The same closure also feeds g.failoverHealth (failover.go), - // feat/failover's own request-path health signal, with the - // IDENTICAL isTransient||isDeadlineExceeded classification - // limiter.recordProviderAttempt already applies — see - // requestHealthTracker.record's own doc comment for why this must - // stay the same classification, not a wider one. + // here. limiter.recordProviderAttempt keeps its own existing + // per-raw-attempt classification unchanged (limits.go); feat/ + // failover's OWN request-path health gate is recorded separately + // below, once per candidate rather than once per raw attempt — + // see requestHealthTracker.record's own doc comment (failover.go) + // for why (F3/F6 fix, adversarial-review). providerName := cand.providerName upstreamModel := cand.upstreamModel ctx := withAttemptRecorder(r.Context(), func(resp *http.Response, attemptErr error) { g.limiter.recordProviderAttempt(providerName, upstreamModel, resp, attemptErr) - g.failoverHealth.record(providerName, !isTransient(resp, attemptErr) && !isDeadlineExceeded(attemptErr)) }) - result, callErr := call(cand.adapter, ctx, respWriter, req) + result, callErr := call(cand.adapter, ctx, respWriter, attemptReq) + + // Request-path health, recorded ONCE per candidate per logical + // request (F3/F6 fix, adversarial-review) — see + // requestHealthTracker.record's own doc comment (failover.go) + // for the full ruling: failoverEligible's classification + // (everything except HTTP 400 is a failure) is reused directly, + // not limiter's narrower isTransient/isDeadlineExceeded one, so + // a provider answering 401/403/404 to every request is + // eventually routed around instead of retried forever. + g.failoverHealth.record(providerName, !failoverEligible(callErr)) // Usage is accounted before the error branch below runs, not // after: every adapter that can fail mid-stream (forwardStream @@ -457,6 +562,9 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop return } + lastErr = callErr + lastProviderName = providerName + hasNext := i < len(candidates)-1 && failoverEligible(callErr) if hasNext && isProviderNotFoundError(callErr) { // Loud, non-suppressed log line (brief: "Failover after a 404 @@ -465,7 +573,9 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop // would hide it") — deliberately g.errorf, not g.logf, and // deliberately its own line rather than folded into the // generic "failing over" line above the next iteration - // already prints. + // already prints, and deliberately NOT gated by + // failoverLogGate (operator ruling: this one must always + // log). g.errorf("%s: provider %q returned 404 for model %q; failing over to %q — this usually means a real provider/model configuration mistake, not a transient outage", logPrefix, providerName, requestedModel, candidates[i+1].providerName) } if !hasNext { diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index f9b5efd..935046e 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -371,7 +371,12 @@ func run() error { })) defer failoverBUpstream.Close() - attemptAccountingOverride := `{"breaker":{"failureThreshold":2,"openDuration":"3s","maxOpenDuration":"6s"},` + + // failover.enabled must be set explicitly here: FailoverConfig.Enabled + // defaults to false (coordinator ruling — a version upgrade with no + // config change must preserve prior behavior), so without this the + // exerciseFailover probe below would find failover-a's 500 returned + // to the client unchanged, never reaching failover-b at all. + attemptAccountingOverride := `{"failover":{"enabled":true},"breaker":{"failureThreshold":2,"openDuration":"3s","maxOpenDuration":"6s"},` + `"providers":{"openai":{"type":"openai","baseUrl":"` + upstream.URL + `","apiKey":"sk-up","models":["` + testDataWantModel + `","` + builtinLookupModelID + `"]},` + `"brk":{"type":"openai","baseUrl":"` + brkUpstream.URL + `","apiKey":"sk-up","discovery":true,"discoveryInterval":"1ms"},"` + slowProviderName + `":{"type":"openai","baseUrl":"` + slowUpstream.URL + `","apiKey":"sk-up","models":["` + slowProviderModel + `"]},` + From 0cf23345b188d2edbb5c317b05bffc6390acee30 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 01:48:04 +0100 Subject: [PATCH 3/3] fix(failover): scope header clear to retries, fix cache pre-pass, gate health record Fix one must-fix and one should-fix finding from re-verification of d45125e, plus two smaller findings from the same pass. MUST FIX: the F2 header clear ran on the first candidate attempt too, so it ran on the default, failover-disabled path and deleted response headers a different, earlier Traefik middleware had already staged before this plugin ran (CORS, HSTS, request-id, and similar). Scope the clear to i > 0, the only case that ever needs it. Add a test proving a header staged before the gateway runs survives the default path, on both a cache hit and a cache miss. SHOULD FIX, coordinator ruling: drop the all-candidates cache pre-pass. It silently served a different candidate's cached response while the primary was perfectly healthy, which is failover with no failure. Check each candidate's own cache entry immediately before that candidate's own attempt instead. Request-path health, now fixed in the prior round, is the correct mechanism for skipping a genuinely dead primary: once it is marked unhealthy it is excluded from the candidate list entirely, so a later request still reaches the next candidate's cache without the dead one being dialed. Update the existing cache regression test to drive enough requests to trip the health breaker before asserting no repeat call, and add a dedicated test proving a healthy primary is never bypassed for another candidate's cached response. Guard the request-health record call with the enabled check. That state is never read on the disabled path, so recording it there was dead work plus a new contention point on requestHealthTracker's tracker-wide mutex on every metered request, disabled or not. Add a test asserting the tracker's internal state stays empty across the disabled path. Add a test proving the 404 loud-log line is not rate-limited: drive two 404 failovers through the same Gateway in one request and assert both lines appear, since a version that wrapped the 404 line in the same rate limiter as the generic line would still pass every other test (each of which only ever drives one 404 event against a fresh gate). Document, but do not fix, that the cost guard cannot see a model declared free via modelMeta: it resolves pricing only through the same source real billing uses (pricing and the built-in table), which is correct and consistent, but means an operator who declares a model free the modelMeta way loses it as a failover candidate without realizing why. README now says to use pricing instead for that case. Every fix is proven by a dedicated regression test, and each was verified by reverting the fix in place and confirming the corresponding test fails before restoring it. --- README.md | 26 ++++- failover_test.go | 266 +++++++++++++++++++++++++++++++++++++++++++--- routes_unified.go | 109 +++++++++++-------- 3 files changed, 340 insertions(+), 61 deletions(-) diff --git a/README.md b/README.md index 0560e59..f165497 100644 --- a/README.md +++ b/README.md @@ -899,6 +899,18 @@ never a second candidate to try. known-zero price, are treated as equal and allowed. There is no config flag for this — it applies unconditionally whenever failover is enabled. +- **The cost guard cannot see `modelMeta`'s `free: true`.** It resolves + pricing only through `pricing` and the built-in table — the same + source `unifiedCostMicros` actually bills against — deliberately never + through `modelMeta` (see [Model metadata](#model-metadata) / + `ModelMetaConfig`), since `modelMeta` drives GET `/v1/models` display + only and never affects billing. A model declared free the `modelMeta` + way is therefore treated as unknown-priced by the cost guard and loses + redundancy: it is skipped as a failover candidate whenever the primary + has a known price (fail-safe, and logged like any other cost skip). + Declare the price through `pricing` (e.g. `{"inputPerM": 0, + "outputPerM": 0}`) if you want a genuinely free model to remain + eligible as a cheaper failover candidate. - **Never mid-stream**: failover only happens while nothing has reached the client yet. Once the first byte of a response is written, status and body are committed and the request is never retried elsewhere. @@ -920,9 +932,17 @@ never a second candidate to try. actually made it. - **Caching**: a cacheable response produced by a failover provider is stored under that provider's own cache key, never the first candidate's. - Every candidate's cache entry is checked before any of them is actually - attempted, so a cached response from a failover provider is served - without a wasted round trip to a still-broken primary. + Each candidate's own cache entry is checked immediately before that + candidate's own attempt, in try order — never for every candidate up + front. Checking every candidate's cache before any of them was + attempted was tried and reverted: it silently served a DIFFERENT + candidate's cached response even while the actual primary was + perfectly healthy, which is failover with no failure. The + request-path health signal above is what actually keeps a genuinely + dead primary from being dialed again — once it is marked unhealthy it + is excluded from the candidate list entirely, so a later request + reaches the next candidate's cache without the dead one ever being + attempted. - **Configurable**: `failover.maxAttempts` caps how many different providers one request will try in total, primary included (default 3, maximum 10) — distinct from `retry.attempts`, which retries the SAME diff --git a/failover_test.go b/failover_test.go index cb1e25e..f893d19 100644 --- a/failover_test.go +++ b/failover_test.go @@ -767,6 +767,54 @@ func TestHandleChat_Failover_FallsThroughAndServes(t *testing.T) { } } +// TestHandleChat_Failover_404LogLine_NeverRateLimited is the direct +// regression test for adversarial-review round 2, finding 5: nothing +// pinned the ruling that the 404 loud-log line is exempt from +// failoverLogGate — every OTHER test only ever drives one 404 event +// against a freshly constructed Gateway, whose gate's first call always +// logs regardless of whether it is rate-limited, so a version that +// wrapped the 404 line in failoverLogGate.shouldLog would still pass +// every one of them. Three providers here all serve the SAME bare model +// id and the first TWO both 404, so this drives two 404-failover events +// through the SAME Gateway (and so the SAME failoverLogGate instance) +// within milliseconds of each other — well inside the gate's own 30s +// window — and asserts BOTH loud lines appear. +func TestHandleChat_Failover_404LogLine_NeverRateLimited(t *testing.T) { + alphaSrv := jsonServer(http.StatusNotFound, `{"error":{"message":"not found","type":"invalid_request_error"}}`) + defer alphaSrv.Close() + betaSrv := jsonServer(http.StatusNotFound, `{"error":{"message":"not found","type":"invalid_request_error"}}`) + defer betaSrv.Close() + gammaSrv := jsonServer(http.StatusOK, successRespBody) + defer gammaSrv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "alpha": {Type: "openai", BaseURL: alphaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + "beta": {Type: "openai", BaseURL: betaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + "gamma": {Type: "openai", BaseURL: gammaSrv.URL, APIKey: "k", Models: []string{"shared"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + yes := true + cfg.Failover = FailoverConfig{Enabled: &yes} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + origStderr := captureStderrStart() + body := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + logOutput := captureStderrStop(origStderr) + + require.Equal(t, http.StatusOK, rec.Code, "body=%s", rec.Body.String()) + alphaLine := `provider "alpha" returned 404` + betaLine := `provider "beta" returned 404` + assert.Contains(t, logOutput, alphaLine, "alpha's own 404 must log, unsuppressed") + assert.Contains(t, logOutput, betaLine, "beta's own 404 must ALSO log, not suppressed by the generic failing-over rate limit even though it fired milliseconds after alpha's") +} + // TestHandleChat_Failover_400_DoesNotFallThrough is the regression test // for the brief's one hard exception: a malformed request must never // retry against a second provider. Would FAIL if failoverEligible treated @@ -1376,6 +1424,82 @@ func TestHandleChat_Failover_F2_StaleSSEHeadersDoNotSurviveToPlainResponse(t *te assert.Empty(t, rec.Header().Get("X-Accel-Buffering"), "alpha's staged SSE header must never survive into beta's plain JSON response") } +// TestHandleChat_Failover_DefaultPath_PreservesPreexistingResponseHeaders is +// the direct regression test for adversarial-review round 2's MUST-FIX +// finding: the round-1 F2 header clear ran unconditionally, including on +// i == 0 — the ONLY iteration the default, failover-disabled path ever +// reaches — deleting response headers a DIFFERENT, earlier Traefik +// middleware had already staged on the shared ResponseWriter before this +// plugin ever ran (CORS, HSTS, request-id, ...), contradicting this +// package's own "byte-identical to a gateway built before this field +// existed" claim. Covers both a cache miss (the real upstream call path) +// and a cache hit (the round-1 harness found headers survived a hit but +// were dropped on a miss on the SAME endpoint for the SAME client). +func TestHandleChat_Failover_DefaultPath_PreservesPreexistingResponseHeaders(t *testing.T) { + t.Run("cache miss", func(t *testing.T) { + srv := jsonServer(http.StatusOK, successRespBody) + defer srv.Close() + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: srv.URL, APIKey: "k", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + // Simulates an EARLIER Traefik middleware in the chain (CORS, + // HSTS, request-id, ...) that already staged headers on the + // SAME underlying ResponseWriter before this plugin ever ran. + rec.Header().Set("Access-Control-Allow-Origin", "*") + rec.Header().Set("X-Preexisting-Mw", "keepme") + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "body=%s", rec.Body.String()) + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin"), "a header staged by an earlier middleware must survive the default (single-candidate) path") + assert.Equal(t, "keepme", rec.Header().Get("X-Preexisting-Mw")) + }) + + t.Run("cache hit", func(t *testing.T) { + srv := jsonServer(http.StatusOK, successRespBody) + defer srv.Close() + redisLn := newBehavioralRedisServer(t) + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: srv.URL, APIKey: "k", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + cfg.Redis = &RedisConfig{Address: redisLn.Addr().String()} + cfg.Cache = CacheConfig{Enabled: true, TTL: "1m", MaxBodyBytes: 1 << 20} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req1 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec1 := httptest.NewRecorder() + h.ServeHTTP(rec1, req1) + require.Equal(t, http.StatusOK, rec1.Code) + require.Equal(t, "miss", rec1.Header().Get("X-Llmgw-Cache")) + + req2 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec2 := httptest.NewRecorder() + rec2.Header().Set("Access-Control-Allow-Origin", "*") + rec2.Header().Set("X-Preexisting-Mw", "keepme") + h.ServeHTTP(rec2, req2) + + require.Equal(t, http.StatusOK, rec2.Code) + require.Equal(t, "hit", rec2.Header().Get("X-Llmgw-Cache")) + assert.Equal(t, "*", rec2.Header().Get("Access-Control-Allow-Origin"), "a header staged by an earlier middleware must survive a cache hit too") + assert.Equal(t, "keepme", rec2.Header().Get("X-Preexisting-Mw")) + }) +} + // TestHandleChat_Failover_F3_401IsAFailure_OpensRequestHealthBreaker is the // direct regression test for adversarial-review finding F3: a provider // answering 401 to every request (a dead API key — exactly the "masking @@ -1504,6 +1628,16 @@ func TestHandleChat_Failover_F6_RetryAttemptsDoNotOpenBreakerOnOneRequest(t *tes // hit again on that second request — proving cache lookups run for // every candidate BEFORE any of them is attempted, not interleaved with // each candidate's own turn. +// TestHandleChat_Failover_F7_CachedFailoverResponse_NeverHitsDeadPrimary is +// the regression test for the ORIGINAL F7 complaint, fixed the way the +// coordinator ruled it must be (adversarial-review round 2): health, not +// an all-candidates cache pre-pass, is what keeps a genuinely dead +// primary from being hit again. alpha fails enough consecutive REAL +// requests to trip its own request-health breaker (requestHealthTracker, +// failover.go) — each one also falling over to beta, whose successful +// response gets cached after the very first request — and only THEN, +// once alpha is excluded from the candidate list entirely, does a +// further request reach beta's cache without alpha being dialed again. func TestHandleChat_Failover_F7_CachedFailoverResponse_NeverHitsDeadPrimary(t *testing.T) { var alphaCalls, betaCalls int64 alphaSrv := countingServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`, &alphaCalls) @@ -1521,23 +1655,83 @@ func TestHandleChat_Failover_F7_CachedFailoverResponse_NeverHitsDeadPrimary(t *t require.NoError(t, err) reqBody := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} - req1 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) - rec1 := httptest.NewRecorder() - h.ServeHTTP(rec1, req1) - require.Equal(t, http.StatusOK, rec1.Code) - require.Equal(t, int64(1), atomic.LoadInt64(&alphaCalls)) - require.Equal(t, int64(1), atomic.LoadInt64(&betaCalls)) - req2 := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) - rec2 := httptest.NewRecorder() - h.ServeHTTP(rec2, req2) + // Drive enough requests to trip alpha's OWN request-health breaker. + // beta's response is cached after the first of these, so alphaCalls + // keeps climbing (alpha itself is still healthy enough to be tried + // every time) while betaCalls stops climbing once its cache entry + // exists. + for i := 0; i < requestBreakerFailureThreshold; i++ { + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + } + require.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls)) + require.Equal(t, int64(1), atomic.LoadInt64(&betaCalls), "beta's own cache entry must absorb every request after the first") - assert.Equal(t, http.StatusOK, rec2.Code) - assert.Equal(t, "hit", rec2.Header().Get("X-Llmgw-Cache")) - assert.Equal(t, int64(1), atomic.LoadInt64(&alphaCalls), "alpha's dead backend must NOT be hit again just to discover beta's cache entry") + // One more request: alpha is now request-unhealthy and must be + // excluded from the candidate list entirely — it must not be dialed + // a further time just to discover beta's cache entry. + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "hit", rec.Header().Get("X-Llmgw-Cache")) + assert.Equal(t, int64(requestBreakerFailureThreshold), atomic.LoadInt64(&alphaCalls), "alpha must not be dialed again once it is request-unhealthy") assert.Equal(t, int64(1), atomic.LoadInt64(&betaCalls)) } +// TestHandleChat_Failover_HealthyPrimaryNeverServedFromAnotherCandidatesCache +// is the direct regression test for adversarial-review round 2, finding +// 2: the round-1 F7 pre-pass checked every candidate's cache before +// attempting any of them, which silently substituted a DIFFERENT +// candidate's cached response while the actual primary was perfectly +// healthy — proven: alpha healthy and serving a distinct body, never +// called, the client instead got beta's stale cached body with +// X-Llmgw-Cache: hit and no "failing over" log line at all. beta's cache +// entry here is planted directly (simulating a real entry left over +// from an EARLIER, unrelated failover event, now stale) while alpha has +// never failed — a fresh request must reach alpha for real. +func TestHandleChat_Failover_HealthyPrimaryNeverServedFromAnotherCandidatesCache(t *testing.T) { + const alphaRespBody = `{"id":"chatcmpl-alpha","object":"chat.completion","model":"shared","choices":[{"index":0,"message":{"role":"assistant","content":"from alpha"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}` + + var alphaCalls, betaCalls int64 + alphaSrv := countingServer(http.StatusOK, alphaRespBody, &alphaCalls) + defer alphaSrv.Close() + betaSrv := countingServer(http.StatusOK, successRespBody, &betaCalls) + defer betaSrv.Close() + + redisLn := newBehavioralRedisServer(t) + cfg := twoProviderFailoverConfig(alphaSrv, betaSrv) + cfg.Redis = &RedisConfig{Address: redisLn.Addr().String()} + cfg.Cache = CacheConfig{Enabled: true, TTL: "1m", MaxBodyBytes: 1 << 20} + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + reqBody := map[string]any{"model": "shared", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + hashed := map[string]any{"model": "shared", "messages": reqBody["messages"]} + betaKey := cacheKey("beta", "shared", "shared", cacheEndpointChat, hashed) + gw.cache.store(betaKey, http.StatusOK, "application/json", []byte(successRespBody), time.Minute) + + origStderr := captureStderrStart() + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", reqBody) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + logOutput := captureStderrStop(origStderr) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, alphaRespBody, rec.Body.String(), "a healthy primary's OWN real response must be served, never another candidate's unrelated cache entry") + assert.Equal(t, int64(1), atomic.LoadInt64(&alphaCalls), "alpha must actually be dialed") + assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls)) + assert.NotContains(t, logOutput, "failing over", "no failover happened, so no failover log line may appear") +} + // TestHandleChat_Failover_SingleProvider_UpstreamError_BehavesExactlyAsBefore // pins the "single-provider deployment behaves exactly as before" // requirement explicitly for this branch, with failover left at its @@ -1610,6 +1804,48 @@ func TestHandleChat_Failover_DefaultConfig_ByteIdenticalToPreFailover(t *testing assert.Equal(t, int64(0), atomic.LoadInt64(&betaCalls), "beta must never be called when failover was never opted into") } +// TestHandleChat_Failover_DisabledPath_NeverTouchesRequestHealthTracker is +// the direct regression test for adversarial-review round 2, finding 3: +// on the disabled path (single candidate, the common case for most +// deployments) g.failoverHealth.record was still called unconditionally, +// taking requestHealthTracker's tracker-wide mutex (stateFor's t.mu) on +// EVERY metered request even though that state is never READ on this +// path at all (orderedFailoverCandidates' own early return never calls +// healthy() when disabled) — dead work plus a new global contention +// point. Proven the only way observable from outside the package: the +// tracker's internal per-provider state must never even be CREATED for +// a provider whose every request goes through the disabled path, +// including one that fails. +func TestHandleChat_Failover_DisabledPath_NeverTouchesRequestHealthTracker(t *testing.T) { + srv := jsonServer(http.StatusInternalServerError, `{"error":{"message":"down","type":"server_error"}}`) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: srv.URL, APIKey: "k", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + // cfg.Failover left at its zero value: disabled, the common case. + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + require.NoError(t, err) + gw, ok := h.(*Gateway) + require.True(t, ok) + + body := map[string]any{"model": "gpt-test", "messages": []any{}} + req := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusInternalServerError, rec.Code) + + gw.failoverHealth.mu.Lock() + stateCount := len(gw.failoverHealth.states) + gw.failoverHealth.mu.Unlock() + assert.Equal(t, 0, stateCount, "the disabled path must never record into requestHealthTracker at all, even for a failing provider") +} + // TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough proves // the on/off switch actually gates the behavior end to end. func TestHandleChat_Failover_ExplicitlyDisabled_5xxNeverFallsThrough(t *testing.T) { @@ -1660,6 +1896,12 @@ func TestHandleChat_Failover_ContextDeadlineExceeded_RecordsRequestHealthFailure } cfg.Groups = map[string]*GroupConfig{"default": {}} cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} + // Explicitly opt in: request-health is only ever recorded while + // failover is enabled (adversarial-review round 2, finding 3 — it + // is never READ on the disabled path, so recording it there was + // dead work and a needless global-mutex hit on every request). + yes := true + cfg.Failover = FailoverConfig{Enabled: &yes} next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) h, err := New(context.Background(), next, cfg, "llmgw") require.NoError(t, err) diff --git a/routes_unified.go b/routes_unified.go index bdbcbf4..9ef5a58 100644 --- a/routes_unified.go +++ b/routes_unified.go @@ -336,27 +336,6 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop pristineReq["model"] = candidates[0].upstreamModel } - // F7 fix (adversarial-review): check EVERY candidate's cache entry - // before attempting ANY of them, so a cached failover response never - // costs a real round trip to a dead primary just because normal - // ordering tries that primary first — the lookup used to sit inside - // each candidate's own turn, after every EARLIER candidate's real - // upstream call had already failed. - if cacheable { - for _, cand := range candidates { - key := cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, pristineReq) - if cached, hit := g.cache.lookup(key); hit { - sw.Header().Set("X-Llmgw-Cache", "hit") - if cached.ContentType != "" { - sw.Header().Set("Content-Type", cached.ContentType) - } - sw.WriteHeader(cached.Status) - _, _ = sw.Write(cached.Body) - return - } - } - } - var lastErr error var lastProviderName string for i, cand := range candidates { @@ -400,22 +379,29 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop } g.warnf("%s: failing over from provider %q to %q for model %q%s", logPrefix, candidates[i-1].providerName, cand.providerName, requestedModel, suffix) } - } - // F2 fix (adversarial-review): clear every header a PRIOR - // candidate's own writer may have staged on sw.Header() without - // committing it — newSSEWriter (sse.go) sets Content-Type: text/ - // event-stream, Cache-Control: no-cache, and X-Accel-Buffering: - // no the moment a streaming attempt starts, before any byte is - // written. If that candidate then fails before writing (still - // safe to fail over, THE HARD CONSTRAINT below), those staged - // headers must not survive into a plain-JSON response from the - // next candidate. Safe to clear unconditionally: this loop only - // ever reaches a new iteration while sw.wroteHeader is false, so - // nothing committed is ever at risk of being wiped. - hdr := sw.Header() - for k := range hdr { - hdr.Del(k) + // F2 fix, correctly scoped (adversarial-review round 2 — the + // round-1 version cleared unconditionally, including i == 0, + // which deleted response headers a DIFFERENT, earlier Traefik + // middleware had already staged before this plugin ever ran + // (CORS, HSTS, request-id, ...) on the default, non-failover + // path — proven by differential harness, and it contradicted + // this package's own "byte-identical to a gateway built + // before this field existed" claim. Only i > 0 needs this at + // all: newSSEWriter (sse.go) sets Content-Type: text/event- + // stream, Cache-Control: no-cache, and X-Accel-Buffering: no + // the moment a streaming attempt starts, before any byte is + // written, and if THAT PRIOR candidate then failed before + // writing (still safe to fail over, THE HARD CONSTRAINT + // below), those staged headers must not survive into a + // plain-JSON response from THIS candidate. Safe to clear + // unconditionally once i > 0: this loop only ever reaches a + // new iteration while sw.wroteHeader is false, so nothing + // committed is ever at risk of being wiped. + hdr := sw.Header() + for k := range hdr { + hdr.Del(k) + } } // attemptReq is a FRESH top-level copy of pristineReq for every @@ -432,17 +418,40 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop } attemptReq[gatewayAliasKey] = requestedModel - // cacheKeyStr is still computed per candidate — cache - // correctness (brief): a response produced by a failover - // provider must be stored under THAT provider's own key, never - // the first candidate's. Computed from pristineReq (matching the - // pre-pass above exactly): gatewayAliasKey is a purely internal - // echo-back mechanism cacheKey does not itself strip (its own - // doc comment, cache.go), so hashing attemptReq instead would - // add noise no two requests would otherwise disagree on. + // Cache lookup for THIS candidate, immediately before its own + // attempt (adversarial-review round 2 ruling — supersedes the + // round-1 "check every candidate's cache up front" pre-pass, + // which silently substituted ANOTHER candidate's cached response + // while the actual primary was perfectly healthy: proven — + // alpha healthy and serving a distinct body, never called, the + // client instead got beta's cached body with no failure and no + // "failing over" log line, i.e. failover with no failure. Health + // (F3/F6, this file/failover.go) is the correct mechanism for + // "do not call a dead primary": once a genuinely broken + // candidate is marked request-unhealthy it is excluded from the + // candidate list entirely (orderedFailoverCandidates, + // failover.go), so the NEXT candidate's cache is still reached + // on a later request without ever dialing the dead one — the + // pre-pass solved that problem by accident, with the side + // effect of mis-serving healthy traffic from an unrelated + // candidate's cache. cacheKeyStr is computed from pristineReq, + // BEFORE gatewayAliasKey was injected above would have mattered + // — gatewayAliasKey is a purely internal echo-back mechanism + // cacheKey does not itself strip (its own doc comment, + // cache.go), so hashing attemptReq instead would add noise no + // two requests would otherwise disagree on. var cacheKeyStr string if cacheable { cacheKeyStr = cacheKey(cand.providerName, cand.upstreamModel, requestedModel, endpoint, pristineReq) + if cached, hit := g.cache.lookup(cacheKeyStr); hit { + sw.Header().Set("X-Llmgw-Cache", "hit") + if cached.ContentType != "" { + sw.Header().Set("Content-Type", cached.ContentType) + } + sw.WriteHeader(cached.Status) + _, _ = sw.Write(cached.Body) + return + } // Set before call() below writes anything — headers must // precede the body a miss is about to produce (spec §2). A // later iteration's own miss overwrites this harmlessly: it @@ -494,8 +503,16 @@ func (g *Gateway) runMeteredCall(sw *statusTrackingWriter, r *http.Request, scop // (everything except HTTP 400 is a failure) is reused directly, // not limiter's narrower isTransient/isDeadlineExceeded one, so // a provider answering 401/403/404 to every request is - // eventually routed around instead of retried forever. - g.failoverHealth.record(providerName, !failoverEligible(callErr)) + // eventually routed around instead of retried forever. Guarded + // by g.failover.enabled (adversarial-review round 2, finding 3): + // this state is never READ on the disabled path + // (orderedFailoverCandidates' own early return never calls + // healthy()), so recording it there was dead work plus a new + // global-mutex contention point (requestHealthTracker.stateFor's + // own t.mu) on EVERY metered request, disabled or not. + if g.failover.enabled { + g.failoverHealth.record(providerName, !failoverEligible(callErr)) + } // Usage is accounted before the error branch below runs, not // after: every adapter that can fail mid-stream (forwardStream