-
Notifications
You must be signed in to change notification settings - Fork 751
fix(client): preserve underlying status code in AutoDetect probe #1530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On net472 this overload can't carry the status code, so callers on that target framework only get the status text in the exception message and not a programmatic StatusCode. That is acceptable given the framework limitation, but it means the preserved-status-code behavior is net5+ only. A one line note here would make it clear this is intentional rather than an oversight. |
||
| #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); | ||
| } | ||
|
|
||
| [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); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the body here is correct since it runs before the response is disposed. One thing worth a short comment: when the Streamable HTTP response is application/json but simply isn't a JSON-RPC error, TryReadJsonRpcErrorAsync above has already read the body once. HttpContent buffers after the first read so this second read returns the same buffered content and is safe, but a note would stop a future reader from mistaking it for a stream-consumption bug. For the common non-JSON error responses (415, 405, plain text) there is no double read because TryReadJsonRpcErrorAsync returns early on the content type.