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)) + }) + } +}