fix(client): classify discover outcome at source, not at the error type (#1133)
`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 #1040.
This commit is contained in:
ip2a
GitHub
parent
02c62aef2e
commit
22be2cc5ad
2 changed files with 468 additions and 45 deletions
|
|
@ -125,11 +125,18 @@ async fn test_client_init_handles_jsonrpc_error() {
|
|||
});
|
||||
|
||||
tokio::spawn(async move {
|
||||
let _init_request = server.receive().await;
|
||||
let request = server.receive().await;
|
||||
// Echo the request's own id back on the error so it correlates: an
|
||||
// uncorrelated id would surface as `UncorrelatedErrorResponse`
|
||||
// instead of the `JsonRpcError` this test exercises.
|
||||
let request_id = request
|
||||
.and_then(|message| message.into_request())
|
||||
.map(|(_, id)| id)
|
||||
.expect("client sent an initialize request");
|
||||
|
||||
let error_msg = ServerJsonRpcMessage::Error(JsonRpcError {
|
||||
jsonrpc: JsonRpcVersion2_0,
|
||||
id: Some(RequestId::Number(1)),
|
||||
id: Some(request_id),
|
||||
error: ErrorData {
|
||||
code: ErrorCode(-32600),
|
||||
message: Cow::Borrowed("Invalid Request"),
|
||||
|
|
|
|||
|
|
@ -337,6 +337,334 @@ async fn auto_startup_falls_back_after_discover_method_not_found() {
|
|||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
/// Drives an `Auto` client through a single `server/discover` probe and asserts
|
||||
/// the legacy fallback decision against the response the server sends back.
|
||||
///
|
||||
/// When `expect_fallback` is set, the server also accepts the subsequent
|
||||
/// `initialize` request and the client is expected to connect. Otherwise the
|
||||
/// client must surface the discover error without sending `initialize`, and the
|
||||
/// server's next receive must not be an initialize request.
|
||||
async fn run_auto_discover_response_scenario(error: ErrorData, expect_fallback: bool) {
|
||||
let (server_transport, client_transport) = tokio::io::duplex(4096);
|
||||
let mut server = IntoTransport::<rmcp::RoleServer, _, _>::into_transport(server_transport);
|
||||
let server_task = tokio::spawn(async move {
|
||||
let ClientJsonRpcMessage::Request(discover) =
|
||||
server.receive().await.expect("expected discover request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
assert!(matches!(
|
||||
discover.request,
|
||||
ClientRequest::DiscoverRequest(_)
|
||||
));
|
||||
server
|
||||
.send(ServerJsonRpcMessage::error(error, Some(discover.id)))
|
||||
.await
|
||||
.expect("send discover error response");
|
||||
|
||||
if expect_fallback {
|
||||
let ClientJsonRpcMessage::Request(initialize) =
|
||||
server.receive().await.expect("expected initialize request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
assert!(matches!(
|
||||
initialize.request,
|
||||
ClientRequest::InitializeRequest(_)
|
||||
));
|
||||
server
|
||||
.send(ServerJsonRpcMessage::response(
|
||||
ServerResult::InitializeResult(InitializeResult::new(
|
||||
ServerCapabilities::default(),
|
||||
)),
|
||||
initialize.id,
|
||||
))
|
||||
.await
|
||||
.expect("send initialize response");
|
||||
assert!(matches!(
|
||||
server.receive().await,
|
||||
Some(ClientJsonRpcMessage::Notification(_))
|
||||
));
|
||||
} else {
|
||||
// The client must surface the error without falling back, so no
|
||||
// initialize request should follow. The transport closes when the
|
||||
// failed client is dropped.
|
||||
if let Some(ClientJsonRpcMessage::Request(request)) = server.receive().await {
|
||||
panic!(
|
||||
"client fell back to {:?} but should have surfaced the modern error",
|
||||
request.request
|
||||
);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let client_result = DiscoverClient
|
||||
.serve_with_lifecycle(
|
||||
client_transport,
|
||||
ClientLifecycleMode::Auto {
|
||||
preferred_versions: vec![ProtocolVersion::V_2026_07_28],
|
||||
legacy_version: Some(ProtocolVersion::V_2025_11_25),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
if expect_fallback {
|
||||
let client = client_result.expect("auto client should fall back to initialize");
|
||||
client.cancel().await.expect("cancel client");
|
||||
} else {
|
||||
assert!(
|
||||
client_result.is_err(),
|
||||
"modern error should surface without legacy fallback"
|
||||
);
|
||||
}
|
||||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_falls_back_after_discover_invalid_request() {
|
||||
// Legacy servers commonly reject an unknown pre-initialize request with
|
||||
// `-32600` (e.g. a session middleware that requires `initialize` first).
|
||||
run_auto_discover_response_scenario(
|
||||
ErrorData::new(ErrorCode::INVALID_REQUEST, "Bad Request", None),
|
||||
true,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_falls_back_after_discover_invalid_params() {
|
||||
// `-32602` is explicitly called out by the specification as an
|
||||
// implementation-defined response legacy servers use for unknown requests.
|
||||
run_auto_discover_response_scenario(
|
||||
ErrorData::new(ErrorCode::INVALID_PARAMS, "Invalid params", None),
|
||||
true,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_does_not_fall_back_for_missing_required_capability() {
|
||||
// A `MISSING_REQUIRED_CLIENT_CAPABILITY` response identifies a modern
|
||||
// server; falling back to `initialize` would not address it.
|
||||
run_auto_discover_response_scenario(
|
||||
ErrorData::new(
|
||||
ErrorCode::MISSING_REQUIRED_CLIENT_CAPABILITY,
|
||||
"Missing required client capability",
|
||||
None,
|
||||
),
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_does_not_fall_back_for_header_mismatch() {
|
||||
// A `HEADER_MISMATCH` response identifies a modern server performing
|
||||
// header validation; falling back to `initialize` would not address it.
|
||||
run_auto_discover_response_scenario(
|
||||
ErrorData::new(ErrorCode::HEADER_MISMATCH, "Header mismatch", None),
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_preserves_both_errors_when_fallback_also_fails() {
|
||||
let (server_transport, client_transport) = tokio::io::duplex(4096);
|
||||
let mut server = IntoTransport::<rmcp::RoleServer, _, _>::into_transport(server_transport);
|
||||
let server_task = tokio::spawn(async move {
|
||||
// Discover: legacy rejection.
|
||||
let ClientJsonRpcMessage::Request(discover) =
|
||||
server.receive().await.expect("expected discover request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
server
|
||||
.send(ServerJsonRpcMessage::error(
|
||||
ErrorData::new(ErrorCode::METHOD_NOT_FOUND, "not found", None),
|
||||
Some(discover.id),
|
||||
))
|
||||
.await
|
||||
.expect("send discover error");
|
||||
|
||||
// Initialize: close the transport instead of responding.
|
||||
let ClientJsonRpcMessage::Request(_) =
|
||||
server.receive().await.expect("expected initialize request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
drop(server); // closes the transport
|
||||
});
|
||||
|
||||
let result = DiscoverClient
|
||||
.serve_with_lifecycle(
|
||||
client_transport,
|
||||
ClientLifecycleMode::Auto {
|
||||
preferred_versions: vec![ProtocolVersion::V_2026_07_28],
|
||||
legacy_version: Some(ProtocolVersion::V_2025_11_25),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
let err = result.err().expect("both phases should fail");
|
||||
match err {
|
||||
rmcp::service::ClientInitializeError::LegacyFallbackFailed { discover, fallback } => {
|
||||
assert!(
|
||||
matches!(
|
||||
*discover,
|
||||
rmcp::service::ClientInitializeError::JsonRpcError(_)
|
||||
),
|
||||
"discover phase should be a JsonRpcError, got {discover:?}"
|
||||
);
|
||||
assert!(
|
||||
matches!(
|
||||
*fallback,
|
||||
rmcp::service::ClientInitializeError::ConnectionClosed(_)
|
||||
),
|
||||
"fallback phase should be ConnectionClosed, got {fallback:?}"
|
||||
);
|
||||
}
|
||||
other => panic!("expected LegacyFallbackFailed, got {other:?}"),
|
||||
}
|
||||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_surfaces_uncorrelated_error_id() {
|
||||
let (server_transport, client_transport) = tokio::io::duplex(4096);
|
||||
let mut server = IntoTransport::<rmcp::RoleServer, _, _>::into_transport(server_transport);
|
||||
let server_task = tokio::spawn(async move {
|
||||
let ClientJsonRpcMessage::Request(_) =
|
||||
server.receive().await.expect("expected discover request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
// Respond with an error carrying a different id.
|
||||
server
|
||||
.send(ServerJsonRpcMessage::error(
|
||||
ErrorData::new(ErrorCode::METHOD_NOT_FOUND, "not found", None),
|
||||
Some(RequestId::Number(999)),
|
||||
))
|
||||
.await
|
||||
.expect("send mismatched-id error");
|
||||
});
|
||||
|
||||
let result = DiscoverClient
|
||||
.serve_with_lifecycle(
|
||||
client_transport,
|
||||
ClientLifecycleMode::Auto {
|
||||
preferred_versions: vec![ProtocolVersion::V_2026_07_28],
|
||||
legacy_version: Some(ProtocolVersion::V_2025_11_25),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
matches!(
|
||||
result,
|
||||
Err(rmcp::service::ClientInitializeError::UncorrelatedErrorResponse { .. })
|
||||
),
|
||||
"mismatched id should surface as UncorrelatedErrorResponse, got {:?}",
|
||||
result.as_ref().err()
|
||||
);
|
||||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_startup_falls_back_for_absent_error_id() {
|
||||
// Spec: error id is optional; a server that cannot read the request id
|
||||
// omits it. The error is still a response to our request, so it should
|
||||
// trigger legacy fallback.
|
||||
let (server_transport, client_transport) = tokio::io::duplex(4096);
|
||||
let mut server = IntoTransport::<rmcp::RoleServer, _, _>::into_transport(server_transport);
|
||||
let server_task = tokio::spawn(async move {
|
||||
let ClientJsonRpcMessage::Request(_discover) =
|
||||
server.receive().await.expect("expected discover request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
server
|
||||
.send(ServerJsonRpcMessage::error(
|
||||
ErrorData::new(ErrorCode::METHOD_NOT_FOUND, "not found", None),
|
||||
None,
|
||||
))
|
||||
.await
|
||||
.expect("send no-id error");
|
||||
|
||||
// Client should fall back to initialize.
|
||||
let ClientJsonRpcMessage::Request(initialize) =
|
||||
server.receive().await.expect("expected initialize request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
server
|
||||
.send(ServerJsonRpcMessage::response(
|
||||
ServerResult::InitializeResult(
|
||||
InitializeResult::new(ServerCapabilities::default()),
|
||||
),
|
||||
initialize.id,
|
||||
))
|
||||
.await
|
||||
.expect("send initialize response");
|
||||
assert!(matches!(
|
||||
server.receive().await,
|
||||
Some(ClientJsonRpcMessage::Notification(_))
|
||||
));
|
||||
});
|
||||
|
||||
let client = DiscoverClient
|
||||
.serve_with_lifecycle(
|
||||
client_transport,
|
||||
ClientLifecycleMode::Auto {
|
||||
preferred_versions: vec![ProtocolVersion::V_2026_07_28],
|
||||
legacy_version: Some(ProtocolVersion::V_2025_11_25),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect("absent-id error should trigger legacy fallback");
|
||||
client.cancel().await.expect("cancel client");
|
||||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn discover_mode_surfaces_legacy_error() {
|
||||
let (server_transport, client_transport) = tokio::io::duplex(4096);
|
||||
let mut server = IntoTransport::<rmcp::RoleServer, _, _>::into_transport(server_transport);
|
||||
let server_task = tokio::spawn(async move {
|
||||
let ClientJsonRpcMessage::Request(discover) =
|
||||
server.receive().await.expect("expected discover request")
|
||||
else {
|
||||
panic!("expected request");
|
||||
};
|
||||
server
|
||||
.send(ServerJsonRpcMessage::error(
|
||||
ErrorData::new(ErrorCode::METHOD_NOT_FOUND, "not found", None),
|
||||
Some(discover.id),
|
||||
))
|
||||
.await
|
||||
.expect("send discover error");
|
||||
});
|
||||
|
||||
let result = DiscoverClient
|
||||
.serve_with_lifecycle(
|
||||
client_transport,
|
||||
ClientLifecycleMode::Discover {
|
||||
preferred_versions: vec![ProtocolVersion::V_2026_07_28],
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
matches!(
|
||||
result,
|
||||
Err(rmcp::service::ClientInitializeError::JsonRpcError(_))
|
||||
),
|
||||
"Discover mode should surface a legacy error, not fall back, got {:?}",
|
||||
result.as_ref().err()
|
||||
);
|
||||
server_task.await.expect("server task");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn discover_startup_retries_a_mutually_supported_version() {
|
||||
let unsupported: ProtocolVersion =
|
||||
|
|
|
|||
Loading…
Reference in a new issue