diff --git a/.design/tier-routing.md b/.design/tier-routing.md index c60461bb5..56ca51453 100644 --- a/.design/tier-routing.md +++ b/.design/tier-routing.md @@ -142,6 +142,8 @@ Each request outcome feeds **two channels**: So 429 and 401/403 exclude a service on the *first* hit (health channel), well before the breaker trips. The `harness lb` simulator models both faithfully (reusing `reportHealthStatus` + the breaker recorder), driving them off one shared clock so a single simulated advance recovers both. +Health recovery runs an optional lightweight probe (`Server.SetProbeFunc`, OPTIONS + `/models`) once the rate-limit window has elapsed; a failed probe pushes the window forward. The probe parses the serviceID with `loadbalance.ParseServiceID` ("provider/model") — it originally split on `":"`, which never matches, so every probe failed and a single 429 excluded the service *forever* (traffic could never return to the primary). Pinned by `TestFailoverTimeline_PrimaryDownThenRecover/429-*` in `internal/protocoltest/failover_timeline_test.go`. + There is also a **third** time-based input: the **affinity TTL** (`AffinityEntry.ExpiresAt`). A lock expires *strictly* at `LockedAt + SessionAffinity` — an in-window request is honored but does **not** slide the expiry; once past it the pin is dropped and the session is re-selected and re-locked. The simulator puts this on the same clock seam (`loadbalance.SetClock` + `routing.SetClock`, both fed one fake clock), so a single `advance` moves breaker recovery, health recovery, *and* affinity expiry together — matching production, where all three read the wall clock. This lets the scenario suite assert the strict-TTL contract and cross-model failover (a failover hop must carry the fallback service's own model, not reuse the primary's) deterministically. ### End-to-end flow: how the tactic switch actually takes effect @@ -201,6 +203,8 @@ The "user moves a service card to a different tier" event has to cross five laye ┌─ Per-request feedback into the breaker ──────────────────────────────┐ │ dispatchWithPriorityFailover owns breaker accounting per attempt: │ │ gate committed → RecordServiceSuccess(ruleUUID, serviceID) │ +│ terminal 2xx (buffered, e.g. non-streaming c.JSON) │ +│ → RecordServiceSuccess(ruleUUID, serviceID) │ │ retryable failure → RecordServiceFailure(ruleUUID, serviceID) │ │ Same DefaultBreakerStore the selection logic consulted, so the next │ │ request's TierTactic sees the updated state. State is keyed per rule │ @@ -333,9 +337,9 @@ v1 only handled cross-request failover: a request that failed returned the error 2. **`firstChunkGate`** is a passive, protocol-agnostic byte buffer wrapping `c.Writer`. It makes no decisions in its write path: writes land in memory until an explicit signal commits them. Crucially, **single-service requests skip the gate entirely** (`len(GetActiveServices()) <= 1`), so the common case never touches the buffer — zero blast radius. 3. **Orchestrator** (`dispatchWithPriorityFailover`) owns the retry decision. After each attempt it reads the gate's state: - `gate.Committed()` → the stream's first real chunk already reached the wire; retry is impossible, return. - - else `gate.Status()` retryable (429, 500, 502, 503, 504) → `gate.Discard()`, pick the next tier via `selectFallbackService`, try again. + - else `gate.Status()` retryable (429 or **any 5xx**, 500–599) → `gate.Discard()`, pick the next tier via `selectFallbackService`, try again. The full 5xx range matters because error forwarding propagates the upstream's status verbatim — enumerating individual codes silently dropped provider-specific statuses like Anthropic's 529 `overloaded_error` (no failover on exactly the outage it signals). - else (200, other 4xx, status 0) → terminal; the deferred `gate.CommitIfBuffered()` flushes the captured error to the client. -4. **Commit seam.** Streaming producers raise `CommitFirstChunk` on their first real chunk (centralised in `ProcessStream`/`StreamLoop`, plus the explicit `message_start` senders), which flushes captured headers + body and switches the gate to pass-through — preserving incremental delivery. +4. **Commit seam.** Streaming producers raise `CommitFirstChunk` on their first real chunk (centralised in `ProcessStream`/`StreamLoop`, the explicit `message_start` senders, plus the MCP generic stream interceptor's `sendEvent` — the A→A v1 path runs through the interceptor unconditionally, and before it committed, multi-service rules on that path buffered the whole stream and never fed the breaker a success), which flushes captured headers + body and switches the gate to pass-through — preserving incremental delivery. 5. Budget caps at the number of active services so we never loop unbounded. The same service is never tried twice in one request (in-request `tried` map, complementary to the cross-request breaker). The recorder's bound provider is re-set before each attempt (`SetActiveService`), so a second-attempt failure trips the *second* service's breaker — not the first's. ### Streaming: priming the first event diff --git a/internal/loadbalance/health_monitor.go b/internal/loadbalance/health_monitor.go index 7b63d9186..04c37c980 100644 --- a/internal/loadbalance/health_monitor.go +++ b/internal/loadbalance/health_monitor.go @@ -73,8 +73,17 @@ type HealthMonitor struct { probeFunc HealthProbeFunc // Optional probe function for recovery checking } -// NewHealthMonitor creates a new health monitor with the given configuration +// NewHealthMonitor creates a new health monitor with the given configuration. +// Zero values fall back to defaults (mirroring NewBreaker): the server builds +// the monitor straight from the persisted config, and an absent health_monitor +// section used to yield a ZERO recovery window — a 429-marked service +// "auto-recovered" on the very next request, so the documented rate-limit +// window never held. ProbeEnabled is left as-is (false is a valid choice and +// a bool cannot express "unset"). func NewHealthMonitor(config HealthMonitorConfig) *HealthMonitor { + if config.RecoveryTimeoutSeconds <= 0 { + config.RecoveryTimeoutSeconds = DefaultHealthMonitorConfig().RecoveryTimeoutSeconds + } return &HealthMonitor{ services: make(map[string]*ServiceHealth), config: config, @@ -303,11 +312,15 @@ func (hm *HealthMonitor) RemoveHealth(serviceID string) { delete(hm.services, serviceID) } -// UpdateConfig updates the health monitor configuration +// UpdateConfig updates the health monitor configuration. Zero values fall +// back to defaults, matching NewHealthMonitor. func (hm *HealthMonitor) UpdateConfig(config HealthMonitorConfig) { hm.mutex.Lock() defer hm.mutex.Unlock() + if config.RecoveryTimeoutSeconds <= 0 { + config.RecoveryTimeoutSeconds = DefaultHealthMonitorConfig().RecoveryTimeoutSeconds + } hm.config = config hm.defaultRecoveryTimeout = time.Duration(config.RecoveryTimeoutSeconds) * time.Second } diff --git a/internal/loadbalance/service_id.go b/internal/loadbalance/service_id.go index 056111c25..5f69961f0 100644 --- a/internal/loadbalance/service_id.go +++ b/internal/loadbalance/service_id.go @@ -1,6 +1,9 @@ package loadbalance -import "fmt" +import ( + "fmt" + "strings" +) // ServiceID uniquely identifies a provider+model combination in load balancing. type ServiceID struct { @@ -27,6 +30,18 @@ func FormatServiceID(providerUUID, model string) string { return fmt.Sprintf("%s/%s", providerUUID, model) } +// ParseServiceID splits a canonical "provider/model" service ID back into its +// parts (the inverse of FormatServiceID). Provider UUIDs are slash-free, so +// splitting on the first "/" is unambiguous even when the model name itself +// contains slashes. model is "" when serviceID carries no separator. +func ParseServiceID(serviceID string) (providerUUID, model string) { + parts := strings.SplitN(serviceID, "/", 2) + if len(parts) < 2 { + return parts[0], "" + } + return parts[0], parts[1] +} + // FormatBreakerKey formats a (ruleUUID, serviceID) pair into the canonical // "ruleUUID/serviceID" string used as the breaker-store key. The breaker is // rule-scoped: each rule owns independent breaker state per service so a busy diff --git a/internal/protocoltest/failover_timeline.go b/internal/protocoltest/failover_timeline.go new file mode 100644 index 000000000..9768f1bcf --- /dev/null +++ b/internal/protocoltest/failover_timeline.go @@ -0,0 +1,142 @@ +package protocoltest + +// Timeline failover fixtures: switchable vmodel upstreams whose availability +// can be flipped at runtime, so a test can script a wall-clock scenario like +// "00:00 all up → 00:05 vm1 down (429/500/529) → vm1 up again → traffic must +// return to vm1 after breaker recovery". Complements failover.go, whose error +// mocks are statically always-failing. + +import ( + "fmt" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/tingly-dev/tingly-box/internal/constant" + "github.com/tingly-dev/tingly-box/internal/loadbalance" + "github.com/tingly-dev/tingly-box/internal/protocol" + "github.com/tingly-dev/tingly-box/internal/typ" + anthropicvm "github.com/tingly-dev/tingly-box/vmodel/anthropic" + openaivm "github.com/tingly-dev/tingly-box/vmodel/openai" + "github.com/tingly-dev/tingly-box/vmodel/virtualserver" +) + +// SwitchableUpstream is a vmodel-backed httptest provider that can be flipped +// between healthy (serves the registered virtual models) and down (every +// request — including probes — gets the configured HTTP status with an error +// envelope) at any point during a test. +type SwitchableUpstream struct { + Server *httptest.Server + + hits atomic.Int64 + downStatus atomic.Int64 // 0 = up; otherwise the HTTP status to fail with +} + +// SetDown makes every subsequent request fail with the given HTTP status. +func (u *SwitchableUpstream) SetDown(status int) { u.downStatus.Store(int64(status)) } + +// SetUp restores normal vmodel serving. +func (u *SwitchableUpstream) SetUp() { u.downStatus.Store(0) } + +// Hits reports how many HTTP requests reached this upstream (successful or +// failed, chat or probe endpoints alike). +func (u *SwitchableUpstream) Hits() int64 { return u.hits.Load() } + +// newSwitchableUpstream builds the upstream: a virtualserver with both +// protocol registries (so it serves either API style), wrapped by a counting +// middleware that injects the outage when SetDown is active. +func newSwitchableUpstream(t *testing.T) *SwitchableUpstream { + t.Helper() + gin.SetMode(gin.TestMode) + + u := &SwitchableUpstream{} + + svc := virtualserver.NewService() + openaivm.RegisterDefaults(svc.GetOpenAIRegistry()) + anthropicvm.RegisterDefaults(svc.GetAnthropicRegistry()) + + engine := gin.New() + engine.Use(func(c *gin.Context) { + u.hits.Add(1) + if status := int(u.downStatus.Load()); status != 0 { + // Shape follows the Anthropic error envelope; both SDKs surface + // the HTTP status either way, which is all failover keys off. + c.AbortWithStatusJSON(status, gin.H{ + "type": "error", + "error": gin.H{ + "type": "api_error", + "message": fmt.Sprintf("simulated outage: upstream returned %d", status), + }, + }) + return + } + c.Next() + }) + svc.SetupRoutes(engine.Group("/v1")) + + srv := httptest.NewServer(engine) + t.Cleanup(srv.Close) + u.Server = srv + return u +} + +// TimelineFailoverRoute is the handle returned by SetupTimelineFailoverRoute. +type TimelineFailoverRoute struct { + ModelName string // gateway-facing request model + RuleUUID string // for breaker-store introspection + VMs []*SwitchableUpstream // index == tier (VMs[0] is T0) + ServiceIDs []string // loadbalance service IDs, index == tier +} + +// timelineSuccessModel is the vmodel every tier serves while up. +const timelineSuccessModel = "echo-model" + +// SetupTimelineFailoverRoute wires an N-tier rule (T0..T(n-1)) whose tiers are +// independent switchable upstreams, all serving echo-model while up. All +// providers use the source protocol's API style (homogeneous failover — the +// cross-style path has its own suite). label must be unique per test case; it +// namespaces the provider/rule UUIDs so parallel or sequential cases cannot +// collide in the shared breaker store. +func (env *TestEnv) SetupTimelineFailoverRoute(t *testing.T, source protocol.APIType, tiers int, label string) TimelineFailoverRoute { + t.Helper() + + apiStyle := targetToAPIStyle(source) + requestModel := fmt.Sprintf("fo-timeline-%s-%s", source, label) + + route := TimelineFailoverRoute{ + ModelName: requestModel, + RuleUUID: requestModel, + } + + services := make([]*loadbalance.Service, 0, tiers) + for i := 0; i < tiers; i++ { + vm := newSwitchableUpstream(t) + route.VMs = append(route.VMs, vm) + + apiBase := vm.Server.URL + if apiStyle == protocol.APIStyleOpenAI { + apiBase = vm.Server.URL + "/v1" + } + + providerUUID := fmt.Sprintf("timeline-%s-vm%d", label, i+1) + if err := env.appConfig.AddProvider(&typ.Provider{ + UUID: providerUUID, Name: providerUUID, APIBase: apiBase, APIStyle: apiStyle, + Token: "timeline-token", Enabled: true, Timeout: int64(constant.DefaultRequestTimeout), + }); err != nil { + t.Fatalf("add timeline provider vm%d: %v", i+1, err) + } + + services = append(services, tieredService(providerUUID, timelineSuccessModel, i)) + route.ServiceIDs = append(route.ServiceIDs, loadbalance.FormatServiceID(providerUUID, timelineSuccessModel)) + } + + rule := newHarnessRule(requestModel, sourceToRuleScenario(source), requestModel, timelineSuccessModel, services...) + rule.LBTactic = tierFailoverTactic() + if err := env.appConfig.GetGlobalConfig().AddRequestConfig(rule); err != nil { + t.Fatalf("add timeline rule: %v", err) + } + + return route +} diff --git a/internal/protocoltest/failover_timeline_test.go b/internal/protocoltest/failover_timeline_test.go new file mode 100644 index 000000000..c6e2fd7a2 --- /dev/null +++ b/internal/protocoltest/failover_timeline_test.go @@ -0,0 +1,269 @@ +//go:build e2e +// +build e2e + +package protocoltest_test + +import ( + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/tingly-dev/tingly-box/internal/clock" + "github.com/tingly-dev/tingly-box/internal/loadbalance" + "github.com/tingly-dev/tingly-box/internal/protocol" + pt "github.com/tingly-dev/tingly-box/internal/protocoltest" +) + +// fakeClock drives the breaker / health-monitor / affinity time source +// (internal/clock) so the wall-clock timeline can be scripted without real +// sleeps. HTTP traffic itself is unaffected. +type fakeClock struct { + mu sync.Mutex + base time.Time + now time.Time +} + +func newFakeClock() *fakeClock { + b := time.Now() + return &fakeClock{base: b, now: b} +} + +func (f *fakeClock) Now() time.Time { + f.mu.Lock() + defer f.mu.Unlock() + return f.now +} + +// Advance sets the fake time to base+offset (absolute offsets keep the +// timeline readable: Advance(5*time.Second) == "00:00:05"). +func (f *fakeClock) Advance(offset time.Duration) { + f.mu.Lock() + f.now = f.base.Add(offset) + f.mu.Unlock() +} + +// TestFailoverTimeline_PrimaryDownThenRecover scripts the user-facing +// direct+fallback contract on a two-tier rule (T0=vm1, T1=vm2): +// +// 00:00:00 vm1, vm2 up → requests served by vm1 (T0) +// 00:00:05 vm1 down (S) → requests STILL succeed (mid-request +// failover to vm2); after 3 failures +// vm1's breaker opens +// 00:00:06 (vm1 still down) → requests go straight to vm2, vm1 not hit +// 00:00:10 vm1 back up → breaker still open → still vm2 +// 00:00:20 (steady state) → still vm2, vm1 not hit +// recovery (30s breaker window; → traffic returns to vm1: 3 half-open +// +300s health window probes succeed, breaker closes, vm1 +// for 429) serves everything again +// +// Run for every outage status the user reported (429 / 500 / 529) in both +// streaming and non-streaming modes. 529 is Anthropic's overloaded status; +// 429 additionally exercises the health-monitor rate-limit window. +func TestFailoverTimeline_PrimaryDownThenRecover(t *testing.T) { + cases := []struct { + name string + status int + streaming bool + // recoverAt must be past the breaker open window (opened 00:00:05, + // 30s) and, for 429, past the health rate-limit window (300s). + recoverAt time.Duration + // 5xx feeds only the (rule-scoped) breaker: vm1 is attempted on every + // request until 3 failures open it. 429 additionally feeds the health + // monitor, which excludes the service from selection on the FIRST hit + // — so vm1 sees a single failed attempt and its breaker stays closed. + vm1AttemptsWhileDown int64 + breakerOpens bool + }{ + {"500-nonstream", 500, false, 36 * time.Second, 3, true}, + {"500-stream", 500, true, 36 * time.Second, 3, true}, + {"529-nonstream", 529, false, 36 * time.Second, 3, true}, + {"529-stream", 529, true, 36 * time.Second, 3, true}, + {"429-nonstream", 429, false, 315 * time.Second, 1, false}, + {"429-stream", 429, true, 315 * time.Second, 1, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + loadbalance.DefaultBreakerStore().Reset() + fc := newFakeClock() + restore := clock.SetClock(fc.Now) + defer restore() + + env := pt.NewTestEnv(t) + defer env.Close() + + route := env.SetupTimelineFailoverRoute(t, protocol.TypeAnthropicV1, 2, tc.name) + vm1, vm2 := route.VMs[0], route.VMs[1] + vm1Breaker := loadbalance.DefaultBreakerStore().Get(route.RuleUUID, route.ServiceIDs[0]) + + send := func() *pt.RoundTripResult { + return env.SendWithModel(t, protocol.TypeAnthropicV1, route.ModelName, tc.streaming) + } + + // ── 00:00:00 — everything up: T0 (vm1) serves ──────────────── + for i := 0; i < 2; i++ { + r := send() + require.Equal(t, 200, r.HTTPStatus, "t=0 request %d must be served", i+1) + assert.Contains(t, r.Content, "Echo:", "t=0 request %d content", i+1) + } + require.EqualValues(t, 2, vm1.Hits(), "t=0: vm1 (T0) must serve all traffic") + require.EqualValues(t, 0, vm2.Hits(), "t=0: vm2 (T1) must be idle") + + // ── 00:00:05 — vm1 goes down with the outage status ───────── + fc.Advance(5 * time.Second) + vm1.SetDown(tc.status) + for i := 0; i < 3; i++ { + r := send() + require.Equalf(t, 200, r.HTTPStatus, + "t=5s request %d: primary returned %d — failover to T1 must be seamless (client saw %d, body: %s)", + i+1, tc.status, r.HTTPStatus, string(r.RawBody)) + assert.Contains(t, r.Content, "Echo:", "t=5s request %d must carry fallback content", i+1) + } + downHits := 2 + tc.vm1AttemptsWhileDown + require.EqualValues(t, downHits, vm1.Hits(), + "t=5s: vm1 attempts while down (breaker path retries per request; a 429 is health-excluded after the first hit)") + require.EqualValues(t, 3, vm2.Hits(), "t=5s: vm2 must have served all three requests") + if tc.breakerOpens { + require.Equal(t, loadbalance.BreakerOpen, vm1Breaker.State(), + "after 3 consecutive failures vm1's breaker must be open") + } + + // ── 00:00:06 — vm1 excluded (breaker open / health-unhealthy) ─ + fc.Advance(6 * time.Second) + r := send() + require.Equal(t, 200, r.HTTPStatus) + require.EqualValues(t, downHits, vm1.Hits(), "t=6s: vm1 must be routed around without being touched") + require.EqualValues(t, 4, vm2.Hits()) + + // ── 00:00:10 — vm1 recovers, but the breaker / health window has + // not elapsed: traffic must stay on vm2 ────────────────────── + fc.Advance(10 * time.Second) + vm1.SetUp() + fc.Advance(20 * time.Second) + r = send() + require.Equal(t, 200, r.HTTPStatus) + require.EqualValues(t, downHits, vm1.Hits(), "t=20s: exclusion window still active — vm1 must not be probed early") + require.EqualValues(t, 5, vm2.Hits()) + + // ── recovery — traffic must RETURN to vm1 ──────────────────── + fc.Advance(tc.recoverAt) + vm1Before, vm2Before := vm1.Hits(), vm2.Hits() + for i := 0; i < 3; i++ { + r := send() + require.Equalf(t, 200, r.HTTPStatus, "recovery request %d must succeed", i+1) + } + require.Equal(t, loadbalance.BreakerClosed, vm1Breaker.State(), + "vm1's breaker must be closed after recovery (3 successful half-open probes for the 5xx path)") + assert.GreaterOrEqual(t, vm1.Hits()-vm1Before, int64(3), + "recovery: the three requests must be served by vm1 (half-open probes)") + assert.EqualValues(t, vm2Before, vm2.Hits(), + "recovery: vm2 must not receive traffic once vm1 is probing successfully") + + // ── steady state after recovery: T0 owns traffic again ─────── + vm1Before, vm2Before = vm1.Hits(), vm2.Hits() + for i := 0; i < 2; i++ { + r := send() + require.Equal(t, 200, r.HTTPStatus) + assert.Contains(t, r.Content, "Echo:") + } + assert.EqualValues(t, 2, vm1.Hits()-vm1Before, "post-recovery: vm1 serves everything") + assert.EqualValues(t, vm2Before, vm2.Hits(), "post-recovery: vm2 idle again") + }) + } +} + +// TestFailoverTimeline_ThreeTierCascade extends the timeline to three tiers +// (vm1/vm2/vm3 = T0/T1/T2), matching the requested scenario: +// +// 00:00:00 vm1 vm2 vm3 ready → vm1 serves +// 00:00:05 vm1 down (500), +// vm2 down (529) → requests cascade through both failures +// and are served by vm3, seamlessly +// 00:00:06 breakers open → straight to vm3 +// 00:00:07 vm3 down too (503) → all tiers down: client sees the real +// upstream error, not a hang or a 200 +// 00:00:08 all back up → breakers still open +// 00:00:36 breaker window elapsed → vm1 probed, recovers, owns traffic +func TestFailoverTimeline_ThreeTierCascade(t *testing.T) { + loadbalance.DefaultBreakerStore().Reset() + fc := newFakeClock() + restore := clock.SetClock(fc.Now) + defer restore() + + env := pt.NewTestEnv(t) + defer env.Close() + + route := env.SetupTimelineFailoverRoute(t, protocol.TypeAnthropicV1, 3, "cascade") + vm1, vm2, vm3 := route.VMs[0], route.VMs[1], route.VMs[2] + vm1Breaker := loadbalance.DefaultBreakerStore().Get(route.RuleUUID, route.ServiceIDs[0]) + + send := func() *pt.RoundTripResult { + return env.SendWithModel(t, protocol.TypeAnthropicV1, route.ModelName, false) + } + + // ── 00:00:00 ───────────────────────────────────────────────────────── + r := send() + require.Equal(t, 200, r.HTTPStatus) + require.EqualValues(t, 1, vm1.Hits()) + require.EqualValues(t, 0, vm2.Hits()) + require.EqualValues(t, 0, vm3.Hits()) + + // ── 00:00:05 — vm1 AND vm2 down: cascade to T2 ─────────────────────── + fc.Advance(5 * time.Second) + vm1.SetDown(500) + vm2.SetDown(529) + for i := 0; i < 3; i++ { + r := send() + require.Equalf(t, 200, r.HTTPStatus, + "t=5s request %d must cascade to vm3 (T0=500, T1=529 both retryable); body: %s", + i+1, string(r.RawBody)) + assert.Contains(t, r.Content, "Echo:") + } + require.EqualValues(t, 4, vm1.Hits(), "each t=5s request attempts vm1 first") + require.EqualValues(t, 3, vm2.Hits(), "each t=5s request attempts vm2 second") + require.EqualValues(t, 3, vm3.Hits(), "vm3 serves all three") + + // ── 00:00:06 — both breakers open: straight to vm3 ─────────────────── + fc.Advance(6 * time.Second) + r = send() + require.Equal(t, 200, r.HTTPStatus) + require.EqualValues(t, 4, vm1.Hits(), "t=6s: vm1 skipped (breaker open)") + require.EqualValues(t, 3, vm2.Hits(), "t=6s: vm2 skipped (breaker open)") + require.EqualValues(t, 4, vm3.Hits()) + + // ── 00:00:07 — vm3 down as well: degrade honestly, no fake 200 ─────── + fc.Advance(7 * time.Second) + vm3.SetDown(503) + r = send() + require.NotEqual(t, 200, r.HTTPStatus, + "all tiers down: the client must see a real upstream error") + + // ── 00:00:08 — everyone recovers ───────────────────────────────────── + fc.Advance(8 * time.Second) + vm1.SetUp() + vm2.SetUp() + vm3.SetUp() + + // ── 00:00:36 — past the breaker window: back to vm1 ────────────────── + fc.Advance(36 * time.Second) + vm1Before, vm2Before, vm3Before := vm1.Hits(), vm2.Hits(), vm3.Hits() + for i := 0; i < 3; i++ { + r := send() + require.Equalf(t, 200, r.HTTPStatus, "recovery request %d must succeed", i+1) + assert.Contains(t, r.Content, "Echo:") + } + require.Equal(t, loadbalance.BreakerClosed, vm1Breaker.State(), + "vm1 must recover after 3 half-open probe successes") + assert.GreaterOrEqual(t, vm1.Hits()-vm1Before, int64(3), "probes and traffic must go to vm1") + assert.EqualValues(t, vm2Before, vm2.Hits(), "vm2 must stay idle during T0 recovery") + assert.EqualValues(t, vm3Before, vm3.Hits(), "vm3 must stay idle during T0 recovery") + + // steady state + vm1Before = vm1.Hits() + r = send() + require.Equal(t, 200, r.HTTPStatus) + assert.EqualValues(t, 1, vm1.Hits()-vm1Before, "post-recovery traffic belongs to vm1") +} diff --git a/internal/server/failover_dispatch.go b/internal/server/failover_dispatch.go index c39851088..39be13b20 100644 --- a/internal/server/failover_dispatch.go +++ b/internal/server/failover_dispatch.go @@ -69,28 +69,24 @@ func (ph *ProtocolHandler) FailAttemptSetup(c *gin.Context, err error) { }) } -// retryableUpstreamStatuses are the HTTP status codes treated as -// "upstream transiently sick, try the next priority tier". -// -// 500 is included because in-process error helpers (SendStreamingError, -// SendErrorResponse on forwarding failure) wrap upstream pre-stream -// errors as 500. 502 covers the explicit "upstream stream failed" path. -// Keeping both means refactors that change one helper's status code -// don't silently break failover. -var retryableUpstreamStatuses = map[int]bool{ - http.StatusTooManyRequests: true, - http.StatusBadGateway: true, - http.StatusServiceUnavailable: true, - http.StatusGatewayTimeout: true, - http.StatusInternalServerError: true, -} - // isRetryableStatus reports whether a buffered status code from a // dispatch attempt should trigger failover. Status 0 means the writer // was never touched — treat as terminal (the handler ran to completion // without writing, retrying would just repeat the no-op). +// +// Retryable = 429 (rate limit) plus the WHOLE 5xx range, meaning "upstream +// transiently sick, try the next priority tier". The full range matters: +// error forwarding propagates the upstream provider's real status verbatim +// (protocol.UpstreamStatus), so non-IANA provider statuses like Anthropic's +// 529 overloaded_error or Cloudflare's 52x family land here unmapped — +// enumerating individual codes silently dropped 529 and broke failover for +// exactly the outage it signals. In-process error helpers wrap pre-stream +// failures as 500 and stream failures as 502, both inside the range. +// A 5xx-triggered extra attempt is always safe: nothing was delivered to +// the client (the gate is still buffered), and if every tier fails the last +// upstream error is flushed unchanged. func isRetryableStatus(status int) bool { - return status != 0 && retryableUpstreamStatuses[status] + return status == http.StatusTooManyRequests || (status >= 500 && status <= 599) } // firstChunkGate is a passive, protocol-agnostic byte buffer placed @@ -410,6 +406,17 @@ func (ph *ProtocolHandler) DispatchWithPriorityFailover( } status := gate.Status() if !isRetryableStatus(status) { + // A buffered 2xx is a success that never raised CommitFirstChunk: + // non-streaming responses (c.JSON lands in the gate, flushed by the + // deferred CommitIfBuffered) and buffering stream producers (e.g. + // the MCP interceptor paths). The breaker must see these successes + // — recording only on committed gates left half-open probe slots + // claimed forever, so a recovered primary could never close its + // breaker under non-streaming traffic and traffic never returned + // to T0. + if status >= 200 && status < 300 { + loadbalance.RecordServiceSuccess(rule.UUID, serviceID) + } fields := failoverLogFields(c, rule, provider, model, serviceID) fields["stage"] = "failover_terminal" fields["attempt"] = i + 1 diff --git a/internal/server/failover_dispatch_test.go b/internal/server/failover_dispatch_test.go index bd455503d..7386da894 100644 --- a/internal/server/failover_dispatch_test.go +++ b/internal/server/failover_dispatch_test.go @@ -241,7 +241,14 @@ func TestIsRetryableStatus(t *testing.T) { {502, true}, {503, true}, {504, true}, - {501, false}, // not in the gateway-error set + // The whole 5xx range is retryable: error forwarding propagates the + // upstream's status verbatim, so provider-specific codes must not + // slip through the failover net. 529 is Anthropic's overloaded_error + // (the original escape); 52x also covers Cloudflare-fronted providers. + {529, true}, + {520, true}, + {599, true}, + {501, true}, // heterogeneous fallback may well implement what this tier didn't } for _, tc := range cases { if got := isRetryableStatus(tc.code); got != tc.want { diff --git a/internal/server/module/mcp/generic_stream_interceptor.go b/internal/server/module/mcp/generic_stream_interceptor.go index 79ef19bb7..e60a4ebc6 100644 --- a/internal/server/module/mcp/generic_stream_interceptor.go +++ b/internal/server/module/mcp/generic_stream_interceptor.go @@ -98,6 +98,21 @@ func NewGenericStreamInterceptor( } } +// sendEvent forwards one client-bound SSE event through the adapter, raising +// the failover gate's CommitFirstChunk signal first (idempotent, no-op when no +// gate is installed). The interceptor is a streaming producer, so like +// ProcessStream/StreamLoop it must commit on its first real chunk: without the +// signal a multi-service rule's firstChunkGate buffered the WHOLE stream until +// the request ended (no incremental delivery) and the orchestrator never saw a +// committed gate, so successful attempts were invisible to the circuit +// breaker. Every client-bound event goes through here — keep-alives +// deliberately do not commit (they can be emitted while the upstream verdict +// is still unknown, and committing forecloses failover). +func (i *GenericStreamInterceptor) sendEvent(eventType string, payload []byte) error { + protocol.CommitFirstChunk(i.c) + return i.adapter.SendEvent(i.c, eventType, payload) +} + // Run executes the streaming interceptor loop func (i *GenericStreamInterceptor) Run(req any) error { // Setup SSE headers @@ -180,16 +195,19 @@ func (i *GenericStreamInterceptor) Run(req any) error { // message (e.g. [DONE] for OpenAI Chat). func (i *GenericStreamInterceptor) sendFinalEvents() error { if i.roundMessageDelta == nil { + // SendFinalMessage writes through the adapter directly; commit first so + // even a stream that produced no forwardable events flushes the gate. + protocol.CommitFirstChunk(i.c) return i.adapter.SendFinalMessage(i.c) } - if err := i.adapter.SendEvent(i.c, "message_delta", i.roundMessageDelta); err != nil { + if err := i.sendEvent("message_delta", i.roundMessageDelta); err != nil { return err } stop := i.roundMessageStop if stop == nil { stop, _ = json.Marshal(map[string]interface{}{"type": "message_stop"}) } - return i.adapter.SendEvent(i.c, "message_stop", stop) + return i.sendEvent("message_stop", stop) } // handlePureExternal hands non-virtual tools back to the client. Only virtual @@ -201,12 +219,12 @@ func (i *GenericStreamInterceptor) handlePureExternal(response any) error { func (i *GenericStreamInterceptor) finishClientNativeToolUse() error { i.stopAfterRound = true if i.roundMessageDelta != nil { - if err := i.adapter.SendEvent(i.c, "message_delta", i.roundMessageDelta); err != nil { + if err := i.sendEvent("message_delta", i.roundMessageDelta); err != nil { return err } } if i.roundMessageStop != nil { - if err := i.adapter.SendEvent(i.c, "message_stop", i.roundMessageStop); err != nil { + if err := i.sendEvent("message_stop", i.roundMessageStop); err != nil { return err } } @@ -430,7 +448,7 @@ func (i *GenericStreamInterceptor) routeEvent(event any, eventType EventType) er // Nothing forwardable; drop rather than emit an empty frame. return nil } - return i.adapter.SendEvent(i.c, "", payload) + return i.sendEvent("", payload) } } @@ -444,7 +462,7 @@ func (i *GenericStreamInterceptor) handleTextEvent(event any) error { // Mark TTFT on the first content token; MarkFirstToken is idempotent. i.recordTTFT() - return i.adapter.SendEvent(i.c, "content_block_delta", payload) + return i.sendEvent("content_block_delta", payload) } // handleToolStartEvent handles tool use start event @@ -460,7 +478,7 @@ func (i *GenericStreamInterceptor) handleToolStartEvent(event any) error { if err != nil { return err } - return i.adapter.SendEvent(i.c, "content_block_start", payload) + return i.sendEvent("content_block_start", payload) } i.recordRoundTool(tool) @@ -477,7 +495,7 @@ func (i *GenericStreamInterceptor) handleToolStartEvent(event any) error { if err != nil { return err } - return i.adapter.SendEvent(i.c, "content_block_start", payload) + return i.sendEvent("content_block_start", payload) } // handleToolDeltaEvent handles tool parameter delta event @@ -496,7 +514,7 @@ func (i *GenericStreamInterceptor) handleToolDeltaEvent(event any) error { } // A tool input delta is content; mark TTFT (idempotent). i.recordTTFT() - return i.adapter.SendEvent(i.c, "content_block_delta", payload) + return i.sendEvent("content_block_delta", payload) } // handleToolStopEvent handles tool stop event @@ -511,7 +529,7 @@ func (i *GenericStreamInterceptor) handleToolStopEvent(event any) error { if err != nil { return err } - return i.adapter.SendEvent(i.c, "content_block_stop", payload) + return i.sendEvent("content_block_stop", payload) } // classifyResponse classifies the response to determine next action diff --git a/internal/server/server.go b/internal/server/server.go index b82c4ecfc..875b47d92 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "net/http" - "strings" "sync" "time" @@ -519,12 +518,13 @@ func NewServer(cfg *config.Config, opts ...ServerOption) *Server { // per-endpoint capability (which is now declared, not probed). if server.healthMonitor != nil { server.healthMonitor.SetProbeFunc(func(serviceID string) bool { - // serviceID format: ":" (from Service.ServiceID()) - parts := strings.Split(serviceID, ":") - if len(parts) < 1 { - return false - } - providerUUID := parts[0] + // serviceID format: "/" (FormatServiceID). + // This used to split on ":" — which never matches the "/" format — + // so the probe failed for EVERY service and each failed probe + // pushed the recovery window forward: a service marked unhealthy + // by one 429 stayed excluded forever and traffic never returned + // to it. + providerUUID, _ := loadbalance.ParseServiceID(serviceID) provider, err := cfg.GetProviderByUUID(providerUUID) if err != nil || provider == nil { diff --git a/internal/server/usage_tracking.go b/internal/server/usage_tracking.go index c9bf50eb3..dcb30ae1a 100644 --- a/internal/server/usage_tracking.go +++ b/internal/server/usage_tracking.go @@ -295,7 +295,8 @@ func classifyErrorCode(err error) string { // Same signals as isRateLimitError, tested against the already-lowered // string so a large error payload is not lowercased twice. case strings.Contains(errStr, "429") || strings.Contains(errStr, "rate limit") || - strings.Contains(errStr, "ratelimit") || strings.Contains(errStr, "1302"): + strings.Contains(errStr, "ratelimit") || strings.Contains(errStr, "1302") || + strings.Contains(errStr, "1305"): return "rate_limit" case strings.Contains(errStr, "401") || strings.Contains(errStr, "unauthorized"): return "auth_401" @@ -328,10 +329,12 @@ func isRateLimitError(err error) bool { return false } errStr := strings.ToLower(err.Error()) + // 1302/1305 are Zhipu GLM body codes for concurrency / request-rate limits. return strings.Contains(errStr, "429") || strings.Contains(errStr, "rate limit") || strings.Contains(errStr, "ratelimit") || - strings.Contains(errStr, "1302") + strings.Contains(errStr, "1302") || + strings.Contains(errStr, "1305") } // recordDetailedUsage writes a detailed usage record to the database. @@ -505,9 +508,9 @@ func (s *Server) reportHealthStatus(provider *typ.Provider, model string, err er // Error - classify and report appropriately errStr := err.Error() - // Check for rate limit (429, 1302) + // Check for rate limit (429, plus Zhipu GLM body codes 1302/1305) if strings.Contains(errStr, "429") || strings.Contains(errStr, "rate limit") || strings.Contains(errStr, "RateLimit") || - strings.Contains(errStr, "1302") || strings.Contains(errStr, "\"code\":\"1302\"") { + strings.Contains(errStr, "1302") || strings.Contains(errStr, "1305") { logrus.WithFields(logrus.Fields{ "service_id": serviceID, "provider": provider.Name, diff --git a/vmodel/README.md b/vmodel/README.md index 1f6f1ddce..2b6c8bb57 100644 --- a/vmodel/README.md +++ b/vmodel/README.md @@ -269,6 +269,7 @@ Registered by `RegisterExtendedErrorMocks()`: | `virtual-fail-auth-401` | Pre-content | 401 | auth | Authentication failure | | `virtual-fail-502` | Pre-content | 502 | upstream | Bad gateway | | `virtual-fail-503` | Pre-content | 503 | overloaded | Service unavailable | +| `virtual-fail-529` | Pre-content | 529 | overloaded | Anthropic overloaded_error | | `virtual-fail-400` | Pre-content | 400 | invalid | Invalid request | | `virtual-fail-timeout` | Mid-stream | — | timeout | Mid-stream timeout | diff --git a/vmodel/defaults_shared.go b/vmodel/defaults_shared.go index 5a367dad7..1343f5871 100644 --- a/vmodel/defaults_shared.go +++ b/vmodel/defaults_shared.go @@ -215,6 +215,20 @@ func ExtendedErrorSpecs() []SharedMockSpec { IsRetryable: true, Severity: "high", }, + { + ID: "virtual-fail-529", + Name: "Virtual Fail 529", + Content: "unreachable", + Error: &ErrorInjection{ + Stage: ErrorStagePreContent, + Status: 529, + Message: "simulated overloaded", + Type: "overloaded_error", + }, + ErrorCategory: ErrorCategoryOverloaded, + IsRetryable: true, + Severity: "medium", + }, { ID: "virtual-fail-503", Name: "Virtual Fail 503", diff --git a/vmodel/error_integration_test.go b/vmodel/error_integration_test.go index 50e73e99d..4328a1be5 100644 --- a/vmodel/error_integration_test.go +++ b/vmodel/error_integration_test.go @@ -49,7 +49,7 @@ func TestSharedDefaultMocksIncludesErrorModels(t *testing.T) { func TestExtendedErrorSpecs(t *testing.T) { specs := ExtendedErrorSpecs() - require.Len(t, specs, 5, "Should have 5 extended error specs") + require.Len(t, specs, 6, "Should have 6 extended error specs") // Test authentication error auth401 := findSpec(t, specs, "virtual-fail-auth-401") @@ -71,6 +71,13 @@ func TestExtendedErrorSpecs(t *testing.T) { assert.Equal(t, ErrorCategoryOverloaded, unavailable.ErrorCategory) assert.True(t, unavailable.IsRetryable, "503 should be retryable") + // Test 529 Anthropic overloaded + overloaded := findSpec(t, specs, "virtual-fail-529") + require.NotNil(t, overloaded) + assert.Equal(t, ErrorCategoryOverloaded, overloaded.ErrorCategory) + assert.True(t, overloaded.IsRetryable, "529 should be retryable") + assert.Equal(t, 529, overloaded.Error.Status) + // Test invalid request invalid := findSpec(t, specs, "virtual-fail-400") require.NotNil(t, invalid) @@ -87,13 +94,13 @@ func TestExtendedErrorSpecs(t *testing.T) { } func TestAllErrorSpecs(t *testing.T) { - // Total error models = 4 (basic in SharedDefaultMocks) + 5 (extended) + // Total error models = 4 (basic in SharedDefaultMocks) + 6 (extended) basicErrorModels := findErrorModels(SharedDefaultMocks()) extendedSpecs := ExtendedErrorSpecs() // Verify we have the expected counts assert.Len(t, basicErrorModels, 4, "Should have 4 basic error models in SharedDefaultMocks") - assert.Len(t, extendedSpecs, 5, "Should have 5 extended error specs") + assert.Len(t, extendedSpecs, 6, "Should have 6 extended error specs") // Verify basic specs are in SharedDefaultMocks assert.Contains(t, specIDs(basicErrorModels), "virtual-fail-429")