Persist plugin MCP registrations and rehydrate on Agents restart - #949
Persist plugin MCP registrations and rehydrate on Agents restart#949nickmisasi wants to merge 2 commits into
Conversation
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
📝 WalkthroughWalkthroughPlugin server registrations now persist in KV storage, restore during client-manager construction, and reconcile with enabled plugins. Administrative updates use ChangesPlugin registration persistence and updates
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp/client_manager.go (1)
468-493: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRelease
pluginServersMubefore KV I/O, but preserve mutation order.
mutatePersistedPluginRegistrationsperforms KV reads, compare-and-set writes, and retries with delays. HoldingpluginServersMublocks registry readers during this work. Release it before persistence, but serialize each registration mutation from the in-memory update through the KV update. A separate KV lock alone can persist concurrentRegisterPluginServerandUnregisterPluginServercalls out of order. Apply the same rule to the prune write inhydratePluginRegistrations.🤖 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 `@mcp/client_manager.go` around lines 468 - 493, Update RegisterPluginServer, UnregisterPluginServer, and UpdatePluginServer so pluginServersMu is released immediately after the in-memory mutation, before mutatePersistedPluginRegistrations performs KV I/O. Add or reuse a separate serialization mechanism that covers each registration’s in-memory update through its persistence mutation, preserving call order for concurrent register/unregister operations. Apply the same locking and ordering rule to the prune persistence write in hydratePluginRegistrations.
🧹 Nitpick comments (1)
mcp/client_manager.go (1)
568-585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable nil-map guard.
Line 570 creates a non-nil map.
json.Unmarshalof a JSONnullinto a pointer to a non-nil map leaves the map unchanged, soregistrationscannot be nil at line 576.♻️ Proposed simplification
if len(oldValue) > 0 { if err := json.Unmarshal(oldValue, ®istrations); err != nil { return nil, fmt.Errorf("unmarshal plugin MCP registrations: %w", err) } } - if registrations == nil { - registrations = make(map[string]PluginServerConfig) - } update(registrations)🤖 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 `@mcp/client_manager.go` around lines 568 - 585, Remove the redundant nil-map guard in mutatePersistedPluginRegistrations, including the second make(map[string]PluginServerConfig) assignment, while preserving the initial map creation and all existing update and persistence behavior.
🤖 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.
Outside diff comments:
In `@mcp/client_manager.go`:
- Around line 468-493: Update RegisterPluginServer, UnregisterPluginServer, and
UpdatePluginServer so pluginServersMu is released immediately after the
in-memory mutation, before mutatePersistedPluginRegistrations performs KV I/O.
Add or reuse a separate serialization mechanism that covers each registration’s
in-memory update through its persistence mutation, preserving call order for
concurrent register/unregister operations. Apply the same locking and ordering
rule to the prune persistence write in hydratePluginRegistrations.
---
Nitpick comments:
In `@mcp/client_manager.go`:
- Around line 568-585: Remove the redundant nil-map guard in
mutatePersistedPluginRegistrations, including the second
make(map[string]PluginServerConfig) assignment, while preserving the initial map
creation and all existing update and persistence behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b6ef135a-a0b8-4dd4-99ba-7a9f2ef539b6
📒 Files selected for processing (7)
api/api.goapi/api_admin.goapi/api_admin_test.goapi/api_test.goexternal/pluginmcp/README.mdmcp/client_manager.gomcp/client_manager_test.go
Co-authored-by: nick.misasi <nick.misasi@mattermost.com>
2 participants
Summary
Cross-plugin MCP registrations (e.g. Playbooks' 29 tools) were stored only in-memory in
mcp.ClientManager. When the Agents plugin restarted without a server config change — plugin upgrade/redeploy or crash-supervisor restart — the registry was wiped, and source plugins never re-registered because their client-side retry loop is one-shot per their ownOnActivate. The tools stayed missing until the source plugin itself restarted. (Restarts via System Console disable/enable only self-healed by accident: thePluginStatesconfig change firesOnConfigurationChangein source plugins, and that path breaks entirely on instances where config saves fail.)This PR persists live registrations in the plugin KV store and rehydrates them on activation:
RegisterPluginServer/UnregisterPluginServermirror the registration into a single KV entry (mcp_plugin_registrations_v1) usingKV.SetAtomicWithRetries, so concurrent nodes in HA cannot lose updates. Persistence failures log and degrade; the in-memory registration always succeeds.NewClientManager, persisted registrations are restored as live (IsPluginRegistered == true) for source plugins whosePluginSettings.PluginStates[id].Enableis true. Entries for disabled/removed plugins are pruned from KV — safe because a re-enabled plugin re-registers itself via its ownOnActivate. If the server config is unavailable during hydration (RPC failure), all entries are kept and nothing is pruned (fail open).Enabled,ToolConfigs) remain authoritative in the persisted plugin config; hydration runs beforesyncPluginServersFromConfigso the existing admin merge wins, same as a live registration.UpdatePluginServer, so a directPUT /admin/mcp/plugin-servers/:pluginIDagainst a config-only orphan can no longer create a durable fake registration.external/pluginmcp/README.md.QA test steps (verified live):
Playbooks MCP(29 tools) appears inGET /plugins/mattermost-ai/admin/mcp/tools.Release Note
Summary by CodeRabbit
New Features
Bug Fixes
Documentation