Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe streamable HTTP session test now explicitly disconnects the authenticated GET stream. Cleanup runs after session assertions and verifies that the background GET thread terminates, including when an assertion fails. ChangesStreamable HTTP test
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change only makes the integration test explicitly finish its background request before client teardown; no product transport behavior changes are introduced. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The test now signals http.disconnect and joins the background GET thread in finally before TestClient teardown. This addresses the primary test-fix objective in issue ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superseded by #867, which fixes the actual MCP 2.1 ASGI contract violation by delivering the initial http.request, handles portal-task cancellation on every failure path, and adds the suite-level timeout backstop. Closing this narrower teardown-only attempt to avoid duplicate fixes. |
Summary
Make the long-lived authenticated SSE GET in the Streamable HTTP integration test finish explicitly before
TestClienttears down its portal.Why this path
The test itself owns the background ASGI request. A
finallyblock now signalshttp.disconnectand joins that request before leaving the client context. This keeps the existing DELETE-driven assertion meaningful: it still verifies that DELETE alone shuts the request down before fallback cleanup runs.Non-goals
Verification
Independent current-diff review: approved.
Fixes #860
Summary
tests/test_mcp_streamable_http.pyto disconnect the background SSEGETrequest during cleanup.TestClientexits, including when assertions fail.DELETEassertion that verifies request shutdown.Architecture impact
54 passedintests/test_mcp_streamable_http.py.