fix: Only hand polling over to the chat relay once the room is joined on the HPB

NextcloudTalk/Chat/NCChatController.swift

@@ -34,7 +34,7 @@ public class NCChatController: NSObject {
     private var getHistoryTask: URLSessionDataTask?
     private var pullMessagesTask: URLSessionDataTask?
 
-    private enum ChatRelayState {
+    enum ChatRelayState {
         case inactive, active, catchingUp
     }
 
@@ -58,7 +58,9 @@ public class NCChatController: NSObject {
 
         super.init()
 
-        setupChatRelay()
+        let signalingController = NCSettingsController.sharedInstance().externalSignalingController(forAccountId: account.accountId)
+        setupChatRelay(with: signalingController)
+
         AllocationTracker.shared.addAllocation("NCChatController")
     }
 
@@ -71,7 +73,9 @@ public class NCChatController: NSObject {
 
         super.init()
 
-        setupChatRelay()
+        let signalingController = NCSettingsController.sharedInstance().externalSignalingController(forAccountId: account.accountId)
+        setupChatRelay(with: signalingController)
+
         AllocationTracker.shared.addAllocation("NCChatController")
     }
 
@@ -425,9 +429,17 @@ public class NCChatController: NSObject {
 
     // MARK: - External Signaling / Chat Relay
 
-    private func setupChatRelay() {
-        guard let signalingController = NCSettingsController.sharedInstance().externalSignalingController(forAccountId: account.accountId),
-              signalingController.hasChatRelay else { return }
+    // The signaling server only sends us the events of the room our session joined, so this is the
+    // condition for letting the relay take over from polling the chat API.
+    private var canChatRelayTakeOverPolling: Bool {
+        guard let externalSignalingController, externalSignalingController.hasChatRelay else { return false }
+
+        return externalSignalingController.joinedRoomToken == room.token
+    }
+
+    private func setupChatRelay(with signalingController: NCExternalSignalingController?) {
+        guard let signalingController, signalingController.hasChatRelay else { return }
+
         externalSignalingController = signalingController
         chatRelayMessagesQueue = DispatchQueue(label: "chat.relay.message.queue")
         NotificationCenter.default.addObserver(self, selector: #selector(didReceiveChatMessageFromExternalSignaling(_:)), name: .extSignalingDidReceiveChatMessage, object: signalingController)
@@ -463,6 +475,32 @@ public class NCChatController: NSObject {
         }
     }
 
+    // Called when the messages long poll reports the chat is up to date on a server with the chat relay.
+    // Handing over before our session joined the room would lose every message posted until the join is
+    // acked: the long poll stops, nothing is relayed to us yet, and the next relayed message advances the
+    // chat block past the gap. So keep long polling instead, every long poll ends up here again.
+    private func handOverPollingToChatRelay(fromMessagesId messageId: Int) {
+        if canChatRelayTakeOverPolling {
+            print("Chat is up to date, now processing new messages from the chat relay")
+            startProcessingChatRelayMessages()
+            return
+        }
+
+        print("Chat is up to date, but we did not join the room on the signaling server yet, keep polling")
+
+        // This long poll didn't arm the relay, so we are back to plain long polling
+        resetChatRelayState()
+        startReceivingChatMessages(fromMessagesId: messageId, withTimeout: true)
+    }
+
+    // Without this, a request that ends without arming the relay and without continuing the long poll
+    // (brute-force protection, blocked chat, thread not found) leaves it in `.catchingUp` forever.
+    private func resetChatRelayState() {
+        chatRelayMessagesQueue?.async {
+            self.chatRelayState = .inactive
+        }
+    }
+
     private func startProcessingChatRelayMessages() {
         chatRelayMessagesQueue?.async {
             self.chatRelayState = .active
@@ -1147,18 +1185,21 @@ public class NCChatController: NSObject {
 
             if let error {
                 if self.isChatBeingBlocked(statusCode) {
+                    self.resetChatRelayState()
                     self.notifyChatIsBlocked()
                     return
                 }
 
                 if statusCode == 404 {
                     NCLog.log("Thread not found error: \(error.description)")
+                    self.resetChatRelayState()
                     NotificationCenter.default.post(name: .NCChatControllerDidReceiveThreadNotFound, object: self, userInfo: nil)
                     return
                 }
 
                 if statusCode == 429 {
                     NCLog.log("Brute-force protected, received 429 while receiving messages. No further polling.")
+                    self.resetChatRelayState()
                     return
                 }
 
@@ -1180,6 +1221,7 @@ public class NCChatController: NSObject {
                         // When we receive a "history_cleared" message, we don't continue here, as otherwise
                         // we would request new messages, but instead, we need to request the initial history again
                         if message?.systemMessage == "history_cleared" {
+                            self.resetChatRelayState()
                             return
                         }
                     }
@@ -1194,9 +1236,8 @@ public class NCChatController: NSObject {
                 let chatIsUpToDate = statusCode == 304
                 let lastChatBlock = self.chatBlocksForRoomOrThread().last
 
-                if chatIsUpToDate, let extSignaling = self.externalSignalingController, extSignaling.hasChatRelay {
-                    print("Chat is up to date, now processing new messages from the chat relay")
-                    self.startProcessingChatRelayMessages()
+                if chatIsUpToDate, self.externalSignalingController?.hasChatRelay == true {
+                    self.handOverPollingToChatRelay(fromMessagesId: lastChatBlock?.newestMessageId ?? 0)
                     return
                 }
 
@@ -1436,4 +1477,17 @@ extension NCChatController {
     // triggerChatRelayCatchUpForTesting() actually schedules the restart on the main queue, mirroring
     // a catch-up that fires while the user is still in the room (just before they leave).
     func markChatRelayActiveForTesting() { chatRelayState = .active }
+
+    var chatRelayStateForTesting: ChatRelayState { chatRelayState }
+
+    // Mirrors the messages long poll reporting the chat is up to date (304) on a server with the relay.
+    func handOverPollingToChatRelayForTesting() { handOverPollingToChatRelay(fromMessagesId: 0) }
+
+    // Waits until everything queued on the relay queue ran, so tests don't have to guess timings.
+    func waitForChatRelayQueueForTesting() { chatRelayMessagesQueue?.sync {} }
+
+    // There is no signaling controller configured for the fake account, so tests pass in their own.
+    func setupChatRelayForTesting(with signalingController: NCExternalSignalingController) {
+        setupChatRelay(with: signalingController)
+    }
 }

NextcloudTalk/WebRTC/NCExternalSignalingController.swift

@@ -43,6 +43,9 @@ public enum NCExternalSignalingSendMessageStatus {
 
     public var currentRoom: String?
 
+    // The room our session is in on the signaling server right now, cleared on every reconnect unlike `currentRoom`.
+    public private(set) var joinedRoomToken: String?
+
     public private(set) var account: TalkAccount
     public private(set) var disconnected: Bool = true
     public private(set) var hasMCU: Bool = false
@@ -210,6 +213,7 @@ public enum NCExternalSignalingSendMessageStatus {
         self.webSocket?.cancel()
         self.webSocket = nil
         self.helloResponseReceived = false
+        self.joinedRoomToken = nil
         self.helloMessage?.ignoreCompletionBlock()
         self.helloMessage = nil
         self.disconnected = true
@@ -340,6 +344,14 @@ public enum NCExternalSignalingSendMessageStatus {
         let sessionChanged = self.sessionId != newSessionId
         self.sessionId = newSessionId
 
+        if sessionChanged {
+            // The new session did not join any room yet, the re-join below takes care of that
+            self.joinedRoomToken = nil
+        } else {
+            // The session was resumed, so the server kept us in the room and replays what we missed
+            self.joinedRoomToken = self.currentRoom
+        }
+
         guard let serverDict = helloDict["server"] as? [AnyHashable: Any],
               let serverFeatures = serverDict["features"] as? [String],
               let serverVersion = serverDict["version"] as? String
@@ -402,8 +414,10 @@ public enum NCExternalSignalingSendMessageStatus {
                   let roomId = roomDict["roomid"] as? String
             else { return }
 
-            // If we are aware that we were in this room before, we should treat this as a success
+            // If we are aware that we were in this room before, we should treat this as a success.
+            // No room message follows here, so we have to set the joined room ourselves.
             if currentRoom == roomId {
+                self.joinedRoomToken = roomId
                 self.executeCompletionBlock(forMessageId: messageId, withStatus: .success)
                 return
             }
@@ -462,6 +476,7 @@ public enum NCExternalSignalingSendMessageStatus {
     func leaveRoom(withRoomId roomId: String) {
         if self.currentRoom == roomId {
             self.currentRoom = nil
+            self.joinedRoomToken = nil
             self.joinRoom(withRoomId: "", withSessionId: "", withFederation: nil, withCompletionBlock: nil)
         } else {
             print("External signaling: Not leaving because it's not the room we joined")
@@ -556,6 +571,10 @@ public enum NCExternalSignalingSendMessageStatus {
             self.currentRoom = newRoomId.isEmpty ? nil : newRoomId
         }
 
+        // Outside the check above on purpose: re-joining after a reconnect leaves `currentRoom`
+        // unchanged, but it is the moment we are part of the room on the signaling server again.
+        self.joinedRoomToken = newRoomId.isEmpty ? nil : newRoomId
+
         if let messageId = messageDict["id"] as? String {
             self.executeCompletionBlock(forMessageId: messageId, withStatus: .success)
         }

NextcloudTalkTests/Unit/Chat/UnitChatViewControllerTest.swift

@@ -196,6 +196,86 @@ final class UnitChatViewControllerTest: TestBaseRealm {
                       "A chat-relay catch-up scheduled before stop() must not resume polling once stop() has run")
     }
 
+    // MARK: - Chat relay
+
+    // A signaling controller that advertises the chat relay but never connects: without a websocket
+    // the callbacks of the failing connection attempt are ignored, so nothing reconnects underneath
+    // the assertions.
+    private func inertSignalingController() -> NCExternalSignalingController {
+        let account = NCDatabaseManager.sharedInstance().activeAccount()
+        let signalingController = NCExternalSignalingController(account: account, serverUrl: TestConstants.server, ticket: "fakeTicket")
+
+        signalingController.disconnect()
+        signalingController.helloResponseReceived(messageDict: [
+            "type": "hello",
+            "id": "1",
+            "hello": [
+                "sessionid": "session-1",
+                "server": ["version": "2.0.0", "features": ["chat-relay"]]
+            ]
+        ])
+
+        return signalingController
+    }
+
+    private func drainMainQueue() {
+        let exp = expectation(description: "\(#function)\(#line)")
+        DispatchQueue.main.async { exp.fulfill() }
+        waitForExpectations(timeout: TestConstants.timeoutShort, handler: nil)
+    }
+
+    func testUpToDateChatKeepsPollingUntilTheRoomIsJoinedOnTheSignalingServer() throws {
+        let room = addRoom(withToken: "relayJoinRoom")
+        let chatController = NCChatController(for: room)!
+        let signalingController = inertSignalingController()
+
+        chatController.setupChatRelayForTesting(with: signalingController)
+
+        // The chat is up to date, but our session is not in the room yet, so the relay would not
+        // deliver anything to us: keep long polling instead of handing over.
+        chatController.handOverPollingToChatRelayForTesting()
+        chatController.waitForChatRelayQueueForTesting()
+        XCTAssertEqual(chatController.chatRelayStateForTesting, .inactive)
+        XCTAssertFalse(chatController.isReceivingMessagesStoppedForTesting)
+
+        // Being in *another* room is not enough either
+        signalingController.roomMessageReceived(messageDict: ["type": "room", "room": ["roomid": "someOtherRoom"]])
+        chatController.handOverPollingToChatRelayForTesting()
+        chatController.waitForChatRelayQueueForTesting()
+        XCTAssertEqual(chatController.chatRelayStateForTesting, .inactive)
+
+        // Our room was acked, so the first long poll reporting an up-to-date chat after that hands over
+        signalingController.roomMessageReceived(messageDict: ["type": "room", "room": ["roomid": room.token]])
+        chatController.handOverPollingToChatRelayForTesting()
+        chatController.waitForChatRelayQueueForTesting()
+        XCTAssertEqual(chatController.chatRelayStateForTesting, .active)
+
+        chatController.stop()
+        drainMainQueue()
+    }
+
+    func testCatchUpDoesNotHandOverToTheRelayWhileTheRoomIsNotJoined() throws {
+        let room = addRoom(withToken: "relayCatchUpRoom")
+        let chatController = NCChatController(for: room)!
+        let signalingController = inertSignalingController()
+
+        chatController.setupChatRelayForTesting(with: signalingController)
+
+        // The relay is active and a catch-up is running when the session is replaced by a reconnect
+        // that could not resume. The catch-up must not re-arm the relay: the new session did not
+        // re-join the room yet.
+        signalingController.roomMessageReceived(messageDict: ["type": "room", "room": ["roomid": room.token]])
+        chatController.markChatRelayActiveForTesting()
+        signalingController.resetWebSocket()
+
+        chatController.handOverPollingToChatRelayForTesting()
+        chatController.waitForChatRelayQueueForTesting()
+        XCTAssertEqual(chatController.chatRelayStateForTesting, .inactive)
+
+        chatController.stop()
+        drainMainQueue()
+    }
+
     func testContentInsetAdjustsForOverlayViews() throws {
         let activeAccount = NCDatabaseManager.sharedInstance().activeAccount()
         let room = NCRoom()