Skip to content

fix: drain completed streamable HTTP SSE responses#2750

Open
he-yufeng wants to merge 2 commits into
modelcontextprotocol:mainfrom
he-yufeng:fix/streamable-http-drain
Open

fix: drain completed streamable HTTP SSE responses#2750
he-yufeng wants to merge 2 commits into
modelcontextprotocol:mainfrom
he-yufeng:fix/streamable-http-drain

Conversation

@he-yufeng
Copy link
Copy Markdown

Summary

Fixes #2707.

The streamable HTTP client was closing SSE responses as soon as a terminal JSON-RPC response arrived. That leaves the underlying HTTP/1.1 connection partially drained, so the next request can pay the keepalive penalty described in the issue.

This change keeps reading the POST SSE response, resumption GET response, and resumed GET stream to EOF after the terminal response has been delivered. Terminal responses still stop further message handling and still avoid reconnecting; the only behavior change is that the transport no longer aborts the response early on the normal completion path.

To verify

  • uv run pytest tests\shared\test_streamable_http.py -k "drains_after_terminal_response"
  • uv run pytest tests\shared\test_streamable_http.py::test_streamable_http_client_tool_invocation tests\shared\test_streamable_http.py::test_streamable_http_client_resumption
  • uv run ruff check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
  • uv run ruff format --check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
  • git diff --check -- src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py

@he-yufeng he-yufeng force-pushed the fix/streamable-http-drain branch 5 times, most recently from 3c9edc5 to edeb3e0 Compare June 1, 2026 21:49
Copy link
Copy Markdown

@StantonMatt StantonMatt left a comment

Choose a reason for hiding this comment

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

I found one post-terminal error path that looks branch-specific. The new response_complete flag is only checked inside the SSE loop, so if EventSource.aiter_sse() yields the terminal JSON-RPC response and then raises while draining, _handle_sse_response() falls through the outer except and still reconnects because last_event_id is set. A local fake-EventSource probe delivered response id req-1, raised after that terminal event, and _handle_reconnection was called with terminal-response-id even though the caller had already received the response.

The same pattern shows up in _handle_reconnection(): with a resumed stream that yields the terminal response and then raises during the post-terminal drain, the branch retries and forwards the same terminal response twice. My local probe with retry_interval_ms=0 produced resume_attempts=2 and forwarded_ids=['req-1', 'req-1'].

I think the exception path needs to treat response_complete as terminal too, returning instead of reconnecting/retrying once the response has already been delivered.

Checks I ran on head edeb3e0:

  • uv run --frozen pytest tests\shared\test_streamable_http.py -k drains_after_terminal_response -q -> 2 passed
  • uv run --frozen pytest tests\shared\test_streamable_http.py::test_streamable_http_client_tool_invocation tests\shared\test_streamable_http.py::test_streamable_http_client_resumption -q -> 2 passed
  • uv run --frozen pytest tests\shared\test_streamable_http.py::test_reconnection_retries_after_failed_resume -q -> 1 passed
  • uv run --frozen ruff check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py
  • uv run --frozen ruff format --check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py
  • uv run --frozen pyright src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py
  • git diff --check origin/main...HEAD

@he-yufeng
Copy link
Copy Markdown
Author

Updated in bb834da.

You were right about the exception path. I moved response_complete outside the try scopes and now both paths return immediately if the terminal response was already delivered before a drain error:

  • _handle_sse_response(): no reconnect after terminal POST SSE response + post-terminal drain error.
  • _handle_reconnection(): no recursive retry after resumed stream delivers the terminal response + post-terminal drain error, so the response is not forwarded twice.

Added focused regressions for both probes:

  • test_sse_response_does_not_reconnect_after_terminal_then_drain_error
  • test_reconnection_does_not_retry_after_terminal_then_drain_error

Validation run locally:

