fix: clean idle mcp stream proxy sessions#461
Conversation
📝 WalkthroughWalkthroughThe MCP stream path now stores per-session server names for idle-timeout lookup, refactors SSE and STREAM proxy handling into cached handler factories, and adds a test that checks idle cleanup while the client stream remains open. ChangesMCP stream cleanup and proxy refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 ESLint
plugin/controller/lib/impl/mcp/MCPControllerRegister.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. plugin/controller/test/mcp/mcp.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. plugin/mcp-proxy/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
There was a problem hiding this comment.
Code Review
This pull request improves the idle session cleanup mechanism for streamable MCP sessions by tracking stream server names and scheduling cleanup during proxy handling. It also refactors the SSE and Stream proxy handlers in plugin/mcp-proxy/index.ts to use cached handlers via WeakMap and adds a test case for idle session cleanup. Feedback on the changes highlights a logical bug in the SSE proxy handler where a missing transport incorrectly triggers a 400 error instead of a 404 error, making the 404 block unreachable. It is recommended to check for the existence of the transport before verifying its type.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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 `@plugin/mcp-proxy/index.ts`:
- Around line 177-186: The proxy cleanup path in MCP proxy loses the
server-specific idle timeout because scheduleStreamMcpServerCleanup(sessionId)
is invoked without a server name, so the proxy worker cannot resolve
this.streamServerNames[sessionId] and falls back to the global timeout. Update
the request flow in mcp-proxy/index.ts to propagate the server name alongside
sessionId (or store it in shared state) and pass it into
scheduleStreamMcpServerCleanup so the per-server streamSessionIdleTimeout is
preserved in both the request handling and finally cleanup paths.
- Around line 101-126: The error handling in the MCP proxy request path is using
Koa response state even though the server is created with native
http.createServer, so `ctx.body` is never sent and failed `handlePostMessage`
calls can leave the connection hanging. Update the catch path in
`plugin/mcp-proxy/index.ts` around `handlePostMessage` to write the JSON-RPC
error directly to `res` (matching the existing validation branches) and avoid
relying on `ctx.body`. Also remove the unreachable “No transport found for
sessionId” branch after the transport lookup/assignment logic, since the earlier
`SSEServerTransport` checks already guarantee that case cannot occur.
🪄 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: f1ad828d-c999-4e0b-9b29-069af6a087bf
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.tsplugin/controller/test/mcp/mcp.test.tsplugin/mcp-proxy/index.ts
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit