feat: add MCP state-change callback and degraded cluster-aggregate state with per-instance drill-down - #5920
Conversation
oauth_configs to new mcp_oauth_flows table
#5709
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds MCP connection-state callbacks for actual transitions. It also adds the distributed ChangesMCP state observability
Distributed MCP state reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClientConnectionChecker
participant MCPManager
participant StateChangeCallback
ClientConnectionChecker->>MCPManager: Report a connection-state transition
MCPManager->>StateChangeCallback: Send client identifiers and old/new states
sequenceDiagram
participant HTTPHandler
participant MCPClusterStateAggregator
participant MCPRegistryUI
HTTPHandler->>MCPClusterStateAggregator: AggregateMCPClientState(clientID, localState)
MCPClusterStateAggregator-->>HTTPHandler: Return aggregate state and node states
HTTPHandler-->>MCPRegistryUI: Return MCPClientResponse
MCPRegistryUI->>MCPRegistryUI: Render degraded state popover
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/clientmanager.go`:
- Around line 2185-2201: Preserve callback delivery order by routing committed
state-transition events through one bounded, lifecycle-safe ordered dispatcher
instead of invoking callbacks directly after unlock. Update the
failure-transition path around clientmanager.go:2185-2201 to enqueue its event
in commit order, and update the checker-transition path around
connectionchecker.go:377-386 to use the same dispatcher; both files require
changes. Keep shared state race-safe and add a concurrent test covering Healthy
→ Unstable → NeedsReauth that verifies observer order.
🪄 Autofix
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: be253bc7-2f8c-4a37-a7f2-7b143dcc0696
📒 Files selected for processing (12)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/clientmanager_test.gocore/mcp/connectionchecker.gocore/mcp/connectionchecker_test.gocore/mcp/interface.gocore/mcp/mcp.gocore/schemas/mcp.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/types/mcp.ts
569aa6e to
c80bf09
Compare
ad8cffb to
b4ccbdd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
c80bf09 to
a096a8a
Compare
b4ccbdd to
2c5dab2
Compare
Merge activity
|
The base branch was changed.
a096a8a to
f951789
Compare

Summary
Adds a
stateChangeCallbackhook toMCPManagerthat fires on every reactive (non-admin-driven) MCP client connection-state transition, and introduces adegradedaggregate state for distributed deployments where multiple instances report conflicting states for the same client.Changes
stateChangeCallbackfield onMCPManageris invoked whenever a client's state changes throughClientConnectionChecker.setState(periodic checker transitions) orfailConnectAttempt(reactive connect-failure classification intoNeedsReauth). Deliberately excluded from admin-driven transitions (CloseAndMarkNeedsReauth,DisableClient/EnableClient,UpdateClient) since those already have their own call sites. The callback is always fired outside the manager lock to avoid holdingm.muacross arbitrary caller-supplied work.SetMCPStateChangeCallbackonBifrost: Exposes the callback registration through the top-levelBifrosttype, no-op when MCP is not configured.MCPConnectionStateDegraded: A new read-time aggregateMCPConnectionStatevalue ("degraded") that is never a node's own local state — it signals that instances in a distributed deployment currently disagree about a client's state. Single-instance deployments never produce it.MCPClusterStateAggregatorinterface: An optional interface the configuredMCPManagermay implement. When present,getMCPClientsPaginatedcallsAggregateMCPClientStateand, if the result isDegraded, replaces the response state and attaches anode_statesper-instance breakdown map.MCPClientResponse.NodeStates: New optional field (node_states) on the HTTP response carrying the per-instance state breakdown whenstateis"degraded".StateBadgecomponent: Replaces the inline<Badge>in the MCP clients table with aStateBadgethat renders a plain badge for all states except"degraded", which additionally gets a hoverPopoverdrill-down.summarizeNodeStatesgroups instance IDs by their reported state so the drill-down reads as counts ("2 instances: Healthy, 1 instance: Unstable") rather than a raw ID list.degradedcolor token: Addeddegraded: "bg-blue-100 text-blue-800"toMCP_STATUS_COLORS— a distinct blue/purple rather than amber/red to signal "instances disagree" rather than "one instance is unhealthy."TestFailConnectAttempt_NeedsReauthTransition_FiresStateChangeCallback,TestFailConnectAttempt_NoStateChange_DoesNotFireCallback, andTestSetState_FiresStateChangeCallbackOnGenuineTransitioncovering genuine transitions firing the callback exactly once and no-op transitions not firing it.Type of change
Affected areas
How to test
To exercise the
degradedstate manually, implementMCPClusterStateAggregatoron a customMCPManagerthat returnsMCPConnectionStateDegradedwith a populatednodeStatesmap for a given client, register it, and callGET /api/mcp/clients. The response for that client should include"state": "degraded"and a populatednode_statesobject. In the UI, hovering the "Degraded" badge should show the per-instance breakdown grouped by state.Screenshots/Recordings
The "Degraded" badge renders in blue/purple and, on hover, shows a popover listing instance counts per state (e.g. "2 instances: Healthy, 1 instance: Unstable").
Breaking changes
Related issues
Security considerations
The
stateChangeCallbackis caller-supplied and may perform arbitrary work including I/O. It is always invoked outsidem.muto prevent lock contention or deadlocks. No secrets or PII are passed through the callback — only client ID, name, and state enum values.Checklist
docs/contributing/README.mdand followed the guidelines