fix(client): fall back from discover for any non-modern error - #1133
Conversation
1e29393 to
2af076e
Compare
|
Redesigned per your feedback. Replaced the Three calls I'd like your input on:
|
2af076e to
a81d9ed
Compare
|
Rewrote this to classify inside discover_startup instead of on the error The three open review comments are addressed by the rewrite:
|
| return Err(match &error.id { | ||
| Some(id) if expected_id.matches_response_id(id) => { | ||
| ClientInitializeError::JsonRpcError(error.error) | ||
| } |
There was a problem hiding this comment.
Response-scoped errors without an ID should remain available to the legacy classifier.
| } | |
| } | |
| None => ClientInitializeError::JsonRpcError(error.error), |
| Cancelled, | ||
|
|
||
| #[error("discover and legacy initialize both failed")] | ||
| LegacyFallbackFailed { |
There was a problem hiding this comment.
In ClientInitializeError::auth_challenge and ClientInitializeError::is_authorization_required, which drives reactive OAuth, the new wrapper hides fallback 401/403 challenges and causes authorization-required fallback errors to return false. Recurse into the fallback error so callers keep the documented classification behavior.
`ClientLifecycleMode::Auto` only fell back from `server/discover` on `-32601`, so legacy servers that reject the probe with other codes (`-32600`, `-32602`, implementation-defined errors) failed to connect even though `initialize` would have succeeded. The previous attempt (indicates_legacy_server) classified the failure after the fact by reverse-engineering the error type. This rewrite moves the classification into `discover_startup` itself, where the full context (request id, response correlation, transport state) is still available. `discover_startup` now returns `DiscoverOutcome`: `Modern` on success, `Legacy(error)` when the probe received a complete, correlated JSON-RPC error whose code is not a modern-era rejection. Every other failure becomes `Err`, so `Auto` simply matches the outcome — no methods on `ClientInitializeError`, no downcast, no transport-specific types leaking into the generic lifecycle layer. Additional fixes that fall out naturally: - Response correlation is now checked in `expect_response` for both success and error branches. Previously error responses skipped id correlation entirely. A new `UncorrelatedErrorResponse` variant surfaces responses that cannot be tied to the request. - When both discover and the legacy fallback fail, a `LegacyFallbackFailed` compound error preserves both phases instead of discarding the discover error. Fixes modelcontextprotocol#1040.
a81d9ed to
f3885a5
Compare
Fixes #1040.
Problem
ClientLifecycleMode::Autoonly fell back to the legacyinitializehandshake when
server/discoverfailed with-32601(METHOD_NOT_FOUND).Legacy servers commonly reject an unknown pre-
initializerequest with otherimplementation-defined errors (
-32600,-32602, session-middleware errors),so Auto broke against servers that previously worked.
Approach
Classification happens inside
discover_startup, where the full context(request id, response correlation, transport state) is still available — not
after the fact on
ClientInitializeError, where that context is gone.discover_startupreturns aDiscoverOutcome:Modern— discover succeeded.Legacy(error)— the probe received a complete, correlated JSON-RPC errorwhose code is not a modern-era rejection. The transport delivered a full
response and is ready for the next request, so a legacy
initializecanfollow on the same connection.
Err(error)— everything else (transport failure, uncorrelated response,modern rejection, client-side state). Surfaced, not retried.
Autosimply matches the outcome. No methods onClientInitializeError, nodowncast, no transport-specific types leaking into the lifecycle layer.
Two additional fixes that fall out naturally:
Response correlation:
expect_responsenow checks the request id onboth success and error responses. Previously error responses skipped id
correlation entirely. A new
UncorrelatedErrorResponsevariant surfacesresponses that cannot be tied to the request.
Fallback failure preservation: when both discover and the legacy
fallback fail, a
LegacyFallbackFailedcompound error preserves bothphases instead of discarding the discover error.
A silently legacy server that ignores the probe and hangs
expect_responseis a separate concern (needs a discover timeout); tracked in #1142.
Tests
The existing lifecycle tests verify the behavior end-to-end:
-32601,-32600, and-32602discover responses trigger fallback;-32021and-32020are surfaced. The client-initialization test was fixed to echo thereal request id (it previously hardcoded
1against an id provider thatstarts at
0, which only worked because error responses were nevercorrelated).
Scope
Non-JSON HTTP responses (e.g. a legacy server returning a plain-text 422) are
not covered by this PR. Recognizing them as a legacy signal requires the
transport layer to carry a structured HTTP-status signal across the transport
boundary, which is an architectural change best discussed separately.