fix(server): cap lobby subscribers before AddSubscriber - #2117
Conversation
Reject SubscribeLobby once MAX_LOBBY_SUBSCRIBERS is reached to bound fan-out sender storage and full lobby snapshot work. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a connection capacity guard for lobby subscribers to prevent unbounded memory usage and broadcast fan-out by limiting subscribers to 128. Feedback on the changes points out a critical issue where stale or closed subscriber channels are not pruned before checking the capacity, which can lead to false rejections of new subscribers as inactive connections continue to count toward the limit.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut subs = lobby_subscribers.lock().await; | ||
| if let Err(reason) = guard_lobby_subscriber_capacity(subs.len()) { | ||
| let _ = tx.send(ServerMessage::Error { message: reason }); | ||
| continue; | ||
| } | ||
| subs.push(tx.clone()); |
There was a problem hiding this comment.
[HIGH] Stale/closed subscribers are not pruned before checking capacity.
Evidence: crates/phase-server/src/main.rs:1705-1710.
Why it matters: If closed subscriber channels are not pruned before checking capacity, stale connections will be counted toward the limit, leading to false rejections of new subscribers.
Suggested fix: Retain only active senders before checking capacity.
let mut subs = lobby_subscribers.lock().await;
subs.retain(|s| !s.is_closed());
if let Err(reason) = guard_lobby_subscriber_capacity(subs.len()) {
let _ = tx.send(ServerMessage::Error { message: reason });
continue;
}
subs.push(tx.clone());There was a problem hiding this comment.
Code Review
This pull request introduces a server-wide capacity limit of 128 live lobby subscribers to prevent unbounded memory usage and broadcast fan-out, implementing a new validation guard with corresponding unit tests. However, the review identifies a critical state desynchronization issue where checking the capacity in apply_outbounds occurs after the connection state has already committed identity.lobby_subscribed = true, resulting in a "ghost" subscription. The reviewer recommends moving this capacity check to handle_client_message under ClientMessage::SubscribeLobby to safely reject subscriptions before mutating the broker state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut subs = lobby_subscribers.lock().await; | ||
| if let Err(reason) = guard_lobby_subscriber_capacity(subs.len()) { | ||
| let _ = tx.send(ServerMessage::Error { message: reason }); | ||
| continue; | ||
| } | ||
| subs.push(tx.clone()); |
There was a problem hiding this comment.
[HIGH] State desynchronization when lobby subscriber capacity is exceeded. Evidence: crates/phase-server/src/main.rs:1706-1709.
Why it matters: apply_outbounds is executed after identity.absorb_conn_state(conn) has already committed identity.lobby_subscribed = true. Rejecting the subscription here leaves the socket identity in a desynchronized "ghost" subscription state where the client and broker believe they are subscribed, but they will never receive any lobby updates.
Suggested fix: Move the capacity check to handle_client_message under ClientMessage::SubscribeLobby so the subscription is rejected before the broker state is mutated and absorbed.
ClientMessage::SubscribeLobby => {
let subs_len = lobby_subscribers.lock().await.len();
if let Err(reason) = guard_lobby_subscriber_capacity(subs_len) {
let _ = tx.send(ServerMessage::Error { message: reason });
return;
}
dispatch_broker(
&client_msg,
lobby,
lobby_subscribers,
player_count,
tx,
identity,
)
.await;
} let mut subs = lobby_subscribers.lock().await;
subs.push(tx.clone());Co-authored-by: Cursor <cursoragent@cursor.com>
🤖 Architecture Review (automated)Verdict: ✅ Approve w/ nits Seam: PASS — Pure transport/wire guard in Findings
CR verification: N/A — no game-rule logic in this diff (server transport only), so no CR annotations expected or required. |
🏛️ Architecture & Idiom Deep-Dive (automated)Verdict: 🔧 Minor polish What's idiomatic here: The new Idiom / architecture findings
|
🔁 Re-review (upgraded process)Prior verdict: ✅ Approve w/ nits → Revised:
Missed or re-rated
Both fixes converge: lift subscriber reservation into a single |
|
Maintainer cleanup pushed on head e1db3b4. What changed:
Local verification:
Broad validation is left to GitHub CI. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer cleanup on head e1db3b4. The subscriber cap now reserves the slot before broker state absorption, prunes stale senders, dedups same-channel subscriptions, fixes unsubscribe cleanup, and GitHub CI is green.
# Conflicts: # crates/phase-server/src/main.rs # crates/server-core/src/lib.rs
Closes #2115.
SubscribeLobbynow rejects new lobby subscribers onceMAX_LOBBY_SUBSCRIBERS(128) is reached, beforeAddSubscriberpushes the sender.Real Behavior Proof
apply_outboundsbeforesubs.pushServerMessage::Errorvia its connection queueTest plan
cargo test -p server-core lobby_subscriber_wire_guard -- --nocapturecargo fmt --all -- --check