Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions internal/runtime/supervisor/supervisor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
82 changes: 82 additions & 0 deletions internal/runtime/supervisor/supervisor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
63 changes: 47 additions & 16 deletions internal/upstream/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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<<uint(exponent)) * time.Second //nolint:gosec // exponent is bounds-checked above
maxBackoff := 5 * time.Minute
if backoffDuration > 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
Expand Down Expand Up @@ -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<<uint(retryCount)) * time.Second //nolint:gosec // retryCount is bounds-checked above
maxBackoff := 5 * time.Minute
if backoffDuration > 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
Expand Down
48 changes: 48 additions & 0 deletions internal/upstream/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})
}
}