From 9412d6cf5941dfb2831885b2f767812b9794a7f5 Mon Sep 17 00:00:00 2001 From: Camille Lawrence Date: Wed, 14 Oct 2026 21:44:12 +0200 Subject: [PATCH 1/5] fix(client): answer a server ping during the initialize handshake expect_response dropped a server ping while waiting for the initialize response, so the ping never got a reply. Answer it with an empty result during the initialize wait, the same way the server answers an early client ping. The server/discover wait still leaves it unanswered. Refs #745 Fixes #1362 --- crates/rmcp/src/service/client.rs | 48 ++++++++++++-- .../rmcp/tests/test_client_initialization.rs | 54 ++++++++++++++- .../rmcp/tests/test_client_lifecycle_modes.rs | 65 ++++++++++++++++++- 3 files changed, 157 insertions(+), 10 deletions(-) diff --git a/crates/rmcp/src/service/client.rs b/crates/rmcp/src/service/client.rs index 5dad6554a..dc11b2843 100644 --- a/crates/rmcp/src/service/client.rs +++ b/crates/rmcp/src/service/client.rs @@ -165,15 +165,20 @@ where /// success response is `ConflictInitResponseId`; on an error response (whose /// `id` is optional per spec) it is `UncorrelatedErrorResponse`. The caller /// never sees an uncorrelated response. +/// +/// A server ping is answered only when `answer_ping` is set. The discover +/// wait, including Auto's timeout-bounded probe, leaves it unanswered: +/// 2026-07-28 has no ping, and a cancelled send could leave a partial frame. async fn expect_response( transport: &mut T, context: &str, service: &S, peer: Peer, expected_id: &RequestId, + answer_ping: bool, ) -> Result where - T: Transport, + T: Transport + 'static, S: Service, { loop { @@ -231,10 +236,25 @@ where } } // Server could send pings before handshake - ServerJsonRpcMessage::Request(ref request) + ServerJsonRpcMessage::Request(request) if matches!(request.request, ServerRequest::PingRequest(_)) => { - tracing::trace!("Received ping request. Ignored.") + if answer_ping { + transport + .send(ClientJsonRpcMessage::response( + ClientResult::empty(()), + request.id, + )) + .await + .map_err(|error| { + ClientInitializeError::transport::( + error, + "send pre-init ping response", + ) + })?; + } else { + tracing::trace!("Received ping request. Ignored.") + } } // Server SHOULD NOT send any other messages before handshake. We ignore them anyway _ => tracing::warn!(?message, "Received unexpected message"), @@ -915,8 +935,15 @@ where context: "send initialize request".into(), })?; - let response = - expect_response(transport, "initialize response", service, peer.clone(), &id).await?; + let response = expect_response( + transport, + "initialize response", + service, + peer.clone(), + &id, + true, + ) + .await?; let ServerResult::InitializeResult(initialize_result) = response else { return Err(ClientInitializeError::ExpectedInitResult(Some(response))); @@ -986,7 +1013,16 @@ where ClientInitializeError::transport::(error, "send discover request") })?; - match expect_response(transport, "discover response", service, peer.clone(), &id).await { + match expect_response( + transport, + "discover response", + service, + peer.clone(), + &id, + false, + ) + .await + { Ok(ServerResult::DiscoverResult(result)) => { let Some(selected) = select_protocol_version(&preferred_versions, &result.supported_versions) diff --git a/crates/rmcp/tests/test_client_initialization.rs b/crates/rmcp/tests/test_client_initialization.rs index 0ca0182f9..e3990fc02 100644 --- a/crates/rmcp/tests/test_client_initialization.rs +++ b/crates/rmcp/tests/test_client_initialization.rs @@ -9,8 +9,9 @@ use common::handlers::TestClientHandler; use rmcp::{ ServiceExt, model::{ - ClientJsonRpcMessage, ErrorCode, ErrorData, InitializeResult, JsonRpcError, - JsonRpcVersion2_0, RequestId, ServerCapabilities, ServerJsonRpcMessage, ServerResult, + ClientConfig, ClientJsonRpcMessage, ClientResult, ErrorCode, ErrorData, InitializeResult, + JsonRpcError, JsonRpcVersion2_0, PingRequest, ProtocolVersion, RequestId, + ServerCapabilities, ServerJsonRpcMessage, ServerRequest, ServerResult, }, transport::{IntoTransport, Transport}, }; @@ -157,3 +158,52 @@ async fn test_client_init_handles_jsonrpc_error() { _ => panic!("Expected ClientInitializeError::JsonRpcError"), } } + +#[tokio::test] +async fn client_answers_server_ping_during_initialize() { + let (server_transport, client_transport) = tokio::io::duplex(4096); + let mut server = IntoTransport::::into_transport(server_transport); + let server_task = tokio::spawn(async move { + let ClientJsonRpcMessage::Request(initialize) = + server.receive().await.expect("expected initialize request") + else { + panic!("expected initialize request"); + }; + let ping_id = RequestId::String("server-ping".into()); + server + .send(ServerJsonRpcMessage::request( + ServerRequest::PingRequest(PingRequest::default()), + ping_id.clone(), + )) + .await + .expect("send ping"); + server + .send(ServerJsonRpcMessage::response( + ServerResult::InitializeResult( + InitializeResult::new(ServerCapabilities::default()) + .with_protocol_version(ProtocolVersion::LATEST_WITH_INITIALIZE), + ), + initialize.id, + )) + .await + .expect("send initialize response"); + + let Some(ClientJsonRpcMessage::Response(response)) = server.receive().await else { + panic!("expected the ping response before notifications/initialized"); + }; + assert_eq!(response.id, ping_id); + assert!(matches!(response.result, ClientResult::EmptyResult(_))); + assert!(matches!( + server.receive().await, + Some(ClientJsonRpcMessage::Notification(_)) + )); + }); + + let client = ClientConfig::default() + .with_protocol_version(ProtocolVersion::LATEST_WITH_INITIALIZE) + .serve(client_transport) + .await + .expect("initialize client"); + client.cancel().await.expect("cancel client"); + server_task.await.expect("server task"); +} diff --git a/crates/rmcp/tests/test_client_lifecycle_modes.rs b/crates/rmcp/tests/test_client_lifecycle_modes.rs index 5fa2cd191..373f43118 100644 --- a/crates/rmcp/tests/test_client_lifecycle_modes.rs +++ b/crates/rmcp/tests/test_client_lifecycle_modes.rs @@ -4,8 +4,8 @@ use rmcp::{ ClientHandler, ClientLifecycleMode, ClientServiceExt, ServerHandler, ServiceExt, model::{ ClientJsonRpcMessage, ClientRequest, DiscoverResult, ErrorCode, ErrorData, GetMeta, - Implementation, InitializeResult, ProtocolVersion, RequestId, ServerCapabilities, - ServerJsonRpcMessage, ServerResult, + Implementation, InitializeResult, PingRequest, ProtocolVersion, RequestId, + ServerCapabilities, ServerJsonRpcMessage, ServerRequest, ServerResult, }, service::PeerRequestOptions, transport::{IntoTransport, Transport}, @@ -277,6 +277,67 @@ async fn discover_startup_omits_initialize() { server_task.await.expect("server task"); } +// 2026-07-28 has no ping and servers send no requests before discover +// completes, so a stray ping stays unanswered. +#[tokio::test] +async fn discover_startup_ignores_server_ping() { + let (server_transport, client_transport) = tokio::io::duplex(4096); + let mut server = IntoTransport::::into_transport(server_transport); + let server_task = tokio::spawn(async move { + let ClientJsonRpcMessage::Request(request) = + server.receive().await.expect("expected discover request") + else { + panic!("expected discover request"); + }; + server + .send(ServerJsonRpcMessage::request( + ServerRequest::PingRequest(PingRequest::default()), + RequestId::String("server-ping".into()), + )) + .await + .expect("send ping"); + server + .send(ServerJsonRpcMessage::response( + ServerResult::DiscoverResult(DiscoverResult::new( + vec![ProtocolVersion::V_2026_07_28], + ServerCapabilities::default(), + )), + request.id, + )) + .await + .expect("send discover response"); + + // Anything written for the ping would arrive before this request. + let Some(ClientJsonRpcMessage::Request(request)) = server.receive().await else { + panic!("expected the next client request, not a ping response"); + }; + assert!(matches!( + request.request, + ClientRequest::ListToolsRequest(_) + )); + server + .send(ServerJsonRpcMessage::response( + ServerResult::ListToolsResult(Default::default()), + request.id, + )) + .await + .expect("send tools response"); + }); + + let client = DiscoverClient + .serve_with_lifecycle( + client_transport, + ClientLifecycleMode::Discover { + preferred_versions: vec![ProtocolVersion::V_2026_07_28], + }, + ) + .await + .expect("discover client should start"); + client.list_tools(None).await.expect("list tools"); + client.cancel().await.expect("cancel client"); + server_task.await.expect("server task"); +} + #[tokio::test] async fn auto_startup_falls_back_after_discover_method_not_found() { let (server_transport, client_transport) = tokio::io::duplex(4096); From 7819189eafa9a2710dfe48adcbc2d6c91d529869 Mon Sep 17 00:00:00 2001 From: Camille Lawrence Date: Fri, 16 Oct 2026 19:06:46 +0200 Subject: [PATCH 2/5] test(client): drop duplicated discover ping comment --- crates/rmcp/tests/test_client_lifecycle_modes.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/rmcp/tests/test_client_lifecycle_modes.rs b/crates/rmcp/tests/test_client_lifecycle_modes.rs index 373f43118..13ff77c8a 100644 --- a/crates/rmcp/tests/test_client_lifecycle_modes.rs +++ b/crates/rmcp/tests/test_client_lifecycle_modes.rs @@ -277,8 +277,6 @@ async fn discover_startup_omits_initialize() { server_task.await.expect("server task"); } -// 2026-07-28 has no ping and servers send no requests before discover -// completes, so a stray ping stays unanswered. #[tokio::test] async fn discover_startup_ignores_server_ping() { let (server_transport, client_transport) = tokio::io::duplex(4096); From 8c5652a3da0491f8627e8e304a4ac4bcde9e7297 Mon Sep 17 00:00:00 2001 From: Camille Lawrence Date: Fri, 16 Oct 2026 19:15:17 +0200 Subject: [PATCH 3/5] refactor(client): replace answer_ping bool with PreInitPing enum --- crates/rmcp/src/service/client.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/crates/rmcp/src/service/client.rs b/crates/rmcp/src/service/client.rs index dc11b2843..dbcee0771 100644 --- a/crates/rmcp/src/service/client.rs +++ b/crates/rmcp/src/service/client.rs @@ -158,6 +158,14 @@ where .ok_or_else(|| ClientInitializeError::ConnectionClosed(context.to_string())) } +/// Whether `expect_response` answers a server ping that arrives before the +/// handshake completes. +#[derive(Clone, Copy, PartialEq, Eq)] +enum PreInitPing { + Answer, + Ignore, +} + /// Helper function to expect a response from the stream, correlated to /// `expected_id`. /// @@ -166,7 +174,7 @@ where /// `id` is optional per spec) it is `UncorrelatedErrorResponse`. The caller /// never sees an uncorrelated response. /// -/// A server ping is answered only when `answer_ping` is set. The discover +/// A server ping is answered only with `PreInitPing::Answer`. The discover /// wait, including Auto's timeout-bounded probe, leaves it unanswered: /// 2026-07-28 has no ping, and a cancelled send could leave a partial frame. async fn expect_response( @@ -175,7 +183,7 @@ async fn expect_response( service: &S, peer: Peer, expected_id: &RequestId, - answer_ping: bool, + pre_init_ping: PreInitPing, ) -> Result where T: Transport + 'static, @@ -239,7 +247,7 @@ where ServerJsonRpcMessage::Request(request) if matches!(request.request, ServerRequest::PingRequest(_)) => { - if answer_ping { + if pre_init_ping == PreInitPing::Answer { transport .send(ClientJsonRpcMessage::response( ClientResult::empty(()), @@ -941,7 +949,7 @@ where service, peer.clone(), &id, - true, + PreInitPing::Answer, ) .await?; @@ -1019,7 +1027,7 @@ where service, peer.clone(), &id, - false, + PreInitPing::Ignore, ) .await { From 4b06f7203613e627e29a98dab62feca3a3548540 Mon Sep 17 00:00:00 2001 From: Camille Lawrence Date: Fri, 16 Oct 2026 19:15:18 +0200 Subject: [PATCH 4/5] test(client): assert notifications/initialized follows the ping response --- crates/rmcp/tests/test_client_initialization.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/rmcp/tests/test_client_initialization.rs b/crates/rmcp/tests/test_client_initialization.rs index e3990fc02..d5a98f1a9 100644 --- a/crates/rmcp/tests/test_client_initialization.rs +++ b/crates/rmcp/tests/test_client_initialization.rs @@ -9,8 +9,8 @@ use common::handlers::TestClientHandler; use rmcp::{ ServiceExt, model::{ - ClientConfig, ClientJsonRpcMessage, ClientResult, ErrorCode, ErrorData, InitializeResult, - JsonRpcError, JsonRpcVersion2_0, PingRequest, ProtocolVersion, RequestId, + ClientConfig, ClientJsonRpcMessage, ClientNotification, ClientResult, ErrorCode, ErrorData, + InitializeResult, JsonRpcError, JsonRpcVersion2_0, PingRequest, ProtocolVersion, RequestId, ServerCapabilities, ServerJsonRpcMessage, ServerRequest, ServerResult, }, transport::{IntoTransport, Transport}, @@ -195,7 +195,8 @@ async fn client_answers_server_ping_during_initialize() { assert!(matches!(response.result, ClientResult::EmptyResult(_))); assert!(matches!( server.receive().await, - Some(ClientJsonRpcMessage::Notification(_)) + Some(ClientJsonRpcMessage::Notification(notification)) + if matches!(notification.notification, ClientNotification::InitializedNotification(_)) )); }); From 582124188024068717161eea64e45cd5e23ab903 Mon Sep 17 00:00:00 2001 From: Camille Lawrence Date: Fri, 16 Oct 2026 22:13:50 +0200 Subject: [PATCH 5/5] refactor(client): match on PreInitPing instead of comparing it The compiler now flags this spot if PreInitPing gains a variant, and the PartialEq and Eq derives are no longer needed. --- crates/rmcp/src/service/client.rs | 33 ++++++++++++++++--------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/rmcp/src/service/client.rs b/crates/rmcp/src/service/client.rs index dbcee0771..b5611bb20 100644 --- a/crates/rmcp/src/service/client.rs +++ b/crates/rmcp/src/service/client.rs @@ -160,7 +160,7 @@ where /// Whether `expect_response` answers a server ping that arrives before the /// handshake completes. -#[derive(Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy)] enum PreInitPing { Answer, Ignore, @@ -247,21 +247,22 @@ where ServerJsonRpcMessage::Request(request) if matches!(request.request, ServerRequest::PingRequest(_)) => { - if pre_init_ping == PreInitPing::Answer { - transport - .send(ClientJsonRpcMessage::response( - ClientResult::empty(()), - request.id, - )) - .await - .map_err(|error| { - ClientInitializeError::transport::( - error, - "send pre-init ping response", - ) - })?; - } else { - tracing::trace!("Received ping request. Ignored.") + match pre_init_ping { + PreInitPing::Answer => { + transport + .send(ClientJsonRpcMessage::response( + ClientResult::empty(()), + request.id, + )) + .await + .map_err(|error| { + ClientInitializeError::transport::( + error, + "send pre-init ping response", + ) + })?; + } + PreInitPing::Ignore => tracing::trace!("Received ping request. Ignored."), } } // Server SHOULD NOT send any other messages before handshake. We ignore them anyway