Skip to content

server: fix client switching desync on reconnect - #85

Open
justdoGIT wants to merge 1 commit into
htrefil:masterfrom
justdoGIT:fix/client-switching-slab-desync
Open

server: fix client switching desync on reconnect#85
justdoGIT wants to merge 1 commit into
htrefil:masterfrom
justdoGIT:fix/client-switching-slab-desync

Conversation

@justdoGIT

@justdoGIT justdoGIT commented Sep 4, 2026

Copy link
Copy Markdown

Fixes #65

Using a Slab for client storage causes client indices to become non-contiguous when clients disconnect and reconnect. Because the switching logic assumed contiguous 0..clients.len() indexing:

let exists = |idx| idx == 0 || clients.contains(idx - 1);
loop {
    current = (current + 1) % (clients.len() + 1);
    if exists(current) {
        break;
    }
}

A reconnected client assigned a slab key > 0 while earlier slots are vacant would fail exists(current) and immediately cycle back to current = 0 (server), producing the exact symptom described in #65:

INFO rkvm_server::server: Switched client idx=0

The switch hotkey becomes completely unresponsive to that client until the server process is restarted.

This PR replaces Slab with a Vec for client storage so indices remain contiguous, eliminating index drift and ensuring reliable round-robin switching across disconnects and multiple clients.

Using a Slab for client storage causes client indices to become
non-contiguous when clients disconnect and reconnect. Because the
switching logic assumed contiguous 0..clients.len() indexing, a
reconnected client with key > 0 would fail exists(current) and
immediately reset back to the local server (index 0).

Replace the Slab with a Vec so client indices are always contiguous
and modular rotation safely cycles through all connected clients.

Signed-off-by: KK <pandeykamal13526@gmail.com>
Copilot AI lite review requested due to automatic review settings September 4, 2026 01:17
@justdoGIT justdoGIT mentioned this pull request Sep 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new Vec compaction/removal paths don’t consistently adjust both current and previous, which can lead to incorrect targeting and potential out-of-bounds panics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes client switching becoming unresponsive after disconnect/reconnect by changing client storage from Slab (non-contiguous keys after removals) to Vec (contiguous indices), so the round-robin switch (current + 1) % (clients.len() + 1) remains valid across reconnects.

Changes:

  • Replaced client storage from Slab<(Sender<_>, SocketAddr)> to Vec<(Sender<_>, SocketAddr)>.
  • Simplified the switch-hotkey logic to a single modulo step (no “exists” scan).
  • Adjusted client removal handling when a send fails to keep current consistent with shifted Vec indices.
File summaries
File Description
rkvm-server/src/server.rs Switch client bookkeeping from Slab to Vec to keep indices contiguous and make switching reliable across disconnects/reconnects.
Review details

Suppressed comments (1)

rkvm-server/src/server.rs:259

  • When a send fails and you remove(idx - 1), the vector shifts. The code updates current but not previous, even though previous can later be used as an index (idx = previous while changed is true). This can leave previous out of range and cause an out-of-bounds panic on clients[idx - 1], especially if the removed client was previous or was before it.
                        if clients[idx - 1].0.send(Update::Event { id, event }).await.is_err() {
                            clients.remove(idx - 1);

                            if current == idx {
                                current = 0;
                            } else if current > idx {
                                current -= 1;
                            }
                            break;
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rkvm-server/src/server.rs
Comment on lines 65 to 69
// Remove dead clients.
clients.retain(|_, (client, _)| !client.is_closed());
if !clients.contains(current) {
clients.retain(|(client, _)| !client.is_closed());
if current > clients.len() {
current = 0;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nothing happens

2 participants