feat: enhance MCP client persistence and redacted field handling#1460
Merged
Conversation
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughCentralizes MCP reconnect via a new ReconnectMCPClient flow across handlers, server, and manager; moves some MCP config persistence responsibilities into HTTP handlers/config store sync; UI wiring adds an optional refetch prop; health monitor logging and precondition checks added; minor whitespace/typing tweaks elsewhere. Changes
Sequence DiagramsequenceDiagram
participant UI as UI: MCPClientsTable
participant Handler as HTTP Handler
participant Manager as MCPManager
participant Server as BifrostHTTPServer
participant Store as ConfigStore
participant Sync as MCP Synchronizer
UI->>Handler: POST /clients (create) or POST /reconnect/{id}
alt Create client (no ID)
Handler->>Handler: Generate UUID
Handler->>Manager: AddMCPClient(config)
Manager->>Manager: Register in-memory client
Handler->>Store: Persist new client config
Handler-->>UI: Return created client
else Reconnect requested
Handler->>Manager: ReconnectMCPClient(id)
Manager->>Manager: Attempt in-memory reconnect
alt Not present in-memory
Manager->>Server: Request ReconnectMCPClient(id)
Server->>Store: Fetch client config
Store-->>Server: Return config
Server->>Manager: AddMCPClient(config)
Server->>Sync: Trigger MCP synchronization
Sync->>Sync: Synchronize servers
end
Manager-->>Handler: Reconnect result
Handler-->>UI: Return status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Pratham-Mishra04
force-pushed
the
01-27-fix_mcp_gossip_fixes
branch
from
e4631a9 to
1389548
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@core/mcp/health_monitor.go`:
- Around line 68-77: The Start() and Stop() methods access manager.clientMap
without holding manager.mu and allocate resources (chm.ticker,
chm.ctx/chm.cancel, chm.isMonitoring) before validating the client exists,
leaving the monitor in an inconsistent state on early return; fix by moving the
clientMap lookup (using chm.clientID) inside a manager.mu.Lock/Unlock critical
section in both Start() and Stop() and return early if the client is missing
before creating chm.ticker, calling context.WithCancel, or setting
chm.isMonitoring; ensure Stop() similarly checks and validates the client under
manager.mu before modifying chm.ticker, chm.cancel or calling chm.cancel(), and
keep monitorLoop() launch and logger.Debug() after successful validation and
resource allocation.
- Around line 89-103: In Stop(), acquire chm.manager.mu.RLock() before reading
chm.manager.clientMap[chm.clientID] (and RUnlock after) to avoid the race seen
in performHealthCheck(); retrieve clientState if present and use
clientState.ExecutionConfig.Name for logging but fall back to chm.clientID when
absent; always perform cleanup by setting chm.isMonitoring = false and releasing
resources (stop chm.ticker if non-nil and call chm.cancel() if non-nil) even
when the client is missing, and log the missing-client error with the chosen
display name using MCPLogPrefix; apply the same read-lock and fallback-logging
fix to Start() where it reads clientMap.
- Around line 187-189: In updateClientState(), avoid reading
clientState.ExecutionConfig.Name after releasing the manager lock; capture the
client name while holding the lock (e.g., assign to a local variable like
clientName inside the locked section), then release the lock and use that
captured clientName in the logger.Info call that currently reads
clientState.ExecutionConfig.Name; this prevents a race with concurrent
EditClient() updates.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 307-317: Before calling h.mcpManager.EditMCPClient, fetch and save
the current MCP config (e.g., via h.store.ConfigStore.GetMCPClientConfig or the
mcpManager equivalent) so you can revert if needed; then call EditMCPClient with
mergedConfig, and if UpdateMCPClientConfig(ctx, id, mergedConfig) fails, call
h.mcpManager.EditMCPClient(ctx, id, previousConfig) to rollback the running core
to the prior state, log both the update error and any rollback error, and return
a SendError explaining the partial failure and rollback result.
- Around line 242-252: If CreateMCPClientConfig fails after
h.mcpManager.AddMCPClient succeeded, roll back the in-memory registration to
keep core and DB consistent by calling the corresponding removal on the manager
(e.g., h.mcpManager.RemoveMCPClient or equivalent) and surface a combined error;
update the block around h.mcpManager.AddMCPClient and
h.store.ConfigStore.CreateMCPClientConfig to attempt removal on DB error,
check/remove errors from the rollback call and include both the DB error and any
rollback error in the SendError message so operators know both failures.
🧹 Nitpick comments (2)
ui/lib/store/apis/mcpApi.ts (1)
13-13: Consider defining proper response types instead ofany.The return types for these mutations have been changed from
nulltoany, which reduces type safety. Based on the backend handlers intransports/bifrost-http/handlers/mcp.go, these endpoints return{ status: string, message: string }. Consider defining a shared response type:💡 Suggested improvement
interface MCPMutationResponse { status: string; message: string; } // Then use it: createMCPClient: builder.mutation<MCPMutationResponse, CreateMCPClientRequest>({...}) updateMCPClient: builder.mutation<MCPMutationResponse, { id: string; data: UpdateMCPClientRequest }>({...}) deleteMCPClient: builder.mutation<MCPMutationResponse, string>({...}) reconnectMCPClient: builder.mutation<MCPMutationResponse, string>({...})Also applies to: 23-23, 33-33, 42-42
ui/app/workspace/mcp-gateway/views/mcpClientsTable.tsx (1)
30-30: Type mismatch:refetchreturns a Promise but typed as() => void.The
refetchfunction from RTK Query returns a Promise, and the handlersawaitit. The current type() => voiddoesn't reflect this. Consider updating the type for accuracy:💡 Suggested fix
interface MCPClientsTableProps { mcpClients: MCPClient[]; - refetch?: () => void; + refetch?: () => Promise<unknown> | void; }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/mcp/health_monitor.go`:
- Around line 68-78: The Start and Stop methods read manager.clientMap without
locking and Start sets chm.isMonitoring, chm.ctx/chm.cancel and chm.ticker
before verifying the client exists, which can leave a live ticker/flag when
client is missing; fix by acquiring manager.mu around accesses to
manager.clientMap in both Start and Stop (use the same lock semantics the
manager expects), check existence while holding the lock, and if the client is
missing perform full cleanup before returning (stop and nil the ticker, call
cancel if set, and reset isMonitoring) so no goroutine or ticker is leaked;
ensure monitorLoop is only launched after the locked verification and that Stop
always performs cleanup even when the client entry is absent.
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/mcp.go (1)
269-300: Consider returning an error when old config lookup fails.If
GetMCPClientfails (lines 271-275), the code continues witholdConfig = nil, which causesmergeMCPClientConfigto returnupdatedConfigas-is. If the update request contains redacted header values, these would be persisted to the store, potentially corrupting sensitive data.While the comment acknowledges this is "less likely to happen on edit since client exists," consider returning an error instead of continuing, to prevent accidental data loss.
♻️ Optional: Return error on config lookup failure
// Get old config to de-redact sensitive fields (before updating) oldConfig, err := h.store.GetMCPClient(id) if err != nil { - logger.Warn("Failed to get old MCP client config for de-redaction: %v", err) - // Continue anyway, will use req as-is (less likely to happen on edit since client exists) - oldConfig = nil + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to get existing MCP client config: %v", err)) + return }
Pratham-Mishra04
force-pushed
the
01-27-fix_mcp_gossip_fixes
branch
2 times, most recently
from
949e5f9 to
cb88a64
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 405-467: mergeMCPClientConfig currently only merges Headers and
will overwrite connection_type, connection_string, and stdio_config when the UI
update payload omits them; update mergeMCPClientConfig to copy those fields from
oldConfig into merged when they are zero-valued/missing in updatedConfig, and
treat connection_string like headers by restoring the original raw value if the
updated connection_string is redacted and equals the oldRedactedConfig value
(use oldConfig.connection_string for the real value); ensure stdio_config and
connection_type are preserved from oldConfig when absent in updatedConfig so
UpdateMCPClientConfig doesn't wipe persisted connection info.
- Around line 313-319: Before deleting the MCP client config from the DB
(h.store.ConfigStore.DeleteMCPClientConfig), first fetch and save the existing
config (e.g. via h.store.ConfigStore.GetMCPClientConfig or similar) into a local
variable; after the core removal succeeds, attempt DeleteMCPClientConfig, and if
it fails call the store's create/restore method (e.g. CreateMCPClientConfig or
PutMCPClientConfig) with the saved config to roll back, then surface the
rollback result in the response/log (use SendError to include both the original
delete error and whether the restore succeeded or failed) so callers know if the
DB was restored or needs manual intervention.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2496-2521: The RemoveMCPClient function currently returns nil even
when the supplied id isn't present in c.MCPConfig.ClientConfigs; change it to
return a not-found error when the client ID doesn't exist. After iterating
c.MCPConfig.ClientConfigs in RemoveMCPClient, detect whether any entry was
removed (e.g., track a found flag while looping over c.MCPConfig.ClientConfigs),
and if not found return a clear error such as fmt.Errorf("MCP client not found:
%s", id). Keep the existing checks for c.client.GetMCPClients and
c.client.RemoveMCPClient (so you still attempt to deregister from Bifrost when
present) but ensure the final outcome reflects whether the config entry was
actually removed by referencing RemoveMCPClient, c.MCPConfig.ClientConfigs,
c.client.GetMCPClients and c.client.RemoveMCPClient.
🧹 Nitpick comments (1)
core/mcp/health_monitor.go (1)
68-85: Capture client name while holding the lock to avoid potential race.
clientState.ExecutionConfig.Nameis accessed at line 85 after the lock is released at line 71. IfEditClient()concurrently modifiesExecutionConfig, this could read stale or torn data. Since this only affects debug logging, the impact is low, but capturing the name under the lock would be consistent with the pattern needed inupdateClientState().♻️ Suggested fix
// Check client exists FIRST before allocating resources chm.manager.mu.RLock() clientState, exists := chm.manager.clientMap[chm.clientID] + var clientName string + if exists { + clientName = clientState.ExecutionConfig.Name + } chm.manager.mu.RUnlock() if !exists { // Use clientID for logging when client is missing logger.Error("%s Health monitor failed to start for client %s, client not found in manager", MCPLogPrefix, chm.clientID) return } // Now allocate resources (after validation) chm.isMonitoring = true chm.ctx, chm.cancel = context.WithCancel(context.Background()) chm.ticker = time.NewTicker(chm.interval) go chm.monitorLoop() - logger.Debug("%s Health monitor started for client %s", MCPLogPrefix, clientState.ExecutionConfig.Name) + logger.Debug("%s Health monitor started for client %s", MCPLogPrefix, clientName) }
akshaydeo
force-pushed
the
01-27-fix_mcp_gossip_fixes
branch
from
cb88a64 to
b82fff2
Compare
Contributor
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves MCP client management with better error handling, logging, and configuration persistence. This PR enhances the reliability of MCP client connections and provides a more robust user experience when managing MCP clients through the UI.
Changes
Type of change
Affected areas
How to test
Breaking changes
Related issues
Improves MCP client management reliability and user experience
Security considerations
Ensures sensitive fields in MCP client configurations are properly handled during updates to prevent accidental data loss or exposure.
Checklist