Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,33 @@ 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
// consumption). For the common non-JSON error responses (415, 405, plain text)
// 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);
}
Expand All @@ -119,6 +137,15 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can
}
}

/// <summary>
/// Returns <see langword="true"/> when the failed request was the SEP-2575 <c>server/discover</c> probe and the
/// status is one of the two <see cref="McpClientImpl"/> already reads as "this server requires the initialize
/// handshake", meaning the SSE fallback cannot contribute anything the initialize retry won't.
/// </summary>
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)
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,4 +505,155 @@ await Assert.ThrowsAnyAsync<Exception>(() =>
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<HttpRequestException>(() =>
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<HttpRequestException>(() =>
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<HttpRequestException>(() =>
session.SendMessageAsync(
new JsonRpcRequest { Method = RequestMethods.ServerDiscover, Id = new RequestId(1) },
TestContext.Current.CancellationToken));

Assert.Equal(1, getCount);
}
}