-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(mcp): handle CancelledError in MCPSessionManager.create_session #3835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 @raben, 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 critical issue in the 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
|
|
Response from ADK Triaging Agent Hello @raben, thank you for your contribution! Before we can merge your pull request, you'll need to sign our Contributor License Agreement (CLA). You can find more information and sign the CLA at https://cla.developers.google.com/. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a well-executed fix for an important issue. The pull request correctly identifies that asyncio.CancelledError was not being handled because it inherits from BaseException, and implements a clear solution by adding a specific except block. The inclusion of a new unit test to validate the fix is excellent. My review includes a few minor suggestions to improve code clarity and maintainability by removing a redundant conditional check and some unused code in the new test.
| if exit_stack: | ||
| try: | ||
| await exit_stack.aclose() | ||
| except Exception as exit_stack_error: | ||
| logger.warning( | ||
| 'Error during cancelled session cleanup: %s', exit_stack_error | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if exit_stack: check appears to be redundant. The exit_stack variable is initialized unconditionally on line 331 with exit_stack = AsyncExitStack(), so it will never be falsy within this try...except block. You can simplify the code by removing this conditional check. This also applies to the similar block in the except Exception handler starting on line 388.
try:
await exit_stack.aclose()
except Exception as exit_stack_error:
logger.warning(
'Error during cancelled session cleanup: %s', exit_stack_error
)| @patch("google.adk.tools.mcp_tool.mcp_session_manager.ClientSession") | ||
| async def test_create_session_cancelled_error( | ||
| self, mock_session_class, mock_exit_stack_class, mock_stdio | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch for ClientSession and its corresponding argument mock_session_class are not used in this test. The CancelledError is raised before ClientSession is ever instantiated. To keep the test clean and focused on its specific purpose, you can remove this unnecessary patch and argument.
async def test_create_session_cancelled_error(
self, mock_exit_stack_class, mock_stdio
):| """ | ||
| manager = MCPSessionManager(self.mock_stdio_connection_params) | ||
|
|
||
| mock_session = MockClientSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an MCP server returns an HTTP error (e.g., 401, 403), the MCP SDK uses anyio cancel scopes internally, which raise CancelledError. Since CancelledError is a BaseException (not Exception) in Python 3.8+, the existing `except Exception` block does not catch it. This causes the error to propagate uncaught, leading to: - Application hangs - ASGI callable returned without completing response errors - Inability to handle MCP connection failures gracefully This fix explicitly catches asyncio.CancelledError and converts it to a ConnectionError with a descriptive message, allowing proper error handling and cleanup. Fixes google#3708
7d5be08 to
5787554
Compare
Link to Issue or Description of Change
1. Link to an existing issue:
2. Description:
Problem:
When an MCP server returns an HTTP error (e.g., 401, 403), the MCP SDK uses anyio cancel scopes internally, which raise
CancelledError. SinceCancelledErroris aBaseException(notException) in Python 3.8+, the existingexcept Exceptionblock inMCPSessionManager.create_session()does not catch it.This causes the error to propagate uncaught, leading to:
Solution:
This fix explicitly catches
asyncio.CancelledErrorbefore the generalExceptionhandler and converts it to aConnectionErrorwith a descriptive message. This allows:Testing Plan
Unit Tests:
Manual End-to-End (E2E) Tests:
Tested with an MCP server returning HTTP 403 Forbidden:
ConnectionErroris raised with descriptive message, allowing proper error handlingChecklist
Additional context
This issue affects multiple users as described in #3708. The root cause is that
asyncio.CancelledErrorinherits fromBaseException(notException) in Python 3.8+, so it escapes the existing exception handler.