From 2b41e593a025fc4d50c97124a14a4a5b6b4b5f14 Mon Sep 17 00:00:00 2001 From: Anton Gulin Date: Thu, 20 Aug 2026 11:57:35 -0700 Subject: [PATCH] Respect client retry backoff in supervisor reconciliation The periodic 30s reconciliation planned ActionConnect for any enabled, unconnected server regardless of its failure history, bypassing the managed client's exponential backoff and MaxConnectionRetries give-up (the ConnectAll path already gates on ShouldRetry; the per-server reconcile path did not). A dead or 401-ing remote upstream was therefore re-dialed every 30 seconds forever - roughly 3 HTTP requests per tick, ~9k requests/day against a server that cannot answer. computeReconcilePlan now consults the client's ConnectionInfo and plans ActionNone while the backoff window is open, after the client gave up, and for servers parked in PendingAuth awaiting user OAuth login. The backoff formula is extracted from StateManager.ShouldRetry into RetryBackoffDuration so both paths share one schedule. Fixes #1013 --- internal/runtime/supervisor/supervisor.go | 11 +++ .../runtime/supervisor/supervisor_test.go | 82 +++++++++++++++++++ internal/upstream/types/types.go | 63 ++++++++++---- internal/upstream/types/types_test.go | 48 +++++++++++ 4 files changed, 188 insertions(+), 16 deletions(-) diff --git a/internal/runtime/supervisor/supervisor.go b/internal/runtime/supervisor/supervisor.go index 831687d4..2bd4e718 100644 --- a/internal/runtime/supervisor/supervisor.go +++ b/internal/runtime/supervisor/supervisor.go @@ -488,6 +488,17 @@ func (s *Supervisor) computeReconcilePlan(configSnapshot *configsvc.Snapshot, ac // BUT: Don't auto-reconnect if user explicitly logged out if userLoggedOut[name] { plan.Actions[name] = ActionNone + } else if actual, ok := actualStates[name]; ok && !actual.ConnectionInfo.ShouldAutoReconnect(time.Now()) { + // Respect the client's retry policy: exponential backoff after + // consecutive failures, gave-up after MaxConnectionRetries, and + // PendingAuth servers waiting on user OAuth login. Without this + // gate the periodic 30s reconciliation re-dials a dead upstream + // forever, hammering the remote server (~3 requests per tick). + s.logger.Debug("Skipping auto-reconnect (backoff/pending-auth)", + zap.String("server", name), + zap.String("state", actual.ConnectionInfo.State.String()), + zap.Int("retry_count", actual.ConnectionInfo.RetryCount)) + plan.Actions[name] = ActionNone } else { plan.Actions[name] = ActionConnect } diff --git a/internal/runtime/supervisor/supervisor_test.go b/internal/runtime/supervisor/supervisor_test.go index c395d037..ffb3a924 100644 --- a/internal/runtime/supervisor/supervisor_test.go +++ b/internal/runtime/supervisor/supervisor_test.go @@ -1109,3 +1109,85 @@ func TestSupervisor_RefreshToolsFromDiscovery_StateViewCarriesSchema(t *testing. require.Contains(t, props, "query") require.Contains(t, props, "limit") } + +// TestSupervisor_Reconcile_RespectsRetryBackoff verifies that periodic +// reconciliation does not re-dial a failed upstream while the managed client's +// exponential backoff window is open, after it gave up, or while it is parked +// in PendingAuth — but does reconnect once the backoff has elapsed. +func TestSupervisor_Reconcile_RespectsRetryBackoff(t *testing.T) { + cfg := &config.Config{ + Listen: "127.0.0.1:8080", + Servers: []*config.ServerConfig{ + {Name: "flaky-server", Enabled: true}, + }, + } + + configSvc := configsvc.NewService(cfg, "/tmp/config.json", zap.NewNop()) + defer configSvc.Close() + + mockUpstream := NewMockUpstreamAdapter() + defer mockUpstream.Close() + + supervisor := New(configSvc, mockUpstream, zap.NewNop()) + + // First reconciliation - server is added and connected + require.NoError(t, supervisor.reconcile(configSvc.Current())) + time.Sleep(50 * time.Millisecond) + + setConnectionState := func(connected bool, info *types.ConnectionInfo) { + mockUpstream.mu.Lock() + defer mockUpstream.mu.Unlock() + mockUpstream.connected["flaky-server"] = connected + if state, ok := mockUpstream.states["flaky-server"]; ok { + state.Connected = connected + state.ConnectionInfo = info + } + } + isConnected := func() bool { + mockUpstream.mu.Lock() + defer mockUpstream.mu.Unlock() + return mockUpstream.connected["flaky-server"] + } + + // Simulate a connection failure with the backoff window still open: + // reconciliation must NOT re-dial. + setConnectionState(false, &types.ConnectionInfo{ + State: types.StateError, + RetryCount: 5, + LastRetryTime: time.Now(), + }) + require.NoError(t, supervisor.reconcile(configSvc.Current())) + time.Sleep(50 * time.Millisecond) + require.False(t, isConnected(), "supervisor re-dialed a failed server inside its backoff window") + + // A server that gave up after max retries must not be re-dialed either. + setConnectionState(false, &types.ConnectionInfo{ + State: types.StateError, + RetryCount: types.MaxConnectionRetries, + GaveUp: true, + LastRetryTime: time.Now().Add(-time.Hour), + }) + require.NoError(t, supervisor.reconcile(configSvc.Current())) + time.Sleep(50 * time.Millisecond) + require.False(t, isConnected(), "supervisor re-dialed a server that gave up after max retries") + + // A server parked in PendingAuth (waiting for user OAuth login) must not be + // re-dialed - each attempt fires real requests at the upstream and cannot + // succeed until the user completes the login. + setConnectionState(false, &types.ConnectionInfo{ + State: types.StatePendingAuth, + }) + require.NoError(t, supervisor.reconcile(configSvc.Current())) + time.Sleep(50 * time.Millisecond) + require.False(t, isConnected(), "supervisor re-dialed a server pending OAuth login") + + // Once the backoff window has elapsed, reconciliation reconnects as before. + setConnectionState(false, &types.ConnectionInfo{ + State: types.StateError, + RetryCount: 3, + LastRetryTime: time.Now().Add(-10 * time.Second), // backoff for 3 failures is 4s + }) + require.NoError(t, supervisor.reconcile(configSvc.Current())) + time.Sleep(50 * time.Millisecond) + require.True(t, isConnected(), "supervisor did not reconnect after the backoff window elapsed") +} diff --git a/internal/upstream/types/types.go b/internal/upstream/types/types.go index 78706584..1ec05f3f 100644 --- a/internal/upstream/types/types.go +++ b/internal/upstream/types/types.go @@ -67,6 +67,52 @@ type ConnectionInfo struct { GaveUp bool `json:"gave_up"` // True when max retries exceeded } +// RetryBackoffDuration returns the exponential backoff to wait after the given +// number of consecutive connection failures: 1s, 2s, 4s, ... capped at 5 minutes. +func RetryBackoffDuration(retryCount int) time.Duration { + // Ensure retry count is valid and within safe range to avoid overflow + exponent := retryCount - 1 + if exponent < 0 { + exponent = 0 + } + if exponent > 30 { // Cap at 30 to prevent overflow in 64-bit systems + exponent = 30 + } + backoffDuration := time.Duration(1< maxBackoff { + backoffDuration = maxBackoff + } + return backoffDuration +} + +// ShouldAutoReconnect reports whether an automatic (supervisor-driven) reconnect +// attempt is appropriate given the connection's failure history. It returns false +// while the exponential backoff window from the last failure has not elapsed, +// after the client has given up (MaxConnectionRetries), and for servers parked in +// PendingAuth — redialing cannot succeed until the user completes the OAuth login, +// and each attempt costs real requests against the upstream. Manual reconnects, +// login flows, and reconnect-on-use are not subject to this policy. +func (ci *ConnectionInfo) ShouldAutoReconnect(now time.Time) bool { + if ci == nil { + return true + } + switch ci.State { + case StatePendingAuth: + return false + case StateError: + if ci.GaveUp || ci.RetryCount >= MaxConnectionRetries { + return false + } + if ci.RetryCount == 0 { + return true + } + return now.Sub(ci.LastRetryTime) >= RetryBackoffDuration(ci.RetryCount) + default: + return true + } +} + // StateManager manages the state transitions for an upstream connection type StateManager struct { mu sync.RWMutex @@ -253,22 +299,7 @@ func (sm *StateManager) ShouldRetry() bool { return true } - // Calculate exponential backoff - // Ensure retry count is valid and within safe range to avoid overflow - retryCount := sm.retryCount - 1 - if retryCount < 0 { - retryCount = 0 - } - if retryCount > 30 { // Cap at 30 to prevent overflow in 64-bit systems - retryCount = 30 - } - backoffDuration := time.Duration(1< maxBackoff { - backoffDuration = maxBackoff - } - - return time.Since(sm.lastRetryTime) >= backoffDuration + return time.Since(sm.lastRetryTime) >= RetryBackoffDuration(sm.retryCount) } // IsState checks if the current state matches the given state diff --git a/internal/upstream/types/types_test.go b/internal/upstream/types/types_test.go index 85464b09..ff9737b9 100644 --- a/internal/upstream/types/types_test.go +++ b/internal/upstream/types/types_test.go @@ -182,3 +182,51 @@ func TestShouldRetry_ResetAfterGaveUp(t *testing.T) { assert.True(t, sm.ShouldRetry(), "should retry after manual Reset clears gave-up state") } + +// TestRetryBackoffDuration tests the exponential backoff schedule +func TestRetryBackoffDuration(t *testing.T) { + tests := []struct { + retryCount int + expected time.Duration + }{ + {0, 1 * time.Second}, + {1, 1 * time.Second}, + {2, 2 * time.Second}, + {3, 4 * time.Second}, + {5, 16 * time.Second}, + {10, 5 * time.Minute}, // 512s capped at 5min + {100, 5 * time.Minute}, // exponent capped, then duration capped + } + + for _, tt := range tests { + got := RetryBackoffDuration(tt.retryCount) + assert.Equal(t, tt.expected, got, "retryCount=%d", tt.retryCount) + } +} + +// TestConnectionInfo_ShouldAutoReconnect tests the supervisor-facing retry policy +func TestConnectionInfo_ShouldAutoReconnect(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + info *ConnectionInfo + expected bool + }{ + {"nil info", nil, true}, + {"disconnected fresh server", &ConnectionInfo{State: StateDisconnected}, true}, + {"ready server", &ConnectionInfo{State: StateReady}, true}, + {"pending auth is parked", &ConnectionInfo{State: StatePendingAuth}, false}, + {"error within backoff window", &ConnectionInfo{State: StateError, RetryCount: 5, LastRetryTime: now.Add(-1 * time.Second)}, false}, + {"error with backoff elapsed", &ConnectionInfo{State: StateError, RetryCount: 3, LastRetryTime: now.Add(-10 * time.Second)}, true}, + {"error no failures yet", &ConnectionInfo{State: StateError, RetryCount: 0}, true}, + {"gave up flag", &ConnectionInfo{State: StateError, GaveUp: true, LastRetryTime: now.Add(-time.Hour)}, false}, + {"retry count at max", &ConnectionInfo{State: StateError, RetryCount: MaxConnectionRetries, LastRetryTime: now.Add(-time.Hour)}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.info.ShouldAutoReconnect(now)) + }) + } +}