Fix/sse channel replacement conflict (#682)
* fix(streamable-http): return 409 Conflict when standalone SSE stream already active LocalSessionWorker::resume() unconditionally replaced self.common.tx on every GET request, orphaning the receiver the first SSE stream was reading from. All subsequent server-to-client notifications were sent to the new sender while the original client was still listening on the old, now-dead receiver. notify_tool_list_changed().await returned Ok(()) silently. This is triggered by VS Code's MCP extension which reconnects SSE every ~5 minutes with the same session ID. Fix: Check tx.is_closed() before replacing the common channel sender. If an active stream exists, return SessionError::Conflict which is propagated as HTTP 409 Conflict. This matches the TypeScript SDK behavior (streamableHttp.ts:423). Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> * fix(streamable-http): handle resume with completed request-wise channel When a client sends GET with Last-Event-ID from a completed POST SSE response, the request-wise channel no longer exists in tx_router. Previously this returned ChannelClosed -> 500, causing clients like Cursor to enter an infinite re-initialization loop. Now falls back to the common channel when the request-wise channel is completed, per MCP spec: "Resumption applies regardless of how the original stream was initiated (POST or GET)." * fix: allow SSE channel replacement instead of 409 Conflict Per MCP spec §Streamable HTTP, "The client MAY remain connected to multiple SSE streams simultaneously." Returning 409 Conflict when a second GET arrives causes Cursor to enter an infinite re-initialization loop (~3s cycle). Instead of rejecting, replace the old common channel sender. Dropping the old sender closes the old receiver, cleanly terminating the previous SSE stream so the client can reconnect on the new stream. This fixes both code paths: - GET with Last-Event-ID from a completed POST SSE response - GET without Last-Event-ID (standalone stream reconnection) * fix: skip cache replay when replacing active SSE stream When a client opens a new GET SSE stream while a previous one is still active, the old sender is dropped (terminating the old stream) and a new channel is created. Previously, sync() replayed all cached events to the new stream, but the client already received those events on the old stream. This caused an infinite notification loop: 1. Client receives notifications (e.g. ResourceListChanged) 2. Old SSE stream dies (sender replaced) 3. Client reconnects after sse_retry (3s) 4. sync() replays cached notifications the client already handled 5. Client processes them again → goto 2 Fix: check tx.is_closed() BEFORE replacing the sender. If the old stream was still alive, skip replay entirely — the client already has those events. Only replay when the old stream was genuinely dead (network failure, timeout) so the client catches up on missed events. * fix: use shadow channels to prevent SSE reconnect loops When POST SSE responses include a `retry` field, the browser's EventSource automatically reconnects via GET after the stream ends. This creates multiple competing EventSource connections that each replace the common channel sender, killing the other stream's receiver. Both reconnect every sse_retry seconds, creating an infinite loop. Instead of always replacing the common channel, check if the primary is still active. If so, create a "shadow" stream — an idle SSE connection kept alive by keep-alive pings that doesn't receive notifications or interfere with the primary channel. Also removes cache replay (sync) on common channel resume, as replaying server-initiated list_changed notifications causes clients to re-process old signals. Signed-off-by: Myko Ash <myko@mcpmux.com> Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> * test: comprehensive shadow channel tests (15 cases) Rewrite test suite for SSE channel replacement fix: - Shadow creation: standalone GET returns 200, multiple GETs coexist - Dead primary: replacement, notification delivery, repeated cycles - Notification routing: primary receives, shadow does not - Resume paths: completed request-wise, common alive/dead - Real scenarios: Cursor leapfrog, VS Code reconnect - Edge cases: invalid session, missing header, shadow cleanup Fix Accept header bug (was missing text/event-stream for notifications/initialized POST, causing 406 rejection). * fix: use correct HTTP status codes for session errors per MCP spec MCP spec (2025-11-25) section "Session Management" requires: - Missing session ID header → 400 Bad Request (not 401) - Unknown/terminated session → 404 Not Found (not 401) Using 401 Unauthorized caused MCP clients (e.g. VS Code) to trigger full OAuth re-authentication on server restart, instead of simply re-initializing the session. Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> * fix: address review feedback — remove dead Conflict variant, restore sync on resume, rename test - Remove unused SessionError::Conflict and dead string-matching in tower.rs (leftover from abandoned 409 approach) - Restore sync() replay when replacing a dead primary common channel so server-initiated requests and cached notifications are not lost on reconnect - Rename test from test_sse_channel_replacement_bug to test_sse_concurrent_streams per reviewer suggestion (describe what tests verify, not what triggered them) - Add test for cache replay on dead primary replacement - Use generic "MCP clients" in comments instead of specific client names Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> * fix: use minimal buffer for shadow streams and cap at 32 - Shadow streams only receive SSE keep-alive pings, so use capacity 1 instead of full channel_capacity - Cap shadow_txs at 32 to prevent unbounded growth from misbehaving clients, dropping the oldest shadow when the limit is reached - Add test verifying primary works after exceeding shadow limit Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> * fix: remove redundant single-component `use reqwest` import Fixes clippy::single_component_path_imports lint error in test_sse_concurrent_streams.rs. --------- Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com> Signed-off-by: Myko Ash <myko@mcpmux.com> Co-authored-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>
This commit is contained in:
Alex Hancock
GitHub
parent
3cb855bcf8
commit
332fcbfb91
2 changed files with 891 additions and 37 deletions
|
|
@ -293,6 +293,12 @@ pub struct LocalSessionWorker {
|
|||
tx_router: HashMap<HttpRequestId, HttpRequestWise>,
|
||||
resource_router: HashMap<ResourceKey, HttpRequestId>,
|
||||
common: CachedTx,
|
||||
/// Shadow senders for secondary SSE streams (e.g. from POST EventSource
|
||||
/// reconnections). These keep the HTTP connections alive via SSE keep-alive
|
||||
/// without receiving notifications, preventing MCP clients from entering
|
||||
/// infinite reconnect loops when multiple EventSource connections compete
|
||||
/// to replace the common channel.
|
||||
shadow_txs: Vec<Sender<ServerSseMessage>>,
|
||||
event_rx: Receiver<SessionEvent>,
|
||||
session_config: SessionConfig,
|
||||
}
|
||||
|
|
@ -513,38 +519,94 @@ impl LocalSessionWorker {
|
|||
&mut self,
|
||||
last_event_id: EventId,
|
||||
) -> Result<StreamableHttpMessageReceiver, SessionError> {
|
||||
// Clean up closed shadow senders before processing
|
||||
self.shadow_txs.retain(|tx| !tx.is_closed());
|
||||
|
||||
match last_event_id.http_request_id {
|
||||
Some(http_request_id) => {
|
||||
let request_wise = self
|
||||
.tx_router
|
||||
.get_mut(&http_request_id)
|
||||
.ok_or(SessionError::ChannelClosed(Some(http_request_id)))?;
|
||||
let channel = tokio::sync::mpsc::channel(self.session_config.channel_capacity);
|
||||
let (tx, rx) = channel;
|
||||
request_wise.tx.tx = tx;
|
||||
let index = last_event_id.index;
|
||||
// sync messages after index
|
||||
request_wise.tx.sync(index).await?;
|
||||
Ok(StreamableHttpMessageReceiver {
|
||||
http_request_id: Some(http_request_id),
|
||||
inner: rx,
|
||||
})
|
||||
}
|
||||
None => {
|
||||
let channel = tokio::sync::mpsc::channel(self.session_config.channel_capacity);
|
||||
let (tx, rx) = channel;
|
||||
self.common.tx = tx;
|
||||
let index = last_event_id.index;
|
||||
// sync messages after index
|
||||
self.common.sync(index).await?;
|
||||
Ok(StreamableHttpMessageReceiver {
|
||||
http_request_id: None,
|
||||
inner: rx,
|
||||
})
|
||||
if let Some(request_wise) = self.tx_router.get_mut(&http_request_id) {
|
||||
// Resume existing request-wise channel
|
||||
let channel = tokio::sync::mpsc::channel(self.session_config.channel_capacity);
|
||||
let (tx, rx) = channel;
|
||||
request_wise.tx.tx = tx;
|
||||
let index = last_event_id.index;
|
||||
// sync messages after index
|
||||
request_wise.tx.sync(index).await?;
|
||||
Ok(StreamableHttpMessageReceiver {
|
||||
http_request_id: Some(http_request_id),
|
||||
inner: rx,
|
||||
})
|
||||
} else {
|
||||
// Request-wise channel completed (POST response already delivered).
|
||||
// The client's EventSource is reconnecting after the POST SSE stream
|
||||
// ended. Fall through to common channel handling below.
|
||||
tracing::debug!(
|
||||
http_request_id,
|
||||
"Request-wise channel completed, falling back to common channel"
|
||||
);
|
||||
self.resume_or_shadow_common(last_event_id.index).await
|
||||
}
|
||||
}
|
||||
None => self.resume_or_shadow_common(last_event_id.index).await,
|
||||
}
|
||||
}
|
||||
|
||||
/// Resume the common channel, or create a shadow stream if the primary is
|
||||
/// still active.
|
||||
///
|
||||
/// When the primary common channel is dead (receiver dropped), replace it
|
||||
/// so this stream becomes the new primary notification channel. Cached
|
||||
/// messages are replayed from `last_event_index` so the client receives
|
||||
/// any events it missed (including server-initiated requests).
|
||||
///
|
||||
/// When the primary is still active, create a "shadow" stream — an idle SSE
|
||||
/// connection kept alive by keep-alive pings. This prevents multiple
|
||||
/// EventSource connections (e.g. from POST response reconnections) from
|
||||
/// killing each other by repeatedly replacing the common channel sender.
|
||||
async fn resume_or_shadow_common(
|
||||
&mut self,
|
||||
last_event_index: usize,
|
||||
) -> Result<StreamableHttpMessageReceiver, SessionError> {
|
||||
let is_replacing_dead_primary = self.common.tx.is_closed();
|
||||
let capacity = if is_replacing_dead_primary {
|
||||
self.session_config.channel_capacity
|
||||
} else {
|
||||
1 // Shadow streams only need keep-alive pings
|
||||
};
|
||||
let (tx, rx) = tokio::sync::mpsc::channel(capacity);
|
||||
if is_replacing_dead_primary {
|
||||
// Primary common channel is dead — replace it.
|
||||
tracing::debug!("Replacing dead common channel with new primary");
|
||||
self.common.tx = tx;
|
||||
// Replay cached messages from where the client left off so
|
||||
// server-initiated requests and notifications are not lost.
|
||||
self.common.sync(last_event_index).await?;
|
||||
} else {
|
||||
// Primary common channel is still active. Create a shadow stream
|
||||
// that stays alive via SSE keep-alive but doesn't receive
|
||||
// notifications. This prevents competing EventSource connections
|
||||
// from killing each other's channels.
|
||||
const MAX_SHADOW_STREAMS: usize = 32;
|
||||
|
||||
if self.shadow_txs.len() >= MAX_SHADOW_STREAMS {
|
||||
tracing::warn!(
|
||||
shadow_count = self.shadow_txs.len(),
|
||||
"Shadow stream limit reached, dropping oldest"
|
||||
);
|
||||
self.shadow_txs.remove(0);
|
||||
}
|
||||
tracing::debug!(
|
||||
shadow_count = self.shadow_txs.len(),
|
||||
"Common channel active, creating shadow stream"
|
||||
);
|
||||
self.shadow_txs.push(tx);
|
||||
}
|
||||
Ok(StreamableHttpMessageReceiver {
|
||||
http_request_id: None,
|
||||
inner: rx,
|
||||
})
|
||||
}
|
||||
|
||||
async fn close_sse_stream(
|
||||
&mut self,
|
||||
http_request_id: Option<HttpRequestId>,
|
||||
|
|
@ -584,6 +646,9 @@ impl LocalSessionWorker {
|
|||
let (tx, _rx) = tokio::sync::mpsc::channel(1);
|
||||
self.common.tx = tx;
|
||||
|
||||
// Also close all shadow streams
|
||||
self.shadow_txs.clear();
|
||||
|
||||
tracing::debug!("closed standalone SSE stream for server-initiated disconnection");
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -1036,6 +1101,7 @@ pub fn create_local_session(
|
|||
tx_router: HashMap::new(),
|
||||
resource_router: HashMap::new(),
|
||||
common,
|
||||
shadow_txs: Vec::new(),
|
||||
event_rx,
|
||||
session_config: config.clone(),
|
||||
};
|
||||
|
|
|
|||
|
|
@ -299,10 +299,10 @@ where
|
|||
.and_then(|v| v.to_str().ok())
|
||||
.map(|s| s.to_owned().into());
|
||||
let Some(session_id) = session_id else {
|
||||
// unauthorized
|
||||
// MCP spec: servers that require a session ID SHOULD respond with 400 Bad Request
|
||||
return Ok(Response::builder()
|
||||
.status(http::StatusCode::UNAUTHORIZED)
|
||||
.body(Full::new(Bytes::from("Unauthorized: Session ID is required")).boxed())
|
||||
.status(http::StatusCode::BAD_REQUEST)
|
||||
.body(Full::new(Bytes::from("Bad Request: Session ID is required")).boxed())
|
||||
.expect("valid response"));
|
||||
};
|
||||
// check if session exists
|
||||
|
|
@ -312,10 +312,10 @@ where
|
|||
.await
|
||||
.map_err(internal_error_response("check session"))?;
|
||||
if !has_session {
|
||||
// unauthorized
|
||||
// MCP spec: server MUST respond with 404 Not Found for terminated/unknown sessions
|
||||
return Ok(Response::builder()
|
||||
.status(http::StatusCode::UNAUTHORIZED)
|
||||
.body(Full::new(Bytes::from("Unauthorized: Session not found")).boxed())
|
||||
.status(http::StatusCode::NOT_FOUND)
|
||||
.body(Full::new(Bytes::from("Not Found: Session not found")).boxed())
|
||||
.expect("valid response"));
|
||||
}
|
||||
// Validate MCP-Protocol-Version header (per 2025-06-18 spec)
|
||||
|
|
@ -426,10 +426,10 @@ where
|
|||
.await
|
||||
.map_err(internal_error_response("check session"))?;
|
||||
if !has_session {
|
||||
// unauthorized
|
||||
// MCP spec: server MUST respond with 404 Not Found for terminated/unknown sessions
|
||||
return Ok(Response::builder()
|
||||
.status(http::StatusCode::UNAUTHORIZED)
|
||||
.body(Full::new(Bytes::from("Unauthorized: Session not found")).boxed())
|
||||
.status(http::StatusCode::NOT_FOUND)
|
||||
.body(Full::new(Bytes::from("Not Found: Session not found")).boxed())
|
||||
.expect("valid response"));
|
||||
}
|
||||
|
||||
|
|
@ -629,10 +629,10 @@ where
|
|||
.and_then(|v| v.to_str().ok())
|
||||
.map(|s| s.to_owned().into());
|
||||
let Some(session_id) = session_id else {
|
||||
// unauthorized
|
||||
// MCP spec: servers that require a session ID SHOULD respond with 400 Bad Request
|
||||
return Ok(Response::builder()
|
||||
.status(http::StatusCode::UNAUTHORIZED)
|
||||
.body(Full::new(Bytes::from("Unauthorized: Session ID is required")).boxed())
|
||||
.status(http::StatusCode::BAD_REQUEST)
|
||||
.body(Full::new(Bytes::from("Bad Request: Session ID is required")).boxed())
|
||||
.expect("valid response"));
|
||||
};
|
||||
// Validate MCP-Protocol-Version header (per 2025-06-18 spec)
|
||||
|
|
|
|||
Loading…
Reference in a new issue