diff --git a/internal/upstream/managed/client.go b/internal/upstream/managed/client.go index ea1e0597e..21eb9ea58 100644 --- a/internal/upstream/managed/client.go +++ b/internal/upstream/managed/client.go @@ -787,8 +787,19 @@ func (mc *Client) onStateChange(oldState, newState types.ConnectionState, info * } } -// startBackgroundMonitoring starts monitoring the connection health +// startBackgroundMonitoring starts monitoring the connection health. +// +// Idempotent: tryReconnect() re-enters Connect() after tearing down only the +// core client, so this is reached again on every reconnect while the existing +// monitor is still running (stopBackgroundMonitoring is only called from the +// managed client's Disconnect()). Without this guard each reconnect leaked +// another backgroundHealthCheck goroutine, multiplying the ListTools liveness +// probes sent to the upstream server for the rest of the process's life. func (mc *Client) startBackgroundMonitoring() { + if mc.monitoringStarted { + return + } + // Mark that monitoring has been started mc.monitoringStarted = true mc.monitoringWG.Add(1) diff --git a/internal/upstream/managed/monitoring_leak_test.go b/internal/upstream/managed/monitoring_leak_test.go new file mode 100644 index 000000000..901e0b4c0 --- /dev/null +++ b/internal/upstream/managed/monitoring_leak_test.go @@ -0,0 +1,70 @@ +package managed + +import ( + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +func newTestClientForMonitoring(t *testing.T) *Client { + t.Helper() + mc := &Client{ + logger: zap.NewNop(), + stopMonitoring: make(chan struct{}), + } + mc.SetConfig(&config.ServerConfig{Name: "monitor-server"}) + return mc +} + +// settledGoroutines polls runtime.NumGoroutine until it stops changing, so the +// assertions below compare stable counts rather than racing the scheduler. +func settledGoroutines() int { + prev := -1 + for i := 0; i < 100; i++ { + runtime.Gosched() + time.Sleep(2 * time.Millisecond) + n := runtime.NumGoroutine() + if n == prev { + return n + } + prev = n + } + return runtime.NumGoroutine() +} + +// TestStartBackgroundMonitoring_IsIdempotent pins the fix for a goroutine leak +// that made an upstream server's health check fire N times per interval. +// +// startBackgroundMonitoring is reached from Connect(), and tryReconnect() +// re-enters Connect() after tearing down only the *core* client — it never +// calls the managed Client's Disconnect(), which is the sole caller of +// stopBackgroundMonitoring(). So every reconnect used to add another +// backgroundHealthCheck goroutine that lived until the process exited. +// +// Each of those goroutines probes liveness with ListTools(), so a server whose +// tool catalog is large turns the leak into sustained traffic: an installation +// observed here had ~16 concurrent tickers for a single configured upstream, +// pushing ~80 kB per probe and accumulating 72 GB of audit rows downstream. +func TestStartBackgroundMonitoring_IsIdempotent(t *testing.T) { + mc := newTestClientForMonitoring(t) + + base := settledGoroutines() + + // Simulates Connect() → reconnect → reconnect on one managed client. + mc.startBackgroundMonitoring() + mc.startBackgroundMonitoring() + mc.startBackgroundMonitoring() + + assert.Equal(t, 1, settledGoroutines()-base, + "repeated startBackgroundMonitoring must run exactly one health-check goroutine") + + mc.stopBackgroundMonitoring() + + assert.Equal(t, base, settledGoroutines(), + "stopBackgroundMonitoring must leave no health-check goroutine behind") +}