Fall back to initialize when server/discover fails at the HTTP layer - #1766
Fall back to initialize when server/discover fails at the HTTP layer#1766rbstp wants to merge 1 commit into
Conversation
d155944 to
5a3819d
Compare
5a3819d to
740af43
Compare
PranavSenthilnathan
left a comment
PranavSenthilnathan
left a comment
There was a problem hiding this comment.
Thanks for the thorough investigation here, particularly for identifying the AutoDetect channel-lifetime issue, the unstructured HTTP 400 case, and the cross-target status-code limitation.
I opened two prerequisite PRs:
- #1767 preserves HTTP status codes across all target frameworks and AutoDetect failures.
- #1768 keeps AutoDetect’s shared message channel open after a provisional SSE failure.
Please wait for those changes and then rebase this PR.
The resulting condition should fall back to initialize for plain or empty 400 Bad Request and 404 Not Found responses. Other HTTP errors should continue to surface.
Please also add plain/empty 400 fallback coverage for explicit Streamable HTTP and AutoDetect.
Note
This review comment was generated with GitHub Copilot.
| fallbackToInitialize = true; | ||
| } | ||
| #if NET | ||
| catch (HttpRequestException ex) when ( |
| Assert.Equal(Timeout.InfiniteTimeSpan, options.DiscoverProbeTimeout); | ||
| } | ||
|
|
||
| #if NET |
There was a problem hiding this comment.
Please remove this #if NET guard after #1767 merges so this coverage also runs on net472.
Note
Generated with GitHub Copilot.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Client_OnHttpNotFoundFromProbe_UnderAutoDetect_Surfaces_NoFallback() |
There was a problem hiding this comment.
After #1768 merges, please change this AutoDetect 404 test to expect successful fallback.
Note
Generated with GitHub Copilot.
2 participants
Treat an HTTP 404 from the SEP-2575
server/discoverprobe as evidence of an initialize-handshake server and fall back, instead of failing the connection. Scoped to the explicit Streamable HTTP transport — see the AutoDetect note below.Motivation and Context
Fixes #1765.
Under the 2026-07-28 revision (SEP-2575) a client with
ProtocolVersionunset probesserver/discoverand is expected to fall back to theinitializehandshake for down-level servers. That fallback is keyed entirely onMcpProtocolExceptionplus the probe timeout, so it only fires when the server answers with a JSON-RPC error.A Streamable HTTP server that requires
Mcp-Session-Idon every non-initialize POST rejects the session-lessserver/discoverat the HTTP layer instead — Datadog's hosted server answers404 "Invalid session ID". That surfaces asHttpRequestExceptionfromEnsureSuccessStatusCodeWithResponseBodyAsync, which no catch inConnectAsynchandles, so it escapes and the connection fails outright. The server is reachable and speaks 2025-11-25 happily; only the probe fails. This is a regression from 2.0.0 — the same endpoint and credentials connect fine on 1.4.0 and 1.4.1.StreamableHttpClientSessionTransportdeliberately surfaces non-400 statuses asHttpRequestExceptionfor back-compat, so the gap is at the connect layer and that is where this fixes it.Scope, and what this deliberately does not fix
This PR is intentionally narrow. Three adjacent gaps are real and left for follow-ups:
1.
AutoDetectis excluded.AutoDetectis the defaultTransportMode, but the fallback is gated off for it. Under AutoDetect a 404 causes the provisional Streamable transport to be disposed and an SSE GET attempted; when that fails,SseClientSessionTransport.SetDisconnectedcompletes the shared message channel. An initialize sent afterwards is answered by the server but its response can never be read, so the connect stalls untilInitializationTimeoutand then fails. Measured locally:ClientTransportClosedExceptionafter ~6s withinitializeReceived == true, versus an immediate, clear 404 without this change. Falling back there would be strictly worse than today's behavior, so it is gated out and pinned by a test. Fixing AutoDetect properly needs provisional SSE failure to leave the shared channel open, the wayStreamableHttpClientSessionTransportalready does — happy to open that as a separate issue/PR.2. Plain HTTP 400 is still unhandled.
docs/concepts/stateless/stateless.mdstates that a400carrying anything other than a structured-32022/-32021/-32020error should switch to theinitializeflow. A plain-text or empty 400 fails JSON-RPC parsing, becomesHttpRequestException, and escapes exactly like the 404 did. That documented case predates this PR and is not addressed here; 404 is fixed because it is the case with a reproducible public server behind it.3. netstandard2.0 / net472 do not get the fix.
HttpRequestException.StatusCodeis .NET 5+, andHttpResponseMessageExtensions.CreateHttpRequestExceptiondrops the status entirely on non-NET targets, so there is nothing to read. The catch and its tests are#if NET-guarded, matching the existing pattern inAutoDetectingClientSessionTransport.cs. A portable version would need an internal exception subtype carrying the status code on all TFMs — glad to do that instead if you'd prefer the fix apply everywhere.How Has This Been Tested?
Yes — in a real application as well as unit tests.
New tests in
July2026ProtocolFallbackTests, all driving a realHttpClientTransportoverMockHttpHandler:Client_OnHttpNotFoundFromProbe_FallsBackTo_Initialize— the fix. Verified both directions: fails on unmodifiedmainwith the production error, passes with the fix.Client_OnNonNotFoundHttpErrorFromProbe_Surfaces_NoFallback(500, 403) — guards the deliberate narrowing. Verified it fails if the catch is broadened to allHttpRequestException.Client_OnHttpNotFoundFromProbe_UnderAutoDetect_Surfaces_NoFallback— pins the AutoDetect exclusion above.Full suite, Release: 2344 passed, 1 failed. The failure is
DockerEverythingServerTests.Sampling_Sse_EverythingServer, which reproduces identically on unmodifiedmain.Debug and Release both run clean on the fallback suite (15/15). Called out because an earlier iteration of this fix tripped a
Debug.Asserton a closed channel.Live servers,
TransportMode = StreamableHttp, no pinnedProtocolVersion:404 Invalid session IDGitHub and Sentry genuinely support SEP-2575 and are unaffected.
Real application: a production Slack agent connecting to five MCP servers. Its Datadog provider failed to connect on stock 2.0.0 and connects with this patch, other four unchanged. A/B'd by swapping only
ModelContextProtocol.Core.dllunder the same application binary.Not verified: the net472 leg (cannot build .NET Framework locally) and the net8.0/net9.0 legs (only the 10.0 runtime is installed here). All guarded code is excluded from net472 by construction, so that translation unit is unchanged from
main.Breaking Changes
None. No public API change. The only behavioral difference is that a connect over an explicit Streamable HTTP transport which previously threw
HttpRequestExceptionon a 404 from the discover probe now falls back toinitializeand can succeed. Non-404 statuses, and all AutoDetect behavior, are unchanged.Types of changes
Checklist
Additional context
The
_transport is not AutoDetectingClientSessionTransportguard in the filter is a type check I'd rather not need. If you'd prefer, the alternative is to land the SSE channel-lifetime fix first and then apply the 404 fallback uniformly across both transport modes — that would be a cleaner end state, just a larger change. Happy to go that route instead.