diff --git a/src/Common/HttpResponseMessageExtensions.cs b/src/Common/HttpResponseMessageExtensions.cs index 05ef092ac..6acc6f7a3 100644 --- a/src/Common/HttpResponseMessageExtensions.cs +++ b/src/Common/HttpResponseMessageExtensions.cs @@ -23,25 +23,37 @@ public static async Task EnsureSuccessStatusCodeWithResponseBodyAsync(this HttpR { if (!response.IsSuccessStatusCode) { - string? responseBody = null; - try - { - using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(TimeSpan.FromSeconds(5)); - responseBody = await response.Content.ReadAsStringAsync(cts.Token).ConfigureAwait(false); + throw await CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false); + } + } - if (responseBody.Length > MaxResponseBodyLength) - { - responseBody = responseBody.Substring(0, MaxResponseBodyLength) + "..."; - } - } - catch + /// + /// Creates an for a non-success response, making a best-effort attempt to + /// include the server's response body in the exception message so the diagnostic isn't lost. + /// + /// The non-success HTTP response message. + /// The token to monitor for cancellation requests. + /// An with the response status and, when available, its body. + public static async Task CreateHttpRequestExceptionWithBodyAsync(HttpResponseMessage response, CancellationToken cancellationToken = default) + { + string? responseBody = null; + try + { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(TimeSpan.FromSeconds(5)); + responseBody = await response.Content.ReadAsStringAsync(cts.Token).ConfigureAwait(false); + + if (responseBody.Length > MaxResponseBodyLength) { - // Ignore all errors reading the response body (e.g., stream closed, timeout, cancellation) - we'll throw without it. + responseBody = responseBody.Substring(0, MaxResponseBodyLength) + "..."; } - - throw CreateHttpRequestException(response, responseBody); } + catch + { + // Ignore all errors reading the response body (e.g., stream closed, timeout, cancellation) - we'll throw without it. + } + + return CreateHttpRequestException(response, responseBody); } /// diff --git a/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs b/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs index 015d4048d..9c13054d3 100644 --- a/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs +++ b/src/ModelContextProtocol.Core/Client/AutoDetectingClientSessionTransport.cs @@ -93,11 +93,15 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can // Non-JSON-RPC error response: either the server doesn't speak MCP at all, or this // is an older deployment that expects the SSE transport (which establishes its // protocol via GET /sse rather than POST). Fall back to SSE per the original - // behavior. + // 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); + var streamableHttpError = await HttpResponseMessageExtensions.CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false); + await streamableHttpTransport.DisposeAsync().ConfigureAwait(false); - await InitializeSseTransportAsync(message, cancellationToken).ConfigureAwait(false); + await InitializeSseTransportAsync(message, streamableHttpError, cancellationToken).ConfigureAwait(false); } } catch when (ActiveTransport is null) @@ -110,7 +114,7 @@ private async Task InitializeAsync(JsonRpcMessage message, CancellationToken can } } - private async Task InitializeSseTransportAsync(JsonRpcMessage message, CancellationToken cancellationToken) + private async Task InitializeSseTransportAsync(JsonRpcMessage message, HttpRequestException? streamableHttpError, CancellationToken cancellationToken) { if (_options.KnownSessionId is not null) { @@ -128,6 +132,21 @@ private async Task InitializeSseTransportAsync(JsonRpcMessage message, Cancellat LogUsingSSE(_name); ActiveTransport = sseTransport; } + catch (Exception sseError) when (streamableHttpError is not null && sseError is not OperationCanceledException) + { + // SSE fallback also failed. Surface the original Streamable HTTP error as the primary failure so the + // user sees the real server diagnostic (e.g. 415 Unsupported Media Type) instead of the unrelated + // SSE-fallback error (e.g. a 405 from a Streamable-HTTP-only server that doesn't accept GET). Preserve + // the original status code and attach the SSE failure as the inner exception so neither is lost, and + // keep HttpRequestException as the surfaced type so existing callers can still catch it and read StatusCode. + await sseTransport.DisposeAsync().ConfigureAwait(false); + LogSseFallbackFailedAfterStreamableHttp(_name, sseError); +#if NET + throw new HttpRequestException(streamableHttpError.Message, sseError, streamableHttpError.StatusCode); +#else + throw new HttpRequestException(streamableHttpError.Message, sseError); +#endif + } catch { await sseTransport.DisposeAsync().ConfigureAwait(false); @@ -166,4 +185,7 @@ public async ValueTask DisposeAsync() [LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} using SSE transport.")] private partial void LogUsingSSE(string endpointName); -} \ No newline at end of file + + [LoggerMessage(Level = LogLevel.Warning, Message = "{EndpointName} SSE fallback failed after Streamable HTTP also failed; surfacing both errors.")] + private partial void LogSseFallbackFailedAfterStreamableHttp(string endpointName, Exception sseError); +} diff --git a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs index 768ebf7ea..68735d6e3 100644 --- a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs @@ -1,5 +1,7 @@ using ModelContextProtocol.Client; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Tests.Utils; +using Microsoft.Extensions.Logging; using System.Net; namespace ModelContextProtocol.Tests.Transport; @@ -42,12 +44,12 @@ public async Task AutoDetectMode_UsesStreamableHttp_WhenServerSupportsIt() }; await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); - + // The auto-detecting transport should be returned Assert.NotNull(session); } - [Fact] + [Fact] public async Task AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails() { var options = new HttpClientTransportOptions @@ -102,8 +104,236 @@ public async Task AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails() }; await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); - + // The auto-detecting transport should be returned Assert.NotNull(session); } -} \ No newline at end of file + + // Regression test for https://github.com/modelcontextprotocol/csharp-sdk/issues/1526 + // When Streamable HTTP returns 415 (e.g. wrong Content-Type) and the SSE fallback also fails + // (e.g. a Streamable-HTTP-only server returns 405 to the GET), the surfaced exception must + // preserve the original Streamable HTTP error rather than dropping it on the floor. + [Fact] + public async Task AutoDetectMode_PreservesOriginalError_WhenStreamableHttpReturns415AndSseFallbackFails() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + + const string streamableHttpBody = "Content-Type must be 'application/json'"; + + mockHttpHandler.RequestHandler = (request) => + { + if (request.Method == HttpMethod.Post) + { + // Streamable HTTP fails with 415 - this is the real server diagnostic the user needs to see. + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.UnsupportedMediaType, + Content = new StringContent(streamableHttpBody), + }); + } + + if (request.Method == HttpMethod.Get) + { + // Streamable-HTTP-only server: SSE GET is rejected with 405. Without the fix this is the + // ONLY error the user ever sees, masking the real 415 diagnostic above. + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.MethodNotAllowed, + Content = new StringContent("Method not allowed"), + }); + } + + throw new InvalidOperationException($"Unexpected request: {request.Method}"); + }; + + // ConnectAsync only constructs the AutoDetect transport; the probe runs on the first SendMessageAsync. + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + var ex = await Assert.ThrowsAnyAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + // Walk the exception chain and assert the original 415 (and its body) is somewhere in it. + // We don't pin the exact exception type so this stays robust to future error-shape tweaks, + // but the underlying status code and server body must reach the caller. + var combined = Flatten(ex); + Assert.Contains("415", combined); + Assert.Contains(streamableHttpBody, combined); + + static string Flatten(Exception e) + { + var sb = new System.Text.StringBuilder(); + void Walk(Exception? cur) + { + while (cur is not null) + { + sb.Append(cur.GetType().FullName).Append(": ").AppendLine(cur.Message); + if (cur is AggregateException agg) + { + foreach (var inner in agg.InnerExceptions) + { + Walk(inner); + } + return; + } + cur = cur.InnerException; + } + } + Walk(e); + return sb.ToString(); + } + } + + // When Streamable HTTP fails (non-JSON-RPC) and the SSE fallback also fails, the surfaced exception must remain + // an HttpRequestException carrying the original Streamable HTTP status/body, with the SSE failure as its inner. + [Fact] + public async Task AutoDetectMode_SurfacesStreamableHttpError_WithSseAsInner_WhenSseFallbackFails() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + + const string streamableHttpBody = "Content-Type must be 'application/json'"; + + mockHttpHandler.RequestHandler = (request) => + { + if (request.Method == HttpMethod.Post) + { + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.UnsupportedMediaType, + Content = new StringContent(streamableHttpBody), + }); + } + + if (request.Method == HttpMethod.Get) + { + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.MethodNotAllowed, + Content = new StringContent("Method not allowed"), + }); + } + + throw new InvalidOperationException($"Unexpected request: {request.Method}"); + }; + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + var ex = await Assert.ThrowsAnyAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + // The surfaced exception is the original Streamable HTTP error (the real server diagnostic), not the SSE 405. + var httpEx = Assert.IsType(ex); + Assert.Contains("415", httpEx.Message); + Assert.Contains(streamableHttpBody, httpEx.Message); +#if NET + Assert.Equal(HttpStatusCode.UnsupportedMediaType, httpEx.StatusCode); +#endif + + // The SSE fallback failure (the 405 from the GET) is preserved as the inner exception, not dropped. + Assert.NotNull(httpEx.InnerException); + Assert.Contains("405", httpEx.InnerException.ToString()); + } + + // Cancellation during the AutoDetect probe must surface as an OperationCanceledException, not be masked or + // wrapped in the dual-failure AggregateException. + [Fact] + public async Task AutoDetectMode_SurfacesCancellation_WithoutWrapping() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + + mockHttpHandler.RequestHandler = (request) => + Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.UnsupportedMediaType, + Content = new StringContent("Content-Type must be 'application/json'"), + }); + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + var ex = await Assert.ThrowsAnyAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(1) }, + cts.Token)); + + Assert.IsNotType(ex); + Assert.IsAssignableFrom(ex); + } + + // The dual-failure case must be visible in logs at Warning level, even when callers swallow the exception. + [Fact] + public async Task AutoDetectMode_LogsWarning_WhenSseFallbackFailsAfterStreamableHttp() + { + var options = new HttpClientTransportOptions + { + Endpoint = new Uri("http://localhost"), + TransportMode = HttpTransportMode.AutoDetect, + Name = "AutoDetect test client" + }; + + using var mockHttpHandler = new MockHttpHandler(); + using var httpClient = new HttpClient(mockHttpHandler); + await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory); + + mockHttpHandler.RequestHandler = (request) => + { + if (request.Method == HttpMethod.Post) + { + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.UnsupportedMediaType, + Content = new StringContent("Content-Type must be 'application/json'"), + }); + } + + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.MethodNotAllowed, + Content = new StringContent("Method not allowed"), + }); + }; + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + await Assert.ThrowsAnyAsync(() => + session.SendMessageAsync( + new JsonRpcRequest { Method = RequestMethods.Initialize, Id = new RequestId(1) }, + TestContext.Current.CancellationToken)); + + Assert.Contains( + MockLoggerProvider.LogMessages, + m => m.LogLevel == LogLevel.Warning && m.Message.Contains("SSE fallback failed")); + } +}