Fix failover and health-monitor recovery for 429/5xx outages - #1373
Open
andreasfoo wants to merge 1 commit into
Open
Fix failover and health-monitor recovery for 429/5xx outages#1373andreasfoo wants to merge 1 commit into
andreasfoo wants to merge 1 commit into
Conversation
…urn-to-primary
Timeline e2e test (vm1/vm2/vm3 switchable vmodel upstreams over a fake
clock: all up at 00:00 → T0 down at 00:05 → T0 back up → recovery)
exposed four bugs in the direct+fallback (tier) path; all fixed:
- isRetryableStatus enumerated 429/500/502/503/504 only, while error
forwarding propagates the upstream status verbatim — Anthropic's 529
overloaded_error (and any other 5xx like Cloudflare 52x) terminated
the request instead of failing over. Retryable is now 429 + the whole
5xx range.
- Breaker success was recorded only when the failover gate committed
(streaming first chunk). Non-streaming 200s and the MCP interceptor
path never fed a success, so half-open probe slots stayed claimed and
a recovered primary could never close its breaker — traffic never
returned to T0. Terminal buffered 2xx now records success.
- The MCP generic stream interceptor never raised CommitFirstChunk, so
on multi-service rules the A→A v1 streaming path buffered the whole
stream (no incremental delivery) and successes were invisible to the
breaker. Its sendEvent seam now commits on the first client-bound
event.
- The health-probe func split serviceID on ":" but the format is
"provider/model" — every probe failed and pushed the rate-limit
recovery window forward, so one 429 excluded a service forever.
Added loadbalance.ParseServiceID and fixed the parse. Also default
zero HealthMonitorConfig values (an absent health_monitor config
section yielded a 0s rate-limit window).
Also: recognize Zhipu GLM 1305 (request-rate limit) alongside 1302 in
the rate-limit matchers; add vmodel virtual-fail-529 to the extended
error catalog; document the new behavior in .design/tier-routing.md.
New coverage: internal/protocoltest/failover_timeline{,_test}.go —
SwitchableUpstream fixtures + timeline scenarios for 429/500/529 ×
stream/nonstream on 2 tiers, plus a 3-tier cascade (all-down degrade,
recovery back to T0).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RmgavW26d4LwBi14PA3kjF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes critical bugs in the failover and health-monitor recovery paths that prevented traffic from returning to recovered services after transient outages (429 rate limits, 5xx errors, and Anthropic's 529 overloaded status). The issues affected both streaming and non-streaming request modes.
Key Changes
Failover Circuit Breaker Recovery
failover_dispatch.go: ChangedisRetryableStatus()from an enumerated set to a range check (429 || 5xx), ensuring provider-specific codes like Anthropic's 529 and Cloudflare's 52x family trigger failover instead of being silently droppedfailover_dispatch.go: Added success recording for buffered 2xx responses inDispatchWithPriorityFailover(). Previously, only committed gates recorded successes, leaving half-open probe slots claimed forever on non-streaming traffic — preventing breaker closure and traffic return to T0generic_stream_interceptor.go: IntroducedsendEvent()helper that commits the failover gate before forwarding each client-bound SSE event. Streaming producers must signal on first real chunk; without this, multi-service rules buffered entire streams and never saw committed gates, making successful attempts invisible to the breakerHealth Monitor Recovery Window
health_monitor.go: FixedNewHealthMonitor()to apply default recovery timeout when config specifies zero. Previously, absenthealth_monitorconfig sections yielded zero recovery windows, causing 429-marked services to "auto-recover" on the next request, defeating the documented rate-limit windowhealth_monitor.go: UpdatedUpdateConfig()to apply the same zero-value defaults for consistencyService ID Parsing
service_id.go: AddedParseServiceID()function to split canonical "provider/model" IDs (inverse ofFormatServiceID)server.go: Fixed health probe function to useParseServiceID()instead of splitting on ":" (which never matched the "/" format). This bug caused probes to fail for every service, pushing recovery windows forward indefinitelyTest Coverage & Virtual Models
failover_dispatch_test.go: UpdatedTestIsRetryableStatus()to verify the full 5xx range (529, 520, 599) and 429 are retryabledefaults_shared.go: Addedvirtual-fail-529mock spec for Anthropic's overloaded_error, enabling end-to-end testing of the 529 failover patherror_integration_test.go: Updated count assertion for extended error specs (5 → 6)End-to-End Timeline Tests
failover_timeline_test.go(new): Comprehensive e2e test suite scripting wall-clock failover scenarios using a fake clock:TestFailoverTimeline_PrimaryDownThenRecover: Two-tier failover with 429/500/529 outages in streaming and non-streaming modes, verifying traffic returns after breaker recoveryTestFailoverTimeline_ThreeTierCascade: Three-tier cascade with multiple simultaneous failures, confirming requests flow through all tiers and honest error propagation when all are downfailover_timeline.go(new):SwitchableUpstreamfixture providing runtime-flippable vmodel upstreams andSetupTimelineFailoverRoute()helper for multi-tier test harnessesDocumentation
.design/tier-routing.md: Clarified health recovery probe behavior and serviceID parsing formatImplementation Details
The root causes were: