Skip to content

Commit a83e288

Browse files
committed
fix(notifier): tag every list_changed push with session_id + client_id
The 'Sent tools/list_changed notification' debug line was anonymous — the design routes per-session correctly (each Peer<R> we hand to notify_*_list_changed is the one we stored in SessionEntry, tied to exactly one mcp-session-id), but the log didn't prove it. With six concurrent sessions across two clients, an audit needed cross- referencing peer pointers, which is impractical. Thread session_id + client_id through the send paths: - get_peers_for_space_with_streams now returns Vec<(session_id, client_id, peer)> instead of two parallel Vecs; the third element lets every send_*_list_changed call log who got the push. - send_tools_list_changed, send_prompts_list_changed, send_resources_list_changed iterate the triples and tag each ✅ / Failed line with both ids. - notify_peer_lists_changed (per-client fanout for resolution flips and grant edits) also tags each ✅ / failed line with session_id. Drops the now-dead get_peers_for_space (plain-Vec variant) and the SpaceResolverService field — both unused once every fanout path goes through the streams variant. Constructor signature trimmed accordingly; both call sites (server/mod.rs + the gateway- notifications integration test) updated. cargo check + clippy (-D warnings) clean. Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>
1 parent c9a18b7 commit a83e288

3 files changed

Lines changed: 114 additions & 125 deletions

File tree

crates/mcpmux-gateway/src/consumers/mcp_notifier.rs

Lines changed: 114 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ use tracing::{debug, info, trace, warn};
2626
use uuid::Uuid;
2727

2828
use crate::pool::FeatureService;
29-
use crate::services::{FeatureSetResolverService, SpaceResolverService};
29+
use crate::services::FeatureSetResolverService;
3030

3131
/// MCP Notifier — sends `list_changed` notifications to connected sessions.
3232
///
@@ -53,10 +53,6 @@ use crate::services::{FeatureSetResolverService, SpaceResolverService};
5353
pub struct MCPNotifier {
5454
/// Map: `mcp-session-id` → session handle.
5555
sessions: Arc<RwLock<HashMap<String, SessionEntry>>>,
56-
/// Space resolver for the legacy client-→home-space query (kept for
57-
/// callers that don't have a session id; the new fanout paths use
58-
/// `feature_set_resolver` instead).
59-
space_resolver: Arc<SpaceResolverService>,
6056
/// FeatureSet resolver — same one the request handlers use. Consulted
6157
/// per session to decide whether a notification applies.
6258
feature_set_resolver: Arc<FeatureSetResolverService>,
@@ -112,13 +108,11 @@ impl SessionEntry {
112108

113109
impl MCPNotifier {
114110
pub fn new(
115-
space_resolver: Arc<SpaceResolverService>,
116111
feature_set_resolver: Arc<FeatureSetResolverService>,
117112
feature_service: Arc<FeatureService>,
118113
) -> Self {
119114
Self {
120115
sessions: Arc::new(RwLock::new(HashMap::new())),
121-
space_resolver,
122116
feature_set_resolver,
123117
feature_service,
124118
throttle_tracker: Arc::new(RwLock::new(HashMap::new())),
@@ -368,78 +362,6 @@ impl MCPNotifier {
368362
dead
369363
}
370364

371-
/// Get every peer whose **session** currently routes into `space_id`.
372-
///
373-
/// Iterates the session registry and re-runs the FeatureSet resolver
374-
/// per session — same logic the request handlers use, so a session
375-
/// redirected by `WorkspaceBinding` to a non-default space is matched
376-
/// correctly. Sessions whose stream isn't active yet are skipped (the
377-
/// notification would be queued but not delivered). Dead sessions
378-
/// (transport closed) are GC'd before the resolve pass so we don't
379-
/// waste a `feature_set_resolver.resolve()` call on them.
380-
async fn get_peers_for_space(&self, space_id: Uuid) -> Vec<Arc<Peer<RoleServer>>> {
381-
let session_list: Vec<(String, String, Arc<Peer<RoleServer>>)> = {
382-
let sessions = self.sessions.read();
383-
sessions
384-
.iter()
385-
.filter(|(_, e)| e.has_active_stream)
386-
.map(|(sid, entry)| (sid.clone(), entry.client_id.clone(), entry.peer.clone()))
387-
.collect()
388-
};
389-
390-
// GC dead sessions before resolving — also drops them from the
391-
// snapshot so the resolve loop below skips them.
392-
let dead = self.reap_dead_sessions(
393-
&session_list
394-
.iter()
395-
.map(|(sid, _, peer)| (sid.clone(), peer.clone()))
396-
.collect::<Vec<_>>(),
397-
);
398-
let dead_set: std::collections::HashSet<&str> = dead.iter().map(String::as_str).collect();
399-
400-
let mut matching_peers = Vec::new();
401-
let _space_resolver = &self.space_resolver; // kept-but-unused; resolver below is authoritative
402-
for (session_id, client_id, peer) in session_list {
403-
if dead_set.contains(session_id.as_str()) {
404-
continue;
405-
}
406-
match self
407-
.feature_set_resolver
408-
.resolve(Some(&session_id), Some(&client_id))
409-
.await
410-
{
411-
Ok(resolved) if resolved.space_id == Some(space_id) => {
412-
debug!(
413-
%session_id,
414-
%client_id,
415-
%space_id,
416-
"[MCPNotifier] Session resolves to target space"
417-
);
418-
matching_peers.push(peer);
419-
}
420-
Ok(resolved) => {
421-
debug!(
422-
%session_id,
423-
%client_id,
424-
resolved_space = ?resolved.space_id,
425-
%space_id,
426-
"[MCPNotifier] Session is in a different space, skipping"
427-
);
428-
}
429-
Err(e) => {
430-
warn!(
431-
%session_id,
432-
%client_id,
433-
error = %e,
434-
"[MCPNotifier] ⚠️ Failed to resolve space for session"
435-
);
436-
}
437-
}
438-
}
439-
440-
matching_peers
441-
}
442-
443365
/// Start listening to domain events and notifying peers
444366
///
445367
/// Spawns a background task that listens to DomainEvents and calls
@@ -811,33 +733,47 @@ impl MCPNotifier {
811733
return;
812734
}
813735

814-
// Get peers for this space, filtering to only those with active streams
815-
let (peers, _client_ids) = self.get_peers_for_space_with_streams(space_id).await;
736+
// Get sessions in this space with active streams, paired with
737+
// their session_id + client_id for per-push log attribution.
738+
let targets = self.get_peers_for_space_with_streams(space_id).await;
816739

817-
if peers.is_empty() {
818-
debug!(space_id = %space_id, "[MCPNotifier] No peers with active streams to notify about tools");
740+
if targets.is_empty() {
741+
debug!(
742+
space_id = %space_id,
743+
"[MCPNotifier] No sessions with active streams to notify about tools"
744+
);
819745
return;
820746
}
821747

822748
info!(
823749
space_id = %space_id,
824-
peer_count = peers.len(),
825-
"[MCPNotifier] 📤 Sending tools/list_changed to {} peers with active streams",
826-
peers.len()
750+
session_count = targets.len(),
751+
"[MCPNotifier] 📤 Sending tools/list_changed to {} session(s) with active streams",
752+
targets.len()
827753
);
828754

829755
let mut success_count = 0;
830756
let mut failure_count = 0;
831757

832-
for peer in peers {
758+
for (session_id, client_id, peer) in targets {
833759
match peer.notify_tool_list_changed().await {
834760
Ok(_) => {
835761
success_count += 1;
836-
debug!("[MCPNotifier] ✅ Sent tools/list_changed notification");
762+
debug!(
763+
%session_id,
764+
%client_id,
765+
%space_id,
766+
"[MCPNotifier] ✅ Sent tools/list_changed to session"
767+
);
837768
}
838769
Err(e) => {
839770
failure_count += 1;
840-
warn!(error = ?e, "[MCPNotifier] Failed to send tools/list_changed");
771+
warn!(
772+
%session_id,
773+
%client_id,
774+
error = ?e,
775+
"[MCPNotifier] Failed to send tools/list_changed to session"
776+
);
841777
}
842778
}
843779
}
@@ -853,16 +789,20 @@ impl MCPNotifier {
853789
}
854790
}
855791

856-
/// Get peers for a space that have active SSE streams.
792+
/// Get the sessions in `space_id` that have an active SSE stream and
793+
/// can therefore actually receive a notification.
857794
///
858795
/// Session-keyed: iterates `sessions`, re-runs the FeatureSet resolver
859796
/// per session (same path as the request handlers), and returns the
860-
/// peers whose session resolves into `space_id`. The second tuple
861-
/// element is `client_id`s of those sessions, kept for log clarity.
797+
/// `(session_id, client_id, peer)` triples whose session resolves into
798+
/// `space_id`. Threading session_id through to the call site lets the
799+
/// log lines on each `peer.notify_*_list_changed()` prove *which*
800+
/// session got the push — important for verifying that two windows of
801+
/// the same client routing into different spaces don't cross-talk.
862802
async fn get_peers_for_space_with_streams(
863803
&self,
864804
space_id: Uuid,
865-
) -> (Vec<Arc<Peer<RoleServer>>>, Vec<String>) {
805+
) -> Vec<(String, String, Arc<Peer<RoleServer>>)> {
866806
let session_list: Vec<(String, String, Arc<Peer<RoleServer>>)> = {
867807
let sessions = self.sessions.read();
868808
sessions
@@ -880,8 +820,7 @@ impl MCPNotifier {
880820
);
881821
let dead_set: std::collections::HashSet<&str> = dead.iter().map(String::as_str).collect();
882822

883-
let mut matching_peers = Vec::new();
884-
let mut matching_client_ids = Vec::new();
823+
let mut matching = Vec::new();
885824

886825
for (session_id, client_id, peer) in session_list {
887826
if dead_set.contains(session_id.as_str()) {
@@ -899,8 +838,7 @@ impl MCPNotifier {
899838
%space_id,
900839
"[MCPNotifier] Session in target space with active stream"
901840
);
902-
matching_peers.push(peer);
903-
matching_client_ids.push(client_id);
841+
matching.push((session_id, client_id, peer));
904842
}
905843
Ok(resolved) => {
906844
debug!(
@@ -922,7 +860,7 @@ impl MCPNotifier {
922860
}
923861
}
924862

925-
(matching_peers, matching_client_ids)
863+
matching
926864
}
927865

928866
/// Notify all peers in a space that prompts list has changed (with throttling and deduping)
@@ -967,21 +905,33 @@ impl MCPNotifier {
967905
return;
968906
}
969907

970-
let peers = self.get_peers_for_space(space_id).await;
908+
let targets = self.get_peers_for_space_with_streams(space_id).await;
971909

972-
if peers.is_empty() {
910+
if targets.is_empty() {
973911
return;
974912
}
975913

976914
info!(
977915
space_id = %space_id,
978-
peer_count = peers.len(),
979-
"[MCPNotifier] 📤 Sending prompts/list_changed"
916+
session_count = targets.len(),
917+
"[MCPNotifier] 📤 Sending prompts/list_changed to {} session(s)",
918+
targets.len()
980919
);
981920

982-
for peer in peers {
983-
if let Err(e) = peer.notify_prompt_list_changed().await {
984-
warn!(error = ?e, "[MCPNotifier] Failed to send prompts/list_changed");
921+
for (session_id, client_id, peer) in targets {
922+
match peer.notify_prompt_list_changed().await {
923+
Ok(_) => debug!(
924+
%session_id,
925+
%client_id,
926+
%space_id,
927+
"[MCPNotifier] ✅ Sent prompts/list_changed to session"
928+
),
929+
Err(e) => warn!(
930+
%session_id,
931+
%client_id,
932+
error = ?e,
933+
"[MCPNotifier] Failed to send prompts/list_changed to session"
934+
),
985935
}
986936
}
987937
}
@@ -1028,21 +978,33 @@ impl MCPNotifier {
1028978
return;
1029979
}
1030980

1031-
let peers = self.get_peers_for_space(space_id).await;
981+
let targets = self.get_peers_for_space_with_streams(space_id).await;
1032982

1033-
if peers.is_empty() {
983+
if targets.is_empty() {
1034984
return;
1035985
}
1036986

1037987
info!(
1038988
space_id = %space_id,
1039-
peer_count = peers.len(),
1040-
"[MCPNotifier] 📤 Sending resources/list_changed"
989+
session_count = targets.len(),
990+
"[MCPNotifier] 📤 Sending resources/list_changed to {} session(s)",
991+
targets.len()
1041992
);
1042993

1043-
for peer in peers {
1044-
if let Err(e) = peer.notify_resource_list_changed().await {
1045-
warn!(error = ?e, "[MCPNotifier] Failed to send resources/list_changed");
994+
for (session_id, client_id, peer) in targets {
995+
match peer.notify_resource_list_changed().await {
996+
Ok(_) => debug!(
997+
%session_id,
998+
%client_id,
999+
%space_id,
1000+
"[MCPNotifier] ✅ Sent resources/list_changed to session"
1001+
),
1002+
Err(e) => warn!(
1003+
%session_id,
1004+
%client_id,
1005+
error = ?e,
1006+
"[MCPNotifier] Failed to send resources/list_changed to session"
1007+
),
10461008
}
10471009
}
10481010
}
@@ -1077,13 +1039,12 @@ impl MCPNotifier {
10771039
};
10781040
let dead = self.reap_dead_sessions(&snapshot);
10791041
let dead_set: std::collections::HashSet<&str> = dead.iter().map(String::as_str).collect();
1080-
let peers: Vec<Arc<Peer<RoleServer>>> = snapshot
1042+
let live: Vec<(String, Arc<Peer<RoleServer>>)> = snapshot
10811043
.into_iter()
10821044
.filter(|(sid, _)| !dead_set.contains(sid.as_str()))
1083-
.map(|(_, peer)| peer)
10841045
.collect();
10851046

1086-
if peers.is_empty() {
1047+
if live.is_empty() {
10871048
debug!(
10881049
%client_id,
10891050
"[MCPNotifier] no active session — skipping peer list_changed"
@@ -1093,19 +1054,49 @@ impl MCPNotifier {
10931054

10941055
info!(
10951056
%client_id,
1096-
session_count = peers.len(),
1057+
session_count = live.len(),
10971058
"[MCPNotifier] 📤 per-client list_changed (resolution flipped or grant edited)"
10981059
);
10991060

1100-
for peer in &peers {
1101-
if let Err(e) = peer.notify_tool_list_changed().await {
1102-
warn!(error = ?e, %client_id, "[MCPNotifier] failed tools/list_changed");
1061+
for (session_id, peer) in &live {
1062+
match peer.notify_tool_list_changed().await {
1063+
Ok(_) => debug!(
1064+
%session_id,
1065+
%client_id,
1066+
"[MCPNotifier] ✅ Sent tools/list_changed to session (per-client)"
1067+
),
1068+
Err(e) => warn!(
1069+
%session_id,
1070+
%client_id,
1071+
error = ?e,
1072+
"[MCPNotifier] failed tools/list_changed"
1073+
),
11031074
}
1104-
if let Err(e) = peer.notify_prompt_list_changed().await {
1105-
warn!(error = ?e, %client_id, "[MCPNotifier] failed prompts/list_changed");
1075+
match peer.notify_prompt_list_changed().await {
1076+
Ok(_) => debug!(
1077+
%session_id,
1078+
%client_id,
1079+
"[MCPNotifier] ✅ Sent prompts/list_changed to session (per-client)"
1080+
),
1081+
Err(e) => warn!(
1082+
%session_id,
1083+
%client_id,
1084+
error = ?e,
1085+
"[MCPNotifier] failed prompts/list_changed"
1086+
),
11061087
}
1107-
if let Err(e) = peer.notify_resource_list_changed().await {
1108-
warn!(error = ?e, %client_id, "[MCPNotifier] failed resources/list_changed");
1088+
match peer.notify_resource_list_changed().await {
1089+
Ok(_) => debug!(
1090+
%session_id,
1091+
%client_id,
1092+
"[MCPNotifier] ✅ Sent resources/list_changed to session (per-client)"
1093+
),
1094+
Err(e) => warn!(
1095+
%session_id,
1096+
%client_id,
1097+
error = ?e,
1098+
"[MCPNotifier] failed resources/list_changed"
1099+
),
11091100
}
11101101
}
11111102
}

crates/mcpmux-gateway/src/server/mod.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,6 @@ impl GatewayServer {
229229
// Create MCP notifier (session-keyed fanout, consults the same
230230
// FeatureSet resolver the request handlers use).
231231
let notification_bridge = Arc::new(MCPNotifier::new(
232-
self.services.space_resolver_service.clone(),
233232
self.services.feature_set_resolver.clone(),
234233
self.services.pool_services.feature_service.clone(),
235234
));

tests/rust/tests/streamable_http/gateway_notifications.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,6 @@ impl TestGateway {
198198

199199
// Create MCPNotifier
200200
let notifier = Arc::new(MCPNotifier::new(
201-
services.space_resolver_service.clone(),
202201
services.feature_set_resolver.clone(),
203202
services.pool_services.feature_service.clone(),
204203
));

0 commit comments

Comments
 (0)