Skip to content

Fix httpstreamable errorcode #1194

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

CriticalGameEror
Copy link

Added additional if statements to catch a 400 bad request error messages within the streamable_http.py file

Motivation and Context

This change is needed to fix an issue with the current way that a client uses streamable HTTP to connect to a MCP server. Currently, an unavoidable exception is raised when creating a SessionGroup and adding a steamable HTTP connection to an MCP server that returns an error 4xx code when a primitive (prompts, resources and tools) is requested. As long as at least one of these primitives returns an error 4xx code (except a 404 error), it will cause the exception, which crashes the MCP client.

In cases with independent sessions outside of SessionGroup, this error can be handled by the developer building the MCP client. However, when using SessionGroup, the developer cannot handle these errors, and so cannot prevent the program from crashing. This is because the SessionGroup automatically sends web requests to prompts, resources and tools without checking if they are supported, which I think goes against the specification.

The change is needed to ensure that an MCP client doesn't crash when an MCP server sends an error code for a bad request (which it can do as defined in the specifciation). The change was made to the streamable_http.py file, as this avoids a possible breaking change if the SessionGroup was changed instead.

How Has This Been Tested?

This has been tested with the AWS Knowledge MCP Server, which returns an error code 400 when interacting with a prompt or resource primitive.

Breaking Changes

No breaking changes. The only possible scenario of a breaking change is if a developer relies on the httpx status error exception currently raised to detect an error 400 code for an unsupported capability. In this case, the developer should be following the specification, and check if a capability is supported before it is requested.

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

I apologise in advance if I have missunderstood anything about the code regarding this issue or anything about the MCP specification. Please correct me on any errors I have made so that I can learn from them.

Ensures that any error code response from the MCP server is handled.
This ensures the error message is more specific and focused to a 400 bad request error only
@CriticalGameEror CriticalGameEror requested a review from a team as a code owner July 23, 2025 22:34
@CriticalGameEror CriticalGameEror requested a review from ochafik July 23, 2025 22:34
@Kludex
Copy link
Member

Kludex commented Jul 26, 2025

It seems we are missing a try..except on a lower level that would send the JSON RPC message instead of crashing the server.

@CriticalGameEror
Copy link
Author

@Kludex

It seems we are missing a try..except on a lower level that would send the JSON RPC message instead of crashing the server.

Just a small correction to avoid confusion. The issue is entirely related with the MCP client. The MCP server does not crash in any way, only the client is affected.

async def _handle_post_request(self, ctx: RequestContext) -> None:
"""Handle a POST request with response processing."""
headers = self._prepare_request_headers(ctx.headers)
message = ctx.session_message.message
is_initialization = self._is_initialization_request(message)
async with ctx.client.stream(
"POST",
self.url,
json=message.model_dump(by_alias=True, mode="json", exclude_none=True),
headers=headers,
) as response:
if response.status_code == 202:
logger.debug("Received 202 Accepted")
return
if response.status_code == 400:
if isinstance(message.root, JSONRPCRequest):
await self._send_bad_request_error(
ctx.read_stream_writer,
message.root.id,
)
return
if response.status_code == 404:
if isinstance(message.root, JSONRPCRequest):
await self._send_session_terminated_error(
ctx.read_stream_writer,
message.root.id,
)
return
response.raise_for_status()
if is_initialization:
self._maybe_extract_session_id_from_response(response)
content_type = response.headers.get(CONTENT_TYPE, "").lower()
if content_type.startswith(JSON):
await self._handle_json_response(response, ctx.read_stream_writer, is_initialization)
elif content_type.startswith(SSE):
await self._handle_sse_response(response, ctx, is_initialization)
else:
await self._handle_unexpected_content_type(
content_type,
ctx.read_stream_writer,
)

The main issue stems from line 285 above, since it raises a HTTPStatusError, which isn't caught. In the current main branch code, only an error 404 response is caught. All other responses would result in a crash because of line 285. I'm not sure if there is a lower level in the code than this before the response received from the server.

Would it be best to put a try...except around line 285? Since an error 404 is already caught in the main branch, wouldn't it be best to catch any type of 4xx or 5xx error?

Thank you for taking the time to review my pull request, I really appreciate it.

Copy link

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Thanks @CriticalGameEror !

This looks reasonable to me but would greatly benefit from a test (also to better understand the client-side crash case), could you please add something?

@CriticalGameEror
Copy link
Author

@ochafik

Thank you for taking the time to review my pull request. I have looked at adding a test to showcase the issue that is occuring, but it appears that when using the testing server provided in tests/shared/test_streamable_http.py, requesting an invalid capability does not result in an error. I spent many hours looking at why this would be the case, but it appears the error is handled completely fine during testing. My only thought is that it doesn't error due to how the testing server is setup to handle errors (since the error I am trying to fix is reliant on a particular unhandled HTTPStatusError from the httpx library - see my previous comments), but that is just a guess. For reference the test case I was trying to edit can be seen below (see the last 4 lines):

@pytest.mark.anyio
async def test_streamablehttp_client_error_handling(initialized_client_session):
    """Test error handling in client."""
    # Tests reading an invalid resource
    with pytest.raises(McpError) as exc_info:
        await initialized_client_session.read_resource(uri=AnyUrl("unknown://test-error"))
    assert exc_info.value.error.code == 0
    assert "Unknown resource: unknown://test-error" in exc_info.value.error.message

    # Tests using an invalid capability
    with pytest.raises(McpError) as exc_info:
        await initialized_client_session.list_prompts()
    assert exc_info.value.error.message == "Method not found"

The code I used to test the AWS MCP server mentioned within my pull request can be found below. Without the changes I previously made, this code still results in an unhandled exception:

import asyncio

from mcp.client.session_group import ClientSessionGroup, StreamableHttpParameters

async def testing():
    async with ClientSessionGroup() as group:
        await group.connect_to_server(StreamableHttpParameters(url="https://knowledge-mcp.global.api.aws"))
        print(group.tools.items())
        await group._exit_stack.aclose()

if __name__ == "__main__":
    asyncio.run(testing())

Any advice on this to move forward?

@CriticalGameEror
Copy link
Author

CriticalGameEror commented Aug 6, 2025

@ochafik

After some additional digging, I think I have found the cause for why the error does not occur during testing. It appears that the test server returns a 200 code whenever there is a successful response, even if the response is a JSON-RPC error. This behaviour also occurs with another hosted MCP server https://www.kibo-ui.com/api/mcp/mcp.

It appears that the Amazon MCP server I tested (https://knowledge-mcp.global.api.aws) is one of the few MCP servers which returns an error response code (4xx) if a request tries to call an unsupported primitive/capability, rather than just sending a 200 response and displaying a JSON-RPC error in the response body. This is the behaviour which causes the server to crash.

I am unsure if an MPC server is allowed to return an error 4xx response under the specification when requesting an unsupported primitive/capability. If an MPC server isn't allowed to do this, then this pull request can be closed as the fix lies with how that specific MCP server is implemented. Otherwise, the change within this pull request should go ahead.

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.

3 participants