Skip to content

Commit cff927f

Browse files
committed
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.
1 parent 8754e19 commit cff927f

2 files changed

Lines changed: 183 additions & 2 deletions

File tree

‎src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,15 +96,33 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can
9696
// behavior. Capture the underlying error (status + body) before falling back so that,
9797
// if SSE also fails, we can surface the real Streamable HTTP diagnostic to the caller
9898
// instead of dropping it on the floor (see https://github.com/modelcontextprotocol/csharp-sdk/issues/1526).
99-
LogStreamableHttpFailed(_name, response.StatusCode);
100-
10199
// This reads the response body a second time for the application/json case, where
102100
// TryReadJsonRpcErrorAsync above already read it. HttpContent buffers after the first
103101
// read, so this returns the same buffered content and is safe (not a second stream
104102
// consumption). For the common non-JSON error responses (415, 405, plain text)
105103
// TryReadJsonRpcErrorAsync returns early on the content type, so there is no double read.
106104
var streamableHttpError = await HttpResponseMessageExtensions.CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false);
107105

106+
if (IsDiscoverProbeRejection(message, response.StatusCode))
107+
{
108+
// The server/discover probe is protocol negotiation, not transport detection. A server
109+
// predating SEP-2575 rejects the session-less POST with 400 (can't parse the request) or
110+
// 404 (requires Mcp-Session-Id on every non-initialize POST) whether it speaks Streamable
111+
// HTTP or SSE, so neither status is evidence about which transport to use. McpClientImpl
112+
// .ConnectAsync treats exactly these two statuses as "initialize-handshake server" and
113+
// immediately retries with initialize on this same transport — and that attempt still
114+
// falls back to SSE, so an SSE-only server is reached one POST later rather than not at
115+
// all. Attempting SSE here instead spends a GET whose result is discarded on every
116+
// connect to a Streamable-HTTP-only server that predates SEP-2575, and logs a "falling
117+
// back to SSE transport" line that misreports settled protocol negotiation as a failure.
118+
LogSkippingSseFallbackForDiscoverProbe(_name, response.StatusCode);
119+
120+
await streamableHttpTransport.DisposeAsync().ConfigureAwait(false);
121+
throw streamableHttpError;
122+
}
123+
124+
LogStreamableHttpFailed(_name, response.StatusCode);
125+
108126
await streamableHttpTransport.DisposeAsync().ConfigureAwait(false);
109127
await InitializeSseTransportAsync(message, streamableHttpError, cancellationToken).ConfigureAwait(false);
110128
}
@@ -119,6 +137,15 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can
119137
}
120138
}
121139

