Skip to content

fix(client): treat any 2xx as successful session termination - #3548

Closed
deadczarvc wants to merge 1 commit into
modelcontextprotocol:mainfrom
deadczarvc:fix/termination-2xx
Closed

deadczarvc wants to merge 1 commit into
modelcontextprotocol:mainfrom
deadczarvc:fix/termination-2xx

Conversation

@deadczarvc

Copy link
Copy Markdown

Fixes #3546.

Problem

terminate_session in src/mcp/client/streamable_http.py treats any DELETE response outside (200, 204) as a failure and logs Session termination failed: 202. 202 Accepted is the correct response for a server that processes termination asynchronously; under HTTP semantics any 2xx is success, and the Streamable HTTP spec only carves out 405 for this request.

In a long-running Hermes Agent deployment with several Streamable HTTP MCP servers this produced 72 spurious warnings in ~36 hours in errors.log, burying real failures that surface through the same line.

Change

One condition: elif not 200 <= response.status_code < 300:. The 405 debug path and the warning for genuine failures (4xx/5xx) are unchanged.

Testing

Added test_streamable_http_client_session_termination_status_handling next to the existing termination tests in tests/shared/test_streamable_http.py, reusing the AnswerDeleteWith204 wrapping-transport pattern: parametrized over 202 (no warning logged) and 500 (warning still logged). Verified red on unmodified main for the 202 case, green after the fix.

  • tests/shared/test_streamable_http.py: 75 passed
  • ruff check / ruff format --check: clean
  • pyright on the touched module: 0 errors

Full suite not run locally (Windows env); CI will cover the rest.

Streamable HTTP DELETE termination warned 'Session termination failed: 202'
for spec-compliant asynchronous deletes. HTTP semantics make any 2xx a
success; the Streamable HTTP spec only carves out 405 for this request.
Accept the whole 2xx range and keep the warning for genuine failures.

Fixes modelcontextprotocol#3546
@github-actions github-actions Bot added the missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md) label Sep 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3546.

If a maintainer assigns you to #3546, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take.

You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way.

CONTRIBUTING.md has the full reasoning, but in short:

  • We're a small team with very little capacity to review community PRs right now.
  • Many recent PRs are AI-generated with little human review, and reviewing one carefully still costs a maintainer as much time as it ever did. A well-described issue is usually more useful to us than the code.

Maintainers: reopen, remove missing-issue-link, or add bypass-issue-check to override.

@github-actions github-actions Bot closed this Sep 19, 2026
@deadczarvc

Copy link
Copy Markdown
Author

Pushed c725630 addressing the CI coverage failures (tests themselves were green everywhere):

1. strict-no-cover failure (all Python versions):
strict-no-cover reported:

❎ 1 lines wrongly marked with 'pragma: no cover' are covered
  src/mcp/client/streamable_http.py:677

Line 677 (logger.warning(f"Session termination failed: {response.status_code}")) carried # pragma: no cover inherited from the pre-PR code, but the new parametrized test's 500 case deliberately executes it — so the pragma had become a false claim. Fix: removed the pragma. The warning line is now legitimately covered, and the 202 case covers the no-warning path.

2. Coverage 99.99 < fail-under=100 on Python 3.14 (both locked and lowest-direct, linux and windows):

tests/shared/test_streamable_http.py  ... 99.81%   1246->1250, 1246->1254

The only missing coverage was two partial branches inside the new test itself — the async with context-manager chain at line 1246. On Python 3.14 with anyio 4.10, coverage doesn't attribute CM-chain exits the way earlier versions do (sibling tests in this file mark the identical pattern with # pragma: no branch, e.g. the ClientSession line in AnswerDeleteWith204's test and 29 other occurrences). Fix: marked the two CM lines of the new test with # pragma: no branch, matching the established repo convention.

Local verification (Python 3.13 venv): tests/shared/test_streamable_http.py::test_streamable_http_client_session_termination_status_handling 2/2 passed, warning line 677 executed and no longer excluded, ruff check / ruff format --check clean. The 3.14 partial-branch attribution is environment-specific and validated by CI on this push.

Note: the PR remains closed pending maintainer assignment to #3546 — pushing anyway per the bot's guidance so the fix is review-ready if the PR is reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streamable HTTP client logs a WARNING for valid 202 Accepted on session termination (DELETE)

1 participant