PYTHONUTF8=1 PYTHONPATH=src uv run --no-sync pytest tests\shared\test_streamable_http.py -q -k "terminal_then_drain_error or terminal_response or failed_resume" --basetemp .tmp\pytest-2750-20260604b -p no:cacheprovider
PYTHONUTF8=1 PYTHONPATH=src uv run --no-sync pytest tests\shared\test_streamable_http.py -q --basetemp .tmp\pytest-2750-20260604-full -p no:cacheprovider
PYTHONUTF8=1 uv run --no-sync ruff check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
PYTHONUTF8=1 uv run --no-sync ruff format --check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
PYTHONUTF8=1 uv run --no-sync pyright src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
git diff --check

Results: the focused subset passed, the full tests/shared/test_streamable_http.py file passed (66 passed), ruff/format/pyright/diff-check passed.

I also tried mypy on the touched files, but on this Windows checkout it reports existing project-wide issues while following imports (missing win32/jsonschema/requests stubs, os.killpg/SIGKILL on Windows stubs, existing session/test typing errors). I did not treat that as caused by this patch.

@he-yufeng he-yufeng force-pushed the fix/streamable-http-drain branch from bb834da to 69897d7 Compare June 4, 2026 06:38
@he-yufeng
Copy link
Copy Markdown
Author

Addressed the latest review and rebased this branch on current main.

Changes in the follow-up commit:

  • keep response_complete outside the SSE iteration try-block so terminal responses remain terminal even if draining raises afterwards;
  • return from the POST SSE drain exception path once a terminal response has already been delivered;
  • return from the resumed GET exception path once a terminal response has already been delivered, avoiding duplicate delivery/reconnect after a completed response;
  • add regression coverage for both _handle_sse_response() and _handle_reconnection() when the stream yields a terminal response and then raises during drain.

Validation on Windows:

  • uv run --no-sync pytest tests\shared\test_streamable_http.py -q -k "terminal_then_drain_error or terminal_response or failed_resume" --basetemp .tmp\pytest-2750-rebase -p no:cacheprovider -> 5 passed
  • uv run --no-sync pytest tests\shared\test_streamable_http.py -q --basetemp .tmp\pytest-2750-rebase-full -p no:cacheprovider -> 66 passed
  • uv run --no-sync ruff check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py -> passed
  • uv run --no-sync ruff format --check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py -> passed
  • uv run --no-sync pyright src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py -> 0 errors
  • git diff --check -> passed

@he-yufeng he-yufeng force-pushed the fix/streamable-http-drain branch from 69897d7 to 58bf5a5 Compare June 4, 2026 07:16
@he-yufeng
Copy link
Copy Markdown
Author

Addressed the coverage gap on the non-terminal SSE error path.

What changed:

  • added a focused test that covers the pre-terminal stream error path and verifies it still reconnects with the last SSE id.

Validation on head 58bf5a5:

  • uv run --no-sync pytest tests\shared\test_streamable_http.py -q --basetemp .tmp\pytest-2750-coverage-full -p no:cacheprovider -> 67 passed
  • uv run --no-sync ruff check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py -> passed
  • uv run --no-sync ruff format --check src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py -> passed
  • uv run --no-sync pyright src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py tests\interaction\transports\test_hosting_resume.py -> passed
  • git diff --check -> passed

@he-yufeng he-yufeng force-pushed the fix/streamable-http-drain branch from 58bf5a5 to f122efe Compare June 4, 2026 07:47
@he-yufeng
Copy link
Copy Markdown
Author

Follow-up after the CI strict-no-cover failure:

  • The new regression test now covers the SSE drain exception path, so I removed the stale # pragma: no cover from that line.
  • Re-ran the focused streamable HTTP suite and static checks locally.

Verified:

uv run --no-sync pytest tests\shared\test_streamable_http.py -q --basetemp .tmp\pytest-2750-nocover -p no:cacheprovider
uv run --no-sync ruff check --no-cache src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
uv run --no-sync ruff format --check --no-cache src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
uv run --no-sync pyright src\mcp\client\streamable_http.py tests\shared\test_streamable_http.py
git diff --check

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.

streamable_http: early response.aclose() poisons keepalive connection, causes ~260ms latency on every subsequent tool call

2 participants