fix: close downstream MCP clients on session teardown to stop session leak by matuszeg · Pull Request #325 · obot-platform/nanobot

matuszeg · 2026-06-08T00:51:49Z

Problem

When an mcp.Session is torn down — idle eviction in the session manager (Manager.Release evicts after 10s and calls Session.Close(false)), or a config refresh (Data.Refresh) — the downstream MCP client factories stored in the session's attributes are never closed.

Two gaps combine:

clientFactory had no Close method, so the value.(interface{ Close(bool) }) assertion in Data.Refresh silently no-op'd.
Session.Close ignored attributes entirely.

As a result every torn-down session orphans its downstream clients: the upstream MCP session is never terminated (no DELETE /mcp is sent) and the client's SSE reader goroutine leaks. On the next request the session is rebuilt and a brand-new upstream session is initialized, so a downstream MCP server that nanobot polls on a fixed cadence accumulates one leaked session per cycle.

Evidence (production)

Against a downstream MCP server connected to nanobot v0.0.83, captured from a live container:

A full initialize producing a new session id every 60.000s (0b74…→22e2…→90f4…→52c8…), with no client-close in between.
13,047 GET /mcp opens vs 218 DELETE /mcp closes over 9 days (~1,470 leaked sessions/day).
~3.6 GB/day RSS growth on the downstream server from the accumulated per-session server instances; corresponding unbounded growth on the nanobot side.

Fix

Session.Close now closes any attribute implementing Close(bool), so session teardown reaps the resources its attributes own. Closers are snapshotted under the lock and closed outside it to avoid deadlock/re-entrancy.
clientFactory implements Close, terminating its live client with deleteSession=true so the upstream session receives a DELETE and the reader goroutine stops. The replacement path in get() (env-hash change) is likewise switched from Close(false) → Close(true).

A factory is only closed when its client is being permanently discarded (teardown / refresh / env-hash change), so the upstream session will not be resumed — it is always deleted.

Note on session resume

The persisted-state machinery (Serialize/Deserialize/SessionState) appears intended to let a reconstructed session resume the upstream session id rather than re-initialize. In practice it does not (the 13k distinct upstream sessions show a fresh initialize every cycle), which is the leak. This change makes teardown delete the upstream session unconditionally; since re-init already happens every cycle it regresses nothing observable, and it stays compatible with a future proper resume fix (which would reuse the live client and never reach Close). Happy to adjust if maintainers prefer fixing resume instead.

Tests

pkg/mcp — TestSessionCloseClosesAttributeClosers: Session.Close closes closable attributes (and passes the deleteSession flag through) while leaving non-closers untouched.
pkg/tools — TestClientFactoryCloseDeletesUpstreamSession: a real mcp.Client against an in-process MCP server, stored in a clientFactory; closing the factory sends exactly one upstream DELETE. Plus a compile-time assertion that *clientFactory satisfies Close(bool).

Both were written test-first (red → green).

Validation

Full make validate equivalent on go1.26: gofmt -l . clean · go vet ./... clean · go test ./... all pass · go mod tidy clean (no dependency changes).

… leak When an mcp.Session is torn down (idle eviction in the session manager, or a config refresh) the downstream MCP client factories stored in its attributes were never closed. clientFactory had no Close method, so the closer type-assertion in Data.Refresh silently no-op'd and Session.Close ignored attributes entirely. As a result every torn-down session orphaned its downstream clients: the upstream MCP session was never terminated (no DELETE /mcp was sent) and the client's SSE reader goroutine leaked. On the next request the session is rebuilt and a brand-new upstream session is initialized, so a downstream server polled on a fixed cadence accumulates one leaked session per cycle (observed against a downstream server: ~1 new session/minute, 13,047 opens vs 218 closes over 9 days, ~3.6 GB/day RSS growth there). Fix: - Session.Close now closes any attribute implementing Close(bool). - clientFactory implements Close, terminating its live client with deleteSession=true so the upstream session is freed. A factory is only closed when its client is permanently discarded (teardown / refresh / env-hash change), so the upstream session will not be resumed and is always deleted. Adds tests covering Session.Close attribute reaping and that closing a clientFactory sends an upstream DELETE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds explicit cleanup for downstream MCP client factories and session attributes to prevent leaked upstream MCP sessions and goroutines.

