fix(client): skip the SSE fallback when the server/discover probe is rejected with 400/404 (#1855)
Some checks failed
Build and Test / build (Debug, macos-latest) (push) Has been cancelled
Build and Test / build (Debug, ubuntu-latest) (push) Has been cancelled
Build and Test / build (Debug, windows-latest) (push) Has been cancelled
Build and Test / build (Release, macos-latest) (push) Has been cancelled
Build and Test / build (Release, ubuntu-latest) (push) Has been cancelled
Build and Test / build (Release, windows-latest) (push) Has been cancelled
CodeQL / Analyze (actions) (push) Has been cancelled
CodeQL / Analyze (csharp) (push) Has been cancelled
Build and Test / publish-coverage (push) Has been cancelled
Some checks failed
Build and Test / build (Debug, macos-latest) (push) Has been cancelled
Build and Test / build (Debug, ubuntu-latest) (push) Has been cancelled
Build and Test / build (Debug, windows-latest) (push) Has been cancelled
Build and Test / build (Release, macos-latest) (push) Has been cancelled
Build and Test / build (Release, ubuntu-latest) (push) Has been cancelled
Build and Test / build (Release, windows-latest) (push) Has been cancelled
CodeQL / Analyze (actions) (push) Has been cancelled
CodeQL / Analyze (csharp) (push) Has been cancelled
Build and Test / publish-coverage (push) Has been cancelled
This commit is contained in:
parent
324ccd83c3
commit
c40ee044fd
2 changed files with 183 additions and 2 deletions
|
|
@ -99,8 +99,6 @@ internal sealed partial class AutoDetectingClientSessionTransport : ITransport
|
||||||
// behavior. Capture the underlying error (status + body) before falling back so that,
|
// 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
|
// 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).
|
// 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
|
// This reads the response body a second time for the application/json case, where
|
||||||
// TryReadJsonRpcErrorAsync above already read it. HttpContent buffers after the first
|
// 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
|
// read, so this returns the same buffered content and is safe (not a second stream
|
||||||
|
|
@ -108,6 +106,26 @@ internal sealed partial class AutoDetectingClientSessionTransport : ITransport
|
||||||
// TryReadJsonRpcErrorAsync returns early on the content type, so there is no double read.
|
// TryReadJsonRpcErrorAsync returns early on the content type, so there is no double read.
|
||||||
var streamableHttpError = await HttpResponseMessageExtensions.CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false);
|
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 streamableHttpTransport.DisposeAsync().ConfigureAwait(false);
|
||||||
await InitializeSseTransportAsync(message, streamableHttpError, cancellationToken).ConfigureAwait(false);
|
await InitializeSseTransportAsync(message, streamableHttpError, cancellationToken).ConfigureAwait(false);
|
||||||
}
|
}
|
||||||
|
|
@ -122,6 +140,15 @@ internal sealed partial class AutoDetectingClientSessionTransport : ITransport
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <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)
|
private async Task InitializeSseTransportAsync(JsonRpcMessage message, HttpRequestException? streamableHttpError, CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
if (_options.KnownSessionId is not null)
|
if (_options.KnownSessionId is not null)
|
||||||
|
|
@ -184,6 +211,9 @@ internal sealed partial class AutoDetectingClientSessionTransport : ITransport
|
||||||
[LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} streamable HTTP transport failed with status code {StatusCode}, falling back to SSE transport.")]
|
[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);
|
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.")]
|
[LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} using Streamable HTTP transport.")]
|
||||||
private partial void LogUsingStreamableHttp(string endpointName);
|
private partial void LogUsingStreamableHttp(string endpointName);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -505,4 +505,155 @@ public class HttpClientTransportAutoDetectTests(ITestOutputHelper testOutputHelp
|
||||||
MockLoggerProvider.LogMessages,
|
MockLoggerProvider.LogMessages,
|
||||||
m => m.LogLevel == LogLevel.Warning && m.Message.Contains("SSE fallback failed"));
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue