fix(mcp): ensure MCP transport is closed to prevent memory leaks#18054
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @cbcoutinho, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant memory leak within the MCP client by ensuring that underlying communication transports are properly closed when connections are terminated or encounter errors. By modifying the connection establishment process to retain and manage transport references, the system can now correctly deallocate resources, thereby preventing indefinite memory consumption and improving the stability of long-running CLI sessions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
80601f5 to
efca517
Compare
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a memory leak in the MCP client by ensuring the transport layer is properly closed on disconnection or error. The changes are well-structured, propagating the transport object through the connection logic so it can be managed correctly. The test updates are also appropriate. I've found one area with a redundant call to close the transport, which could be simplified to improve code clarity and prevent potential issues if the close method isn't idempotent. Overall, this is a solid fix for a critical issue.
06c89f7 to
3a3eba1
Compare
Root cause: When MCP servers disconnect or encounter errors, the transport
(Stdio/SSE/HTTP) was not being closed because:
1. `connectToMcpServer()` only returned the Client, not the transport
2. The transport reference was created locally and lost after connection
3. `McpClient.transport` was declared but never assigned
This caused event listeners and buffers to run indefinitely, eventually
causing "JavaScript heap out of memory" errors in long-running sessions.
Changes:
- `connectToMcpServer()` now returns `{client, transport}` instead of just `Client`
- `McpClient.connect()` stores the transport reference
- `McpClient.disconnect()` now properly closes the transport before the client
- Error handlers in both `McpClient` and `connectAndDiscover` now close
the transport to prevent leaks on connection errors
- Helper functions `connectWithSSETransport()` and `retryWithOAuth()` now
return the transport for proper lifecycle management
Addresses the memory leak fix from PR google-gemini#12801.
The SDK's Client.close() method already calls transport.close() internally, making the explicit transport.close() call redundant. This addresses the reviewer feedback about potentially calling close() twice on the transport.
3a3eba1 to
d3c5a88
Compare
|
Hi @iqbalbhatti49 thanks for the approval - it appears that if I update the branch from |
…gle-gemini#18054) Co-authored-by: Jack Wotherspoon <jackwoth@google.com>
Summary
Fixes a memory leak where MCP server transports were never closed on disconnection, leaving event listeners and buffers running indefinitely. This eventually causes "JavaScript heap out of memory" errors in long-running sessions.
Related:
Root Cause
When MCP servers disconnect or encounter errors, the transport (Stdio/SSE/HTTP) was not being closed because:
connectToMcpServer()only returned theClient, not the transportMcpClient.transportwas declared but never assigned (alwaysundefined)This meant
disconnect()checksif (this.transport)but it's always falsy, so transport cleanup never happened.Changes
connectToMcpServer()Return TypePromise<Client>toPromise<{client: Client, transport: Transport}>McpClient.connect()const { client, transport } = await connectToMcpServer(...)this.transport = transportconnectAndDiscover()Helper Functions
connectWithSSETransport()now returns the transportretryWithOAuth()now returns the transport for proper lifecycle managementTest Plan
closemock to transport mock in disconnect testImpact
This is a focused bug fix that:
This PR was generated with the help of AI, and reviewed by a Human