fix: fall back to legacy initialize on general errors - #1088
Conversation
github-actions
Bot
added
T-test
| // On stdio, legacy servers may reject an unknown pre-initialize request with | ||
| // any implementation-defined error (or not respond at all). Treat every | ||
| // failure not recognized as modern version negotiation as a legacy peer. | ||
| Err(_) => { |
There was a problem hiding this comment.
Should we also consider this in the issue?
There was a problem hiding this comment.
The fix might not be quite as straightforward as I'd hoped. The core of the issue is
A client that needs to interoperate with both kinds of servers detects the server’s era with transport-specific mechanics, specified in the binding pages:
- stdio: probe with server/discover and fall back on any error that is not a recognized modern error.
- Streamable HTTP: attempt a modern request and inspect the body of a 400 Bad Request before falling back.
from here
so, I think the proper fix will need to implement this slightly differently.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn auto_startup_falls_back_after_discover_invalid_params() { |
There was a problem hiding this comment.
Would it be worth adding a test to catch a future ClientInitializeError variant being silently swept into Err(_)?
|
I ran into issues form this in rcmp - flagged here: #1040 (comment) I think the relevant bit is other servers are giving transport level errors (rcmp and mcp-go flagged in that coment), and at a quick glance this pr looks like it retries on the same http worker, which would be dead I think. |
3 participants
fixes #1040
Motivation and Context
The fallback logic was too strict, only falling back on a single METHOD_NOT_FOUND error, while the spec states we should fall back on any error that does not indicate a modern server.
How Has This Been Tested?
Tested against FastMCP+goose
Breaking Changes
No
Types of changes
Checklist
Additional context