fix(upstream): stop leaking a health-check goroutine on every reconnect

[matuszeg]

Pull Request

Description

startBackgroundMonitoring() spawns a backgroundHealthCheck goroutine unconditionally, so a managed client can end up with many concurrent health-check tickers.

The path that leaks:

1. startBackgroundMonitoring() is reached from Connect() (client.go:321).
2. tryReconnect() tears down only the core client — mc.coreClient.Disconnect() — then calls mc.Connect(ctx) again.
3. stopBackgroundMonitoring() is only ever called from the managed client's Disconnect() (client.go:353), which that path never reaches.

So every reconnect adds another 30-second ticker and none of the earlier ones ever stop. They accumulate for the life of the process and reset only on restart.

Because each ticker probes liveness via ListTools(), the cost scales with the upstream's tool-catalog size. On the install where I found this, one streamable-http server with a large catalog was being probed by ~16 concurrent tickers — 32.5 calls/min against a configured 2/min, at roughly 80 kB per response. Downstream, the MCP audit log on the server being probed had grown to 72 GB across 911k tools/list rows, and its daily row count climbed steadily between restarts (10k/day → 65k/day over ten days, then back to near-zero after a restart) — the signature of the accumulation rather than a fixed-rate load.

The fix

Make the start idempotent:

func (mc *Client) startBackgroundMonitoring() {
	if mc.monitoringStarted {
		return
	}
	...

No extra locking is needed: both call sites already hold mc.mu (Connect() takes it at client.go:317, Disconnect() defers it), and stopBackgroundMonitoring() already resets monitoringStarted and recreates the stop channel, so a later reconnect still gets a fresh monitor.

I want to flag the alternative I rejected, in case it looks more natural to you: having tryReconnect() call the managed Disconnect() instead of mc.coreClient.Disconnect(). That deadlocks on mc.mu, and it would discard the retry/backoff state that the adjacent ResetForReconnect() call deliberately preserves.

Effect after deploying this

Measured on the affected install, same server, before and after the rebuild:

before:  65 calls / 2 min   (~16 tickers)
after:    4 calls / 2 min   (1 ticker, matching the 30s interval)

Testing

• I have tested these changes locally
• I have added/updated tests that prove my fix is effective or my feature works
• All existing tests pass

internal/upstream/managed/monitoring_leak_test.go calls startBackgroundMonitoring() three times and asserts the goroutine delta. It fails on main (expected: 1, actual: 3) and passes with the guard, then asserts stopBackgroundMonitoring() returns the count to baseline.

What I ran locally:

• go test -race ./internal/upstream/... — pass (this is the changed package and its neighbours)
• go test ./internal/... — every package that completed passes (46 of them). internal/server, internal/tray and internal/tui did not finish inside my sandbox's time limit, so I'm relying on CI for those; nothing in this change touches them.
• go build ./..., go vet ./internal/upstream/managed/, gofmt -l — all clean

I did not run scripts/run-all-tests.sh end-to-end (the E2E portion needs a built binary driving live servers, which my environment couldn't host).

🤖 Generated with Claude Code

https://claude.ai/code/session_01GsbwvZM4fxioyop4PM7z4d

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Dumbris]

Thank you for this contribution — this is a model bug report and fix. The diagnosis of the reconnect path (tryReconnect() tearing down only the core client while stopBackgroundMonitoring() is only reachable from the managed Disconnect()) is exactly right, and the field data quantifying the impact made it easy to confirm.

I verified locally: the regression test fails on main (expected: 1, actual: 3) and passes with the guard, stable across 30 -race runs; go test -race ./internal/upstream/... and the strict CI linter are clean. Also appreciated the write-up of the rejected alternative — calling the managed Disconnect() from tryReconnect() would indeed self-join on monitoringWG.Wait() since tryReconnect runs inside the health-check goroutine.

Merging. 🙏