-
Notifications
You must be signed in to change notification settings - Fork 814
fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body #1849
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 |
|---|---|---|
|
|
@@ -220,7 +220,10 @@ public async Task Client_OnFallbackHttpStatusFromProbe_FallsBackTo_Initialize( | |
| // A server predating SEP-2575 can reject the session-less server/discover probe at the HTTP layer | ||
| // rather than with a JSON-RPC error: 404 when it requires Mcp-Session-Id on every non-initialize | ||
| // POST, or a plain/empty 400 when it cannot parse the request. Both are initialize-handshake | ||
| // servers, so the connect must fall back instead of failing. | ||
| // servers, so the connect must fall back instead of failing. (405 is deliberately excluded: the | ||
| // POST endpoint rejecting the request method does not mean initialize will succeed over the same | ||
| // transport, and the spec routes 405 to the AutoDetect transport's SSE fallback — see | ||
| // Client_On405FromProbe_DoesNotFallBackTo_Initialize.) | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeReceived = false; | ||
|
|
||
|
|
@@ -240,17 +243,20 @@ public async Task Client_OnFallbackHttpStatusFromProbe_FallsBackTo_Initialize( | |
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpTransportMode.AutoDetect)] | ||
| public async Task Client_OnStructuredInvalidRequestFromHttpProbe_FallsBackTo_Initialize( | ||
| HttpTransportMode transportMode) | ||
| [InlineData(HttpStatusCode.BadRequest, HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpStatusCode.BadRequest, HttpTransportMode.AutoDetect)] | ||
| [InlineData(HttpStatusCode.NotFound, HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpStatusCode.NotFound, HttpTransportMode.AutoDetect)] | ||
| public async Task Client_OnStructuredFallbackHttpStatusFromProbe_FallsBackTo_Initialize( | ||
| HttpStatusCode status, HttpTransportMode transportMode) | ||
| { | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeReceived = false; | ||
|
|
||
| using var mockHttpHandler = new MockHttpHandler(); | ||
| using var httpClient = new HttpClient(mockHttpHandler); | ||
| mockHttpHandler.RequestHandler = CreateStructuredInvalidRequestProbeServer( | ||
| mockHttpHandler.RequestHandler = CreateStructuredProbeRejectingServer( | ||
| status, | ||
| () => initializeReceived = true); | ||
|
|
||
| await using var transport = CreateTransport(httpClient, transportMode); | ||
|
|
@@ -261,23 +267,86 @@ public async Task Client_OnStructuredInvalidRequestFromHttpProbe_FallsBackTo_Ini | |
| Assert.Equal(McpProtocolVersions.November2025ProtocolVersion, client.NegotiatedProtocolVersion); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpTransportMode.StreamableHttp, false)] | ||
| [InlineData(HttpTransportMode.AutoDetect, true)] | ||
| public async Task Client_On405FromProbe_DoesNotFallBackTo_Initialize( | ||
| HttpTransportMode transportMode, bool expectSseAttempt) | ||
| { | ||
| // 405 means the POST endpoint rejected the request method, so retrying initialize over the same | ||
| // transport is not useful. The spec routes 405 to the AutoDetect transport's SSE fallback: in | ||
| // Streamable HTTP mode the 405 surfaces directly, and in AutoDetect mode the client attempts the | ||
| // deprecated SSE GET instead of initialize. Neither path may attempt initialize. | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeReceived = false; | ||
| var sseRequested = false; | ||
|
|
||
| using var mockHttpHandler = new MockHttpHandler(); | ||
| using var httpClient = new HttpClient(mockHttpHandler); | ||
| mockHttpHandler.RequestHandler = CreateProbeRejectingServer( | ||
| HttpStatusCode.MethodNotAllowed, "Invalid session ID", | ||
| () => initializeReceived = true, () => sseRequested = true); | ||
|
|
||
| await using var transport = CreateTransport(httpClient, transportMode); | ||
|
|
||
| await Assert.ThrowsAnyAsync<HttpRequestException>(async () => | ||
| { | ||
| await using var client = await McpClient.CreateAsync(transport, new McpClientOptions(), | ||
| loggerFactory: LoggerFactory, cancellationToken: ct); | ||
| }); | ||
|
|
||
| Assert.False(initializeReceived); | ||
| Assert.Equal(expectSseAttempt, sseRequested); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpTransportMode.AutoDetect)] | ||
| public async Task Client_OnStructured405FromProbe_DoesNotFallBackTo_Initialize( | ||
|
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. This currently codifies the original regression. AutoDetect should try SSE for an unrecognized JSON-RPC error on 405; only explicit Streamable HTTP should surface the 405 directly. Can we assert the GET/no-GET behavior here? Please avoid making "no initialize" a general invariant after successful SSE selection, since the revised #1719 will use the initialize handshake once SSE is selected. |
||
| HttpTransportMode transportMode) | ||
| { | ||
| // A 405 carrying a structured JSON-RPC error body means the peer is a Streamable HTTP server | ||
| // that rejected the method; the AutoDetect transport adopts the transport and surfaces the error | ||
| // instead of trying SSE, and neither transport should attempt initialize. | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeReceived = false; | ||
|
|
||
| using var mockHttpHandler = new MockHttpHandler(); | ||
| using var httpClient = new HttpClient(mockHttpHandler); | ||
| mockHttpHandler.RequestHandler = CreateStructuredProbeRejectingServer( | ||
| HttpStatusCode.MethodNotAllowed, () => initializeReceived = true); | ||
|
|
||
| await using var transport = CreateTransport(httpClient, transportMode); | ||
|
|
||
| await Assert.ThrowsAnyAsync<HttpRequestException>(async () => | ||
| { | ||
| await using var client = await McpClient.CreateAsync(transport, new McpClientOptions(), | ||
| loggerFactory: LoggerFactory, cancellationToken: ct); | ||
| }); | ||
|
|
||
| Assert.False(initializeReceived); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpStatusCode.InternalServerError, HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpStatusCode.Forbidden, HttpTransportMode.StreamableHttp)] | ||
| [InlineData(HttpStatusCode.InternalServerError, HttpTransportMode.AutoDetect)] | ||
| [InlineData(HttpStatusCode.Unauthorized, HttpTransportMode.AutoDetect)] | ||
| [InlineData(HttpStatusCode.Forbidden, HttpTransportMode.AutoDetect)] | ||
|
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. #1855 added a 415 regression that preserves the old SSE fallback. This PR intentionally changes that behavior, so that test will need to be updated or removed during the rebase. Can we add 415 here and cover both structured and unstructured non-allowlisted responses with an explicit no-GET assertion? |
||
| public async Task Client_OnOtherHttpErrorFromProbe_Surfaces_NoFallback( | ||
| HttpStatusCode status, HttpTransportMode transportMode) | ||
| { | ||
| // Only 400 and 404 are read as "this server needs the initialize handshake". Any other HTTP failure | ||
| // is a genuine transport error and must surface, so callers are not handed a misleading downstream | ||
| // error. Guards the deliberate narrowing of the status filter. | ||
| // Only 400 and 404 indicate that the server needs the initialize handshake (405 routes to the | ||
| // AutoDetect SSE fallback instead). Authentication and server failures must surface directly, | ||
| // without probing deprecated SSE or attempting initialize. | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeReceived = false; | ||
| var sseRequested = false; | ||
|
|
||
| using var mockHttpHandler = new MockHttpHandler(); | ||
| using var httpClient = new HttpClient(mockHttpHandler); | ||
| mockHttpHandler.RequestHandler = CreateProbeRejectingServer( | ||
| status, "nope", () => initializeReceived = true); | ||
| status, "nope", () => initializeReceived = true, () => sseRequested = true); | ||
|
|
||
| await using var transport = CreateTransport(httpClient, transportMode); | ||
|
|
||
|
|
@@ -288,6 +357,7 @@ await Assert.ThrowsAnyAsync<HttpRequestException>(async () => | |
| }); | ||
|
|
||
| Assert.False(initializeReceived); | ||
| Assert.False(sseRequested); | ||
| } | ||
|
|
||
| private HttpClientTransport CreateTransport(HttpClient httpClient, HttpTransportMode transportMode) | ||
|
|
@@ -303,13 +373,17 @@ private HttpClientTransport CreateTransport(HttpClient httpClient, HttpTransport | |
| /// and, if the client falls back, completes an <c>initialize</c> handshake at 2025-11-25. | ||
| /// </summary> | ||
| private static Func<HttpRequestMessage, Task<HttpResponseMessage>> CreateProbeRejectingServer( | ||
| HttpStatusCode probeStatus, string probeBody, Action onInitialize) | ||
| HttpStatusCode probeStatus, string probeBody, Action onInitialize, Action? onSseRequest = null) | ||
| => async request => | ||
| { | ||
| // The server offers no standalone SSE stream, which the spec permits. | ||
| // net472 does not populate a default Content, so every response sets one explicitly. | ||
| if (request.Method == HttpMethod.Get) | ||
| { | ||
| // Track accidental AutoDetect fallback for non-allowlisted HTTP failures. | ||
| onSseRequest?.Invoke(); | ||
| return EmptyResponse(HttpStatusCode.MethodNotAllowed); | ||
| } | ||
|
|
||
| var body = await request.Content!.ReadAsStringAsync(); | ||
| using var doc = JsonDocument.Parse(body); | ||
|
|
@@ -339,8 +413,8 @@ private static Func<HttpRequestMessage, Task<HttpResponseMessage>> CreateProbeRe | |
| } | ||
| }; | ||
|
|
||
| private static Func<HttpRequestMessage, Task<HttpResponseMessage>> CreateStructuredInvalidRequestProbeServer( | ||
| Action onInitialize) | ||
| private static Func<HttpRequestMessage, Task<HttpResponseMessage>> CreateStructuredProbeRejectingServer( | ||
| HttpStatusCode probeStatus, Action onInitialize) | ||
| => async request => | ||
| { | ||
| if (request.Method == HttpMethod.Get) | ||
|
|
@@ -356,7 +430,7 @@ private static Func<HttpRequestMessage, Task<HttpResponseMessage>> CreateStructu | |
| var id = doc.RootElement.GetProperty("id").GetRawText(); | ||
| var error = "{\"jsonrpc\":\"2.0\",\"id\":" + id | ||
| + ",\"error\":{\"code\":-32600,\"message\":\"Mcp-Session-Id header is required\"}}"; | ||
| return new HttpResponseMessage(HttpStatusCode.BadRequest) | ||
| return new HttpResponseMessage(probeStatus) | ||
| { | ||
| Content = new StringContent(error, Encoding.UTF8, "application/json"), | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
The original structured 405 still never reaches the new fallback check. Could we move
ShouldSurfaceJsonRpcErrorAsProtocolExceptioninto theelse ifcondition, so a parsed but unrecognized error falls through to the common HTTP classification?This preserves recognized modern errors, fixes the advertised JSON-RPC-bodied 405 case, and keeps #1855’s
server/discover400/404 behavior.