diff --git a/registry.go b/registry.go index 5ffdf1c..a8f55fd 100644 --- a/registry.go +++ b/registry.go @@ -50,6 +50,22 @@ const defaultBreakerOpenDuration = time.Minute // MaxOpenDuration is left empty. const defaultBreakerMaxOpenDuration = 30 * time.Minute +// maxBreakerOpenDuration bounds BreakerConfig.MaxOpenDuration — the same +// ceiling-on-a-config-value precedent maxBreakerFailureThreshold already +// sets above, applied here because openBreakerLocked's backoff doubling +// has no overflow guard of its own: it relies on breakerOpenMax staying +// small enough that doubling a value already at or below it can never +// approach time.Duration's int64-nanosecond range. A pathological operator +// value with no ceiling (adversarial-review finding, round 2) would let +// repeated doublings wrap negative after ~28 probes, silently DISABLING +// backoff (a negative openUntil is always in the past). 24 hours is +// already an extremely generous cap — far longer than any deployment +// should ever want a single provider re-probed — and doubling twice from +// it (worst case before the cap check fires) lands nowhere near int64 +// overflow, so the ceiling alone is the fix; openBreakerLocked itself +// needs no separate runtime guard. +const maxBreakerOpenDuration = 24 * time.Hour + // errModelUnknown is returned by modelRegistry.resolve when id does not // match any provider's known model set (explicit config plus the last // successful discovery fetch) — 404 semantics for the caller. @@ -223,14 +239,22 @@ func (st *providerState) setDiscoveredContext(m map[string]int) { // second one. The caller must pair a true result with a later finishRefresh // call. // -// A closed breaker (feat/provider-health) behaves EXACTLY as before this -// feature existed: now far enough past lastRefresh (or this is the first -// refresh) is the only gate — a provider that never fails never touches the -// breaker branch below at all. An open breaker replaces that interval gate -// with its own backoff window: refresh stays blocked until now reaches -// openUntil, regardless of interval, and reaching it starts exactly one -// half-open probe (state moves to halfOpen here, before the caller's fetch -// even runs) rather than resuming normal per-interval refreshing outright — +// The interval gate (now far enough past lastRefresh, or this is the first +// refresh) applies UNCONDITIONALLY, closed or open alike — this is the +// fix for the adversarial-review blocker (round 2): an EARLIER version of +// this method let an open breaker's own backoff window (openUntil) REPLACE +// the interval gate rather than add to it, and since +// defaultBreakerMaxOpenDuration (30m) is shorter than defaultDiscoveryInterval +// (1h), that let a permanently broken provider get probed roughly TWICE as +// often as before this feature existed — the exact opposite of "back off". +// Backoff can only ever make retries LESS frequent than the plain interval, +// never more: a closed breaker is gated by interval alone (exactly as +// before this feature existed — a provider that never fails never touches +// the breaker branch below at all); an open breaker is additionally gated +// by openUntil, so whichever of the two — interval or backoff — is +// currently longer wins. Reaching both starts exactly one half-open probe +// (state moves to halfOpen here, before the caller's fetch even runs) +// rather than resuming normal per-interval refreshing outright — // recordHealthLocked decides whether that probe closes the breaker or // reopens it with a longer backoff. func (st *providerState) tryBeginRefresh(now time.Time) bool { @@ -239,16 +263,14 @@ func (st *providerState) tryBeginRefresh(now time.Time) bool { if st.inFlight { return false } + if now.Sub(st.lastRefresh) < st.interval { + return false + } if st.health == breakerOpen { if now.Before(st.openUntil) { return false } st.health = breakerHalfOpen - st.inFlight = true - return true - } - if now.Sub(st.lastRefresh) < st.interval { - return false } st.inFlight = true return true @@ -286,17 +308,48 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) { // provider-health) from one finished refresh attempt's outcome. Caller must // already hold st.mu. // -// failed reuses limits.go's existing recordProviderAttempt classification — -// isTransient(resp, err) with a nil *http.Response (a discovery fetch has no -// response to inspect on a transport-level failure, unlike a proxied -// request) plus isDeadlineExceeded(err) — rather than inventing a second -// one, per this feature's own design mandate. Net effect: any non-nil error -// except a caller-canceled context counts against the provider, the same -// line recordProviderAttempt already draws for request-path attempts. +// context.Canceled is NEUTRAL — checked first, before any classification — +// per adversarial-review ruling (round 2): a canceled discovery fetch +// reflects the CALLER walking away (plugin shutdown, an outer context +// canceled upstream), not the provider's fault. It must not count as a +// FAILURE (would wrongly trip an otherwise-healthy provider's breaker on a +// well-timed shutdown) and — the sharper bug an earlier version of this +// method had — must not count as a SUCCESS either: treating it as success +// would CLOSE an already-open breaker and wipe its backoff/counters purely +// because a half-open probe got interrupted before it could complete, +// reporting a still-broken provider as healthy again. Every field this +// method could touch is left exactly as it was; only lastRefresh/lastErr +// (finishRefresh, above, unconditionally) and inFlight change. If health +// is still halfOpen afterward, the NEXT real attempt (admitted immediately +// — see tryBeginRefresh's own doc comment: a halfOpen provider is not +// gated by openUntil at all, only by the plain interval) re-decides the +// breaker through this same function, so a canceled probe simply delays +// the decision by one cycle rather than making a wrong one now. +// +// failed classifies every OTHER error explicitly for provider health, +// deliberately NOT reusing isTransient's retry-shaped semantics (retry.go) +// wholesale — an earlier version of this function did, and adversarial +// review found the gap: isTransient returns false for errRequestBuildFailed +// (retrying a malformed request build within the SAME attempt would never +// help, correct for RETRY purposes), so a provider whose baseURL is +// malformed — nothing validates it at construction — built errRequestBuildFailed +// on every single discovery cycle forever, and the old code routed that +// straight to closeBreakerLocked, wiping state every time: the breaker +// could never open despite continuous, permanent failure. For HEALTH +// purposes there is no such nuance to preserve: any non-nil, non-canceled +// error means this discovery attempt did not succeed, full stop. // // While closed, breakerThreshold consecutive failures open the breaker at // its base backoff (breakerOpenBase); any success resets the counter to -// zero. While halfOpen (the only other state finishRefresh can observe — +// zero. breakerThreshold <= 0 (unreachable through newModelRegistry, which +// always resolves it to >= 1 via validateBreakerConfig, but reachable from +// a providerState assembled directly — every test in this file does that) +// disables the breaker entirely rather than tripping on the very first +// failure with a zero backoff: this is what makes providerState's own +// documented "zero value behaves exactly as before" claim (its own doc +// comment, above) actually true, not just aspirational. +// +// While halfOpen (the only other state finishRefresh can observe — // tryBeginRefresh never starts a refresh from open without first moving to // halfOpen), a successful probe closes the breaker outright; a failed one // reopens it with a DOUBLED backoff, capped at breakerOpenMax — the @@ -304,7 +357,10 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) { // xiaomi's ongoing 401s) from being re-probed every single interval // forever. func (st *providerState) recordHealthLocked(now time.Time, err error) { - failed := isTransient(nil, err) || isDeadlineExceeded(err) + if errors.Is(err, context.Canceled) { + return + } + failed := err != nil if st.health == breakerHalfOpen { if failed { st.openBreakerLocked(now, true) @@ -317,6 +373,9 @@ func (st *providerState) recordHealthLocked(now time.Time, err error) { st.closeBreakerLocked() return } + if st.breakerThreshold <= 0 { + return + } st.consecutiveFailures++ if st.consecutiveFailures >= st.breakerThreshold { st.openBreakerLocked(now, false) @@ -355,10 +414,12 @@ func (st *providerState) openBreakerLocked(now time.Time, escalate bool) { st.openUntil = now.Add(st.backoff) } -// usable reports whether st's discovery circuit breaker currently permits -// routing to this provider — see modelRegistry.providerUsable's doc comment -// for the full contract this backs. -func (st *providerState) usable() bool { +// discoveryHealthy reports whether st's DISCOVERY circuit breaker is +// currently closed or half-open (not open) — see modelRegistry. +// discoveryHealthy's doc comment for the full contract this backs, in +// particular the scope limitation its name is deliberately chosen to +// signal: this reflects the discovery endpoint only. +func (st *providerState) discoveryHealthy() bool { st.mu.Lock() defer st.mu.Unlock() return st.health != breakerOpen @@ -533,6 +594,12 @@ func validateBreakerConfig(bc BreakerConfig) (breakerConfig, error) { if d <= 0 { return breakerConfig{}, fmt.Errorf("llmgateway: breaker.maxOpenDuration must be positive, got %q", bc.MaxOpenDuration) } + // Ceiling (adversarial-review finding, round 2): see + // maxBreakerOpenDuration's own doc comment for why an unbounded + // value here is an overflow risk, not merely an odd config choice. + if d > maxBreakerOpenDuration { + return breakerConfig{}, fmt.Errorf("llmgateway: breaker.maxOpenDuration must be at most %s, got %q", maxBreakerOpenDuration, bc.MaxOpenDuration) + } openMax = d } @@ -704,17 +771,29 @@ func (m *modelRegistry) now() time.Time { return m.nowFn() } -// providerUsable reports whether name's discovery circuit breaker -// currently permits routing to it (feat/provider-health's queryable -// health signal — the predicate feat/failover consults before sending a -// request to a provider). false only while the breaker is OPEN: an -// unconfigured provider name, one with discovery disabled (it has no -// health signal to trip the breaker with, so it stays closed forever), -// and one whose breaker is closed OR half-open all report true — a -// half-open provider is actively being re-probed by the next discovery -// cycle and must stay routable, per this feature's own "a provider that -// recovers must never stay dead" requirement; only an OPEN breaker, still -// backing off, is unusable. +// discoveryHealthy reports whether name's DISCOVERY circuit breaker is +// currently closed or half-open (feat/provider-health's queryable health +// signal). false only while the breaker is OPEN: an unconfigured provider +// name, one with discovery disabled (it has no health signal to trip the +// breaker with, so it stays closed forever), and one whose breaker is +// closed OR half-open all report true — a half-open provider is actively +// being re-probed by the next discovery cycle and must stay routable, per +// this feature's own "a provider that recovers must never stay dead" +// requirement; only an OPEN breaker, still backing off, reports false. +// +// SCOPE, per adversarial-review ruling (round 2) — read this before wiring +// it into any routing decision: this reflects the DISCOVERY endpoint +// (/v1/models-equivalent) ONLY. It is driven exclusively by +// finishRefresh's own outcomes and knows nothing about +// /v1/chat/completions or any other request-path traffic. A provider +// whose discovery listing 401s or 404s while its actual completion +// endpoint serves requests perfectly fine — common behind a proxy that +// exposes chat but not model listing — reports false here despite being +// entirely usable for real traffic. feat/failover MUST NOT treat a false +// result here as a sole reason to refuse routing; request-path health is +// a SEPARATE signal, fed by limiter.recordProviderAttempt (limits.go), +// deliberately kept apart so discovery outages and request-path outages +// are never conflated under one name. // // This is a single mutex-guarded read of state finishRefresh/ // tryBeginRefresh already maintain — no round trip, no store lookup — per @@ -722,12 +801,12 @@ func (m *modelRegistry) now() time.Time { // rather than a correctness guarantee. Three pods learning independently // (each with its own view of a provider's health) is accepted and // intended, not a bug. -func (m *modelRegistry) providerUsable(name string) bool { +func (m *modelRegistry) discoveryHealthy(name string) bool { st, ok := m.states[name] if !ok { return true } - return st.usable() + return st.discoveryHealthy() } // finishRefresh records the outcome of a refresh attempt (st.finishRefresh) diff --git a/registry_health_test.go b/registry_health_test.go index 502e53a..74567c0 100644 --- a/registry_health_test.go +++ b/registry_health_test.go @@ -3,6 +3,7 @@ package traefikllmgateway import ( "context" "errors" + "sync" "testing" "time" ) @@ -80,6 +81,18 @@ func TestValidateBreakerConfig_Table(t *testing.T) { wantOpen: 5 * time.Minute, wantMaxOpen: 5 * time.Minute, }, + { + name: "maxOpenDuration at the ceiling is accepted", + bc: BreakerConfig{MaxOpenDuration: "24h"}, + wantThresh: defaultBreakerFailureThreshold, + wantOpen: defaultBreakerOpenDuration, + wantMaxOpen: 24 * time.Hour, + }, + { + name: "maxOpenDuration above the ceiling is an error", + bc: BreakerConfig{MaxOpenDuration: "25h"}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -131,14 +144,14 @@ func TestProviderState_Breaker_OpensAfterThreshold(t *testing.T) { for i := 0; i < 2; i++ { st.finishRefresh(now, nil, boom) - if !st.usable() { - t.Fatalf("after %d failure(s) (threshold 3): usable() = false, want true — must not open early", i+1) + if !st.discoveryHealthy() { + t.Fatalf("after %d failure(s) (threshold 3): discoveryHealthy() = false, want true — must not open early", i+1) } } st.finishRefresh(now, nil, boom) // 3rd consecutive failure reaches the threshold - if st.usable() { - t.Fatal("after reaching the failure threshold: usable() = true, want false (breaker must be open)") + if st.discoveryHealthy() { + t.Fatal("after reaching the failure threshold: discoveryHealthy() = true, want false (breaker must be open)") } } @@ -158,8 +171,112 @@ func TestProviderState_Breaker_ASuccessResetsTheFailureCounter(t *testing.T) { st.finishRefresh(now, nil, boom) st.finishRefresh(now, nil, boom) - if !st.usable() { - t.Error("usable() = false, want true — the intervening success must have reset the consecutive-failure streak") + if !st.discoveryHealthy() { + t.Error("discoveryHealthy() = false, want true — the intervening success must have reset the consecutive-failure streak") + } +} + +// TestProviderState_Breaker_ErrRequestBuildFailed_CountsAsFailure is the +// adversarial-review IMPORTANT-2 regression: an earlier version of +// recordHealthLocked reused isTransient's RETRY-shaped classification +// wholesale (retry.go) — which deliberately excludes errRequestBuildFailed, +// since retrying a malformed request build within the same attempt would +// never help. That is correct for retrying, but wrong for HEALTH: a +// provider whose baseURL is malformed (nothing validates it at +// construction) produces this exact error on every discovery cycle, +// forever. The old code routed that straight to a "success" outcome, +// resetting the breaker every time — it could never open despite +// continuous, permanent failure. It must now count as a plain failure. +func TestProviderState_Breaker_ErrRequestBuildFailed_CountsAsFailure(t *testing.T) { + t.Parallel() + st := newHealthTestState(time.Hour, 3, time.Minute, 10*time.Minute) + now := time.Now() + + for i := 0; i < 3; i++ { + st.finishRefresh(now, nil, errRequestBuildFailed) + } + + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = true after 3 consecutive errRequestBuildFailed outcomes, want false — a persistent request-build failure is a provider fault the breaker must open on") + } +} + +// --- breaker: the zero value never trips (adversarial-review finding, +// round 2 — providerState's own doc comment claims "zero value behaves +// exactly as before this feature existed"; without this guard that claim +// was false: a zero-value breaker opened on the FIRST failure with a +// ZERO backoff, after which the interval throttle was gone too) --- + +func TestProviderState_Breaker_ZeroThreshold_NeverOpens(t *testing.T) { + t.Parallel() + // A providerState assembled directly (bypassing newModelRegistry, + // which always resolves breakerThreshold to >= 1 via + // validateBreakerConfig — every test in this file up to here does + // exactly that, through newHealthTestState) leaves every breaker + // field at its Go zero value: this is that case, deliberately. + st := &providerState{ + explicit: make(map[string]bool), + discovered: make(map[string]bool), + interval: time.Millisecond, + } + now := time.Now() + boom := errors.New("boom") + + for i := 0; i < 50; i++ { + if !st.tryBeginRefresh(now) { + t.Fatalf("iteration %d: tryBeginRefresh = false, want true — a zero-value breaker must behave like plain interval-only gating, never blocked by its own (disabled) backoff", i) + } + st.finishRefresh(now, nil, boom) + if !st.discoveryHealthy() { + t.Fatalf("iteration %d: discoveryHealthy() = false, want true — a zero-value breaker must never open at all", i) + } + now = now.Add(time.Millisecond) + } +} + +// --- breaker: discovery backoff must never retry MORE often than plain +// interval throttling (BLOCKING adversarial-review finding, round 2) --- + +// TestProviderState_Breaker_DefaultCadence_NoWorseThanPreFeatureBaseline +// simulates 24 hours of a permanently-failing provider under every +// DEFAULT setting (the shipped configuration, the xiaomi case this +// feature exists for) and counts actual ADMITTED refresh attempts. +// +// An earlier version of tryBeginRefresh let an open breaker's own backoff +// window (openUntil) REPLACE the plain interval gate instead of adding to +// it. Since defaultBreakerMaxOpenDuration (30m) is shorter than +// defaultDiscoveryInterval (1h), that measured 50 admitted attempts here — +// MORE than the 24 a plain, breaker-less interval gate would ever produce +// for the same 24h window. For a permanently broken provider that means +// MORE upstream 401s and MORE ERROR lines than today, the exact inverse of +// this feature's stated purpose. tryBeginRefresh now applies the interval +// gate unconditionally (closed or open alike), so backoff can only ever +// make retries LESS frequent, never more — and because +// defaultBreakerMaxOpenDuration is itself shorter than +// defaultDiscoveryInterval, the interval remains the sole binding +// constraint under pure defaults, so this asserts EQUALITY with the +// baseline, not just "no worse". +func TestProviderState_Breaker_DefaultCadence_NoWorseThanPreFeatureBaseline(t *testing.T) { + t.Parallel() + st := newHealthTestState(defaultDiscoveryInterval, defaultBreakerFailureThreshold, defaultBreakerOpenDuration, defaultBreakerMaxOpenDuration) + base := time.Now() + boom := errors.New("boom: persistent 401") + + var admits int + for minute := 0; minute < 24*60; minute++ { + now := base.Add(time.Duration(minute) * time.Minute) + if st.tryBeginRefresh(now) { + admits++ + st.finishRefresh(now, nil, boom) + } + } + + // The pre-feature baseline: a plain interval-only gate (no breaker at + // all) admits exactly one attempt per defaultDiscoveryInterval (1h) + // over a 24h window for a provider that never succeeds. + const preFeatureBaseline = 24 + if admits > preFeatureBaseline { + t.Errorf("admitted %d discovery attempts over 24h with default config and a permanently failing provider, want <= %d (the pre-feature, interval-only baseline) — the breaker must never retry MORE often than plain interval throttling", admits, preFeatureBaseline) } } @@ -177,8 +294,8 @@ func TestProviderState_Breaker_StaysOpenForBackoffWindow(t *testing.T) { t.Fatal("first tryBeginRefresh on a fresh provider = false, want true") } st.finishRefresh(base, nil, errors.New("boom")) // trips the breaker (threshold 1) - if st.usable() { - t.Fatal("usable() = true after the trip, want false") + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = true after the trip, want false") } if ok := st.tryBeginRefresh(base.Add(6 * time.Second)); ok { @@ -190,8 +307,8 @@ func TestProviderState_Breaker_StaysOpenForBackoffWindow(t *testing.T) { if ok := st.tryBeginRefresh(base.Add(61 * time.Second)); !ok { t.Fatal("tryBeginRefresh once the backoff window has elapsed = false, want true (half-open probe should start)") } - if !st.usable() { - t.Error("usable() during the half-open probe = false, want true — a provider being re-probed must stay routable") + if !st.discoveryHealthy() { + t.Error("discoveryHealthy() during the half-open probe = false, want true — a provider being re-probed must stay routable") } } @@ -210,8 +327,8 @@ func TestProviderState_Breaker_HalfOpenProbe_SuccessCloses(t *testing.T) { } st.finishRefresh(probeAt, []string{"m1"}, nil) // probe succeeds - if !st.usable() { - t.Error("usable() after a successful half-open probe = false, want true — a recovered provider must not stay dead") + if !st.discoveryHealthy() { + t.Error("discoveryHealthy() after a successful half-open probe = false, want true — a recovered provider must not stay dead") } if !st.hasModel("m1") { t.Error("hasModel(m1) = false after a successful probe, want true — discovery still updates the model set normally") @@ -243,8 +360,8 @@ func TestProviderState_Breaker_HalfOpenProbe_FailureReopens_Escalates(t *testing } st.finishRefresh(probeAt, nil, errors.New("still broken")) // probe fails: reopen, backoff doubles to 2m - if st.usable() { - t.Fatal("usable() after a failed probe = true, want false") + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() after a failed probe = true, want false") } if ok := st.tryBeginRefresh(probeAt.Add(61 * time.Second)); ok { t.Error("tryBeginRefresh after only the ORIGINAL (1m) backoff = true, want false — a failed probe must double it to 2m") @@ -279,19 +396,67 @@ func TestProviderState_Breaker_BackoffCapsAtOpenMax(t *testing.T) { } } -// --- breaker: a context.Canceled discovery error is not the provider's -// fault, and must not count toward the failure threshold (reuses limits.go's -// isTransient/isDeadlineExceeded classification, which draws this same -// line for request-path attempts) --- +// --- breaker: context.Canceled is NEUTRAL, neither failure nor success +// (IMPORTANT adversarial-review finding, round 2) --- -func TestProviderState_Breaker_ContextCanceled_NeverCountsAsFailure(t *testing.T) { +// TestProviderState_Breaker_ContextCanceled_IsNeutral_NotSuccess replaces +// a vacuous predecessor that only asserted discoveryHealthy()==true after +// ONE Canceled outcome on a threshold-1 breaker — true identically whether +// Canceled is ignored (correct) or treated as a success (the bug). This +// sequence distinguishes the two: fail, fail, Canceled, fail on a +// threshold-3 breaker opens it only if Canceled did NOT reset the +// consecutive-failure streak. +func TestProviderState_Breaker_ContextCanceled_IsNeutral_NotSuccess(t *testing.T) { t.Parallel() - st := newHealthTestState(time.Hour, 1, time.Minute, 10*time.Minute) + st := newHealthTestState(time.Second, 3, time.Minute, 10*time.Minute) now := time.Now() + boom := errors.New("boom") - st.finishRefresh(now, nil, context.Canceled) - if !st.usable() { - t.Error("usable() = false after a context.Canceled discovery error, want true — a canceled attempt is not a provider fault") + st.finishRefresh(now, nil, boom) // failure 1 + st.finishRefresh(now, nil, boom) // failure 2 + st.finishRefresh(now, nil, context.Canceled) // neutral: must NOT reset the streak + st.finishRefresh(now, nil, boom) // failure 3: reaches the threshold only if Canceled was neutral + + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = true after fail,fail,Canceled,fail on a threshold-3 breaker, want false — Canceled must not reset the consecutive-failure streak (that would treat cancellation as a success)") + } +} + +// TestProviderState_Breaker_ContextCanceled_DuringHalfOpen_PreservesBackoff +// is the sharper half of the same finding: an earlier version treated a +// Canceled half-open probe as a SUCCESS, which CLOSED an already-open +// breaker and wiped its backoff — reporting a still-401ing provider +// healthy purely because a probe was interrupted at shutdown. This proves +// the backoff survives: the NEXT real failure must escalate from the +// ORIGINAL backoff, not restart fresh at the base. +func TestProviderState_Breaker_ContextCanceled_DuringHalfOpen_PreservesBackoff(t *testing.T) { + t.Parallel() + st := newHealthTestState(time.Second, 1, time.Minute, 10*time.Minute) + base := time.Now() + st.tryBeginRefresh(base) + st.finishRefresh(base, nil, errors.New("boom")) // opens, backoff = 1m, openUntil = base+1m + + probeAt := base.Add(61 * time.Second) + if !st.tryBeginRefresh(probeAt) { + t.Fatal("tryBeginRefresh after the backoff window = false, want true") + } + st.finishRefresh(probeAt, nil, context.Canceled) // probe interrupted at shutdown: must be neutral + + // The next real attempt must see the ORIGINAL backoff still in + // force, not a wiped/reset one: if Canceled had (incorrectly) closed + // the breaker, this next failure would re-open it FRESH at the 1m + // base instead of escalating to 2m. + retryAt := probeAt.Add(2 * time.Second) + if !st.tryBeginRefresh(retryAt) { + t.Fatal("tryBeginRefresh right after a canceled probe = false, want true (interval elapsed, not blocked by the cancel)") + } + st.finishRefresh(retryAt, nil, errors.New("still broken")) + + if ok := st.tryBeginRefresh(retryAt.Add(119 * time.Second)); ok { + t.Error("tryBeginRefresh before the escalated 2m backoff = true, want false — a canceled probe must not have reset the backoff to its 1m base") + } + if ok := st.tryBeginRefresh(retryAt.Add(121 * time.Second)); !ok { + t.Error("tryBeginRefresh after the escalated 2m backoff = false, want true") } } @@ -300,7 +465,8 @@ func TestProviderState_Breaker_ContextCanceled_NeverCountsAsFailure(t *testing.T func TestProviderState_Breaker_HealthyProviderNeverAffected(t *testing.T) { t.Parallel() - st := newHealthTestState(time.Hour, defaultBreakerFailureThreshold, defaultBreakerOpenDuration, defaultBreakerMaxOpenDuration) + const interval = time.Hour + st := newHealthTestState(interval, defaultBreakerFailureThreshold, defaultBreakerOpenDuration, defaultBreakerMaxOpenDuration) now := time.Now() for i := 0; i < 5; i++ { @@ -308,25 +474,63 @@ func TestProviderState_Breaker_HealthyProviderNeverAffected(t *testing.T) { t.Fatalf("cycle %d: tryBeginRefresh = false, want true (interval already elapsed)", i) } st.finishRefresh(now, []string{"m1"}, nil) - if !st.usable() { - t.Fatalf("cycle %d: usable() = false, want true — a never-failing provider must never trip its breaker", i) + if !st.discoveryHealthy() { + t.Fatalf("cycle %d: discoveryHealthy() = false, want true — a never-failing provider must never trip its breaker", i) } - if !st.openUntil.IsZero() { - t.Fatalf("cycle %d: openUntil = %v, want zero — breaker must never have opened", i, st.openUntil) + // Behavioral proxy for "the breaker never opened" (assert + // behavior, not the internal openUntil field, per the brief): + // an opened breaker would admit again as soon as its own + // (likely shorter) backoff elapsed. Checking one instant BEFORE + // the plain interval elapses proves nothing shorter is gating + // this provider. + if ok := st.tryBeginRefresh(now.Add(interval - time.Nanosecond)); ok { + t.Fatalf("cycle %d: tryBeginRefresh one nanosecond before the interval elapses = true, want false", i) } - // Immediately after: interval-gated exactly like before this - // feature existed, no breaker-induced extra wait. if ok := st.tryBeginRefresh(now); ok { t.Fatalf("cycle %d: tryBeginRefresh immediately after a fresh refresh = true, want false (interval not elapsed)", i) } - now = now.Add(2 * time.Hour) // past the interval for the next cycle + now = now.Add(2 * interval) // past the interval for the next cycle } } -// --- modelRegistry.providerUsable: the exported predicate feat/failover +// --- breaker: concurrent access is race-free (IMPORTANT adversarial- +// review finding, round 2 — no prior test in this file ever exercised the +// mutex-guarded breaker fields from more than one goroutine, so -race had +// nothing to catch a locking bug with; the locking was correct, but +// unproven) --- + +func TestProviderState_Breaker_ConcurrentRefreshes_RaceFree(t *testing.T) { + t.Parallel() + st := newHealthTestState(time.Millisecond, 3, 2*time.Millisecond, 20*time.Millisecond) + base := time.Now() + + const workers = 8 + const iterations = 500 + var wg sync.WaitGroup + for w := 0; w < workers; w++ { + wg.Add(1) + go func(worker int) { + defer wg.Done() + for i := 0; i < iterations; i++ { + now := base.Add(time.Duration(worker*iterations+i) * time.Microsecond) + if st.tryBeginRefresh(now) { + var err error + if (worker+i)%2 == 0 { + err = errors.New("boom") + } + st.finishRefresh(now, []string{"m1"}, err) + } + _ = st.discoveryHealthy() + } + }(w) + } + wg.Wait() +} + +// --- modelRegistry.discoveryHealthy: the exported predicate feat/failover // consumes --- -func TestModelRegistry_ProviderUsable(t *testing.T) { +func TestModelRegistry_DiscoveryHealthy(t *testing.T) { t.Parallel() adapters := map[string]providerAdapter{ "flaky": newFakeAdapter("flaky"), @@ -346,21 +550,21 @@ func TestModelRegistry_ProviderUsable(t *testing.T) { now := time.Now() reg.nowFn = func() time.Time { return now } - if !reg.providerUsable("flaky") || !reg.providerUsable("healthy") { - t.Fatal("a freshly constructed registry's providers must both be usable") + if !reg.discoveryHealthy("flaky") || !reg.discoveryHealthy("healthy") { + t.Fatal("a freshly constructed registry's providers must both be discovery-healthy") } - if !reg.providerUsable("does-not-exist") { - t.Error(`providerUsable("does-not-exist") = false, want true — an unconfigured name is not a reason to block routing`) + if !reg.discoveryHealthy("does-not-exist") { + t.Error(`discoveryHealthy("does-not-exist") = false, want true — an unconfigured name is not a reason to block routing`) } boom := errors.New("boom") reg.states["flaky"].finishRefresh(now, nil, boom) reg.states["flaky"].finishRefresh(now, nil, boom) // reaches FailureThreshold: 2 - if reg.providerUsable("flaky") { - t.Error(`providerUsable("flaky") = true after tripping its breaker, want false`) + if reg.discoveryHealthy("flaky") { + t.Error(`discoveryHealthy("flaky") = true after tripping its breaker, want false`) } - if !reg.providerUsable("healthy") { - t.Error(`providerUsable("healthy") = false, want true — a sibling provider's breaker must not affect it`) + if !reg.discoveryHealthy("healthy") { + t.Error(`discoveryHealthy("healthy") = false, want true — a sibling provider's breaker must not affect it`) } }