Skip to content

Conversation

@lnbc1QWFyb24
Copy link
Contributor

When running the SSE transport in production, we noticed a massive memory leak. We have a Kubernetes health check process that was hitting the SSE endpoint periodically and then just disconnecting which was causing the memory leak. Digging in to the current implementation, there is no disconnection handling in the case of an abrupt disconnection, so this PR adds some cleanup when the connection drops.

Motivation and Context

Removes a memory leak in the case that a client disconnects abruptly.

How Has This Been Tested?

You can replicate the memory leak by simply making a GET request to the SSE endpoint and then disconnecting many times to see memory grow quite rapidly. I applied this fix to our production environment and the instance memory usage has been steady since.

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

There were two errors that were failing on main already, so these changes have not added any new errors.

@4t145
Copy link
Collaborator

4t145 commented May 9, 2025

Good point, would it better to release the resource when SseServerTransport drop?

impl Drop for SseServerTransport {
    // release the resource...
}

@lnbc1QWFyb24
Copy link
Contributor Author

In the case of a disconnection, the SseServerTransport is never dropped since it is not cleared from the tx store upon disconnection. When it is cleared from the tx store it is dropped though.

Copy link
Collaborator

@4t145 4t145 left a comment

Choose a reason for hiding this comment

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

LGTM

@4t145 4t145 merged commit 030b6f0 into modelcontextprotocol:main May 12, 2025
8 of 9 checks passed
@github-actions github-actions bot mentioned this pull request Jul 2, 2025
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.

2 participants