Skip to content
This repository was archived by the owner on May 4, 2026. It is now read-only.

fix: keep stdin open in stdio test to survive mcp>=1.27.0 transport-close cancel - #28

Merged
teh-hippo merged 2 commits into
mainfrom
copilot/update-action-run-references
May 4, 2026
Merged

fix: keep stdin open in stdio test to survive mcp>=1.27.0 transport-close cancel#28
teh-hippo merged 2 commits into
mainfrom
copilot/update-action-run-references

Conversation

Copilot AI commented May 4, 2026

Copy link
Copy Markdown
Contributor

MCP 1.27.0 added finally: tg.cancel_scope.cancel() to Server.run() (PR modelcontextprotocol/python-sdk#2334). When stdin closes, all in-flight handlers are immediately cancelled — including tools/list — before they can respond. The previous test used subprocess.run() which closes stdin immediately after writing all messages, creating a race where the handler was cancelled before it could flush its response.

Changes

  • tests/test_mcp_server.py: Replace subprocess.run() with subprocess.Popen() in test_stdio_initialize_and_tools_list
    • Write messages to stdin one at a time, keeping the pipe open
    • Collect responses in a daemon thread; block until id=2 (tools/list) arrives or 5 s elapses
    • Close stdin only after the response is confirmed received, then wait() for clean exit
    • Capture stderr before assertions; add explicit timeout assertion for clearer failure messages
# Before — stdin closed immediately after writes, races against handler cancel
proc = subprocess.run([...], input=input_text, ...)

# After — stdin stays open until tools/list response is in hand
proc = subprocess.Popen([...], stdin=PIPE, stdout=PIPE, ...)
for msg in (init, initialized, tools_list):
    proc.stdin.write(json.dumps(msg) + "\n")
    proc.stdin.flush()

t = threading.Thread(target=_collect, daemon=True)  # stops when id==2 seen
t.start()
t.join(timeout=5)

proc.stdin.close()   # only now signal EOF to the server
proc.wait(timeout=5)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the stdio integration test to keep the subprocess stdin open long enough for tools/list to respond, avoiding handler cancellation introduced by MCP >=1.27.0 when the transport closes.

Changes:

  • Switch test_stdio_initialize_and_tools_list from subprocess.run() to subprocess.Popen() to avoid closing stdin immediately.
  • Send JSON-RPC messages incrementally and keep stdin open until the tools/list response is observed (or a timeout occurs).
  • Collect stdout responses asynchronously and improve failure diagnostics by asserting on explicit timeouts and capturing stderr.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_mcp_server.py Outdated
@teh-hippo
teh-hippo marked this pull request as ready for review May 4, 2026 01:37
@teh-hippo
teh-hippo enabled auto-merge (rebase) May 4, 2026 01:37
Replace the t.join(timeout=...) wait with a threading.Event set by the
reader the moment the id=2 response is observed, and join the reader
explicitly after the subprocess exits. This removes the small race where
the reader could still be appending to responses while the main thread
was evaluating the assertion, addresses reviewer feedback on PR #28, and
also surfaces the captured response ids and stderr in the timeout
assertion message for easier diagnosis.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@teh-hippo

Copy link
Copy Markdown
Owner

Addressed the reviewer's race-condition concern in 7e669fa: the reader thread now signals a threading.Event the instant the tools/list response (id=2) arrives, the main thread waits on that event instead of t.join(timeout=…), and the reader is explicitly joined after the subprocess exits so no further mutations to responses can race the assertions. Stress-tested 50/50 locally; full suite still passes.

@teh-hippo
teh-hippo merged commit bf23522 into main May 4, 2026
9 checks passed
@teh-hippo
teh-hippo deleted the copilot/update-action-run-references branch May 4, 2026 01:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants