FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(client): fall back from discover for any non-modern error by ip2a · Pull Request #1133 · modelcontextprotocol/rust-sdk · GitHub

/ rust-sdk Public

fix(client): fall back from discover for any non-modern error - #1133

Merged
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
ip2a:fix/auto-lifecycle-fallback
Aug 9, 2026
ConversationCommits (1)ChecksFiles changed
Merged

fix(client): fall back from discover for any non-modern error#1133
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
ip2a:fix/auto-lifecycle-fallback

Conversation

ip2a commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #1040.

Problem

ClientLifecycleMode::Auto only fell back to the legacy initialize
handshake when server/discover failed with -32601 (METHOD_NOT_FOUND).
Legacy servers commonly reject an unknown pre-initialize request with other
implementation-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_startup returns a DiscoverOutcome:

  • Modern — discover succeeded.
  • Legacy(error) — the probe received a complete, correlated JSON-RPC error
    whose code is not a modern-era rejection. The transport delivered a full
    response and is ready for the next request, so a legacy initialize can
    follow on the same connection.
  • Err(error) — everything else (transport failure, uncorrelated response,
    modern rejection, client-side state). Surfaced, not retried.

Auto simply matches the outcome. No methods on ClientInitializeError, no
downcast, no transport-specific types leaking into the lifecycle layer.

Two additional fixes that fall out naturally:

  • Response correlation: expect_response now checks the request id on
    both success and error responses. Previously error responses skipped id
    correlation entirely. A new UncorrelatedErrorResponse variant surfaces
    responses that cannot be tied to the request.

  • Fallback failure preservation: when both discover and the legacy
    fallback fail, a LegacyFallbackFailed compound error preserves both
    phases instead of discarding the discover error.

A silently legacy server that ignores the probe and hangs expect_response
is a separate concern (needs a discover timeout); tracked in #1142.

Tests

The existing lifecycle tests verify the behavior end-to-end: -32601,
-32600, and -32602 discover responses trigger fallback; -32021 and
-32020 are surfaced. The client-initialization test was fixed to echo the
real request id (it previously hardcoded 1 against an id provider that
starts at 0, which only worked because error responses were never
correlated).

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.

ip2a requested a review from a team as a code owner August 5, 2026 07:44
github-actions Bot added T-test Testing related changes T-core Core library changes T-service Service layer changes labels Aug 5, 2026
DaleSeo reviewed Aug 5, 2026
View reviewed changes
Comment thread crates/rmcp/src/service/client.rs Outdated
Comment thread crates/rmcp/src/service/client.rs Outdated
j45856021-dev mentioned this pull request Aug 5, 2026
Closed
9 tasks
ip2a force-pushed the fix/auto-lifecycle-fallback branch 2 times, most recently from 1e29393 to 2af076e Compare August 6, 2026 03:03

ip2a commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Redesigned per your feedback. Replaced the Err(_) + modern-code carve-out
with ClientInitializeError::indicates_legacy_server: an exhaustive match
that falls back only on legacy signals (non-modern JSON-RPC error, closed
connection, unexpected response, non-auth transport error) and surfaces the
rest (modern rejections, auth/scope gate, client state). Added
is_authorization_failure for the 403 gap; test_legacy_server_classification
pins each variant.

Three calls I'd like your input on:

  1. ConnectionClosed falls back. On stdio the transport is dead so
    legacy_startup's send fails and masks the original error; on HTTP a new
    initialize POST can still work. Kept fallback, lean that way but ok to
    surface instead.
  2. ExpectedInitResponse / ExpectedInitResult / ConflictInitResponseId
    fall back — rare, transport still alive, seemed worth a try.
  3. Non-auth TransportError (IO) is lumped with non-JSON 400; both fall
    back. Splitting needs a per-transport downcast, didn't seem worth it.

ip2a mentioned this pull request Aug 6, 2026
Closed
DaleSeo reviewed Aug 7, 2026
View reviewed changes
Comment thread crates/rmcp/src/service/client.rs Outdated
Comment thread crates/rmcp/src/service/client.rs Outdated
Comment thread crates/rmcp/src/service/client.rs Outdated
ip2a force-pushed the fix/auto-lifecycle-fallback branch from 2af076e to a81d9ed Compare August 8, 2026 01:24

ip2a commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Rewrote this to classify inside discover_startup instead of on the error
type. It now returns DiscoverOutcome::Modern or DiscoverOutcome::Legacy,
and Auto just matches on it — no indicates_legacy_server, no downcast,
no transport types in the lifecycle layer.

The three open review comments are addressed by the rewrite:

  • ConnectionClosed: doesn't enter the fallback path at all (it's not a
    JsonRpcError, so discover_startup returns Err and Auto surfaces it)
  • mismatched response id: expect_response now correlates ids on error
    responses too; UncorrelatedErrorResponse is surfaced, not retried
  • non-JSON HTTP responses: out of scope for this PR. Needs the transport
    to carry a structured signal, which I'd rather discuss separately

DaleSeo reviewed Aug 8, 2026
View reviewed changes
Comment thread crates/rmcp/src/service/client.rs
Comment thread crates/rmcp/src/service/client.rs
ip2a force-pushed the fix/auto-lifecycle-fallback branch from a81d9ed to f3885a5 Compare August 9, 2026 01:08
fix(client): classify discover outcome at source, not at the error type
e6ee5e0 
`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.
ip2a force-pushed the fix/auto-lifecycle-fallback branch from f3885a5 to e6ee5e0 Compare August 9, 2026 03:44
DaleSeo approved these changes Aug 9, 2026
View reviewed changes

DaleSeo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for addressing all my feedback, @ip2a!

DaleSeo merged commit 22be2cc into modelcontextprotocol:main Aug 9, 2026
22 checks passed
github-actions Bot mentioned this pull request Aug 9, 2026
Merged
nunoya-yuma mentioned this pull request Aug 11, 2026
Closed
jamadeo mentioned this pull request Aug 11, 2026
Closed
9 tasks
github-actions Bot mentioned this pull request Aug 18, 2026
Open
jamadeo mentioned this pull request Aug 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaleSeo DaleSeo approved these changes

Assignees

No one assigned

Labels

T-core Core library changes T-service Service layer changes T-test Testing related changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ClientLifecycleMode::Auto does not fall back for deployed legacy-server responses

2 participants

Back | FazBrowse Home | New Git URL