Address the design finding and remaining defects from round 3 of
adversarial review.
Raise defaultBreakerMaxOpenDuration from 30m to 6h. Round 2 fixed the
breaker so it never retries more than plain interval throttling, but
left it cadence-neutral: with maxOpenDuration shorter than the 1h
default interval, the interval always dominated and backoff never
suppressed anything. Measured over 24h with stock config, admits
equaled the interval-only baseline exactly. At 6h, a permanently broken
provider now backs off 1h, 2h, 4h, 6h, 6h, roughly 5 attempts a day
instead of 24. The 24h cadence test now asserts admits are strictly
fewer than baseline, not merely no worse.
Correct the emitted openUntil to account for the interval floor.
Because the interval gate applies unconditionally, the real next probe
time is max(openUntil, lastRefresh+interval), not the raw backoff value
alone. On pure defaults a fresh trip set the raw value to +1m while the
real next probe was at +1h, a 59 minute skew the dashboard rendered as
a countdown that reached zero and then silently vanished. snapshot now
emits the corrected value while open; closed and half-open are
unaffected.
Revert a canceled half-open probe to open instead of leaving it at
half-open. A half-open breaker reports healthy by design, so a canceled
probe previously left a still-broken provider reading healthy until the
next attempt happened to land. Reverting costs nothing extra, since the
interval gate already governs when that next attempt is admitted either
way.
Rewrite the discovery-suppression claims in doc comments: the earlier
wording claimed re-probing stops "every single interval forever" under
all conditions, which the interval-floor fix from round 2 already made
false, and which contradicted this file's own surrounding explanation.
Both now state plainly that suppression only occurs once backoff
exceeds discoveryInterval.
Add a sentence to recordHealthLocked's doc comment stating that
context.DeadlineExceeded counts as a plain failure, matching
recordProviderAttempt's SHOULD-1 ruling, and a named test proving it.
Tests: two new snapshot tests pin the openUntil correction in both
directions. The cadence test's baseline comparison is now strict. A new
test proves the zero value truly disables the breaker rather than
tripping on the first failure. A new test proves a canceled context
never gets misclassified as a failure, completing the bidirectional
check the existing success-direction test already had. The concurrent
test now asserts backoff stays within its ceiling and health stays a
valid state on every iteration, rather than only checking for panics
and data races.
Address five findings from adversarial review of the discovery circuit
breaker. All are behavioral corrections to registry.go; none touch the
request path.
Interval floor (blocking). tryBeginRefresh let an open breaker's own
backoff window replace the plain discoveryInterval gate instead of
adding to it. Since defaultBreakerMaxOpenDuration (30m) is shorter than
defaultDiscoveryInterval (1h), a permanently broken provider was probed
roughly twice as often as before this feature existed. The interval
gate now applies unconditionally, closed or open alike, so backoff can
only ever lengthen the retry cadence, never shorten it.
Explicit failure classification. recordHealthLocked reused isTransient's
retry-shaped semantics wholesale, including its exclusion of
errRequestBuildFailed. A provider whose baseURL is malformed produced
that exact error on every discovery cycle and never opened its breaker,
because the old code routed it to a successful outcome. Health
classification is now explicit: any non-nil, non-canceled error counts
as a failure.
Neutral cancellation. A context.Canceled outcome no longer counts as
success or failure. Treating it as success previously closed an
already-open breaker and wiped its backoff whenever a half-open probe
was interrupted at shutdown, reporting a still-broken provider healthy.
Zero-value safety. A providerState assembled without going through
newModelRegistry now behaves like plain interval-only gating instead of
opening on the first failure with a zero backoff, matching the type's
own documented claim.
Backoff overflow ceiling. Repeated doubling with no ceiling on
maxOpenDuration could wrap the backoff negative after roughly 28
probes, silently disabling it. validateBreakerConfig now caps
maxOpenDuration at 24 hours.
Also renames modelRegistry.providerUsable and providerState.usable to
discoveryHealthy, and documents that the signal reflects the discovery
endpoint only. It must never be the sole reason to refuse routing to a
provider whose request path works fine. feat/failover should combine it
with a separate request-path signal fed by recordProviderAttempt.
Tests: a 24-hour simulated cadence test proves the fixed gate admits no
more attempts than plain interval throttling did before this feature.
A concurrent test exercises the breaker fields under -race for the
first time. Two tests replace a vacuous predecessor and distinguish
neutral cancellation from treating it as success. One test proves
errRequestBuildFailed opens the breaker. One test proves the zero value
never opens. A rewritten healthy-provider test asserts behavior instead
of reading the internal openUntil field directly.
Add a three-state circuit breaker to providerState: closed, open, and
half-open. It is driven only by discovery-refresh outcomes, never by
request-path traffic.
A provider whose refresh fails breakerThreshold times in a row opens
the breaker and backs off before its next half-open probe, instead of
retrying every discovery interval forever. A failed probe reopens the
breaker with a doubled backoff, capped at breakerOpenMax. A successful
probe closes the breaker again, so a recovered provider never stays
dead.
The failure classification reuses limits.go's isTransient and
isDeadlineExceeded, the same rule recordProviderAttempt already
applies to request-path attempts. It does not add a second
classification.
Add Config.Breaker (BreakerConfig), validated and defaulted by
validateBreakerConfig. Every default preserves current behavior for a
provider that never fails: a closed breaker never changes the existing
interval-based refresh gating.
Export modelRegistry.providerUsable(name) as the queryable health
predicate. A sibling branch (feat/failover) will use it to skip
unusable providers before routing. This commit does not touch the
request path or routes_unified.go.
lukaszraczylo