[Client] Fix spurious server room join error (#7259)

* [Client] Fix spurious server room join error

The server replies RespContextError when a join command is received for a
room that connection is already registered in. The client was sending such
duplicate joins in benign situations - double-clicking to join a room, or
clicking a room the selector was already auto-joining - and answered them
with a modal telling users to restart the client.

Joins for the same room are now deduplicated while one is in flight, and a
remaining RespContextError is healed by leaving and rejoining the room so
the tab appears without a client restart. Error dialogs are only shown for
user-initiated joins, so failed auto-joins no longer spam critical popups.

* [Client] Bound stale-membership room join heal to one attempt

The RespContextError heal (leave + rejoin) previously recurred
unconditionally, so a server that kept returning RespContextError for a
reason other than stale membership would loop forever. Track room ids
that already received a heal and surface the error dialog after one
attempt instead of retrying indefinitely.

* [Client] Scope room-join heal guard to one join attempt

* [Client] Hoist room-join heal guard lookup out of response switch

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/widgets/tabs/tab_server.cpp

@@ -13,6 +13,7 @@
 #include <libcockatrice/protocol/pb/event_list_rooms.pb.h>
 #include <libcockatrice/protocol/pb/event_server_message.pb.h>
 #include <libcockatrice/protocol/pb/response_join_room.pb.h>
+#include <libcockatrice/protocol/pb/room_commands.pb.h>
 #include <libcockatrice/protocol/pb/session_commands.pb.h>
 #include <libcockatrice/protocol/pending_command.h>
 
@@ -185,25 +186,37 @@ void TabServer::processServerMessageEvent(const Event_ServerMessage &event)
 void TabServer::joinRoom(int id, bool setCurrent)
 {
     TabRoom *room = tabSupervisor->getRoomTabs().value(id);
-    if (!room) {
+    if (room) {
-        Command_JoinRoom cmd;
+        if (setCurrent) {
-        cmd.set_room_id(id);
+            tabSupervisor->setCurrentWidget((QWidget *)room);
-
+        }
-        PendingCommand *pend = client->prepareSessionCommand(cmd);
-        pend->setExtraData(setCurrent);
-        connect(pend, &PendingCommand::finished, this,
-                [this, id](const Response &r, const CommandContainer &c, const QVariant &v) {
-                    joinRoomFinished(r, c, v, id);
-                });
-
-        client->sendCommand(pend);
-
         return;
     }
 
-    if (setCurrent) {
+    auto pendingIt = pendingRoomJoins.find(id);
-        tabSupervisor->setCurrentWidget((QWidget *)room);
+    if (pendingIt != pendingRoomJoins.end()) {
+        // A join for this room is already in flight: the room tab opens when its response
+        // arrives. Fold the new request into the pending one so that, for example, clicking
+        // a room the selector is auto-joining does not send a second Command_JoinRoom - the
+        // server would reject that duplicate with RespContextError.
+        if (setCurrent) {
+            pendingIt.value() = true;
+        }
+        return;
     }
+
+    pendingRoomJoins.insert(id, setCurrent);
+
+    Command_JoinRoom cmd;
+    cmd.set_room_id(id);
+
+    PendingCommand *pend = client->prepareSessionCommand(cmd);
+    pend->setExtraData(setCurrent);
+    connect(
+        pend, &PendingCommand::finished, this,
+        [this, id](const Response &r, const CommandContainer &c, const QVariant &v) { joinRoomFinished(r, c, v, id); });
+
+    client->sendCommand(pend);
 }
 
 void TabServer::joinRoomFinished(const Response &r,
@@ -211,34 +224,72 @@ void TabServer::joinRoomFinished(const Response &r,
                                  const QVariant &extraData,
                                  int roomId)
 {
+    const bool setCurrent = pendingRoomJoins.value(roomId, extraData.toBool());
+    pendingRoomJoins.remove(roomId);
+    const bool healedJoin = healedRoomJoins.contains(roomId);
+    healedRoomJoins.remove(roomId);
+
     switch (r.response_code()) {
         case Response::RespOk:
             break;
         case Response::RespNameNotFound:
-            QMessageBox::critical(this, tr("Error"),
+            if (setCurrent) {
-                                  tr("Failed to join the server room: it doesn't exist on the server."));
+                QMessageBox::critical(this, tr("Error"),
+                                      tr("Failed to join the server room: it doesn't exist on the server."));
+            }
             emit roomJoinFailed(roomId);
             return;
         case Response::RespContextError:
-            QMessageBox::critical(
+            if (healedJoin) {
-                this, tr("Error"),
+                // The rejoin below was already answered and the server still rejects the join, so
-                tr("The server thinks you are in the server room but your client is unable to display it. "
+                // the stale-membership heal cannot help: surface the error. The guard was already
-                   "Try restarting your client."));
+                // released above so a later user-initiated join may try a fresh heal.
-            emit roomJoinFailed(roomId);
+                if (setCurrent) {
+                    QMessageBox::critical(
+                        this, tr("Error"),
+                        tr("The server thinks you are in the server room but your client is unable to display it. "
+                           "Try restarting your client."));
+                }
+                emit roomJoinFailed(roomId);
+                return;
+            }
+            // The server already had us registered in the room even though no tab was open,
+            // usually because two join attempts for the same room overlapped. Leaving and
+            // rejoining makes the server reply with a fresh RespOk so the tab is displayed
+            // without requiring a client restart. The guard above covers exactly the rejoin that
+            // leaveAndRejoinRoom triggers, so a server that keeps replying with RespContextError
+            // gets one heal attempt per join instead of an endless recursion.
+            healedRoomJoins.insert(roomId);
+            leaveAndRejoinRoom(roomId, setCurrent);
             return;
         case Response::RespUserLevelTooLow:
-            QMessageBox::critical(this, tr("Error"),
+            if (setCurrent) {
-                                  tr("You do not have the required permission to join this server room."));
+                QMessageBox::critical(this, tr("Error"),
+                                      tr("You do not have the required permission to join this server room."));
+            }
             emit roomJoinFailed(roomId);
             return;
         default:
-            QMessageBox::critical(
+            if (setCurrent) {
-                this, tr("Error"),
+                QMessageBox::critical(
-                tr("Failed to join the server room due to an unknown error: %1.").arg(r.response_code()));
+                    this, tr("Error"),
+                    tr("Failed to join the server room due to an unknown error: %1.").arg(r.response_code()));
+            }
             emit roomJoinFailed(roomId);
             return;
     }
 
     const Response_JoinRoom &resp = r.GetExtension(Response_JoinRoom::ext);
-    emit roomJoined(resp.room_info(), extraData.toBool());
+    emit roomJoined(resp.room_info(), setCurrent);
+}
+
+void TabServer::leaveAndRejoinRoom(int roomId, bool setCurrent)
+{
+    // Clear the stale room membership server-side. The leave is sent before the rejoin below,
+    // so the server no longer considers us a member by the time the join arrives. The leave
+    // response is intentionally not awaited: commands are processed in send order on the
+    // connection, and a failed leave (RespNotInRoom) only means the membership was already gone.
+    client->sendCommand(client->prepareRoomCommand(Command_LeaveRoom(), roomId));
+
+    joinRoom(roomId, setCurrent);
 }

cockatrice/src/interface/widgets/tabs/tab_server.h

@@ -10,6 +10,8 @@
 #include "tab.h"
 
 #include <QGroupBox>
+#include <QHash>
+#include <QSet>
 #include <QTextBrowser>
 #include <QTreeWidget>
 
@@ -58,10 +60,17 @@ private slots:
                           int roomId);
 
 private:
+    void leaveAndRejoinRoom(int roomId, bool setCurrent);
+
     AbstractClient *client;
     RoomSelector *roomSelector;
     QTextBrowser *serverInfoBox;
     bool shouldEmitUpdate = false;
+    /** Room ids with a join command in flight, mapped to whether the tab should be focused once it opens. */
+    QHash<int, bool> pendingRoomJoins;
+    /** Room ids for which a stale-membership heal (leave + rejoin) is currently in flight. Released as soon as the
+     *  rejoin has been answered, so a heal is attempted at most once per join. */
+    QSet<int> healedRoomJoins;
 
 public:
     TabServer(TabSupervisor *_tabSupervisor, AbstractClient *_client);