fix(mcp): resolve TOCTOU race condition and resource leak

[yinwm]

Summary

• Use atomic.Bool for closed flag to prevent TOCTOU race between CallTool and Close operations
• Add double-check pattern in CallTool for thread-safe closed state detection
• Use atomic Swap in Close to ensure no new calls can start after closed flag is set
• Move MCP manager cleanup defer before initialization to handle partial initialization failures
• Update tests to use atomic.Bool operations

Problem

TOCTOU Race Condition

Original code had a race window between checking closed flag and adding to WaitGroup:

CallTool:  check closed=false → release lock → [not yet Add(1)]
Close:                      acquire lock → closed=true → Wait() returns immediately!
                                                       → starts closing connections while CallTool hasn't started

Resource Leak

If LoadFromMCPConfig partially succeeded then failed, MCP connections were not cleaned up because defer mcpManager.Close() was only called on success path.

Solution

1. atomic.Bool + Swap: Close() uses Swap(true) which atomically sets closed and returns previous value
2. Double-check in CallTool: Check closed before lock (fast path) and after lock (TOCTOU prevention)
3. Defer before init: Move cleanup defer before initialization to handle all exit paths

Test plan

• make check passes
• Existing tests updated for atomic.Bool
• Race condition fix verified through code review

🤖 Generated with Claude Code

[alexhoshina]

+github.com/gdamore/tcell/v2 v2.13.8
+github.com/rivo/tview v0.42.0

These two dependencies have changed from indirect to direct dependencies, but there is no code referencing them in the PR. This could be a side effect of go mod tidy, or there may be other uncommitted local code that introduced these dependencies.

[yinwm]

it's go mod tidy effect

[nikolasdehor]

LGTM. This is a solid fix for a real TOCTOU race condition in the MCP manager.

What was broken: The original code checked m.closed (a plain bool) under a read-lock in CallTool, then released the lock, then called wg.Add(1). Meanwhile Close() could set closed=true and call wg.Wait() which would return immediately because Add(1) had not happened yet. This means Close() would start tearing down connections while a CallTool was about to execute.

What this fixes:

1. atomic.Bool with Swap(true) in Close() -- atomic set + get-previous-value in one operation, no lock needed
2. Double-check pattern in CallTool -- fast-path check before lock, TOCTOU-safe check after lock, with wg.Add(1) inside the critical section
3. defer mcpManager.Close() moved before LoadFromMCPConfig() -- handles partial initialization failures

The resource leak fix (defer before init) is an important secondary fix. If LoadFromMCPConfig connects 3 servers successfully then fails on the 4th, those 3 connections would previously leak.

Clean, correct, well-documented.