From 056f1f5deb530105583373cf2008cf7a5a6b87bc Mon Sep 17 00:00:00 2001 From: Xue Cai Date: Sat, 14 Mar 2026 12:14:27 -0700 Subject: [PATCH] fix: swallow _receiveTask exceptions in SseClientSessionTransport.CloseAsync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When SseClientSessionTransport.ConnectAsync fails (e.g. server returns 405), the catch block calls CloseAsync() before wrapping the error in InvalidOperationException. However, CloseAsync() awaits the already-faulted _receiveTask without a try-catch, causing the original HttpRequestException to propagate out of CloseAsync and preempt the InvalidOperationException wrapping. Callers then receive a bare HttpRequestException instead of the documented InvalidOperationException("Failed to connect transport", ex). The fix wraps `await _receiveTask` in CloseAsync with a try-catch to swallow already-observed exceptions from the faulted task. Code references: SseClientSessionTransport.ConnectAsync (catch block calls CloseAsync): https://github.com/modelcontextprotocol/csharp-sdk/blob/v0.9.0-preview.2/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs#L52-L67 SseClientSessionTransport.CloseAsync (awaits _receiveTask without try-catch): https://github.com/modelcontextprotocol/csharp-sdk/blob/v0.9.0-preview.2/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs#L108-L130 Testing: # Build all frameworks (only netstandard2.0 fails — pre-existing on main): dotnet build tests/ModelContextProtocol.Tests/ '/p:NoWarn=NU1903%3BMCPEXP001' # Run full transport suite (74 tests × 3 frameworks = 222 total, 0 failures): dotnet test tests/ModelContextProtocol.Tests/ '/p:NoWarn=NU1903%3BMCPEXP001' --no-build --filter "FullyQualifiedName~Transport" --framework net10.0 dotnet test tests/ModelContextProtocol.Tests/ '/p:NoWarn=NU1903%3BMCPEXP001' --no-build --filter "FullyQualifiedName~Transport" --framework net9.0 dotnet test tests/ModelContextProtocol.Tests/ '/p:NoWarn=NU1903%3BMCPEXP001' --no-build --filter "FullyQualifiedName~Transport" --framework net8.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Client/SseClientSessionTransport.cs | 18 ++++++- .../HttpClientTransportAutoDetectTests.cs | 54 +++++++++++++++++++ .../Transport/HttpClientTransportTests.cs | 12 +++-- 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs b/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs index 0bcc69417..8e70b1582 100644 --- a/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs +++ b/src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs @@ -115,7 +115,23 @@ private async Task CloseAsync() { if (_receiveTask != null) { - await _receiveTask.ConfigureAwait(false); + // Swallow exceptions from _receiveTask so that callers (e.g. ConnectAsync's + // catch block) are not disrupted. The exception was already observed and + // forwarded via _connectionEstablished.TrySetException in ReceiveMessagesAsync. + try + { + await _receiveTask.ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // Expected during normal shutdown via _connectionCts cancellation. + } + catch (Exception) + { + // Already observed by ReceiveMessagesAsync — logged and set on + // _connectionEstablished. Swallowing here prevents the exception + // from escaping CloseAsync and preempting the caller's own throw. + } } } finally diff --git a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs index 768ebf7ea..d509c4baa 100644 --- a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs @@ -47,6 +47,60 @@ public async Task AutoDetectMode_UsesStreamableHttp_WhenServerSupportsIt() Assert.NotNull(session); } + [Fact] + public async Task AutoDetectMode_WhenBothTransportsFail_ThrowsInvalidOperationException() + { + // Regression test: when Streamable HTTP POST fails (e.g. 403) and the SSE GET + // fallback also fails (e.g. 405), ConnectAsync should wrap the error in an + // InvalidOperationException. Previously, CloseAsync() would re-throw the + // HttpRequestException from the faulted _receiveTask, preempting the wrapping. + 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) + { + // Streamable HTTP POST fails with 403 (auth error) + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.Forbidden, + Content = new StringContent("Forbidden") + }); + } + + if (request.Method == HttpMethod.Get) + { + // SSE GET fallback fails with 405 + return Task.FromResult(new HttpResponseMessage + { + StatusCode = HttpStatusCode.MethodNotAllowed, + Content = new StringContent("Method Not Allowed") + }); + } + + throw new InvalidOperationException($"Unexpected request: {request.Method}"); + }; + + // ConnectAsync for AutoDetect mode just creates the transport without sending + // any HTTP request. The auto-detection is triggered lazily by the first + // SendMessageAsync call, which happens inside McpClient.CreateAsync when it + // sends the JSON-RPC "initialize" message. + var ex = await Assert.ThrowsAsync( + () => McpClient.CreateAsync(transport, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Equal("Failed to connect transport", ex.Message); + Assert.IsType(ex.InnerException); + } + [Fact] public async Task AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails() { diff --git a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportTests.cs index 60384d3c2..62be34fcd 100644 --- a/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/HttpClientTransportTests.cs @@ -78,8 +78,11 @@ public async Task ConnectAsync_Throws_Exception_On_Failure() throw new Exception("Test exception"); }; - var exception = await Assert.ThrowsAsync(() => transport.ConnectAsync(TestContext.Current.CancellationToken)); - Assert.Equal("Test exception", exception.Message); + // SseClientSessionTransport.ConnectAsync wraps errors in InvalidOperationException + var exception = await Assert.ThrowsAsync(() => transport.ConnectAsync(TestContext.Current.CancellationToken)); + Assert.Equal("Failed to connect transport", exception.Message); + Assert.IsType(exception.InnerException); + Assert.Equal("Test exception", exception.InnerException!.Message); Assert.Equal(1, retries); } @@ -101,7 +104,10 @@ public async Task ConnectAsync_Throws_HttpRequestException_With_ResponseBody_On_ }); }; - var httpException = await Assert.ThrowsAsync(() => transport.ConnectAsync(TestContext.Current.CancellationToken)); + // SseClientSessionTransport.ConnectAsync wraps errors in InvalidOperationException + var wrappedException = await Assert.ThrowsAsync(() => transport.ConnectAsync(TestContext.Current.CancellationToken)); + Assert.Equal("Failed to connect transport", wrappedException.Message); + var httpException = Assert.IsType(wrappedException.InnerException); Assert.Contains(errorDetails, httpException.Message); Assert.Contains("400", httpException.Message); #if NET