Skip to content

Commit 056f1f5

Browse files
Xue CaiCopilot
andcommitted
fix: swallow _receiveTask exceptions in SseClientSessionTransport.CloseAsync
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>
1 parent aae77b1 commit 056f1f5

File tree

3 files changed

+80
-4
lines changed

3 files changed

+80
-4
lines changed

src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,23 @@ private async Task CloseAsync()
115115
{
116116
if (_receiveTask != null)
117117
{
118-
await _receiveTask.ConfigureAwait(false);
118+
// Swallow exceptions from _receiveTask so that callers (e.g. ConnectAsync's
119+
// catch block) are not disrupted. The exception was already observed and
120+
// forwarded via _connectionEstablished.TrySetException in ReceiveMessagesAsync.
121+
try
122+
{
123+
await _receiveTask.ConfigureAwait(false);
124+
}
125+
catch (OperationCanceledException)
126+
{
127+
// Expected during normal shutdown via _connectionCts cancellation.
128+
}
129+
catch (Exception)
130+
{
131+
// Already observed by ReceiveMessagesAsync — logged and set on
132+
// _connectionEstablished. Swallowing here prevents the exception
133+
// from escaping CloseAsync and preempting the caller's own throw.
134+
}
119135
}
120136
}
121137
finally

tests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,60 @@ public async Task AutoDetectMode_UsesStreamableHttp_WhenServerSupportsIt()
4747
Assert.NotNull(session);
4848
}
4949

50+
[Fact]
51+
public async Task AutoDetectMode_WhenBothTransportsFail_ThrowsInvalidOperationException()
52+
{
53+
// Regression test: when Streamable HTTP POST fails (e.g. 403) and the SSE GET
54+
// fallback also fails (e.g. 405), ConnectAsync should wrap the error in an
55+
// InvalidOperationException. Previously, CloseAsync() would re-throw the
56+
// HttpRequestException from the faulted _receiveTask, preempting the wrapping.
57+
var options = new HttpClientTransportOptions
58+
{
59+
Endpoint = new Uri("http://localhost"),
60+
TransportMode = HttpTransportMode.AutoDetect,
61+
Name = "AutoDetect test client"
62+
};
63+
64+
using var mockHttpHandler = new MockHttpHandler();
65+
using var httpClient = new HttpClient(mockHttpHandler);
66+
await using var transport = new HttpClientTransport(options, httpClient, LoggerFactory);
67+
68+
mockHttpHandler.RequestHandler = (request) =>
69+
{
70+
if (request.Method == HttpMethod.Post)
71+
{
72+
// Streamable HTTP POST fails with 403 (auth error)
73+
return Task.FromResult(new HttpResponseMessage
74+
{
75+
StatusCode = HttpStatusCode.Forbidden,
76+
Content = new StringContent("Forbidden")
77+
});
78+
}
79+
80+
if (request.Method == HttpMethod.Get)
81+
{
82+
// SSE GET fallback fails with 405
83+
return Task.FromResult(new HttpResponseMessage
84+
{
85+
StatusCode = HttpStatusCode.MethodNotAllowed,
86+
Content = new StringContent("Method Not Allowed")
87+
});
88+
}
89+
90+
throw new InvalidOperationException($"Unexpected request: {request.Method}");
91+
};
92+
93+
// ConnectAsync for AutoDetect mode just creates the transport without sending
94+
// any HTTP request. The auto-detection is triggered lazily by the first
95+
// SendMessageAsync call, which happens inside McpClient.CreateAsync when it
96+
// sends the JSON-RPC "initialize" message.
97+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
98+
() => McpClient.CreateAsync(transport, cancellationToken: TestContext.Current.CancellationToken));
99+
100+
Assert.Equal("Failed to connect transport", ex.Message);
101+
Assert.IsType<HttpRequestException>(ex.InnerException);
102+
}
103+
50104
[Fact]
51105
public async Task AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails()
52106
{

tests/ModelContextProtocol.Tests/Transport/HttpClientTransportTests.cs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,11 @@ public async Task ConnectAsync_Throws_Exception_On_Failure()
7878
throw new Exception("Test exception");
7979
};
8080

81-
var exception = await Assert.ThrowsAsync<Exception>(() => transport.ConnectAsync(TestContext.Current.CancellationToken));
82-
Assert.Equal("Test exception", exception.Message);
81+
// SseClientSessionTransport.ConnectAsync wraps errors in InvalidOperationException
82+
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => transport.ConnectAsync(TestContext.Current.CancellationToken));
83+
Assert.Equal("Failed to connect transport", exception.Message);
84+
Assert.IsType<Exception>(exception.InnerException);
85+
Assert.Equal("Test exception", exception.InnerException!.Message);
8386
Assert.Equal(1, retries);
8487
}
8588

@@ -101,7 +104,10 @@ public async Task ConnectAsync_Throws_HttpRequestException_With_ResponseBody_On_
101104
});
102105
};
103106

104-
var httpException = await Assert.ThrowsAsync<HttpRequestException>(() => transport.ConnectAsync(TestContext.Current.CancellationToken));
107+
// SseClientSessionTransport.ConnectAsync wraps errors in InvalidOperationException
108+
var wrappedException = await Assert.ThrowsAsync<InvalidOperationException>(() => transport.ConnectAsync(TestContext.Current.CancellationToken));
109+
Assert.Equal("Failed to connect transport", wrappedException.Message);
110+
var httpException = Assert.IsType<HttpRequestException>(wrappedException.InnerException);
105111
Assert.Contains(errorDetails, httpException.Message);
106112
Assert.Contains("400", httpException.Message);
107113
#if NET

0 commit comments

Comments
 (0)