[yeliheng-010]

Description

Closing one slow MCP server can block MCP requests to unrelated services, including services in other workspaces. Disconnect() is network I/O: the Streamable HTTP transport sends a session DELETE with a timeout of up to 5 seconds, but CloseClient, stale-client replacement, and failed connection cleanup call it while holding clientsMu.

Detach connections under the lock and disconnect outside it. For stale-client replacement, let the existing connection worker close the retired session before connecting its replacement, so callers can still cancel independently. Old cleanup cannot remove a concurrently cached replacement.

Type of Change

• 🐛 Bug fix
• 🧪 Test

Related Issue

No linked issue. #3706 already applies detach-then-disconnect to CloseAll; this covers the remaining manager teardown paths. #3610 touches the same file but addresses request-scoped headers rather than this lock contention.

Testing

Based on main@602d0c38, using Go 1.26.0 on Windows:

• Added real HTTP MCP regression tests for unrelated-service isolation during close/replacement, cancellation while retiring a session, shared replacement ownership, and concurrent reconnect preservation. They fail against the original manager and pass with the fix.
• go test -race ./internal/mcp -count=1 -timeout 180s
• go vet ./internal/mcp
• golangci-lint run --new-from-patch=<complete-change.patch> ./...
• Native backend builds succeeded for both original and fixed code.

Full-backend experiment: two separate test accounts/workspaces, authenticated MCP tools/deletion APIs, SQLite, and actual HTTP MCP sessions. Workspace A's test server deliberately delays session deletion by 4 seconds. Workspace B lists tools from both an already-cached server and a new server while deletion is pending:

Trigger in A	B cached: original → fixed	B new: original → fixed
Delete MCP service	4.0021 s → 0.0050 s	4.0032 s → 0.0195 s
Replace stale connection	4.0026 s → 0.0040 s	4.0042 s → 0.0043 s

Recovery requests also succeeded. These are controlled fault-injection measurements, not production performance claims. MCP fixture rows were seeded directly into the isolated database because of the unrelated SQLite MCP metadata migration gap tracked in #4060; the replacement case advances the fixture's updated_at. Connection/list/delete operations use the production backend and MCP transport. No LLM inference was needed for this transport isolation test.

Additional go test ./internal/agent/tools -count=1 -timeout 180s has the same five failing top-level tests with both the original manager and this patch: TestMCPSchemaDoesNotLoadExternalResources (Windows file URI), TestReadFileCombinesSourcesWithoutGrantingHostAccess (symlink privilege), and TestShellStdinPreservesLiteralDataForTheEntireCommand, TestSkillPythonPackageRecoveryInstallsWithoutPip, TestSkillPackageCommandsRefuseMissingDirectory (/bin/bash unavailable). Full-repository tests were not run.

Checklist

• git diff --check origin/main...HEAD passes
• Changed source files are formatted
• Targeted tests for the changed packages/components pass
• Diff-scoped lint passes where applicable
• Unrelated/environment-dependent check failures are documented above
• Self-reviewed the code
• Added tests covering the change
• No public API, configuration, schema, or dependency changes

Prepared with Codex assistance; findings and results above were verified locally.