Skip to content

Commit 2463369

Browse files
committed
fix(session): tolerate dead worker in close_session
Swallow SessionServiceTerminated in close_session when the worker has already exited. This prevents a spurious ERROR log during the post-exit cleanup path in spawn_session_worker.
1 parent d41d3c8 commit 2463369

2 files changed

Lines changed: 115 additions & 11 deletions

File tree

crates/rmcp/src/transport/streamable_http_server/session/local.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,16 @@ impl SessionManager for LocalSessionManager {
6666
Ok(response)
6767
}
6868
async fn close_session(&self, id: &SessionId) -> Result<(), Self::Error> {
69-
let mut sessions = self.sessions.write().await;
70-
if let Some(handle) = sessions.remove(id) {
71-
handle.close().await?;
69+
let handle = {
70+
let mut sessions = self.sessions.write().await;
71+
sessions.remove(id)
72+
};
73+
if let Some(handle) = handle {
74+
match handle.close().await {
75+
// Worker already exited — nothing left to clean up.
76+
Ok(()) | Err(SessionError::SessionServiceTerminated) => {}
77+
Err(e) => return Err(e.into()),
78+
}
7279
}
7380
Ok(())
7481
}

crates/rmcp/tests/test_streamable_http_idle_timeout_log.rs

Lines changed: 105 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,15 @@ use std::{
1010
};
1111

1212
use rmcp::transport::streamable_http_server::{
13-
StreamableHttpServerConfig, StreamableHttpService, session::local::LocalSessionManager,
13+
StreamableHttpServerConfig, StreamableHttpService,
14+
session::{SessionManager, local::LocalSessionManager},
1415
};
1516
use tokio_util::sync::CancellationToken;
1617
use tracing_subscriber::layer::SubscriberExt;
1718

1819
mod common;
1920
use common::calculator::Calculator;
2021

21-
// Issue #817: keep-alive timeout emits tracing::error! for normal idle reaping.
22-
2322
struct CapturedEvent {
2423
level: tracing::Level,
2524
message: String,
@@ -92,7 +91,6 @@ async fn test_keep_alive_timeout_does_not_emit_error_log() {
9291

9392
let client = reqwest::Client::new();
9493

95-
// Initialize session
9694
let response = client
9795
.post(format!("http://{addr}/mcp"))
9896
.header("Content-Type", "application/json")
@@ -107,7 +105,6 @@ async fn test_keep_alive_timeout_does_not_emit_error_log() {
107105
.unwrap()
108106
.to_string();
109107

110-
// Complete handshake
111108
client
112109
.post(format!("http://{addr}/mcp"))
113110
.header("Content-Type", "application/json")
@@ -119,19 +116,37 @@ async fn test_keep_alive_timeout_does_not_emit_error_log() {
119116
.await
120117
.unwrap();
121118

122-
// Wait for keep_alive timeout (200ms) plus margin
123119
tokio::time::sleep(Duration::from_millis(400)).await;
124120

121+
// Wait until close_session() has completed so all logs are captured.
122+
let session_id_parsed: Arc<str> = Arc::from(session_id.as_str());
123+
for _ in 0..20 {
124+
if !session_manager
125+
.has_session(&session_id_parsed)
126+
.await
127+
.unwrap()
128+
{
129+
break;
130+
}
131+
tokio::time::sleep(Duration::from_millis(50)).await;
132+
}
133+
assert!(
134+
!session_manager
135+
.has_session(&session_id_parsed)
136+
.await
137+
.unwrap(),
138+
"session should have been removed after idle reap"
139+
);
140+
125141
let captured = events.lock().unwrap();
126142

127143
let error_events: Vec<_> = captured
128144
.iter()
129145
.filter(|e| e.level == tracing::Level::ERROR)
130-
.filter(|e| e.message.contains("keep alive timeout") || e.message.contains("IdleTimeout"))
131146
.collect();
132147
assert!(
133148
error_events.is_empty(),
134-
"keep-alive timeout should not produce ERROR logs, found {}: {:?}",
149+
"idle reap should not produce any ERROR logs, found {}: {:?}",
135150
error_events.len(),
136151
error_events.iter().map(|e| &e.message).collect::<Vec<_>>()
137152
);
@@ -147,3 +162,85 @@ async fn test_keep_alive_timeout_does_not_emit_error_log() {
147162

148163
ct.cancel();
149164
}
165+
166+
#[tokio::test(flavor = "current_thread")]
167+
async fn test_explicit_close_on_live_session_succeeds() {
168+
let ct = CancellationToken::new();
169+
let mut session_manager = LocalSessionManager::default();
170+
session_manager.session_config.keep_alive = Some(Duration::from_secs(60));
171+
let session_manager = Arc::new(session_manager);
172+
173+
let service = StreamableHttpService::new(
174+
|| Ok(Calculator::new()),
175+
session_manager.clone(),
176+
StreamableHttpServerConfig::default()
177+
.with_sse_keep_alive(None)
178+
.with_cancellation_token(ct.child_token()),
179+
);
180+
181+
let router = axum::Router::new().nest_service("/mcp", service);
182+
let tcp_listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
183+
let addr = tcp_listener.local_addr().unwrap();
184+
185+
tokio::spawn({
186+
let ct = ct.clone();
187+
async move {
188+
let _ = axum::serve(tcp_listener, router)
189+
.with_graceful_shutdown(async move { ct.cancelled_owned().await })
190+
.await;
191+
}
192+
});
193+
194+
let client = reqwest::Client::new();
195+
196+
let response = client
197+
.post(format!("http://{addr}/mcp"))
198+
.header("Content-Type", "application/json")
199+
.header("Accept", "application/json, text/event-stream")
200+
.body(r#"{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"test","version":"1.0"}}}"#)
201+
.send()
202+
.await
203+
.unwrap();
204+
assert_eq!(response.status(), 200);
205+
let session_id = response.headers()["mcp-session-id"]
206+
.to_str()
207+
.unwrap()
208+
.to_string();
209+
210+
client
211+
.post(format!("http://{addr}/mcp"))
212+
.header("Content-Type", "application/json")
213+
.header("Accept", "application/json, text/event-stream")
214+
.header("mcp-session-id", &session_id)
215+
.header("Mcp-Protocol-Version", "2025-06-18")
216+
.body(r#"{"jsonrpc":"2.0","method":"notifications/initialized"}"#)
217+
.send()
218+
.await
219+
.unwrap();
220+
221+
let session_id_parsed: Arc<str> = Arc::from(session_id.as_str());
222+
223+
assert!(
224+
session_manager
225+
.has_session(&session_id_parsed)
226+
.await
227+
.unwrap(),
228+
"session should exist before explicit close"
229+
);
230+
231+
let result = session_manager.close_session(&session_id_parsed).await;
232+
assert!(
233+
result.is_ok(),
234+
"close_session on a live worker should succeed: {result:?}"
235+
);
236+
237+
assert!(
238+
!session_manager
239+
.has_session(&session_id_parsed)
240+
.await
241+
.unwrap(),
242+
"session should not exist after explicit close"
243+
);
244+
245+
ct.cancel();
246+
}

0 commit comments

Comments
 (0)