Skip to content

fix: clean idle mcp stream proxy sessions#461

Merged
akitaSummer merged 1 commit into
masterfrom
fix/mcp-stream-idle-cleanup
Jun 26, 2026
Merged

fix: clean idle mcp stream proxy sessions#461
akitaSummer merged 1 commit into
masterfrom
fix/mcp-stream-idle-cleanup

Conversation

@akitaSummer

@akitaSummer akitaSummer commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup for long-lived stream sessions so idle connections are handled more reliably.
    • Fixed cases where stream-related cleanup could use the wrong server configuration.
    • Made proxy handling more stable for streaming and event-based requests, with clearer errors when a session or transport is unavailable.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

MCP stream cleanup and proxy refactor

Layer / File(s) Summary
Stream session name tracking and cleanup timing
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
MCPControllerRegister stores stream server names, reuses them when scheduling idle cleanup, clears the cache on session reset, and deletes entries when a stream session is removed.
Cached SSE and STREAM proxy handlers
plugin/mcp-proxy/index.ts
MCPProxyHook now installs cached SSE and STREAM handlers, moves SSE transport validation and JSON-RPC error shaping into getSseProxyHandler, and uses waitResponseClosed(ctx.res) while rescheduling stream cleanup around request handling.
Idle-cleanup coverage
plugin/controller/test/mcp/mcp.test.ts
Adds a stream test that keeps the client connection open, waits for the idle timeout, and asserts that the transport mapping and cleanup timer entry are removed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • eggjs/tegg#459: Directly changes the same MCP stream idle-session cleanup path and scheduleStreamMcpServerCleanup behavior in MCPControllerRegister.
  • eggjs/tegg#448: Updates the same stream lifecycle and response-close handling across MCPControllerRegister and plugin/mcp-proxy/index.ts.
  • eggjs/tegg#357: Also modifies stream-session tracking and cleanup timing across the controller and proxy paths.

Suggested reviewers

  • jerryliang64

Poem

A bunny hopped through timers bright,
And tucked each stream-name out of sight.
When idle winds began to blow,
The proxy kept its cleanup flow.
🐰✨ The burrow hums in tidy light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing cleanup of idle MCP stream proxy sessions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mcp-stream-idle-cleanup

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

plugin/controller/lib/impl/mcp/MCPControllerRegister.ts

ESLint 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.ts

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.

plugin/mcp-proxy/index.ts

ESLint 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread plugin/mcp-proxy/index.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d68d6ad and 2d79110.

📒 Files selected for processing (3)
  • plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
  • plugin/controller/test/mcp/mcp.test.ts
  • plugin/mcp-proxy/index.ts

Comment thread plugin/mcp-proxy/index.ts
Comment thread plugin/mcp-proxy/index.ts
@akitaSummer akitaSummer merged commit 717a41a into master Jun 26, 2026
12 checks passed
@akitaSummer akitaSummer deleted the fix/mcp-stream-idle-cleanup branch June 26, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant