test: regression test for initialize hang on unexpected content-type#2472
test: regression test for initialize hang on unexpected content-type#2472Christian-Sidak wants to merge 2 commits into
Conversation
…ype (modelcontextprotocol#2432) Add a test that verifies initialize() raises MCPError immediately when the server returns an unexpected Content-Type (e.g. text/plain) instead of hanging forever waiting for a response that never arrives. Fixes modelcontextprotocol#2432
|
Friendly bump -- let me know if anything needs changing. |
1 similar comment
|
Friendly bump -- let me know if anything needs changing. |
|
Does the same fix path also cover a server returning 200 with an empty body, or 200 with valid JSON for a different request id? Both have the same "initialize never resolves" shape, so if the fix is at the right layer they should unblock without new code. If those cases are tested elsewhere already, ignore. |
|
No — this test is scoped to the unexpected content-type path only. The hang in issue #2432 is triggered when the SDK reaches the The two cases you describe follow different paths:
Both are valid "initialize never resolves" failure modes but would need separate tests against the relevant code paths. Happy to add them here or in a follow-up PR if that's useful. |
|
Friendly bump -- let me know if anything needs changing. |
|
Rebased onto current |
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/client/test_notification_response.py">
<violation number="1" location="tests/client/test_notification_response.py:9">
P1: Test collection now fails because the project provides `httpx`, not `httpx2`. Keep the existing `httpx` import and corresponding `httpx.AsyncClient`/`httpx.ASGITransport` references so this regression test module can run.</violation>
<violation number="2" location="tests/client/test_notification_response.py:10">
P1: This changes SDK type imports to an uninstalled `mcp_types` module, so pytest cannot import this file. Import `types` from `mcp` and `RootsListChangedNotification` from `mcp.types` instead.</violation>
<violation number="3" location="tests/client/test_notification_response.py:146">
P2: When the initialization-hang regression occurs, this test will hang the test run instead of producing a bounded failure, and it does not actually assert the claimed “immediately” behavior. Wrap the `initialize()` await in a short `anyio.fail_after(...)` scope (while retaining `pytest.raises`) so a regression fails deterministically.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| import httpx | ||
| import httpx2 | ||
| import mcp_types as types |
There was a problem hiding this comment.
P1: This changes SDK type imports to an uninstalled mcp_types module, so pytest cannot import this file. Import types from mcp and RootsListChangedNotification from mcp.types instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/client/test_notification_response.py, line 10:
<comment>This changes SDK type imports to an uninstalled `mcp_types` module, so pytest cannot import this file. Import `types` from `mcp` and `RootsListChangedNotification` from `mcp.types` instead.</comment>
<file context>
@@ -6,17 +6,18 @@
-import httpx
+import httpx2
+import mcp_types as types
import pytest
+from mcp_types import RootsListChangedNotification
</file context>
| import json | ||
|
|
||
| import httpx | ||
| import httpx2 |
There was a problem hiding this comment.
P1: Test collection now fails because the project provides httpx, not httpx2. Keep the existing httpx import and corresponding httpx.AsyncClient/httpx.ASGITransport references so this regression test module can run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/client/test_notification_response.py, line 9:
<comment>Test collection now fails because the project provides `httpx`, not `httpx2`. Keep the existing `httpx` import and corresponding `httpx.AsyncClient`/`httpx.ASGITransport` references so this regression test module can run.</comment>
<file context>
@@ -6,17 +6,18 @@
import json
-import httpx
+import httpx2
+import mcp_types as types
import pytest
</file context>
| async with streamable_http_client("http://localhost/mcp", http_client=client) as (read_stream, write_stream): | ||
| async with ClientSession(read_stream, write_stream) as session: # pragma: no branch | ||
| with pytest.raises(MCPError, match="Unexpected content type: text/plain"): # pragma: no branch | ||
| await session.initialize() |
There was a problem hiding this comment.
P2: When the initialization-hang regression occurs, this test will hang the test run instead of producing a bounded failure, and it does not actually assert the claimed “immediately” behavior. Wrap the initialize() await in a short anyio.fail_after(...) scope (while retaining pytest.raises) so a regression fails deterministically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/client/test_notification_response.py, line 146:
<comment>When the initialization-hang regression occurs, this test will hang the test run instead of producing a bounded failure, and it does not actually assert the claimed “immediately” behavior. Wrap the `initialize()` await in a short `anyio.fail_after(...)` scope (while retaining `pytest.raises`) so a regression fails deterministically.</comment>
<file context>
@@ -116,6 +132,20 @@ async def test_unexpected_content_type_sends_jsonrpc_error() -> None:
+ async with streamable_http_client("http://localhost/mcp", http_client=client) as (read_stream, write_stream):
+ async with ClientSession(read_stream, write_stream) as session: # pragma: no branch
+ with pytest.raises(MCPError, match="Unexpected content type: text/plain"): # pragma: no branch
+ await session.initialize()
+
+
</file context>
Summary
test_initialize_does_not_hang_on_unexpected_content_type) that directly reproduces the hang described in issue Initialize call hangs forever if MCP server does not return aContent-Type: text/plain#2432Content-Type: text/plainfor all requests (includinginitialize) and asserts thatsession.initialize()raisesMCPErrorimmediately instead of blocking foreverThe underlying fix is already present on
main—_handle_post_requestsends a properJSONRPCError(with the matching request ID) when it encounters an unexpected content type, which letsClientSessionresolve the pending request instead of waiting indefinitely. This PR adds the missing test that exercises that code path for theinitializecall specifically.Test plan
tests/client/test_notification_response.py::test_initialize_does_not_hang_on_unexpected_content_type— new test passestest_notification_response.pytests continue to passFixes #2432