Skip to content

Commit c9a18b7

Browse files
committed
fix(notifier): lazy-GC dead sessions on every fanout / per-peer push
rmcp's ServerHandler doesn't expose a session-close callback and the streamable-HTTP session manager owns the close path internally, so we have no obvious place to call unregister_session. Without GC the sessions map grows unbounded across the gateway's lifetime — every reconnect leaves stale entries that fanout iterates and the resolver attempts to re-route. What rmcp *does* give us is `Peer<R>::is_transport_closed()`, which flips true once the underlying transport has terminated. Reap lazily: each fanout / per-peer push snapshots the live session list, scans for closed peers, and removes them from both `sessions` and the `feature_set_resolver`'s `SessionRootsRegistry` in one pass. After the sweep the regular routing loop runs against the cleaned snapshot. Three call sites covered: get_peers_for_space (broadcasts), get_peers_for_space_with_streams (the variant used by the per-type notify_*_list_changed), and notify_peer_lists_changed (per-client push for resolution flips and grant edits). Logged at info level when dead > 0 so a future spike is visible. Adds `FeatureSetResolverService::session_roots()` accessor so the notifier can keep the two registries in sync — they were drifting silently otherwise. cargo check + clippy (-D warnings) clean. Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>
1 parent f08e8ec commit c9a18b7

2 files changed

Lines changed: 89 additions & 3 deletions

File tree

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

Lines changed: 82 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,61 @@ impl MCPNotifier {
322322
tracker.insert((space_id, NotificationType::All), timestamp);
323323
}
324324

325+
/// Lazy GC for dead sessions.
326+
///
327+
/// rmcp's `ServerHandler` doesn't expose a session-close callback, and
328+
/// the streamable-HTTP session manager owns the close path internally.
329+
/// What we *do* have on every `Peer<R>` is `is_transport_closed()` —
330+
/// it flips true once the underlying transport has terminated. So we
331+
/// reap lazily: every fanout / probe pass scans for closed peers and
332+
/// removes them from both `sessions` and `session_roots`.
333+
///
334+
/// Returns the ids that were reaped (for logging / metrics). Callers
335+
/// pass the live (snapshot) list of `(session_id, peer)` they were
336+
/// about to iterate; this mutates `self.sessions` and the
337+
/// `feature_set_resolver`'s session registry.
338+
fn reap_dead_sessions(&self, snapshot: &[(String, Arc<Peer<RoleServer>>)]) -> Vec<String> {
339+
let dead: Vec<String> = snapshot
340+
.iter()
341+
.filter_map(|(sid, peer)| {
342+
if peer.is_transport_closed() {
343+
Some(sid.clone())
344+
} else {
345+
None
346+
}
347+
})
348+
.collect();
349+
if dead.is_empty() {
350+
return dead;
351+
}
352+
{
353+
let mut sessions = self.sessions.write();
354+
for sid in &dead {
355+
sessions.remove(sid);
356+
}
357+
}
358+
// Also clean the session_roots registry the resolver consults so
359+
// it doesn't keep returning stale roots / capability flags for
360+
// sessions that no longer exist.
361+
for sid in &dead {
362+
self.feature_set_resolver.session_roots().remove(sid);
363+
}
364+
info!(
365+
reaped = dead.len(),
366+
"[MCPNotifier] 🧹 reaped dead sessions (transport closed)"
367+
);
368+
dead
369+
}
370+
325371
/// Get every peer whose **session** currently routes into `space_id`.
326372
///
327373
/// Iterates the session registry and re-runs the FeatureSet resolver
328374
/// per session — same logic the request handlers use, so a session
329375
/// redirected by `WorkspaceBinding` to a non-default space is matched
330376
/// correctly. Sessions whose stream isn't active yet are skipped (the
331-
/// notification would be queued but not delivered).
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.
332380
async fn get_peers_for_space(&self, space_id: Uuid) -> Vec<Arc<Peer<RoleServer>>> {
333381
let session_list: Vec<(String, String, Arc<Peer<RoleServer>>)> = {
334382
let sessions = self.sessions.read();
@@ -339,9 +387,22 @@ impl MCPNotifier {
339387
.collect()
340388
};
341389

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+
342400
let mut matching_peers = Vec::new();
343401
let _space_resolver = &self.space_resolver; // kept-but-unused; resolver below is authoritative
344402
for (session_id, client_id, peer) in session_list {
403+
if dead_set.contains(session_id.as_str()) {
404+
continue;
405+
}
345406
match self
346407
.feature_set_resolver
347408
.resolve(Some(&session_id), Some(&client_id))
@@ -811,10 +872,21 @@ impl MCPNotifier {
811872
.collect()
812873
};
813874

875+
let dead = self.reap_dead_sessions(
876+
&session_list
877+
.iter()
878+
.map(|(sid, _, peer)| (sid.clone(), peer.clone()))
879+
.collect::<Vec<_>>(),
880+
);
881+
let dead_set: std::collections::HashSet<&str> = dead.iter().map(String::as_str).collect();
882+
814883
let mut matching_peers = Vec::new();
815884
let mut matching_client_ids = Vec::new();
816885

817886
for (session_id, client_id, peer) in session_list {
887+
if dead_set.contains(session_id.as_str()) {
888+
continue;
889+
}
818890
match self
819891
.feature_set_resolver
820892
.resolve(Some(&session_id), Some(&client_id))
@@ -995,14 +1067,21 @@ impl MCPNotifier {
9951067
// editors, parallel CLI invocations). Push the notification on
9961068
// every active session for that client_id; client-side dedup is
9971069
// their problem, but missing a session would be ours.
998-
let peers: Vec<Arc<Peer<RoleServer>>> = {
1070+
let snapshot: Vec<(String, Arc<Peer<RoleServer>>)> = {
9991071
let sessions = self.sessions.read();
10001072
sessions
10011073
.iter()
10021074
.filter(|(_, e)| e.client_id == client_id && e.has_active_stream)
1003-
.map(|(_, e)| e.peer.clone())
1075+
.map(|(sid, e)| (sid.clone(), e.peer.clone()))
10041076
.collect()
10051077
};
1078+
let dead = self.reap_dead_sessions(&snapshot);
1079+
let dead_set: std::collections::HashSet<&str> = dead.iter().map(String::as_str).collect();
1080+
let peers: Vec<Arc<Peer<RoleServer>>> = snapshot
1081+
.into_iter()
1082+
.filter(|(sid, _)| !dead_set.contains(sid.as_str()))
1083+
.map(|(_, peer)| peer)
1084+
.collect();
10061085

10071086
if peers.is_empty() {
10081087
debug!(

crates/mcpmux-gateway/src/services/feature_set_resolver.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,13 @@ impl FeatureSetResolverService {
118118
}
119119
}
120120

121+
/// Borrow the session-roots registry. The notifier uses this to GC
122+
/// dead sessions out of the registry when reaping the corresponding
123+
/// peer entries — keeping both stores in sync.
124+
pub fn session_roots(&self) -> &Arc<SessionRootsRegistry> {
125+
&self.session_roots
126+
}
127+
121128
/// Resolve the effective (Space, FS list, source) tuple for a session.
122129
///
123130
/// `session_id`: the client's `mcp-session-id` header (or `None` when

0 commit comments

Comments
 (0)