From cff927f6d2e1b77647ff75c2e2cd944a789c6a3d Mon Sep 17 00:00:00 2001 From: Scott Holodak Date: Fri, 4 Sep 2026 18:05:36 -0400 Subject: [PATCH] fix(client): skip the SSE fallback when the server/discover probe is rejected A server predating SEP-2575 rejects the session-less server/discover POST at the HTTP layer with 400 (cannot parse the request) or 404 (requires Mcp-Session-Id on every non-initialize POST). AutoDetectingClientSessionTransport treated that like any other non-JSON-RPC error and attempted the SSE fallback, but those two statuses say nothing about which transport the peer speaks -- an SSE-only server rejects the probe exactly the same way. McpClientImpl.ConnectAsync already reads 400/404 from the probe as 'this server requires the initialize handshake' and retries with initialize on the same transport, and that retry still falls back to SSE. So the SSE GET spent during the probe is always discarded: a Streamable-HTTP-only server that predates SEP-2575 pays a wasted round trip on every connect and logs 'falling back to SSE transport' for settled protocol negotiation, while an SSE-only server is reached one POST later either way. Scoped to the discover probe and to the two statuses ConnectAsync acts on, so every other failure keeps the existing fallback. --- .../AutoDetectingClientSessionTransport.cs | 34 +++- .../HttpClientTransportAutoDetectTests.cs | 151 ++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs b/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs index c9e821adf..6f85a62f1 100644 --- a/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs +++ b/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs @@ -96,8 +96,6 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can // behavior. Capture the underlying error (status + body) before falling back so that, // if SSE also fails, we can surface the real Streamable HTTP diagnostic to the caller // instead of dropping it on the floor (see https://github.com/modelcontextprotocol/csharp-sdk/issues/1526). - LogStreamableHttpFailed(_name, response.StatusCode); - // This reads the response body a second time for the application/json case, where // TryReadJsonRpcErrorAsync above already read it. HttpContent buffers after the first // read, so this returns the same buffered content and is safe (not a second stream @@ -105,6 +103,26 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can // TryReadJsonRpcErrorAsync returns early on the content type, so there is no double read. var streamableHttpError = await HttpResponseMessageExtensions.CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false); + if (IsDiscoverProbeRejection(message, response.StatusCode)) + { + // The server/discover probe is protocol negotiation, not transport detection. A server + // predating SEP-2575 rejects the session-less POST with 400 (can't parse the request) or + // 404 (requires Mcp-Session-Id on every non-initialize POST) whether it speaks Streamable + // HTTP or SSE, so neither status is evidence about which transport to use. McpClientImpl + // .ConnectAsync treats exactly these two statuses as "initialize-handshake server" and + // immediately retries with initialize on this same transport — and that attempt still + // falls back to SSE, so an SSE-only server is reached one POST later rather than not at + // all. Attempting SSE here instead spends a GET whose result is discarded on every + // connect to a Streamable-HTTP-only server that predates SEP-2575, and logs a "falling + // back to SSE transport" line that misreports settled protocol negotiation as a failure. + LogSkippingSseFallbackForDiscoverProbe(_name, response.StatusCode); + + await streamableHttpTransport.DisposeAsync().ConfigureAwait(false); + throw streamableHttpError; + } + + LogStreamableHttpFailed(_name, response.StatusCode); + await streamableHttpTransport.DisposeAsync().ConfigureAwait(false); await InitializeSseTransportAsync(message, streamableHttpError, cancellationToken).ConfigureAwait(false); } @@ -119,6 +137,15 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can } } + /// + /// Returns when the failed request was the SEP-2575 server/discover probe and the + /// status is one of the two already reads as "this server requires the initialize + /// handshake", meaning the SSE fallback cannot contribute anything the initialize retry won't. + /// + private static bool IsDiscoverProbeRejection(JsonRpcMessage message, HttpStatusCode statusCode) => + statusCode is HttpStatusCode.BadRequest or HttpStatusCode.NotFound && + message is JsonRpcRequest { Method: RequestMethods.ServerDiscover }; + private async Task InitializeSseTransportAsync(JsonRpcMessage message, HttpRequestException? streamableHttpError, CancellationToken cancellationToken) { if (_options.KnownSessionId is not null) @@ -181,6 +208,9 @@ public async ValueTask DisposeAsync() [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} streamable HTTP transport failed with status code {StatusCode}, falling back to SSE transport.")] private partial void LogStreamableHttpFailed(string endpointName, HttpStatusCode statusCode); + [LoggerMessage(Level = LogLevel.Debug, Message = "{EndpointName} server/discover probe rejected with status code {StatusCode}; skipping the SSE fallback so the initialize handshake is attempted instead.")] + private partial void LogSkippingSseFallbackForDiscoverProbe(string endpointName, HttpStatusCode statusCode); + [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} using Streamable HTTP transport.")] private partial void LogUsingStreamableHttp(string endpointName); diff --git a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs index 7100e728a..c46383306 100644 --- a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs @@ -505,4 +505,155 @@ await Assert.ThrowsAnyAsync(() => MockLoggerProvider.LogMessages, m => m.LogLevel == LogLevel.Warning && m.Message.Contains("SSE fallback failed")); } + + // A 400/404 on the SEP-2575 server/discover probe is protocol negotiation, not transport detection: + // a pre-SEP-2575 server rejects the session-less POST whether it speaks Streamable HTTP or SSE, and + // McpClientImpl.ConnectAsync reads exactly those two statuses as "initialize-handshake server" and + // retries with initialize. The SSE GET can therefore only waste a round trip here. + [Theory] + [InlineData(HttpStatusCode.NotFound)] + [InlineData(HttpStatusCode.BadRequest)] + public async Task AutoDetectMode_SkipsSseFallback_WhenDiscoverProbeIsRejected(HttpStatusCode statusCode) + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect discover probe test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + var getCount = 0; + + mockHttpHandler.RequestHandler = request => + { + if (request.Method == HttpMethod.Get) + { + getCount++; + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.MethodNotAllowed)); + } + + return Task.FromResult(new HttpResponseMessage(statusCode) + { + Content = new StringContent("Not Found"), + }); + }; + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + var ex = await Assert.ThrowsAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + Assert.Equal(0, getCount); + Assert.Equal(statusCode, ex.Data["ModelContextProtocol.HttpStatusCode"]); + Assert.DoesNotContain( + MockLoggerProvider.LogMessages, + m => m.Message.Contains("falling back to SSE transport")); + } + + // Skipping the SSE attempt on the discover probe must not strand an SSE-only server: the initialize + // retry that McpClientImpl.ConnectAsync issues next goes through this same transport and still falls + // back to SSE, so such a server is reached one POST later rather than not at all. + [Fact] + public async Task AutoDetectMode_StillFallsBackToSse_WhenInitializeFollowsRejectedDiscoverProbe() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect discover probe then initialize test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + var ssePipe = new Pipe(); + var sseEndpointPostCount = 0; + + await ssePipe.Writer.WriteAsync( + System.Text.Encoding.UTF8.GetBytes("event: endpoint\r\ndata: /sse-endpoint\r\n\r\n"), + TestContext.Current.CancellationToken); + await ssePipe.Writer.FlushAsync(TestContext.Current.CancellationToken); + + mockHttpHandler.RequestHandler = request => + { + if (request.Method == HttpMethod.Get) + { + var content = new StreamContent(ssePipe.Reader.AsStream()); + content.Headers.ContentType = new("text/event-stream"); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = content }); + } + + if (request.RequestUri?.AbsolutePath == "/sse-endpoint") + { + sseEndpointPostCount++; + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.Accepted)); + } + + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.NotFound) + { + Content = new StringContent("Mcp-Session-Id required"), + }); + }; + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + await Assert.ThrowsAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + await session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(2) }, + TestContext.Current.CancellationToken); + + Assert.Equal(1, sseEndpointPostCount); + + await ssePipe.Writer.CompleteAsync(); + } + + // The skip is scoped to the two statuses ConnectAsync acts on. Any other failure on the discover probe + // keeps the original fallback, because it is not evidence that an initialize retry is coming. + [Fact] + public async Task AutoDetectMode_FallsBackToSse_WhenDiscoverProbeFailsWithUnrelatedStatus() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect discover probe 415 test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + var getCount = 0; + + mockHttpHandler.RequestHandler = request => + { + if (request.Method == HttpMethod.Get) + { + getCount++; + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.MethodNotAllowed)); + } + + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.UnsupportedMediaType) + { + Content = new StringContent("Content-Type must be 'application/json'"), + }); + }; + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + await Assert.ThrowsAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + Assert.Equal(1, getCount); + } }