Skip to content

Conversation

@Nerja
Copy link

@Nerja Nerja commented Dec 12, 2025

This PR adds resilience to the MCP toolset by automatically attempting to reconnect when the connection to the MCP server is lost. When ListTools() or CallTool() fails, the client first pings the server to verify the connection is actually dead before attempting a reconnect.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @Nerja, 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 significantly enhances the resilience of the MCP toolset by introducing an automatic session reconnection capability. It ensures that any previously established MCP session is validated for health using a Ping mechanism before being returned. If the session is found to be unhealthy, it is gracefully closed, and a new connection is initiated, thereby improving the reliability of interactions with MCP servers and preventing operations on stale connections.

Highlights

  • MCP Session Health Check: Implemented a Ping health check for existing MCP sessions within the getSession method to verify their liveness before use.
  • Automatic Session Reconnection: Added logic to automatically close an unhealthy or disconnected MCP session (if Ping fails) and trigger the establishment of a new connection.
  • Session Reconnection Test: Introduced a new unit test, TestSessionReconnection, to thoroughly validate the new session health check and reconnection mechanism.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

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

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 adds session reconnection logic to the MCP toolset, using a Ping health check to detect stale connections. The implementation in set.go correctly checks for an active session and attempts to reconnect if the ping fails. The accompanying test in set_test.go effectively validates this new reconnection behavior.

My review includes a couple of suggestions for improvement: one to make the session recovery logic more robust in set.go, and another to improve context handling in the new test case to prevent potential resource leaks.

Comment on lines 142 to 185
if err := s.session.Ping(ctx, &mcp.PingParams{}); err == nil {
return s.session, nil
}
s.session.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

After closing a stale session, it's good practice to set s.session to nil. This makes the error recovery more robust. If the subsequent call to s.client.Connect fails, the next invocation of getSession will not attempt to Ping and Close an already-closed session, which could lead to unexpected errors or behavior. Setting it to nil ensures a clean state for the next connection attempt.

                if err := s.session.Ping(ctx, 	&mcp.PingParams{}); err == nil {
                        return s.session, nil
                }
                s.session.Close()
                s.session = nil


func (rt *reconnectableTransport) Connect(ctx context.Context) (mcp.Connection, error) {
ct, st := mcp.NewInMemoryTransports()
_, err := rt.server.Connect(context.Background(), st, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's better to use the ctx from the function arguments here instead of context.Background(). This ensures that if the client-side connection attempt is cancelled, the server-side connection setup is also cancelled. This prevents potential resource leaks in the test and is a better practice for context propagation.

Suggested change
_, err := rt.server.Connect(context.Background(), st, nil)
_, err := rt.server.Connect(ctx, st, nil)

@dpasiukevich
Copy link
Collaborator

Thanks for the PR!

I'm thinking -- now we will always be making 2 network requests: ping + list/call_tool.

What do you think about these approaches:

  1. Making a detached goroutine which will ping over an interval configured by user (with some default) and refresh the connection. And we'd probably would need to expose Toolset struct (which is now called set) with Close() method so users could clean up this goroutine.
  2. Or another option -- handling mcp.ErrConnectionClosed. If list/call_tool return this error, then mcptoolset will refresh the connection. Then no public API changes needed, but we'd still probably would need to call ping before the refresh and do some gradual fallback.

What do you think?

@Nerja
Copy link
Author

Nerja commented Dec 15, 2025

@dpasiukevich I updated the PR to only ping when there is an error. What do you think of the current approach? I noticed that not all errors are mcp.ErrConnectionClosed (for instance, the Close() call in the tests does not cause mcp.ErrConnectionClosed) so made the compromise to issue a ping when there is any error. If ping succeeds, then we will not create a new connection/session.

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