fix: exit stdio server cleanly on interrupt - #2745
Conversation
Sorry, something went wrong.
This comment was marked as spam.
|
Updated the regression test to match the transport-wide scope. Changes:
Validation run locally:
|
Sorry, something went wrong.
|
Pushed a follow-up for the CI strict-no-cover failure. Root cause: after the test was parametrized over stdio, sse, and streamable-http, the sse and streamable-http branches are now covered. Their old # pragma: no cover comments were therefore stale, and CI's strict-no-cover correctly rejected them. Change:
Validation run locally:
I also tried to run uv run --frozen --no-sync strict-no-cover locally, but the Windows environment failed before analysis with Couldn't read ...tmp...toml as a config file; the CI failure itself was the stale pragma report above. |
Sorry, something went wrong.
|
One more CI-specific coverage follow-up is now included in the same amended commit. The next Linux run showed all tests passing, but branch coverage dropped to 99.99% because coverage.py reported a missing match arc on the streamable-http case (server.py:300->exit). I kept the earlier stale no cover removal and marked only that branch arc with # pragma: no branch, matching the project's documented coverage guidance. Validation re-run locally:
|
Sorry, something went wrong.
|
The two remaining 3.14 lowest-direct jobs failed outside the MCPServer.run() change: the stdio subprocess test captured an anyio SyntaxWarning before the server's own stderr marker, which broke the exact stderr snapshot. I pushed a small follow-up commit that keeps the clean-exit assertion but allows dependency warnings to appear before the marker. It also adds a local TextIO cast for the same test so the touched file passes pyright. Local validation: uv run --frozen pytest tests/interaction/transports/test_stdio.py::test_tool_call_and_notification_round_trip_over_a_stdio_subprocess tests/server/mcpserver/test_server.py::TestServer::test_keyboard_interrupt_exits_cleanly tests/server/mcpserver/test_server.py::TestServer::test_run_propagates_non_interrupt_errors -q
# 5 passed
uv run --frozen ruff check tests/interaction/transports/test_stdio.py tests/server/mcpserver/test_server.py src/mcp/server/mcpserver/server.py
uv run --frozen ruff format --check tests/interaction/transports/test_stdio.py tests/server/mcpserver/test_server.py src/mcp/server/mcpserver/server.py
uv run --frozen pyright src/mcp/server/mcpserver/server.py tests/server/mcpserver/test_server.py tests/interaction/transports/test_stdio.py
git diff --check
|
Sorry, something went wrong.
|
Rebased this branch on current main and force-pushed 9d42000. The only conflict was in the stdio subprocess stderr assertion after upstream changed the same test. I kept the newer main-side coverage/env/timeout handling, kept the TextIO cast for pyright, and preserved this PR's suffix check for dependency warnings before the clean-exit marker. Validated locally:
|
Sorry, something went wrong.
ErenAta16
left a comment
There was a problem hiding this comment.
Verified the fix does what it says and the diagnosis in the issue is accurate: run() calls anyio.run(...) with no guard, so Ctrl-C during the stdio receive loop surfaces the whole anyio/asyncio cancellation chain.
Heads up that this is one of three open PRs with a byte-identical source change: yours, #2778, and #3027 all wrap the match transport: block in try/except KeyboardInterrupt: return, same 10 added / 7 removed lines in the same place. Yours has priority by date (Jun 1). I've put the full comparison on #2663 so a maintainer doesn't have to derive it three times.
What distinguishes yours is the test: parametrising over all three transports and asserting run.assert_called_once_with(mcp.run_stdio_async) is stronger than the single-transport version the other two have, since it pins that the guard wraps every branch rather than just stdio.
Two things I'd tidy before it lands.
Exit code. except KeyboardInterrupt: return makes run() return normally, so the process exits 0. Measured both shapes:
except KeyboardInterrupt: return -> exit code 0 raise SystemExit(130) -> exit code 130
130 is 128 + SIGINT, which is what an uncaught KeyboardInterrupt produces today and what a shell or supervisor expects from a Ctrl-C. After this change a script doing mcp-server || handle_failure reads an interrupted run as a clean success.
If the goal is only to drop the traceback, raise SystemExit(130) gets that without changing the signal semantics. If exiting 0 is deliberate (say, because a supervisor is meant to treat Ctrl-C as a normal stop) that's a defensible choice, it just deserves a comment, otherwise someone will "fix" it later assuming it was an oversight.
Unrelated hunks. The cast(TextIO, errlog) change in tests/interaction/transports/test_stdio.py is a typing fix that has nothing to do with this bug, and the # pragma: no cover -> # pragma: no branch edits are a coverage side effect of parametrising. Both are reasonable in themselves, but they make the diff larger than "guard the interrupt" and give a reviewer more to weigh. Splitting them out would make this the easiest of the three to approve.
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
Summary
To verify
Refs #2663