Python: Add regression tests for MCPStreamableHTTPTool GET stream resilience (#5317)

[Giles Odigwe (giles17)]

Motivation and Context

The bug where a failing background GET SSE notification stream crashed the entire agent was already fixed by upgrading to mcp ≥ 1.27.0 (which gracefully handles 405 and connection errors in handle_get_stream). These regression tests ensure the fix is not accidentally reverted.

Fixes #5317

Description

Adds two tests to test_mcp.py that simulate the exact failure modes reported in the issue: a 405 response (Learn MCP behavior) and a connection reset (D365 F&O behavior) on the background GET SSE stream. Both tests verify that the ClientSession remains fully usable after the background notification stream fails — confirming the MCP library upgrade resolved the crash without requiring changes to agent_framework/_mcp.py.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by giles17's agent

[Giles Odigwe (giles17)]

Automated Code Review

Reviewers: 4 | Confidence: 95% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach

---

Automated review by giles17's agents

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds regression coverage to ensure MCP Streamable HTTP’s background GET SSE notification stream failures do not crash or cancel the main ClientSession (guarding against regressions of #5317 after the mcp >= 1.27.0 upgrade).

Changes:

• Add a test for a 405 response on the background GET stream and verify the session remains usable.
• Add a test for a connection error on the background GET stream and verify the session remains usable.

[Giles Odigwe (giles17)]

Automated Code Review

Reviewers: 4 | Confidence: 95%

✓ Correctness

The diff replaces arbitrary sleep-based waits with proper asyncio.Event synchronization in two test functions. An Event is created, set inside the mock GET handler, and awaited with a generous timeout. This is a well-established pattern for making async tests deterministic and less flaky. The logic is correct: the event is set synchronously before the response/exception in the handler, ensuring the wait completes reliably. No correctness issues found.

✓ Security Reliability

This diff replaces fixed asyncio.sleep() calls in two tests with event-based synchronization using asyncio.Event and asyncio.wait_for(timeout=5.0). This is a clean reliability improvement: the tests now deterministically wait for the GET request to be attempted rather than relying on arbitrary sleep durations, while the 5-second timeout prevents indefinite hangs. No security or reliability concerns.

✓ Test Coverage

This PR replaces flaky asyncio.sleep() delays with deterministic asyncio.Event() synchronization in two MCP test functions. The pattern is correct: the event is created before the mock handler, set inside the GET branch, and awaited with asyncio.wait_for(..., timeout=5.0) after session.initialize(). This makes the tests both faster (no arbitrary sleep) and more reliable (event-driven instead of timing-based). No test coverage gaps introduced; the behavioral assertions remain unchanged.

✗ Design Approach

The change makes these tests less representative of the failure they are meant to cover. Replacing the fixed delay with an Event is a good direction, but the event is currently set when the GET request starts rather than when the GET failure has actually been observed and handled, so the test can proceed to list_tools() before the background stream task reaches the 405/connection-error path.

Flagged Issues

• test_streamable_http_get_stream_405_does_not_crash_session: get_attempted.set() fires before the 405 response is returned (line 4202-4204), but the assertion phase only awaits that event (line 4279-4283). The test no longer guarantees the session stays healthy after the GET-stream failure is processed.
• Same issue in the connection-error variant: get_attempted.set() fires before httpx.ConnectError is raised (line 4301-4304), while the test only waits on that early signal (line 4367-4371). This can mask regressions where the background task fails after request dispatch.

---

Automated review by giles17's agents

[Evan Mattson (moonbox3)]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
TOTAL	31840	3696	88%

report-only-changed-files is enabled. No files were changed during this commit :)

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
6265	30 💤	0 ❌	0 🔥	1m 42s ⏱️

[github-actions]

Automated Code Review

Reviewers: 3 | Confidence: 91%

✓ Security Reliability

Two new regression tests verify MCP streamable HTTP client resilience when the background GET SSE notification stream fails with 405 or ConnectError. The tests use httpx.MockTransport with async handlers (confirmed supported in httpx 0.28.1), proper async context manager cleanup, and timeout-bounded waits. No security or reliability issues found beyond the previously-reviewed and resolved event-signaling race condition.

✓ Test Coverage

The two new regression tests for MCP Streamable HTTP GET stream resilience are well-structured and provide meaningful coverage. They use proper asyncio.Event synchronization to confirm the GET stream was attempted, and verify session functionality afterward via list_tools() calls. The previous review concerns about sleep-based timing have been addressed. The event-before-error timing nuance was discussed and resolved in prior reviews. No blocking issues found.

✗ Design Approach

The added tests are aimed at the right regression, but both still synchronize on the GET request starting rather than on the failing GET path completing. Because get_attempted is set before the 405 response is returned and before the ConnectError is raised, wait() can unblock while the background notification task has not actually observed the failure yet, so these tests can still pass as false positives.

Flagged Issues

• The new regression coverage is still unreliable: in python/packages/core/tests/core/test_mcp.py:4202-4208 and 4302-4304, the event is signaled before the failing GET path completes, so list_tools() may run before the background failure has been processed. The tests do not actually prove the session survived the GET-stream failure they are trying to cover.

---

Automated review by giles17's agents