fix: cleanup idle mcp stream sessions#459
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesMCP Stream Idle Session Cleanup
ctx Lifecycle Middleware Await Fix
Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamInit as mcpStreamServerInit
participant Timers as streamCleanupTimers
participant Closer as closeStreamMcpServer
participant State as streamServers / streamTransports
Client->>StreamInit: POST (new session)
StreamInit->>Timers: clearStreamCleanupTimer(sessionId)
StreamInit->>State: store server + transport on onsessioninitialized
StreamInit->>StreamInit: handle request (try/finally)
StreamInit->>Timers: scheduleStreamMcpServerCleanup(sessionId, name)
Client->>StreamInit: POST (existing sessionId)
StreamInit->>Timers: clearStreamCleanupTimer(sessionId)
StreamInit->>StreamInit: handle request (try/finally)
StreamInit->>Timers: scheduleStreamMcpServerCleanup(sessionId, name)
note over Timers: idle timeout elapses (no new requests)
Timers->>Closer: closeStreamMcpServer(sessionId)
Closer->>Closer: dedupe via streamClosePromises
Closer->>State: close server + transport, clearStreamMcpServer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/MCPConfig.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. plugin/controller/lib/impl/mcp/MCPControllerRegister.tsESLint skipped: the ESLint configuration for this file 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.
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 |
![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 introduces idle session cleanup for streamable Model Context Protocol (MCP) sessions. It adds a configurable streamSessionIdleTimeout option (defaulting to 60 seconds) to MCPConfig and implements the scheduling and execution of session cleanup within MCPControllerRegister. Additionally, a test case is added to verify the cleanup of idle sessions, and doDestory() is now properly awaited in the TEgg context lifecycle middleware. There are no review comments, so no further feedback is provided.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/controller/lib/impl/mcp/MCPConfig.ts`:
- Line 51: The `streamSessionIdleTimeout` assignment in the MCPConfig
constructor is accepting any numeric value without validation, which can cause
issues with idle cleanup behavior. Add validation logic to ensure the
`streamSessionIdleTimeout` value from options is a valid positive number before
assigning it to `this._streamSessionIdleTimeout`. If the value is invalid, fall
back to the default of 60 * 1000. Also ensure the getter method for
`streamSessionIdleTimeout` (around lines 180-188) returns the properly validated
value so downstream timer logic receives only valid timeouts.
In `@plugin/controller/lib/impl/mcp/MCPControllerRegister.ts`:
- Around line 595-597: Remove the sessionId parameter from all three error
logging statements in the MCPControllerRegister class where cleanup operations
fail. Update the logger.error calls at lines 595-597, 621-623, and 627-629 to
omit the sessionId and related formatting placeholder from the log messages,
keeping only the error message details. This prevents exposure of
request-derived session identifiers in error logs.
- Around line 160-167: The clean() method is clearing streamCleanupTimers and
resetting several state maps, but it is not clearing the pingIntervals timers.
Add code after the clearTimeout loop for streamCleanupTimers to iterate through
Object.values(this.instance.pingIntervals) and call clearInterval() on each
timer, then reset this.instance.pingIntervals to an empty object like the other
maps being reset in the clean() method. This ensures that all interval callbacks
are stopped and stale state references are eliminated when the controller is
cleaned up.
In `@plugin/controller/test/mcp/mcp.test.ts`:
- Around line 467-473: The test uses a fixed 200ms sleep which creates timing
sensitivity and potential flakiness. Replace the setTimeout promise with a
bounded polling mechanism that repeatedly checks if
register.streamTransports['custom-session-id-idle'] becomes undefined rather
than relying on a fixed delay. Additionally, the pingIntervals assertion is
vacuous because it doesn't confirm the key existed before the close operation;
either capture the initial state of
register.pingIntervals['custom-session-id-idle'] before calling
streamableClient.close() to verify cleanup actually occurred, or remove the
assertion if meaningful verification is not intended.
🪄 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: 645cc5bc-5bbb-4581-bdba-4ffd457c7b2f
📒 Files selected for processing (4)
plugin/controller/lib/impl/mcp/MCPConfig.tsplugin/controller/lib/impl/mcp/MCPControllerRegister.tsplugin/controller/test/mcp/mcp.test.tsplugin/tegg/lib/ctx_lifecycle_middleware.ts
2 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Release Notes