Changes:

Ensure clientFactory teardown always deletes the upstream MCP session.
Extend mcp.Session.Close() to close attribute values that implement Close(bool).
Add regression tests covering session attribute closers and upstream DELETE behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
pkg/tools/service.go	Ensures replaced/closed clients delete upstream sessions; adds `clientFactory.Close`.
pkg/tools/service_leak_test.go	Introduces an httptest MCP server and a regression test for upstream DELETE on factory close.
pkg/mcp/session.go	Updates session close logic to also close closable attribute values.
pkg/mcp/session_close_test.go	Adds a unit test verifying attribute closers are invoked with the correct flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

matuszeg · 2026-06-08T02:11:26Z

+			body, _ := io.ReadAll(r.Body)
+			var msg struct {
+				ID     json.RawMessage `json:"id"`
+				Method string          `json:"method"`
+			}
+			_ = json.Unmarshal(body, &msg)
+			if msg.Method == "initialize" {
+				w.Header().Set("Mcp-Session-Id", "test-session-1")
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":` + string(msg.ID) +
+					`,"result":{"protocolVersion":"2025-06-18","capabilities":{},` +
+					`"serverInfo":{"name":"fake","version":"1"}}}`))


Fixed in 4b00f09: the fake server now defaults the JSON-RPC id to null when absent and returns 400 on a malformed body, so the initialize response can never be invalid JSON.

matuszeg · 2026-06-08T02:11:28Z

+	f.Close(false)
+
+	if got := atomic.LoadInt32(&deletes); got != 1 {
+		t.Fatalf("expected exactly one upstream DELETE after factory close, got %d", got)
+	}


Fixed in 4b00f09: the assertion now polls (2s timeout) until the DELETE is observed, then settles 50ms to confirm exactly one is sent. HTTPClient.Close(true) sends the DELETE synchronously today, but this keeps the test correct if teardown ever becomes async.

matuszeg · 2026-06-08T02:11:47Z

+// Close releases the live client, sending a DELETE to the upstream MCP server
+// so its session is freed and its reader goroutine stops. A factory is only
+// closed when its client is being permanently discarded (session teardown or
+// config refresh); the upstream session will not be resumed, so it is always
+// deleted regardless of the caller's deleteSession hint.
+func (c *clientFactory) Close(bool) {
+	c.clientLock.Lock()
+	defer c.clientLock.Unlock()
+	if c.client != nil {
+		c.client.Close(true)
+		c.client = nil
+	}
+}


Good eye. In current usage the factory is always stored as *clientFactory — GetClient does session.Set(key, &factory) and clientFactory.Deserialize returns &clientFactory{...}, which is what Session.Get writes back into the attribute. So the v.(interface{ Close(bool) }) scan in Session.Close reliably detects it, and the compile-time assertion var _ interface{ Close(bool) } = (*clientFactory)(nil) guards against a value-typed regression.

There is a deeper, pre-existing wrinkle nearby: Session.Get copies the factory by value (required by the Deserializable round-trip), so get() mutations after an env-hash change update a copy rather than the stored object. Reworking that touches the Deserializable machinery broadly, so I kept it out of this focused leak fix — happy to follow up separately if you'd like.

- Always emit a valid JSON-RPC id from the fake MCP server (default to null when absent) and fail fast on a malformed request body, so the initialize response can never be invalid JSON. - Poll (with a short timeout) for the upstream DELETE instead of asserting it synchronously, keeping the test correct if client teardown ever becomes asynchronous, and settle briefly to confirm exactly one DELETE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings June 8, 2026 00:51

Copilot AI reviewed Jun 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: close downstream MCP clients on session teardown to stop session leak#325

fix: close downstream MCP clients on session teardown to stop session leak#325
matuszeg wants to merge 2 commits into
obot-platform:mainfrom
matuszeg:fix/downstream-mcp-client-session-leak

matuszeg commented Jun 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

matuszeg Jun 8, 2026

Uh oh!

matuszeg Jun 8, 2026

Uh oh!

matuszeg Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

matuszeg commented Jun 8, 2026

Problem

Evidence (production)

Fix

Note on session resume

Tests

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

matuszeg Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

matuszeg Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

matuszeg Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants