Files
goclaw/internal/agent/loop_mcp_user.go
T
Kai (Tam Nhu) TranandGitHub 86ef70906a fix(mcp): full reconnect for SSE/HTTP after server-side restart (#812)
* fix(mcp): full reconnect for SSE/HTTP after server-side restart

When an MCP server using SSE or streamable-http transport restarts
(container redeploy, crash, OOM), the old client holds a stale session
ID. tryReconnect only called Ping() on the dead client, which keeps
POSTing to /message?sessionId=<dead> — the server returns 404, and
after maxReconnectAttempts the connection is permanently dead.

For pool connections, poolHealthLoop only set connected=false without
attempting any reconnect at all.

Fix:
- Store connection params (url, headers, command, args, env) in
  serverState so tryReconnect can create fresh connections.
- tryReconnect now has two phases: fast path (ping existing client for
  transient blips) and slow path (close old client, create fresh one
  via createClient + Start + Initialize, swap ss.client).
- Add updateBridgeToolClients to propagate the new client pointer to
  all registered BridgeTools after a full reconnect.
- Add poolTryReconnect with the same two-phase pattern for
  pool-managed connections.
- Add BridgeTool.swapClient for safe client pointer replacement.

Closes #810

* fix(mcp): address review findings — atomic client pointer, close-after-swap, cooldown

Red-team review found critical issues in the initial reconnect fix:

1. Data race: BridgeTool.client written by healthLoop, read by Execute
   concurrently without synchronization. Fix: BridgeTool now holds
   *atomic.Pointer[mcpclient.Client] shared with serverState.clientPtr.
   Execute uses atomic Load(), reconnect uses atomic Store().

2. Close-before-swap: old client was closed before new one was ready.
   If createClient failed, ss.client pointed to closed client permanently.
   Fix: create and validate new client first, swap atomically, then close old.

3. Pool BridgeTool orphaning: poolTryReconnect swapped ss.client but
   existing BridgeTools held old pointer with no update mechanism.
   Fix: atomic.Pointer propagates automatically — no explicit update needed.

4. Permanent death: after maxReconnectAttempts, server was permanently
   dead with no recovery. Fix: 5-minute cooldown then reset attempts.

5. healthFailures not reset in fast path: minor inconsistency fixed.

Also fixes external caller in loop_mcp_user.go and adds ClientPtr()
accessor to poolEntry for atomic pointer sharing.

* refactor(mcp): extract fullReconnect helper, add nil guard, clarify dual-pointer design

- Extract shared fullReconnect() used by both tryReconnect and
  poolTryReconnect — eliminates 30-line duplication (-17 lines net)
- Add nil guard on clientPtr.Load() in BridgeTool.Execute to prevent
  nil deref in edge cases (test fixtures, initialization race)
- Add doc comment on serverState explaining dual-pointer design
  (client for healthLoop, clientPtr for BridgeTools)
2026-04-10 20:25:08 +07:00

107 lines
3.7 KiB
Go

package agent
import (
"context"
"log/slog"
"maps"
mcpbridge "github.com/nextlevelbuilder/goclaw/internal/mcp"
"github.com/nextlevelbuilder/goclaw/internal/tools"
)
// getUserMCPTools returns per-user MCP tools for servers requiring user credentials.
// Tools are cached per-user in mcpUserTools sync.Map and registered in the shared
// tool registry so ExecuteWithContext can resolve them. On first call for a user,
// connections are established via pool.AcquireUser() and BridgeTools created.
func (l *Loop) getUserMCPTools(ctx context.Context, userID string) []tools.Tool {
if len(l.mcpUserCredSrvs) == 0 || l.mcpPool == nil || l.mcpStore == nil || userID == "" {
return nil
}
if cached, ok := l.mcpUserTools.Load(userID); ok {
cachedTools := cached.([]tools.Tool)
// Check if any cached tool's connection was evicted by pool.
// If so, clear cache and re-acquire connections.
allConnected := true
for _, t := range cachedTools {
if bt, ok := t.(interface{ IsConnected() bool }); ok && !bt.IsConnected() {
allConnected = false
break
}
}
if allConnected {
return cachedTools
}
l.mcpUserTools.Delete(userID)
slog.Debug("mcp.user_tools_stale", "user", userID, "reason", "pool_evicted")
}
var userTools []tools.Tool
for _, info := range l.mcpUserCredSrvs {
srv := info.Server
// Check if user has credentials for this server
uc, err := l.mcpStore.GetUserCredentials(ctx, srv.ID, userID)
if err != nil || uc == nil || (uc.APIKey == "" && len(uc.Headers) == 0 && len(uc.Env) == 0) {
continue
}
// Resolve connection params: server defaults merged with user overrides
args := mcpbridge.ParseJSONBytesToStringSlice(srv.Args)
env := mcpbridge.ParseJSONBytesToStringMap(srv.Env)
if env == nil {
env = make(map[string]string)
}
headers := mcpbridge.ParseJSONBytesToStringMap(srv.Headers)
if headers == nil {
headers = make(map[string]string)
}
// Inject server-level API key into headers if present
if srv.APIKey != "" && headers["Authorization"] == "" {
headers["Authorization"] = "Bearer " + srv.APIKey
}
// Merge user credentials (user overrides server defaults)
if uc.APIKey != "" {
headers["Authorization"] = "Bearer " + uc.APIKey
}
maps.Copy(headers, uc.Headers)
maps.Copy(env, uc.Env)
// Acquire user-keyed pool connection
entry, err := l.mcpPool.AcquireUser(ctx, l.tenantID, srv.Name, userID,
srv.Transport, srv.Command, args, env, srv.URL, headers, srv.TimeoutSec)
if err != nil {
slog.Warn("mcp.user_pool_acquire_failed", "server", srv.Name, "user", userID, "error", err)
continue
}
// Release immediately — BridgeTools hold client pointer directly.
// This allows pool idle eviction to work (refCount=0 + lastUsed for TTL).
// When pool evicts the connection, BridgeTool.Execute detects connected=false.
l.mcpPool.ReleaseUser(mcpbridge.UserPoolKey(l.tenantID, srv.Name, userID))
// Create BridgeTools pointing to user's connection and register in the
// shared tool registry so ExecuteWithContext can resolve them by name.
reg, _ := l.tools.(*tools.Registry)
for _, mcpTool := range entry.MCPTools() {
bt := mcpbridge.NewBridgeTool(srv.Name, mcpTool, entry.ClientPtr(), srv.ToolPrefix, srv.TimeoutSec, entry.Connected())
// Register in registry so ExecuteWithContext can find them.
// Skip if already registered (another user loaded this server with same tool names).
if reg != nil {
if _, exists := reg.Get(bt.Name()); !exists {
reg.Register(bt)
}
}
userTools = append(userTools, bt)
}
}
if len(userTools) > 0 {
l.mcpUserTools.Store(userID, userTools)
slog.Info("mcp.user_tools_loaded", "user", userID, "tools", len(userTools))
}
return userTools
}