Skip to content

Commit 9b21522

Browse files
Xue CaiCopilot
andcommitted
Fix: swallow _receiveTask exceptions in SseClientSessionTransport.CloseAsync
When SseClientSessionTransport.ConnectAsync fails (e.g. the SSE GET returns 405), its catch block calls CloseAsync() before wrapping the error in InvalidOperationException("Failed to connect transport", ex). However, CloseAsync() awaits _receiveTask which is already faulted with the same HttpRequestException. Since there is no try-catch around that await, the exception propagates out of CloseAsync, out of the catch block, and the InvalidOperationException wrapping is never reached. This means callers of ConnectAsync receive an unwrapped HttpRequestException instead of the documented InvalidOperationException wrapper, which breaks exception filtering in downstream code. The fix wraps `await _receiveTask` in CloseAsync with a try-catch that swallows both OperationCanceledException (normal shutdown) and other exceptions (already observed and forwarded via _connectionEstablished.TrySetException in ReceiveMessagesAsync). Includes a regression test that verifies ConnectAsync throws InvalidOperationException (not HttpRequestException) when both Streamable HTTP POST and SSE GET fallback fail. Co-authored-by: Copilot <[email protected]>
1 parent aae77b1 commit 9b21522

File tree

2 files changed

+67
-1
lines changed

2 files changed

+67
-1
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: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,56 @@ 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+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
94+
() => transport.ConnectAsync(TestContext.Current.CancellationToken));
95+
96+
Assert.Equal("Failed to connect transport", ex.Message);
97+
Assert.IsType<HttpRequestException>(ex.InnerException);
98+
}
99+
50100
[Fact]
51101
public async Task AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails()
52102
{

0 commit comments

Comments
 (0)