fix: mcp sse connect clear#440
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors MCP Server-Sent Events (SSE) connection lifecycle management by making ping configuration optional, introducing centralized SSE cleanup logic, implementing error-based connection termination alongside duration-based termination, and adding test coverage for session cleanup after transport closure. Changes
Sequence DiagramsequenceDiagram
participant SSEClient as SSE Client
participant Register as MCPControllerRegister
participant Server as MCP Server
participant Cleanup as Cleanup Handler
SSEClient->>Register: Connect via SSE
Register->>Register: Initialize session & ping interval
loop Ping with Error Tracking
Register->>Server: Send ping request
alt Ping succeeds
Server-->>Register: Pong response
Register->>Register: Reset errCount = 0
else Ping fails
Server--xRegister: Error or timeout
Register->>Register: Increment errCount
Note over Register: Log failure with errCount
end
alt Cleanup triggered
alt errCount > 10 OR elapsed > timeout
Register->>Cleanup: Call clearSseMcpServer()
Cleanup->>Cleanup: Remove from transports
Cleanup->>Cleanup: Remove from mcpServerMap
Cleanup->>Cleanup: Clear pingIntervals
Cleanup->>Cleanup: Clear sseTransportsRequestMap
Cleanup->>Cleanup: Clear keepalive interval
end
end
end
SSEClient->>Register: Close transport
Register->>Cleanup: Call clearSseMcpServer()
Cleanup->>Cleanup: Remove all session state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/controller/lib/impl/mcp/MCPConfig.ts (1)
30-46:⚠️ Potential issue | 🟡 MinorSilent behavior change:
pingElapseddefault removed.Previously
getPingElapsed()defaulted to10 * 60 * 1000(10 minutes). With this change, whenpingElapsedis unset the method returnsundefined, and inMCPControllerRegister.mcpServerPingthe elapsed-based cleanup (duration && elapsed >= duration) is skipped entirely. Existing users who enabledssePingEnabled/streamPingEnabledand relied on the 10-minute session rotation will silently lose it — only the newerrCount > 10path will terminate sessions.Consider either (a) keeping a sane default (e.g. 10 minutes) for backward compatibility, or (b) documenting this as a breaking behavior change in the PR description / CHANGELOG.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/lib/impl/mcp/MCPConfig.ts` around lines 30 - 46, The change removed the previous default for pingElapsed causing getPingElapsed() to return undefined; restore the prior default by setting this._pingElapsed = options.pingElapsed ?? 10 * 60 * 1000 in the MCPConfig constructor (or ensure getPingElapsed() returns 10*60*1000 when _pingElapsed is undefined); update the MCPConfig class reference to _pingElapsed and the getPingElapsed method so elapsed-based cleanup in MCPControllerRegister.mcpServerPing continues to work as before.
🧹 Nitpick comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
658-676:errCountnever resets on ping success — clarify semantics.
errCountaccumulates over the entire session lifetime; a single recovered failure contributes forever. If the intent is "terminate after 10 consecutive ping failures" (typical health‑check semantics), reset on success:♻️ Suggested reset on success
try { await server.ping(); + errCount = 0; } catch (e) { errCount++; this.app.logger.warn('mcp server ping failed: %s, errCount: %s', e, errCount);If the current "accumulated over session lifetime" behavior is intentional, a short comment documenting that would help the next maintainer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts` around lines 658 - 676, The errCount variable in the ping loop (used with timerId and server.ping()) never resets on successful pings, so it currently counts failures across the whole session rather than consecutive failures; change the logic to reset errCount = 0 inside the try block after a successful await server.ping() (so termination happens after 10 consecutive failures), and keep the existing increment in the catch; if you intentionally want cumulative failures instead, add a short inline comment near errCount explaining that behavior and why it’s preferred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts`:
- Line 436: In MCPControllerRegister.ts update the log call in the SSE init path
(the self.app.logger.warn call that logs sessionId and
`${ctx.request.socket.remoteAddress}:${ctx.request.socket.remotePort}`) to fix
the typo in the format key from "removeIp" to "remoteIp"; while editing,
consider changing the log level from warn to info since SSE init is a normal
lifecycle event (so replace self.app.logger.warn with self.app.logger.info if
you agree).
- Around line 519-523: The onclose wrapper calls
this.clearSseMcpServer(transport) which is declared async, so any rejection is
currently swallowed; either make the call safe by attaching a rejection handler
(e.g., this.clearSseMcpServer(transport).catch(err => processLogger?.error(...)
) ) inside the transport.onclose wrapper, or if clearSseMcpServer truly has no
async work, remove the async keyword from clearSseMcpServer (the method declared
at/around line with name clearSseMcpServer) so it becomes synchronous and can be
called directly from the transport.onclose wrapper without creating a Promise.
- Around line 668-674: The ping-cleanup branch is using the wrong
container/type: it checks this.sseConnections[sessionId] (sseConnections is a
Map, so that indexing is always undefined) and then passes the wrong object into
clearSseMcpServer. Replace the check to look up the transport from
this.transports using sessionId (this.transports[sessionId]) so you get an
InnerSSEServerTransport, then call clearSseMcpServer(transport) when found;
otherwise log the existing warning. Keep the surrounding condition (if
((duration && elapsed >= duration) || errCount > 10)) and ensure you reference
sessionId, errCount, duration, elapsed, clearSseMcpServer, this.transports and
remove the incorrect this.sseConnections[...] indexing.
In `@plugin/controller/test/mcp/mcp.test.ts`:
- Around line 1361-1371: The test is brittle because it picks the last session
id from MCPControllerRegister.instance.transports; instead record the current
session id set before calling sseClient.connect(sseTransport) (e.g., const
beforeIds = Object.keys(MCPControllerRegister.instance!.transports)), perform
the connect, then read afterIds and compute the new session id as the set
difference (afterIds - beforeIds) and use that id for all subsequent assertions
against MCPControllerRegister.instance, or alternatively assert beforeIds.length
=== 0 (or afterIds.length === 1) if you expect isolation; update the test to use
MCPControllerRegister.instance, sseClient.connect, and sseTransport names to
locate and replace the brittle session selection logic.
---
Outside diff comments:
In `@plugin/controller/lib/impl/mcp/MCPConfig.ts`:
- Around line 30-46: The change removed the previous default for pingElapsed
causing getPingElapsed() to return undefined; restore the prior default by
setting this._pingElapsed = options.pingElapsed ?? 10 * 60 * 1000 in the
MCPConfig constructor (or ensure getPingElapsed() returns 10*60*1000 when
_pingElapsed is undefined); update the MCPConfig class reference to _pingElapsed
and the getPingElapsed method so elapsed-based cleanup in
MCPControllerRegister.mcpServerPing continues to work as before.
---
Nitpick comments:
In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts`:
- Around line 658-676: The errCount variable in the ping loop (used with timerId
and server.ping()) never resets on successful pings, so it currently counts
failures across the whole session rather than consecutive failures; change the
logic to reset errCount = 0 inside the try block after a successful await
server.ping() (so termination happens after 10 consecutive failures), and keep
the existing increment in the catch; if you intentionally want cumulative
failures instead, add a short inline comment near errCount explaining that
behavior and why it’s preferred.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78860add-319f-4643-830d-d425c6642d91
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPConfig.tsplugin/controller/lib/impl/mcp/MCPControllerRegister.tsplugin/controller/test/mcp/mcp.test.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors MCP configuration and session management by centralizing SSE cleanup logic and implementing error-based termination for ping intervals. Feedback highlights critical bugs in the ping cleanup implementation, such as incorrect Map access and type mismatches, along with a typo in a log message and a suggestion to make the cleanup method synchronous.
2 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Bug Fixes
Tests