Keep AutoDetect channel open after provisional SSE failure - #1768
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c0e71f8e-9302-4c6b-835d-223dec24d37d
There was a problem hiding this comment.
Pull request overview
This PR fixes an AutoDetect transport failure mode where a provisional SSE fallback attempt could complete the shared message channel, preventing subsequent retries (e.g., retrying initialize) from receiving responses via the same AutoDetectingClientSessionTransport. It aligns SSE behavior with Streamable HTTP’s “provisional vs adopted” distinction.
Changes:
- Track when SSE is “adopted” (after a successful POST) and only complete the shared channel on disconnect once adopted.
- Update SSE disconnect handling to avoid completing AutoDetect’s shared channel during provisional SSE probe failures.
- Add a regression test covering Streamable HTTP failure + provisional SSE failure + successful retry using the same AutoDetect instance/channel.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs | Adds regression coverage ensuring the shared message channel remains usable after provisional SSE failure in AutoDetect mode. |
| src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs | Introduces “adoption” tracking and conditional disconnect completion to keep AutoDetect’s shared channel open during provisional SSE failures. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c0e71f8e-9302-4c6b-835d-223dec24d37d
| // If AutoDetect is still probing SSE, leave its shared message channel open so it can | ||
| // retry with another transport. A successful POST means SSE was selected and owns the | ||
| // channel from that point on, matching Streamable HTTP's adoption behavior. | ||
| if (_options.TransportMode is not HttpTransportMode.AutoDetect || _sseAdopted) |
There was a problem hiding this comment.
nit: this new guard is only covered for the non-adopted path. Consider adding a test for the inverse case as well: when SSE has been adopted (a POST succeeds so _sseAdopted is true), a later disconnect should still complete the shared channel. That protects against a future change that over-suppresses SetDisconnected for an adopted SSE transport. The existing SSE tests assert message exchange rather than completion on disconnect, so this path is currently untested.
Motivation and Context
When AutoDetect tries Streamable HTTP and then provisionally falls back to SSE, both transports use the message channel owned by the
AutoDetectingClientSessionTransport.If the provisional SSE connection fails,
SseClientSessionTransportcurrently completes that shared channel. Connect-time protocol fallback may subsequently retry withinitializeusing the same AutoDetect instance, but its reader can no longer receive the response because the channel has already completed.Streamable HTTP already distinguishes provisional use from an adopted transport. This change gives SSE equivalent behavior.
This is a prerequisite for handling the AutoDetect scenario in #1765.
Changes
No public API is added or changed.
How Has This Been Tested?
dotnet buildnet10.0,net9.0,net8.0, andnet472The ASP.NET conformance suite could not start locally because Node.js 21.6.2 does not provide
fs.globSync, which is required by the pinned conformance package.Breaking Changes
None.