From 2c897a2214554a59e8863c8e79500afffb0b37d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 11:36:47 +0000 Subject: [PATCH 1/7] fix: skip health filter for tier tactic to preserve fast breaker recovery The HealthMonitor/HealthFilter (5-min recovery window) was pre-filtering services before the TierTactic could see them, defeating the circuit breaker's 30-second recovery. Tier tactic manages its own health via breakers, so the health filter is now bypassed for tier-based rules. https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/server/load_balancer.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/server/load_balancer.go b/internal/server/load_balancer.go index b19b5a438..034489813 100644 --- a/internal/server/load_balancer.go +++ b/internal/server/load_balancer.go @@ -76,6 +76,12 @@ func (lb *LoadBalancer) SelectService(rule *typ.Rule) (*loadbalance.Service, err return nil, fmt.Errorf("no active services for rule %s", rule.RequestModel) } + // Tier tactic has its own circuit-breaker-based health management with + // fast recovery (30 s). Applying the HealthFilter here would hide + // services for the full monitor recovery window (5 min), defeating the + // tier tactic's quick failover/recovery design. + isTier := rule.LBTactic.Type == loadbalance.TacticTier + // Filter healthy services using health filter. When every active service is // currently marked unhealthy (e.g. a transient 429 on a single-service rule, // or all services inside the recovery window at once), fall back to the full @@ -83,11 +89,16 @@ func (lb *LoadBalancer) SelectService(rule *typ.Rule) (*loadbalance.Service, err // is strictly better than a hard "no service available": the service may have // already recovered, and if it really is still failing the caller gets the // real upstream error (e.g. 429) rather than a confusing routing error. - healthyServices := lb.healthFilter.Filter(activeServices) - if len(healthyServices) == 0 { - logrus.Warnf("[load_balancer] all %d active services for rule %s are unhealthy; "+ - "falling back to active set", len(activeServices), rule.RequestModel) + var healthyServices []*loadbalance.Service + if isTier { healthyServices = activeServices + } else { + healthyServices = lb.healthFilter.Filter(activeServices) + if len(healthyServices) == 0 { + logrus.Warnf("[load_balancer] all %d active services for rule %s are unhealthy; "+ + "falling back to active set", len(activeServices), rule.RequestModel) + healthyServices = activeServices + } } // For single healthy service rules, return it directly From 8ed66c7a8a67acf6aa85f4be37168d696d80c276 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 11:41:04 +0000 Subject: [PATCH 2/7] test: add 7 tests challenging tier tactic vs health filter interaction Tests cover: - Tier tactic bypasses health filter (T0 reachable despite HealthMonitor) - Non-tier tactics still respect health filter - Breaker fallback works while health filter would block - 3-tier waterfall with all services HealthMonitor-unhealthy - Within-tier load sharing with partially unhealthy services - Rate limit on T0 does not stick for 5 min - All services down (both HealthMonitor + breaker open) still returns T0 https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/servertest/health_filter_test.go | 320 ++++++++++++++++++++++ 1 file changed, 320 insertions(+) diff --git a/internal/servertest/health_filter_test.go b/internal/servertest/health_filter_test.go index a1049cfaf..e64351c1a 100644 --- a/internal/servertest/health_filter_test.go +++ b/internal/servertest/health_filter_test.go @@ -1,6 +1,7 @@ package servertest import ( + "fmt" "testing" "time" @@ -335,3 +336,322 @@ func TestHealthFilter_InactiveServices(t *testing.T) { assert.Equal(t, "provider-active", service.Provider) } } + +// --- Tier tactic vs health filter interaction tests --- + +// newTierTestLB creates a LoadBalancer with a health monitor that has a long +// recovery timeout (simulating the production 5-min window) so we can verify +// that tier rules bypass it while non-tier rules honour it. +func newTierTestLB(t *testing.T) (*server.LoadBalancer, *loadbalance.HealthMonitor) { + t.Helper() + appConfig, err := config.NewAppConfig(config.WithConfigDir(t.TempDir())) + require.NoError(t, err) + + healthConfig := loadbalance.HealthMonitorConfig{ + ConsecutiveErrorThreshold: 3, + RecoveryTimeoutSeconds: 600, // 10 min — effectively "never recovers during this test" + } + hm := loadbalance.NewHealthMonitor(healthConfig) + hf := typ.NewHealthFilter(hm) + lb := server.NewLoadBalancer(appConfig.GetGlobalConfig(), hf) + t.Cleanup(lb.Stop) + return lb, hm +} + +func tierService(provider, model string, tier int) *loadbalance.Service { + return &loadbalance.Service{ + Provider: provider, + Model: model, + Tier: tier, + Active: true, + Weight: 1, + } +} + +// TestTierTactic_BypassesHealthFilter verifies that a tier-based rule still +// sees all active services even when the HealthMonitor marks T0 as unhealthy. +// Before the fix, the health filter would hide T0 for ~5 min, blocking the +// tier tactic's 30-second breaker recovery. +func TestTierTactic_BypassesHealthFilter(t *testing.T) { + lb, hm := newTierTestLB(t) + + primary := tierService("tier-bypass-p1", "m1", 0) + backup := tierService("tier-bypass-p2", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{primary, backup}, + Active: true, + } + + // Mark T0 as unhealthy via the HealthMonitor (rate limited). + hm.ReportRateLimit(primary.ServiceID()) + assert.False(t, hm.IsHealthy(primary.ServiceID())) + + // Even though the HealthMonitor says T0 is unhealthy, the tier tactic + // should still see it (breaker is closed) and pick it. + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + assert.Equal(t, primary.Provider, svc.Provider, + "tier tactic should bypass health filter and pick T0") +} + +// TestNonTierTactic_StillUsesHealthFilter confirms that the fix only +// bypasses the health filter for tier rules — other tactics still respect it. +func TestNonTierTactic_StillUsesHealthFilter(t *testing.T) { + lb, hm := newTierTestLB(t) + + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticRandom, + Params: nil, + }, + Services: []*loadbalance.Service{ + {Provider: "hf-rand-p1", Model: "m1", Active: true, Weight: 1}, + {Provider: "hf-rand-p2", Model: "m1", Active: true, Weight: 1}, + }, + Active: true, + } + + hm.ReportRateLimit(rule.Services[0].ServiceID()) + + counts := map[string]int{} + for i := 0; i < 20; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + counts[svc.Provider]++ + } + assert.Equal(t, 0, counts["hf-rand-p1"], + "random tactic should still respect health filter") + assert.Equal(t, 20, counts["hf-rand-p2"]) +} + +// TestTierTactic_BreakerFallbackWhileHealthFilterWouldBlock demonstrates the +// end-to-end scenario: T0 is both HealthMonitor-unhealthy (long timeout) and +// breaker-open (short timeout). The tier tactic should fall to T1 via the +// breaker — not because the health filter hid T0. +func TestTierTactic_BreakerFallbackWhileHealthFilterWouldBlock(t *testing.T) { + lb, hm := newTierTestLB(t) + + primary := tierService("brk-hf-p1", "m1", 0) + backup := tierService("brk-hf-p2", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{primary, backup}, + Active: true, + } + + // Trip BOTH the health monitor and the circuit breaker for T0. + hm.ReportRateLimit(primary.ServiceID()) + store := loadbalance.DefaultBreakerStore() + for i := 0; i < loadbalance.DefaultBreakerFailureThreshold; i++ { + store.RecordFailure(primary.ServiceID()) + } + defer store.RecordSuccess(primary.ServiceID()) + + // Breaker is open → tier tactic should fall to T1. + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + assert.Equal(t, backup.Provider, svc.Provider, + "breaker-open T0 should fall to T1") + + // Now recover the breaker (simulating 30 s elapsed). The health monitor + // still says "unhealthy" (10-min window), but the tier tactic bypasses + // the filter, sees T0, checks the breaker, and routes back to T0. + store.RecordSuccess(primary.ServiceID()) + assert.False(t, hm.IsHealthy(primary.ServiceID()), + "health monitor should still say unhealthy") + + svc, err = lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + assert.Equal(t, primary.Provider, svc.Provider, + "breaker recovered T0 should be picked even though health monitor says unhealthy") +} + +// TestTierTactic_MultiTierWaterfallWithUnhealthyServices tests a 3-tier +// setup where tiers are selectively tripped via breakers while the health +// monitor marks everything unhealthy. The tier tactic should waterfall +// through breakers, not be blocked by the health filter. +func TestTierTactic_MultiTierWaterfallWithUnhealthyServices(t *testing.T) { + lb, hm := newTierTestLB(t) + + t0 := tierService("waterfall-p0", "m1", 0) + t1 := tierService("waterfall-p1", "m1", 1) + t2 := tierService("waterfall-p2", "m1", 2) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{t0, t1, t2}, + Active: true, + } + + // Mark ALL services as unhealthy in HealthMonitor. + for _, svc := range rule.Services { + hm.ReportRateLimit(svc.ServiceID()) + } + + store := loadbalance.DefaultBreakerStore() + // Trip T0 and T1 breakers; leave T2 breaker closed. + for _, svc := range []*loadbalance.Service{t0, t1} { + for i := 0; i < loadbalance.DefaultBreakerFailureThreshold; i++ { + store.RecordFailure(svc.ServiceID()) + } + } + defer func() { + store.RecordSuccess(t0.ServiceID()) + store.RecordSuccess(t1.ServiceID()) + }() + + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + assert.Equal(t, t2.Provider, svc.Provider, + "should waterfall to T2 via breakers despite health monitor blocking all") + + // Recover T1 breaker — traffic should go to T1 (not stay on T2). + store.RecordSuccess(t1.ServiceID()) + svc, err = lb.SelectService(rule) + require.NoError(t, err) + assert.Equal(t, t1.Provider, svc.Provider, + "T1 breaker recovery should route back to T1") + + // Recover T0 breaker — traffic should return to T0. + store.RecordSuccess(t0.ServiceID()) + svc, err = lb.SelectService(rule) + require.NoError(t, err) + assert.Equal(t, t0.Provider, svc.Provider, + "T0 breaker recovery should route back to T0") +} + +// TestTierTactic_WithinTierLoadSharing verifies that when multiple services +// share a tier, they still share load even when the health filter would +// remove some of them. +func TestTierTactic_WithinTierLoadSharing(t *testing.T) { + lb, hm := newTierTestLB(t) + + a := tierService("share-a", "m1", 0) + b := tierService("share-b", "m1", 0) + backup := tierService("share-backup", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{a, b, backup}, + Active: true, + } + + // Mark service A as unhealthy in the health monitor. Without the fix, + // only B would be visible and the backup would never get picked — but + // crucially, A's breaker is still closed, so the tier tactic should still + // pick it some of the time. + hm.ReportRateLimit(a.ServiceID()) + + counts := map[string]int{} + for i := 0; i < 200; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + counts[svc.Provider]++ + } + + assert.Greater(t, counts[a.Provider], 0, + "service A should still receive traffic despite health filter marking it unhealthy") + assert.Greater(t, counts[b.Provider], 0, + "service B should receive traffic") + assert.Equal(t, 0, counts[backup.Provider], + "T1 backup should not be picked when T0 breakers are all closed") +} + +// TestTierTactic_RateLimitDoesNotStickFor5Min is the highest-level +// reproduction of the original bug: a single 429 on T0 should not pin +// traffic to T1 for the full health-monitor window. +func TestTierTactic_RateLimitDoesNotStickFor5Min(t *testing.T) { + lb, hm := newTierTestLB(t) + + primary := tierService("ratelim-p0", "m1", 0) + fallback := tierService("ratelim-p1", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{primary, fallback}, + Active: true, + } + + // Simulate a 429 on the primary — HealthMonitor marks it unhealthy. + hm.ReportRateLimit(primary.ServiceID()) + + // Immediately after the 429, the tier tactic should still see T0 + // (breaker is closed) and route there. + for i := 0; i < 10; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + assert.Equal(t, primary.Provider, svc.Provider, + fmt.Sprintf("attempt %d: T0 breaker closed, should still pick T0", i)) + } +} + +// TestTierTactic_AllServicesHealthMonitorUnhealthy_AllBreakersOpen tests the +// extreme case: every service is HealthMonitor-unhealthy AND breaker-open. +// The tier tactic should still return a T0 service so the caller gets the +// real upstream error. +func TestTierTactic_AllServicesHealthMonitorUnhealthy_AllBreakersOpen(t *testing.T) { + lb, hm := newTierTestLB(t) + + t0 := tierService("alldown-p0", "m1", 0) + t1 := tierService("alldown-p1", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{t0, t1}, + Active: true, + } + + // Mark all as unhealthy + breakers open. + store := loadbalance.DefaultBreakerStore() + for _, svc := range rule.Services { + hm.ReportRateLimit(svc.ServiceID()) + for i := 0; i < loadbalance.DefaultBreakerFailureThreshold; i++ { + store.RecordFailure(svc.ServiceID()) + } + } + defer func() { + store.RecordSuccess(t0.ServiceID()) + store.RecordSuccess(t1.ServiceID()) + }() + + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc, "should still return a service for the upstream-error path") + assert.Equal(t, 0, svc.Tier, + "all-open fallback should pick T0 to surface the real upstream error") +} From a25c35abde846aa4456c1199bfc388367c0e0a7d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 12:34:11 +0000 Subject: [PATCH 3/7] fix: refine tier tactic health filter bypass to still block auth errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial fix bypassed the health filter entirely for tier rules. This left a gap: auth errors (401/403) are permanent — a revoked API key never self-heals — so the breaker would cycle through half-open every 30 s, wasting a request each time. Refined approach: for tier rules, only filter out services with auth errors (permanent). Transient states (rate limits, consecutive errors) pass through to the tier tactic's circuit breaker for fast recovery. Changes: - HealthMonitor.HasAuthError(): exposes auth-error state for a service - HealthFilter.FilterAuthErrors(): removes only auth-error services - LoadBalancer.SelectService(): uses FilterAuthErrors for tier rules - 3 new tests for auth-error edge cases https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/loadbalance/health_monitor.go | 14 +++ internal/server/load_balancer.go | 12 ++- internal/servertest/health_filter_test.go | 104 ++++++++++++++++++++++ internal/typ/health_filter.go | 17 ++++ 4 files changed, 143 insertions(+), 4 deletions(-) diff --git a/internal/loadbalance/health_monitor.go b/internal/loadbalance/health_monitor.go index 926d7a1c8..86f2a04c9 100644 --- a/internal/loadbalance/health_monitor.go +++ b/internal/loadbalance/health_monitor.go @@ -317,6 +317,20 @@ func (hm *HealthMonitor) GetAllHealth() map[string]*ServiceHealth { return result } +// HasAuthError returns true if the service is currently marked with an auth +// error (401/403). Auth errors are permanent until the key is fixed, so +// callers that manage their own transient-failure recovery (e.g. circuit +// breakers) can still exclude these. +func (hm *HealthMonitor) HasAuthError(serviceID string) bool { + health := hm.getHealth(serviceID) + if health == nil { + return false + } + health.mutex.RLock() + defer health.mutex.RUnlock() + return health.AuthError +} + // ResetHealth manually resets a service's health to healthy func (hm *HealthMonitor) ResetHealth(serviceID string) { hm.recoverService(serviceID) diff --git a/internal/server/load_balancer.go b/internal/server/load_balancer.go index 034489813..bb572226a 100644 --- a/internal/server/load_balancer.go +++ b/internal/server/load_balancer.go @@ -77,9 +77,10 @@ func (lb *LoadBalancer) SelectService(rule *typ.Rule) (*loadbalance.Service, err } // Tier tactic has its own circuit-breaker-based health management with - // fast recovery (30 s). Applying the HealthFilter here would hide - // services for the full monitor recovery window (5 min), defeating the - // tier tactic's quick failover/recovery design. + // fast recovery (30 s). The full HealthFilter would hide services for the + // monitor's recovery window (5 min), defeating the breaker's quick + // failover/recovery. However auth errors (401/403) are permanent — a + // revoked key never self-heals — so those must still be filtered out. isTier := rule.LBTactic.Type == loadbalance.TacticTier // Filter healthy services using health filter. When every active service is @@ -91,7 +92,10 @@ func (lb *LoadBalancer) SelectService(rule *typ.Rule) (*loadbalance.Service, err // real upstream error (e.g. 429) rather than a confusing routing error. var healthyServices []*loadbalance.Service if isTier { - healthyServices = activeServices + healthyServices = lb.healthFilter.FilterAuthErrors(activeServices) + if len(healthyServices) == 0 { + healthyServices = activeServices + } } else { healthyServices = lb.healthFilter.Filter(activeServices) if len(healthyServices) == 0 { diff --git a/internal/servertest/health_filter_test.go b/internal/servertest/health_filter_test.go index e64351c1a..82ccd1907 100644 --- a/internal/servertest/health_filter_test.go +++ b/internal/servertest/health_filter_test.go @@ -655,3 +655,107 @@ func TestTierTactic_AllServicesHealthMonitorUnhealthy_AllBreakersOpen(t *testing assert.Equal(t, 0, svc.Tier, "all-open fallback should pick T0 to surface the real upstream error") } + +// TestTierTactic_AuthErrorStillFiltered verifies that auth errors (401/403) +// are still filtered out for tier rules. Auth errors are permanent — a +// revoked API key never self-heals — so the tier tactic should not keep +// probing the broken service every 30 seconds via the breaker half-open cycle. +func TestTierTactic_AuthErrorStillFiltered(t *testing.T) { + lb, hm := newTierTestLB(t) + + broken := tierService("auth-broken", "m1", 0) + fallback := tierService("auth-fallback", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{broken, fallback}, + Active: true, + } + + // T0 has a revoked API key → auth error. + hm.ReportAuthError(broken.ServiceID(), 401) + + // The tier tactic should NOT pick T0 despite its breaker being closed. + for i := 0; i < 10; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + assert.Equal(t, fallback.Provider, svc.Provider, + fmt.Sprintf("attempt %d: auth-error service should be filtered", i)) + } +} + +// TestTierTactic_AuthErrorOnlyFiltersAuthNotRateLimit confirms the filter is +// surgical: a T0 with a rate limit is kept (breaker handles it), while a T0 +// with an auth error is removed. +func TestTierTactic_AuthErrorOnlyFiltersAuthNotRateLimit(t *testing.T) { + lb, hm := newTierTestLB(t) + + rateLimited := tierService("auth-rl-p0", "m1", 0) + authBroken := tierService("auth-rl-p1", "m1", 0) + backup := tierService("auth-rl-p2", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{rateLimited, authBroken, backup}, + Active: true, + } + + // One T0 is rate-limited (transient), the other has an auth error (permanent). + hm.ReportRateLimit(rateLimited.ServiceID()) + hm.ReportAuthError(authBroken.ServiceID(), 403) + + counts := map[string]int{} + for i := 0; i < 50; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + require.NotNil(t, svc) + counts[svc.Provider]++ + } + + assert.Greater(t, counts[rateLimited.Provider], 0, + "rate-limited T0 should still be reachable (breaker handles transient failures)") + assert.Equal(t, 0, counts[authBroken.Provider], + "auth-error T0 should be filtered out") + assert.Equal(t, 0, counts[backup.Provider], + "T1 should not be picked while a healthy T0 exists") +} + +// TestTierTactic_AllT0AuthError_FallsToT1 ensures that when every T0 service +// has an auth error, the tier tactic falls to T1 via filtering, not via +// breaker cycling. +func TestTierTactic_AllT0AuthError_FallsToT1(t *testing.T) { + lb, hm := newTierTestLB(t) + + t0a := tierService("allauth-a", "m1", 0) + t0b := tierService("allauth-b", "m1", 0) + t1 := tierService("allauth-c", "m1", 1) + rule := &typ.Rule{ + UUID: uuid.New().String(), + RequestModel: "test-model", + LBTactic: typ.Tactic{ + Type: loadbalance.TacticTier, + Params: typ.DefaultTierParams(), + }, + Services: []*loadbalance.Service{t0a, t0b, t1}, + Active: true, + } + + hm.ReportAuthError(t0a.ServiceID(), 401) + hm.ReportAuthError(t0b.ServiceID(), 403) + + for i := 0; i < 10; i++ { + svc, err := lb.SelectService(rule) + require.NoError(t, err) + assert.Equal(t, t1.Provider, svc.Provider, + "all T0 auth errors should route to T1 immediately") + } +} diff --git a/internal/typ/health_filter.go b/internal/typ/health_filter.go index 0bbfd0598..7346050e0 100644 --- a/internal/typ/health_filter.go +++ b/internal/typ/health_filter.go @@ -47,6 +47,23 @@ func (hf *HealthFilter) FilterWithFallback(services []*loadbalance.Service) []*l return healthy } +// FilterAuthErrors removes only services with auth errors (401/403). Transient +// states like rate limits and consecutive errors are kept so that callers with +// their own recovery mechanisms (e.g. tier tactic's circuit breaker) can still +// see those services. +func (hf *HealthFilter) FilterAuthErrors(services []*loadbalance.Service) []*loadbalance.Service { + if hf.monitor == nil { + return services + } + var out []*loadbalance.Service + for _, svc := range services { + if svc != nil && !hf.monitor.HasAuthError(svc.ServiceID()) { + out = append(out, svc) + } + } + return out +} + // IsHealthy checks if a specific service is healthy func (hf *HealthFilter) IsHealthy(serviceID string) bool { if hf.monitor == nil { From 115d57b524b887d45e44311f6bd62f53f82b386d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 12:37:38 +0000 Subject: [PATCH 4/7] feat: add exponential backoff to circuit breaker half-open probes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without backoff, a sustained outage causes the breaker to probe the broken service every 30s indefinitely — wasting one user's round-trip each time and potentially disrupting provider-side caches. Now each consecutive half-open probe failure doubles the open window: 30s → 60s → 120s → 240s → cap (5 min). A successful probe resets the backoff to the base duration. This gives fast recovery for transient issues while reducing overhead during extended outages. https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/loadbalance/breaker.go | 27 ++++++++- internal/loadbalance/breaker_test.go | 83 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/internal/loadbalance/breaker.go b/internal/loadbalance/breaker.go index 8553b4ba7..f53ed6588 100644 --- a/internal/loadbalance/breaker.go +++ b/internal/loadbalance/breaker.go @@ -18,6 +18,7 @@ const ( const ( DefaultBreakerFailureThreshold = 3 DefaultBreakerOpenDuration = 30 * time.Second + DefaultBreakerMaxOpenDuration = 5 * time.Minute ) // Breaker is a simple three-state circuit breaker for a single service. @@ -37,9 +38,11 @@ type Breaker struct { consecFails int openedAt time.Time halfOpenInFlight bool + halfOpenFails int // consecutive half-open probe failures for backoff FailureThreshold int OpenDuration time.Duration + MaxOpenDuration time.Duration } // NewBreaker creates a breaker with the supplied thresholds. Zero values @@ -55,6 +58,7 @@ func NewBreaker(failureThreshold int, openDuration time.Duration) *Breaker { state: BreakerClosed, FailureThreshold: failureThreshold, OpenDuration: openDuration, + MaxOpenDuration: DefaultBreakerMaxOpenDuration, } } @@ -69,7 +73,7 @@ func (b *Breaker) Allow() bool { case BreakerClosed: return true case BreakerOpen: - if time.Since(b.openedAt) >= b.OpenDuration { + if time.Since(b.openedAt) >= b.currentOpenDuration() { b.state = BreakerHalfOpen b.halfOpenInFlight = true return true @@ -91,11 +95,13 @@ func (b *Breaker) RecordSuccess() { defer b.mu.Unlock() b.state = BreakerClosed b.consecFails = 0 + b.halfOpenFails = 0 b.halfOpenInFlight = false } // RecordFailure increments failure tracking and trips the breaker when -// the threshold is reached. A failure during HalfOpen immediately re-opens. +// the threshold is reached. A failure during HalfOpen immediately re-opens +// and increases the backoff for the next open window. func (b *Breaker) RecordFailure() { b.mu.Lock() defer b.mu.Unlock() @@ -103,6 +109,7 @@ func (b *Breaker) RecordFailure() { if b.state == BreakerHalfOpen { b.state = BreakerOpen b.openedAt = time.Now() + b.halfOpenFails++ b.halfOpenInFlight = false return } @@ -113,12 +120,26 @@ func (b *Breaker) RecordFailure() { } } +// currentOpenDuration returns the backoff-adjusted open duration. +// Each consecutive half-open failure doubles the wait: 30s → 60s → 120s → 240s → cap. +// Must be called with b.mu held. +func (b *Breaker) currentOpenDuration() time.Duration { + d := b.OpenDuration + for i := 0; i < b.halfOpenFails; i++ { + d *= 2 + if d >= b.MaxOpenDuration { + return b.MaxOpenDuration + } + } + return d +} + // State returns the current breaker state. Intended for introspection / UI. func (b *Breaker) State() BreakerState { b.mu.Lock() defer b.mu.Unlock() // Apply the lazy Open→HalfOpen transition for read consistency. - if b.state == BreakerOpen && time.Since(b.openedAt) >= b.OpenDuration { + if b.state == BreakerOpen && time.Since(b.openedAt) >= b.currentOpenDuration() { return BreakerHalfOpen } return b.state diff --git a/internal/loadbalance/breaker_test.go b/internal/loadbalance/breaker_test.go index af7b15ad5..29df56fb2 100644 --- a/internal/loadbalance/breaker_test.go +++ b/internal/loadbalance/breaker_test.go @@ -73,6 +73,89 @@ func TestBreakerSuccessResetsCounter(t *testing.T) { } } +func TestBreakerExponentialBackoff(t *testing.T) { + base := 20 * time.Millisecond + b := NewBreaker(1, base) + b.MaxOpenDuration = 200 * time.Millisecond + + // Trip the breaker. + b.RecordFailure() + + // 1st open window: base (20 ms). + time.Sleep(base + 5*time.Millisecond) + if !b.Allow() { + t.Fatal("should allow probe after 1st open window") + } + b.RecordFailure() // half-open → open, halfOpenFails=1 + + // 2nd open window: 40 ms (base * 2^1). + time.Sleep(base + 5*time.Millisecond) // 25 ms — too early + if b.Allow() { + t.Fatal("should NOT allow probe yet (backoff doubled to 40 ms)") + } + time.Sleep(base) // total ~45 ms ≥ 40 ms + if !b.Allow() { + t.Fatal("should allow probe after 2nd backoff window") + } + b.RecordFailure() // halfOpenFails=2 + + // 3rd open window: 80 ms (base * 2^2). + time.Sleep(60 * time.Millisecond) // too early for 80 ms + if b.Allow() { + t.Fatal("should NOT allow probe yet (backoff at 80 ms)") + } + time.Sleep(25 * time.Millisecond) // total ~85 ms ≥ 80 ms + if !b.Allow() { + t.Fatal("should allow probe after 3rd backoff window") + } + b.RecordFailure() // halfOpenFails=3, next would be 160 ms + + // 4th open window: 160 ms (base * 2^3). + time.Sleep(165 * time.Millisecond) + if !b.Allow() { + t.Fatal("should allow probe after 4th backoff window") + } + b.RecordFailure() // halfOpenFails=4, next would be 320 ms → capped at 200 ms + + // 5th open window: capped at 200 ms. + time.Sleep(195 * time.Millisecond) // just under cap + if b.Allow() { + t.Fatal("should NOT allow probe yet (capped at 200 ms)") + } + time.Sleep(10 * time.Millisecond) // total ~205 ms ≥ 200 ms + if !b.Allow() { + t.Fatal("should allow probe after cap-limited window") + } +} + +func TestBreakerBackoffResetsOnSuccess(t *testing.T) { + base := 20 * time.Millisecond + b := NewBreaker(1, base) + b.MaxOpenDuration = 500 * time.Millisecond + + // Build up backoff: trip → probe fail → trip → probe fail. + b.RecordFailure() + time.Sleep(base + 5*time.Millisecond) + b.Allow() + b.RecordFailure() // halfOpenFails=1, next window=40 ms + + time.Sleep(45 * time.Millisecond) + b.Allow() + + // Probe succeeds → everything resets. + b.RecordSuccess() + if b.State() != BreakerClosed { + t.Fatal("should be closed after success") + } + + // Trip again — backoff should be back to base, not 80 ms. + b.RecordFailure() + time.Sleep(base + 5*time.Millisecond) + if !b.Allow() { + t.Fatal("after success+retrip, should probe at base duration, not backed-off") + } +} + func TestBreakerStoreLazyCreation(t *testing.T) { store := NewBreakerStore(2, time.Second) b1 := store.Get("svc:a") From a0370e6c647aa17401312e0d54f570cd9c6f4e09 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 12:40:18 +0000 Subject: [PATCH 5/7] tune: raise breaker base open duration from 30s to 60s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 30s was too aggressive for LLM API providers — rate-limit windows are typically 60s+, so the first probe was almost guaranteed to fail, wasting a user's round-trip latency for nothing. 60s aligns better with real-world provider rate-limit recovery while still being fast enough for transient server errors. Backoff sequence is now: 60s → 120s → 240s → 5 min cap. https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- .design/priority-routing.md | 4 ++-- internal/loadbalance/breaker.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.design/priority-routing.md b/.design/priority-routing.md index c8a8fa993..3fdea3817 100644 --- a/.design/priority-routing.md +++ b/.design/priority-routing.md @@ -58,8 +58,8 @@ Three properties fall out: The breaker is a three-state machine (`Closed → Open → HalfOpen`): - **Closed** — normal. `Allow()` returns true, failures are counted. -- **Open** — too many consecutive failures (`FailureThreshold`, default 3). `Allow()` returns false. After `OpenDuration` (default 30 s) the next `Allow()` call lazily flips to HalfOpen. -- **HalfOpen** — exactly one probe is permitted. Success → Closed, failure → Open with a fresh timer. +- **Open** — too many consecutive failures (`FailureThreshold`, default 3). `Allow()` returns false. After `OpenDuration` (default 60 s) the next `Allow()` call lazily flips to HalfOpen. +- **HalfOpen** — exactly one probe is permitted. Success → Closed, failure → Open with exponential backoff (60 s → 120 s → 240 s → 5 min cap). Success resets backoff to the base duration. Recovery requires **no separate scheduler**. Selection re-evaluates the tier list every request, and the breaker's lazy state transition admits one probe naturally. Active probing was considered and rejected for v1 — for hot rules it's redundant, and for cold rules there is no one to serve anyway. diff --git a/internal/loadbalance/breaker.go b/internal/loadbalance/breaker.go index f53ed6588..dcfb662f2 100644 --- a/internal/loadbalance/breaker.go +++ b/internal/loadbalance/breaker.go @@ -17,7 +17,7 @@ const ( // Default circuit breaker tunables. const ( DefaultBreakerFailureThreshold = 3 - DefaultBreakerOpenDuration = 30 * time.Second + DefaultBreakerOpenDuration = 60 * time.Second DefaultBreakerMaxOpenDuration = 5 * time.Minute ) From e94afbcae11b7ba5db349eccb18acb568f4f437d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 15:25:20 +0000 Subject: [PATCH 6/7] simplify: zero-alloc FilterAuthErrors for common case, fix stale comment FilterAuthErrors is called per-request for tier rules. In the common case (no auth errors), it now returns the input slice directly with zero allocation. Only allocates when an auth-error service is actually encountered. Also fixes a stale "30s" reference in the backoff comment. https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/loadbalance/breaker.go | 2 +- internal/typ/health_filter.go | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/loadbalance/breaker.go b/internal/loadbalance/breaker.go index dcfb662f2..9c3034e9e 100644 --- a/internal/loadbalance/breaker.go +++ b/internal/loadbalance/breaker.go @@ -121,7 +121,7 @@ func (b *Breaker) RecordFailure() { } // currentOpenDuration returns the backoff-adjusted open duration. -// Each consecutive half-open failure doubles the wait: 30s → 60s → 120s → 240s → cap. +// Each consecutive half-open failure doubles the wait (e.g. 60s → 120s → 240s → cap). // Must be called with b.mu held. func (b *Breaker) currentOpenDuration() time.Duration { d := b.OpenDuration diff --git a/internal/typ/health_filter.go b/internal/typ/health_filter.go index 7346050e0..186d823b5 100644 --- a/internal/typ/health_filter.go +++ b/internal/typ/health_filter.go @@ -56,12 +56,22 @@ func (hf *HealthFilter) FilterAuthErrors(services []*loadbalance.Service) []*loa return services } var out []*loadbalance.Service - for _, svc := range services { - if svc != nil && !hf.monitor.HasAuthError(svc.ServiceID()) { + for i, svc := range services { + if svc == nil || hf.monitor.HasAuthError(svc.ServiceID()) { + if out == nil { + out = make([]*loadbalance.Service, 0, len(services)) + out = append(out, services[:i]...) + } + continue + } + if out != nil { out = append(out, svc) } } - return out + if out != nil { + return out + } + return services } // IsHealthy checks if a specific service is healthy From 9312d35d5e9c1790bafe61f7b532c9b5df93ee4a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 15:28:13 +0000 Subject: [PATCH 7/7] test: replace time.Sleep with time manipulation in breaker backoff tests Use windForward() to move openedAt backwards instead of sleeping through real wall-clock time. Reduces TestBreakerExponentialBackoff from 0.53s to <1ms and TestBreakerBackoffResetsOnSuccess from 0.10s to <1ms. https://claude.ai/code/session_01GjvoWzPe8qvowivo7P1oG4 --- internal/loadbalance/breaker_test.go | 67 ++++++++++++++-------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/internal/loadbalance/breaker_test.go b/internal/loadbalance/breaker_test.go index 29df56fb2..461807188 100644 --- a/internal/loadbalance/breaker_test.go +++ b/internal/loadbalance/breaker_test.go @@ -73,73 +73,74 @@ func TestBreakerSuccessResetsCounter(t *testing.T) { } } +// windForward moves a breaker's openedAt backwards in time so the open +// window appears to have elapsed, without any real time.Sleep. +func windForward(b *Breaker, d time.Duration) { + b.mu.Lock() + b.openedAt = b.openedAt.Add(-d) + b.mu.Unlock() +} + func TestBreakerExponentialBackoff(t *testing.T) { - base := 20 * time.Millisecond + base := 100 * time.Millisecond b := NewBreaker(1, base) - b.MaxOpenDuration = 200 * time.Millisecond + b.MaxOpenDuration = 500 * time.Millisecond - // Trip the breaker. - b.RecordFailure() + b.RecordFailure() // → open - // 1st open window: base (20 ms). - time.Sleep(base + 5*time.Millisecond) + // 1st window: base (100 ms). Fast-forward past it. + windForward(b, base) if !b.Allow() { t.Fatal("should allow probe after 1st open window") } b.RecordFailure() // half-open → open, halfOpenFails=1 - // 2nd open window: 40 ms (base * 2^1). - time.Sleep(base + 5*time.Millisecond) // 25 ms — too early + // 2nd window: 200 ms. Advance only 150 ms — too early. + windForward(b, 150*time.Millisecond) if b.Allow() { - t.Fatal("should NOT allow probe yet (backoff doubled to 40 ms)") + t.Fatal("should NOT allow probe yet (backoff doubled to 200 ms)") } - time.Sleep(base) // total ~45 ms ≥ 40 ms + // Advance the remaining 50 ms. + windForward(b, 50*time.Millisecond) if !b.Allow() { t.Fatal("should allow probe after 2nd backoff window") } b.RecordFailure() // halfOpenFails=2 - // 3rd open window: 80 ms (base * 2^2). - time.Sleep(60 * time.Millisecond) // too early for 80 ms + // 3rd window: 400 ms. Advance only 300 ms — too early. + windForward(b, 300*time.Millisecond) if b.Allow() { - t.Fatal("should NOT allow probe yet (backoff at 80 ms)") + t.Fatal("should NOT allow probe yet (backoff at 400 ms)") } - time.Sleep(25 * time.Millisecond) // total ~85 ms ≥ 80 ms + windForward(b, 100*time.Millisecond) if !b.Allow() { t.Fatal("should allow probe after 3rd backoff window") } - b.RecordFailure() // halfOpenFails=3, next would be 160 ms + b.RecordFailure() // halfOpenFails=3, next would be 800 ms → capped at 500 ms - // 4th open window: 160 ms (base * 2^3). - time.Sleep(165 * time.Millisecond) - if !b.Allow() { - t.Fatal("should allow probe after 4th backoff window") - } - b.RecordFailure() // halfOpenFails=4, next would be 320 ms → capped at 200 ms - - // 5th open window: capped at 200 ms. - time.Sleep(195 * time.Millisecond) // just under cap + // 4th window: capped at 500 ms. Advance only 450 ms — too early. + windForward(b, 450*time.Millisecond) if b.Allow() { - t.Fatal("should NOT allow probe yet (capped at 200 ms)") + t.Fatal("should NOT allow probe yet (capped at 500 ms)") } - time.Sleep(10 * time.Millisecond) // total ~205 ms ≥ 200 ms + windForward(b, 50*time.Millisecond) if !b.Allow() { t.Fatal("should allow probe after cap-limited window") } } func TestBreakerBackoffResetsOnSuccess(t *testing.T) { - base := 20 * time.Millisecond + base := 100 * time.Millisecond b := NewBreaker(1, base) - b.MaxOpenDuration = 500 * time.Millisecond + b.MaxOpenDuration = 5 * time.Second // Build up backoff: trip → probe fail → trip → probe fail. b.RecordFailure() - time.Sleep(base + 5*time.Millisecond) + windForward(b, base) b.Allow() - b.RecordFailure() // halfOpenFails=1, next window=40 ms + b.RecordFailure() // halfOpenFails=1, next window=200 ms - time.Sleep(45 * time.Millisecond) + windForward(b, 200*time.Millisecond) b.Allow() // Probe succeeds → everything resets. @@ -148,9 +149,9 @@ func TestBreakerBackoffResetsOnSuccess(t *testing.T) { t.Fatal("should be closed after success") } - // Trip again — backoff should be back to base, not 80 ms. + // Trip again — backoff should be back to base, not 400 ms. b.RecordFailure() - time.Sleep(base + 5*time.Millisecond) + windForward(b, base) if !b.Allow() { t.Fatal("after success+retrip, should probe at base duration, not backed-off") }