From 82a465103e840a7629bc64c2d5b00738d173f21c Mon Sep 17 00:00:00 2001 From: Graham Matuszewski Date: Sat, 1 Aug 2026 11:49:17 +0000 Subject: [PATCH] fix(upstream): stop leaking a health-check goroutine on every reconnect startBackgroundMonitoring() spawned a backgroundHealthCheck goroutine unconditionally. It 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 added another 30s ticker that lived until the process exited, and none of the previous ones stopped. Each leaked ticker probes liveness with ListTools(), so the cost scales with the upstream's tool-catalog size. On an affected install one server with a large catalog was being probed by ~16 concurrent tickers -- 32.5 calls/min against a configured interval of 2/min -- pushing ~80 kB per probe. The downstream MCP audit log had accumulated 72 GB, and the daily row count grew steadily between restarts, which is the signature of the accumulation. Guard the start so it is idempotent. Safe without extra locking: both call sites already hold mc.mu, and stopBackgroundMonitoring() resets monitoringStarted and recreates the stop channel, so a later reconnect still gets a fresh monitor. Note the tempting alternative -- having tryReconnect() call the managed Disconnect() -- deadlocks on mc.mu and discards the retry/backoff state that the adjacent ResetForReconnect() deliberately preserves. Co-Authored-By: Claude Opus 5 (1M context) --- internal/upstream/managed/client.go | 13 +++- .../upstream/managed/monitoring_leak_test.go | 70 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 internal/upstream/managed/monitoring_leak_test.go 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") +}