[code-review] MCP Client Abort() data race: reads shared fields without c.mu when called from Close()

[topcheer]

Bug Description

Abort() in internal/mcp/client.go (lines 375-401) reads c.wsConn, c.stdin, c.cmd, c.procCancel without holding c.mu. The comment acknowledges this is intentional to avoid deadlock when called from sendRequest's cancel path (which already holds c.mu), but Abort() is also called from Close() (line 328) which does not hold c.mu.

Meanwhile, Start() (lines 129, 165-168) writes these same fields without holding c.mu. If Close() and Start() are called concurrently on the same Client instance, Abort() reads these fields without synchronization while Start() writes them — a data race.

Trigger Scenario

1. Goroutine A calls Client.Close() → Abort() reads fields without c.mu
2. Goroutine B calls Client.Start() → writes fields without c.mu
3. Data race — go test -race would flag it

Severity

Medium — requires concurrent Close()+Start() on the same Client.

Fix Suggestion

In Abort(), for the Close() call path, acquire c.mu before reading the fields. For the sendRequest cancel path (already holds c.mu), skip the lock.

File

internal/mcp/client.go, lines 375-401, 325-343, 113-168

[topcheer]

Fixed in commit 79cda6b. See commit message for details.