fix(mcp): resync /mcp gateway tools/list on connect, reconnect, and tool-sync drift#5047
Open
Shaik-Sirajuddin wants to merge 4 commits into
Open
fix(mcp): resync /mcp gateway tools/list on connect, reconnect, and tool-sync drift#5047Shaik-Sirajuddin wants to merge 4 commits into
Shaik-Sirajuddin wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a retained "tools updated" callback mechanism to Bifrost and MCPManager. MCPManager gains a coalescing dispatcher and non-blocking notify path triggered from client connect and diff-gated tool sync flows. Bifrost exposes SetOnMCPToolsUpdated, re-applying it across lazy manager construction paths, and the HTTP server bootstrap wires it to resync gateway tool registries. ChangesMCP Tools-Updated Hook
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant ClientManager
participant MCPManager
participant Dispatcher as ToolsUpdatedDispatcher
participant Bifrost
participant Server as HTTP Server
Bifrost->>MCPManager: applyMCPToolsUpdatedHook()
Client->>ClientManager: connect / performSync
ClientManager->>MCPManager: notifyToolsUpdated() (on connect or diff detected)
MCPManager->>Dispatcher: signal via toolsUpdatedCh
Dispatcher->>Bifrost: invoke onMCPToolsUpdated callback
Bifrost->>Server: SetOnMCPToolsUpdated callback fires
Server->>Server: SyncAllMCPServers(context.Background())
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/fix..." | Re-trigger Greptile |
…ool-sync drift
The gateway's tools/list served a stale snapshot rebuilt only on admin config
mutations (add/update/remove/enable/disable a client). Boot-time client dialing,
manual/health-monitor reconnects, and periodic tool-sync drift all mutated a
client's live ToolMap without ever triggering a resync, so /mcp could serve
{"tools":[]} indefinitely after a clean restart until an admin toggled a client.
Adds an onToolsUpdated hook on MCPManager, coalesced through a buffered channel
so bursts of concurrent connects collapse into at most one extra resync, fired
from connectToMCPClient and performSync only on their success paths. The
transport wires this to SyncAllMCPServers in Bootstrap, and Bifrost retains the
callback so it's re-applied to any MCPManager constructed later by a lazy-init
path (AddMCPClient, VerifyPerUserOAuthConnection, VerifyHeadersConnection,
SetMCPManager) for processes that start with no MCP configured.
Fixes maximhq#4998
Shaik-Sirajuddin
force-pushed
the
fix/mcp-gateway-tools-sync
branch
from
f991995 to
3ec80c9
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/mcp_lazy_hook_verify_test.go`:
- Line 1: The Go test file name violates the repo naming convention because it
uses underscores before the _test.go suffix. Rename the test file to a
concatenated lowercase form matching the existing symbol/package context in
core/mcp_lazy_hook_verify_test.go, such as mcplazyhookverify_test.go, and ensure
any references in build or test tooling still point to the renamed file.
In `@core/mcp/mcp.go`:
- Around line 236-256: `runToolsUpdatedDispatcher` calls the registered
`onToolsUpdated` hook without any panic protection, so a panic in `fn()` can
crash the whole process. Wrap the `fn()` invocation in
`runToolsUpdatedDispatcher` with a defer/recover guard inside the same
goroutine, and log the recovered panic as a warning while keeping the dispatcher
alive. Use the existing `MCPManager` hook wiring (`onToolsUpdated`,
`onToolsUpdatedMu`, `toolsUpdatedCh`) to locate the call site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac562f0a-c699-479f-b10e-fca3b48cf25d
📒 Files selected for processing (7)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.gocore/mcp/mcp.gocore/mcp/toolsync.gocore/mcp_lazy_hook_verify_test.gotransports/bifrost-http/server/server.go
| @@ -0,0 +1,125 @@ | |||
| package bifrost | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Filename uses underscores beyond the permitted _test.go suffix.
Per repo convention, Go filenames should have no underscores except the _test.go suffix, with multi-word names concatenated (e.g. pluginpipeline_test.go, not plugin_pipeline_test.go). mcp_lazy_hook_verify_test.go should be mcplazyhookverify_test.go.
As per coding guidelines: "Go filenames use no underscores except for _test.go suffix; use concatenated lowercase words (e.g., pluginpipeline.go, not plugin_pipeline.go)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/mcp_lazy_hook_verify_test.go` at line 1, The Go test file name violates
the repo naming convention because it uses underscores before the _test.go
suffix. Rename the test file to a concatenated lowercase form matching the
existing symbol/package context in core/mcp_lazy_hook_verify_test.go, such as
mcplazyhookverify_test.go, and ensure any references in build or test tooling
still point to the renamed file.
Source: Coding guidelines
…anics Addresses two follow-up issues found after the initial fix: - performSync now only calls notifyToolsUpdated when the synced tool set actually changed (deep comparison, not just count). Notifying on every tick regardless invalidated the entire per-VK gateway-server cache on every tool-sync interval across every configured client even when nothing changed, eroding the lazy per-VK caching that exists to avoid O(virtual keys) work at scale. - runToolsUpdatedDispatcher now recovers a panic in the registered hook instead of crashing the process; since the dispatcher goroutine never restarts, an unrecovered panic would silently stop all future gateway resyncs for the rest of the process lifetime. - Renamed new test files to match repo convention (no underscores beyond the _test.go suffix).
… fix/mcp-gateway-tools-sync
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
1 participant
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
Fixes #4998. The
/mcpgateway'stools/listserved a stale, cached snapshot rebuilt only on admin config mutations (add/update/remove/enable/disable a client). Boot-time client dialing, manual/health-monitor reconnects, and periodic tool-sync drift all mutated a client's live tool state without ever triggering a resync, sotools/listcould serve{"tools":[]}indefinitely after a clean restart until an admin toggled a client.Changes
onToolsUpdatedhook onMCPManager(core/mcp/mcp.go), fired fromconnectToMCPClient(covers boot, manual reconnect, health-monitor reconnect) andperformSync(covers periodic tool-sync drift), only on their success paths.MCPManagerInterface,Bifrost, and the transport'sBootstrapto callSyncAllMCPServers.Bifrostretains the callback and re-applies it to anyMCPManagerconstructed later by a lazy-init path (AddMCPClient,VerifyPerUserOAuthConnection,VerifyHeadersConnection,SetMCPManager), covering processes that start with no MCP configured.core/mcp_lazy_hook_verify_test.gocovering the lazy-init path.