140+
/// <summary>
141+
/// Returns <see langword="true"/> when the failed request was the SEP-2575 <c>server/discover</c> probe and the
142+
/// status is one of the two <see cref="McpClientImpl"/> already reads as "this server requires the initialize
143+
/// handshake", meaning the SSE fallback cannot contribute anything the initialize retry won't.
144+
/// </summary>
145+
private static bool IsDiscoverProbeRejection(JsonRpcMessage message, HttpStatusCode statusCode) =>
146+
statusCode is HttpStatusCode.BadRequest or HttpStatusCode.NotFound &&
147+
message is JsonRpcRequest { Method: RequestMethods.ServerDiscover };
148+
122149
private async Task InitializeSseTransportAsync(JsonRpcMessage message, HttpRequestException? streamableHttpError, CancellationToken cancellationToken)
123150
{
124151
if (_options.KnownSessionId is not null)
@@ -181,6 +208,9 @@ public async ValueTask DisposeAsync()
181208
[LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} streamable HTTP transport failed with status code {StatusCode}, falling back to SSE transport.")]
182209
private partial void LogStreamableHttpFailed(string endpointName, HttpStatusCode statusCode);
183210

211+
[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.")]
212+
private partial void LogSkippingSseFallbackForDiscoverProbe(string endpointName, HttpStatusCode statusCode);
213+
184214
[LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} using Streamable HTTP transport.")]
185215
private partial void LogUsingStreamableHttp(string endpointName);
186216

‎tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,4 +505,155 @@ await Assert.ThrowsAnyAsync<Exception>(() =>
505505
MockLoggerProvider.LogMessages,
506506
m => m.LogLevel == LogLevel.Warning && m.Message.Contains("SSE fallback failed"));
507507
}
508+
509+
// A 400/404 on the SEP-2575 server/discover probe is protocol negotiation, not transport detection:
510+
// a pre-SEP-2575 server rejects the session-less POST whether it speaks Streamable HTTP or SSE, and
511+
// McpClientImpl.ConnectAsync reads exactly those two statuses as "initialize-handshake server" and
512+
// retries with initialize. The SSE GET can therefore only waste a round trip here.
513+
[Theory]
514+
[InlineData(HttpStatusCode.NotFound)]
515+
[InlineData(HttpStatusCode.BadRequest)]
516+
public async Task AutoDetectMode_SkipsSseFallback_WhenDiscoverProbeIsRejected(HttpStatusCode statusCode)
517+
{
518+
var options = new HttpClientTransportOptions
519+
{
520+
Endpoint = new Uri("http://localhost"),
521+
TransportMode = HttpTransportMode.AutoDetect,
522+
Name = "AutoDetect discover probe test client"
523+
};
524+
525+
using var mockHttpHandler = new MockHttpHandler();
526+
using var httpClient = new HttpClient(mockHttpHandler);
527+
await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory);
528+
var getCount = 0;
529+
530+
mockHttpHandler.RequestHandler = request =>
531+
{
532+
if (request.Method == HttpMethod.Get)
533+
{
534+
getCount++;
535+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.MethodNotAllowed));
536+
}
537+
538+
return Task.FromResult(new HttpResponseMessage(statusCode)
539+
{
540+
Content = new StringContent("Not Found"),
541+
});
542+
};
543+
544+
await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken);
545+
546+
var ex = await Assert.ThrowsAsync<HttpRequestException>(() =>
547+
session.SendMessageAsync(
548+
new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) },
549+
TestContext.Current.CancellationToken));
550+
551+
Assert.Equal(0, getCount);
552+
Assert.Equal(statusCode, ex.Data["ModelContextProtocol.HttpStatusCode"]);
553+
Assert.DoesNotContain(
554+
MockLoggerProvider.LogMessages,
555+
m => m.Message.Contains("falling back to SSE transport"));
556+
}
557+
558+
// Skipping the SSE attempt on the discover probe must not strand an SSE-only server: the initialize
559+
// retry that McpClientImpl.ConnectAsync issues next goes through this same transport and still falls
560+
// back to SSE, so such a server is reached one POST later rather than not at all.
561+
[Fact]
562+
public async Task AutoDetectMode_StillFallsBackToSse_WhenInitializeFollowsRejectedDiscoverProbe()
563+
{
564+
var options = new HttpClientTransportOptions
565+
{
566+
Endpoint = new Uri("http://localhost"),
567+
TransportMode = HttpTransportMode.AutoDetect,
568+
Name = "AutoDetect discover probe then initialize test client"
569+
};
570+
571+
using var mockHttpHandler = new MockHttpHandler();
572+
using var httpClient = new HttpClient(mockHttpHandler);
573+
await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory);
574+
var ssePipe = new Pipe();
575+
var sseEndpointPostCount = 0;
576+
577+
await ssePipe.Writer.WriteAsync(
578+
System.Text.Encoding.UTF8.GetBytes("event: endpoint\r\ndata: /sse-endpoint\r\n\r\n"),
579+
TestContext.Current.CancellationToken);
580+
await ssePipe.Writer.FlushAsync(TestContext.Current.CancellationToken);
581+
582+
mockHttpHandler.RequestHandler = request =>
583+
{
584+
if (request.Method == HttpMethod.Get)
585+
{
586+
var content = new StreamContent(ssePipe.Reader.AsStream());
587+
content.Headers.ContentType = new("text/event-stream");
588+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = content });
589+
}
590+
591+
if (request.RequestUri?.AbsolutePath == "/sse-endpoint")
592+
{
593+
sseEndpointPostCount++;
594+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.Accepted));
595+
}
596+
597+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.NotFound)
598+
{
599+
Content = new StringContent("Mcp-Session-Id required"),
600+
});
601+
};
602+
603+
await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken);
604+
605+
await Assert.ThrowsAsync<HttpRequestException>(() =>
606+
session.SendMessageAsync(
607+
new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) },
608+
TestContext.Current.CancellationToken));
609+
610+
await session.SendMessageAsync(
611+
new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(2) },
612+
TestContext.Current.CancellationToken);
613+
614+
Assert.Equal(1, sseEndpointPostCount);
615+
616+
await ssePipe.Writer.CompleteAsync();
617+
}
618+
619+
// The skip is scoped to the two statuses ConnectAsync acts on. Any other failure on the discover probe
620+
// keeps the original fallback, because it is not evidence that an initialize retry is coming.
621+
[Fact]
622+
public async Task AutoDetectMode_FallsBackToSse_WhenDiscoverProbeFailsWithUnrelatedStatus()
623+
{
624+
var options = new HttpClientTransportOptions
625+
{
626+
Endpoint = new Uri("http://localhost"),
627+
TransportMode = HttpTransportMode.AutoDetect,
628+
Name = "AutoDetect discover probe 415 test client"
629+
};
630+
631+
using var mockHttpHandler = new MockHttpHandler();
632+
using var httpClient = new HttpClient(mockHttpHandler);
633+
await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory);
634+
var getCount = 0;
635+
636+
mockHttpHandler.RequestHandler = request =>
637+
{
638+
if (request.Method == HttpMethod.Get)
639+
{
640+
getCount++;
641+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.MethodNotAllowed));
642+
}
643+
644+
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.UnsupportedMediaType)
645+
{
646+
Content = new StringContent("Content-Type must be 'application/json'"),
647+
});
648+
};
649+
650+
await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken);
651+
652+
await Assert.ThrowsAsync<HttpRequestException>(() =>
653+
session.SendMessageAsync(
654+
new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) },
655+
TestContext.Current.CancellationToken));
656+
657+
Assert.Equal(1, getCount);
658+
}
508659
}

0 commit comments

Comments
 (0)