diff --git a/registry.go b/registry.go index a8f55fd..205a643 100644 --- a/registry.go +++ b/registry.go @@ -47,8 +47,18 @@ const defaultBreakerOpenDuration = time.Minute // defaultBreakerMaxOpenDuration caps the exponential backoff the breaker // doubles into on every further failed probe, applied when BreakerConfig. -// MaxOpenDuration is left empty. -const defaultBreakerMaxOpenDuration = 30 * time.Minute +// MaxOpenDuration is left empty. Deliberately LONGER than +// defaultDiscoveryInterval (1h) — design ruling, adversarial review round +// 3: a breaker whose backoff can never exceed the plain interval cannot +// suppress anything, since tryBeginRefresh's interval gate (see its own +// doc comment) already applies unconditionally and only the LONGER of the +// two ever binds. An earlier value here (30m, shorter than the 1h +// interval) made the breaker cadence-neutral under stock config — exactly +// as strict an interval-only gate would already produce, with none of the +// suppression this feature exists to provide. At 6h, a permanently broken +// provider under every default backs off 1h, 2h, 4h, 6h, 6h, ... — roughly +// 5 attempts a day instead of 24. +const defaultBreakerMaxOpenDuration = 6 * time.Hour // maxBreakerOpenDuration bounds BreakerConfig.MaxOpenDuration — the same // ceiling-on-a-config-value precedent maxBreakerFailureThreshold already @@ -240,23 +250,32 @@ func (st *providerState) setDiscoveredContext(m map[string]int) { // call. // // 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 — +// refresh) applies UNCONDITIONALLY, closed or open alike — 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. Since that round's default +// maxOpenDuration was shorter than the default interval, a permanently +// broken provider got 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. +// +// PRACTICAL CONSEQUENCE, design ruling (round 3): whichever of interval +// and maxOpenDuration is larger is the one that actually governs a +// persistently broken provider's re-probe cadence — the smaller of the +// two is masked entirely, never merely "adds a little more delay". A +// deployment whose maxOpenDuration does not exceed its discoveryInterval +// gets NO suppression benefit from this feature at all; see +// defaultBreakerMaxOpenDuration's own doc comment for why the shipped +// defaults are chosen to avoid exactly that trap. func (st *providerState) tryBeginRefresh(now time.Time) bool { st.mu.Lock() defer st.mu.Unlock() @@ -317,14 +336,29 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) { // 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. +// reporting a still-broken provider as healthy again. Every OTHER field +// this method could touch is left exactly as it was; only lastRefresh/ +// lastErr (finishRefresh, above, unconditionally) and inFlight change. +// +// A canceled probe while halfOpen is the one exception (round 3, MEDIUM +// finding): health reverts to OPEN with its backoff/openUntil left +// completely untouched, rather than staying at halfOpen. discoveryHealthy +// reports true for halfOpen (by design — a provider actively being +// re-probed must stay routable), so leaving a canceled probe's state at +// halfOpen indefinitely would report a still-broken provider healthy +// until the next attempt happens to land, which the plain interval gate +// alone does not bound tightly. Reverting to open costs nothing extra — +// tryBeginRefresh's own interval gate already runs before this is ever +// reached, so the NEXT attempt is admitted at the identical instant +// either way; only what discoveryHealthy reports in the meantime differs. +// +// context.DeadlineExceeded is NOT given the same neutral treatment as +// Canceled — deliberately: it counts as a plain failure, below, matching +// recordProviderAttempt's own SHOULD-1 ruling (limits.go) that a deadline +// the GATEWAY set and the upstream never answered inside is a genuine +// provider-health signal, not a caller-walked-away one. Three consecutive +// backgroundRefreshTimeout (30s) overruns trip the breaker exactly like +// three plain errors would. // // failed classifies every OTHER error explicitly for provider health, // deliberately NOT reusing isTransient's retry-shaped semantics (retry.go) @@ -352,12 +386,20 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) { // 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 -// exponential escalation that keeps a persistently broken provider (like -// xiaomi's ongoing 401s) from being re-probed every single interval -// forever. +// reopens it with a DOUBLED backoff, capped at breakerOpenMax. This ONLY +// suppresses re-probing when the resulting backoff exceeds discoveryInterval +// — design ruling, round 3: tryBeginRefresh's interval gate (its own doc +// comment) applies regardless of breaker state, so a maxOpenDuration +// shorter than discoveryInterval is masked entirely and this escalation +// changes nothing observable. defaultBreakerMaxOpenDuration is deliberately +// set longer than defaultDiscoveryInterval so this DOES suppress re-probing +// under stock config — see that constant's own doc comment for the +// resulting cadence. func (st *providerState) recordHealthLocked(now time.Time, err error) { if errors.Is(err, context.Canceled) { + if st.health == breakerHalfOpen { + st.health = breakerOpen + } return } failed := err != nil @@ -451,10 +493,30 @@ func (st *providerState) discoveryHealthy() bool { // lastRefresh/lastErr: a concurrent finishRefresh must never be able to // hand the admin dashboard a breaker state that disagrees with the // lastErr/models it is shown alongside. +// +// openUntil is NOT returned raw (round 3 MEDIUM finding): since +// tryBeginRefresh's interval gate applies unconditionally (its own doc +// comment), the REAL next probe time is max(st.openUntil, +// lastRefresh+interval), which can differ sharply from st.openUntil alone +// — under stock defaults, a fresh trip sets st.openUntil to +1m while the +// first admitted probe is actually at +1h, a 59-minute skew an operator +// watching the dashboard would otherwise see as a broken countdown +// (formatUntil renders "in 60s", nothing happens, then the timestamp goes +// stale and the countdown vanishes for the remaining 59 minutes). This +// correction only ever applies while OPEN — st.openUntil is always the +// zero time while closed (closeBreakerLocked) or a past instant while +// half-open (a probe only ever starts once now has already reached it), +// neither of which this dashboard-facing value should be adjusted for. func (st *providerState) snapshot() (models []string, lastRefresh time.Time, lastErr string, health breakerState, openUntil time.Time) { st.mu.Lock() defer st.mu.Unlock() - return st.knownIDsLocked(), st.lastRefresh, st.lastErr, st.health, st.openUntil + effectiveOpenUntil := st.openUntil + if st.health == breakerOpen { + if floor := st.lastRefresh.Add(st.interval); floor.After(effectiveOpenUntil) { + effectiveOpenUntil = floor + } + } + return st.knownIDsLocked(), st.lastRefresh, st.lastErr, st.health, effectiveOpenUntil } // modelRegistry aggregates every configured provider's explicit and diff --git a/registry_health_test.go b/registry_health_test.go index 74567c0..f0d3ee9 100644 --- a/registry_health_test.go +++ b/registry_health_test.go @@ -201,6 +201,28 @@ func TestProviderState_Breaker_ErrRequestBuildFailed_CountsAsFailure(t *testing. } } +// TestProviderState_Breaker_ContextDeadlineExceeded_CountsAsFailure proves +// context.DeadlineExceeded is a plain failure, NOT given Canceled's +// neutral treatment — deliberate, matching recordProviderAttempt's own +// SHOULD-1 ruling (limits.go): a deadline the GATEWAY set and the upstream +// never answered inside is a genuine provider-health signal, unlike a +// caller-canceled context. Three consecutive backgroundRefreshTimeout +// (30s) overruns against a merely slow provider trip the breaker exactly +// like three plain errors would. +func TestProviderState_Breaker_ContextDeadlineExceeded_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, context.DeadlineExceeded) + } + + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = true after 3 consecutive context.DeadlineExceeded outcomes, want false — a deadline the gateway set and the upstream never answered inside is a provider fault, not a neutral cancellation") + } +} + // --- 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 @@ -234,29 +256,39 @@ func TestProviderState_Breaker_ZeroThreshold_NeverOpens(t *testing.T) { } } -// --- breaker: discovery backoff must never retry MORE often than plain -// interval throttling (BLOCKING adversarial-review finding, round 2) --- +// --- breaker: discovery backoff must actually suppress re-probing under +// DEFAULT config, strictly, not merely stay no-worse-than plain interval +// throttling (BLOCKING adversarial-review findings, rounds 2 and 3) --- -// 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. +// TestProviderState_Breaker_DefaultCadence_StrictlyFewerThanPreFeatureBaseline +// 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) { +// Round 2's earlier version of tryBeginRefresh let an open breaker's own +// backoff window (openUntil) REPLACE the plain interval gate instead of +// adding to it. Since that round's defaultBreakerMaxOpenDuration (30m) was +// 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: the exact inverse of this +// feature's purpose. The round-2 fix (interval gate applies +// unconditionally) closed that regression, but left the feature CADENCE- +// NEUTRAL: with defaultBreakerMaxOpenDuration still shorter than +// defaultDiscoveryInterval, the interval alone remained the sole binding +// constraint and this test measured EXACTLY 24 — no worse than baseline, +// but no better either, doing nothing a plain interval gate would not +// already do. +// +// Round 3's design ruling raised defaultBreakerMaxOpenDuration to 6h +// (longer than the 1h interval) specifically so the breaker's backoff can +// eventually become the binding constraint instead of the interval — see +// that constant's own doc comment for the resulting cadence (roughly 5 +// attempts a day). This test now asserts the STRONGER, actually-intended +// property: strictly fewer admits than the pre-feature baseline. Reverting +// EITHER the round-2 interval-gate fix OR the round-3 default bump makes +// this fail — the first by exceeding the baseline, the second by merely +// equaling it. +func TestProviderState_Breaker_DefaultCadence_StrictlyFewerThanPreFeatureBaseline(t *testing.T) { t.Parallel() st := newHealthTestState(defaultDiscoveryInterval, defaultBreakerFailureThreshold, defaultBreakerOpenDuration, defaultBreakerMaxOpenDuration) base := time.Now() @@ -275,8 +307,64 @@ func TestProviderState_Breaker_DefaultCadence_NoWorseThanPreFeatureBaseline(t *t // 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) + if admits >= preFeatureBaseline { + t.Errorf("admitted %d discovery attempts over 24h with default config and a permanently failing provider, want STRICTLY FEWER than %d (the pre-feature, interval-only baseline) — the breaker must actually suppress re-probing under stock config, not merely match plain interval throttling", admits, preFeatureBaseline) + } +} + +// --- breaker: the admin-facing openUntil accounts for the interval floor +// (MEDIUM adversarial-review finding, round 3) --- + +// TestProviderState_Snapshot_OpenUntil_ReflectsIntervalFloor proves +// snapshot()'s emitted openUntil is the REAL next probe time — +// max(raw openUntil, lastRefresh+interval) — not the raw backoff-only +// value. Since tryBeginRefresh's interval gate applies unconditionally, +// a fresh trip's raw openUntil (lastRefresh + breakerOpenBase) can be far +// EARLIER than the actual next admitted attempt whenever breakerOpenBase +// is shorter than interval — measured on pure defaults, a 59-minute skew +// (raw +1m versus the real +1h). An operator watching the dashboard would +// otherwise see "open (in 60s)", nothing happen, and then the countdown +// silently vanish once formatUntil (webui) reads the now-past raw value. +func TestProviderState_Snapshot_OpenUntil_ReflectsIntervalFloor(t *testing.T) { + t.Parallel() + // interval (1h) far longer than the backoff base (1m) — the exact + // shape of the shipped defaults on a fresh trip. + st := newHealthTestState(time.Hour, 1, time.Minute, 10*time.Minute) + base := time.Now() + st.tryBeginRefresh(base) + st.finishRefresh(base, nil, errors.New("boom")) // opens: raw openUntil = base+1m + + _, lastRefresh, _, health, openUntil := st.snapshot() + if health != breakerOpen { + t.Fatalf("health = %v, want breakerOpen", health) + } + want := lastRefresh.Add(time.Hour) + if !openUntil.Equal(want) { + t.Errorf("snapshot openUntil = %v, want %v (lastRefresh + interval, not the raw +1m backoff — tryBeginRefresh will not actually admit a probe before then)", openUntil, want) + } +} + +// TestProviderState_Snapshot_OpenUntil_UnchangedWhenBackoffExceedsInterval +// is the complementary case: once backoff is the LONGER of the two (the +// steady state this feature is designed to reach — see +// defaultBreakerMaxOpenDuration's own doc comment), snapshot's openUntil +// is exactly the raw value, with no floor adjustment needed. +func TestProviderState_Snapshot_OpenUntil_UnchangedWhenBackoffExceedsInterval(t *testing.T) { + t.Parallel() + // interval (1m) far shorter than the backoff base (1h) — backoff is + // already the binding constraint from the very first trip. + st := newHealthTestState(time.Minute, 1, time.Hour, 10*time.Hour) + base := time.Now() + st.tryBeginRefresh(base) + st.finishRefresh(base, nil, errors.New("boom")) // opens: raw openUntil = base+1h + + _, _, _, health, openUntil := st.snapshot() + if health != breakerOpen { + t.Fatalf("health = %v, want breakerOpen", health) + } + want := base.Add(time.Hour) + if !openUntil.Equal(want) { + t.Errorf("snapshot openUntil = %v, want %v (the raw backoff value, unchanged — it already exceeds the interval floor)", openUntil, want) } } @@ -422,13 +510,37 @@ func TestProviderState_Breaker_ContextCanceled_IsNeutral_NotSuccess(t *testing.T } } +// TestProviderState_Breaker_ContextCanceled_IsNeutral_NotFailure is the +// other direction of the bidirectional check (round 3 finding): its +// _NotSuccess sibling above proves Canceled does not reset the streak, +// but says nothing about whether Canceled ITSELF is wrongly counted as a +// failure — a mutant that classified Canceled as a failure would still +// pass that test unchanged, since either way the threshold is reached by +// the trailing plain failure. A LONE Canceled call on a threshold-1 +// breaker settles it directly: if Canceled counted as a failure, one call +// would be enough to trip a threshold of 1. +func TestProviderState_Breaker_ContextCanceled_IsNeutral_NotFailure(t *testing.T) { + t.Parallel() + st := newHealthTestState(time.Hour, 1, time.Minute, 10*time.Minute) + now := time.Now() + + st.finishRefresh(now, nil, context.Canceled) + + if !st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = false after a single context.Canceled outcome on a threshold-1 breaker, want true — Canceled must not be counted as a failure") + } +} + // 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. +// two things: discoveryHealthy reports the provider unhealthy immediately +// (round 3 finding: a canceled probe reverts to OPEN rather than being +// left stuck at halfOpen, which would report healthy), and 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) @@ -442,6 +554,10 @@ func TestProviderState_Breaker_ContextCanceled_DuringHalfOpen_PreservesBackoff(t } st.finishRefresh(probeAt, nil, context.Canceled) // probe interrupted at shutdown: must be neutral + if st.discoveryHealthy() { + t.Fatal("discoveryHealthy() = true immediately after a canceled half-open probe, want false — a still-broken provider must not read healthy just because its re-probe was interrupted") + } + // 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 @@ -499,6 +615,17 @@ func TestProviderState_Breaker_HealthyProviderNeverAffected(t *testing.T) { // nothing to catch a locking bug with; the locking was correct, but // unproven) --- +// TestProviderState_Breaker_ConcurrentRefreshes_RaceFree is primarily a +// -race and panic net (round 2), but round 3 found it carried ZERO +// assertions and so survived every one of the seven fix-reversion +// mutations tried against this file — useful for what it catches, but not +// a behavioral test of its own. It now asserts two invariants on every +// iteration, under the SAME lock discoveryHealthy/tryBeginRefresh already +// take, so the read is race-free too: backoff never exceeds breakerOpenMax +// (the overflow-ceiling property, round 2 finding 7, checked here under +// real concurrent mutation rather than only sequentially), and health is +// always one of the three declared states — breakerState is a bare int32 +// under the hood, so nothing enforces that at compile time. func TestProviderState_Breaker_ConcurrentRefreshes_RaceFree(t *testing.T) { t.Parallel() st := newHealthTestState(time.Millisecond, 3, 2*time.Millisecond, 20*time.Millisecond) @@ -521,6 +648,18 @@ func TestProviderState_Breaker_ConcurrentRefreshes_RaceFree(t *testing.T) { st.finishRefresh(now, []string{"m1"}, err) } _ = st.discoveryHealthy() + + st.mu.Lock() + gotBackoff, gotMax, gotHealth := st.backoff, st.breakerOpenMax, st.health + st.mu.Unlock() + if gotBackoff > gotMax { + t.Errorf("worker %d iteration %d: backoff = %v, want <= breakerOpenMax %v", worker, i, gotBackoff, gotMax) + } + switch gotHealth { + case breakerClosed, breakerOpen, breakerHalfOpen: + default: + t.Errorf("worker %d iteration %d: health = %v, want one of closed/open/half-open", worker, i, gotHealth) + } } }(w) }