Multiple sessions from different devices broken

[SystemKeeper]

How to use GitHub

• Please use the 👍 reaction to show that you are affected by the same issue.
• Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
• Subscribe to receive notifications on status change and new comments.

---

Steps to reproduce

1. Join "Conversation 1" in Web
2. Join "Conversation 1" in Talk-iOS
3. Web gets redirected to "Conversation not found"

Expected behaviour

Both should be able to join.

Actual behaviour

The web session is redirected to "Conversation not found"

Talk app

Tested on sermo.

[nickvergessen]

Tried reverting #8265 on sermo but that did not fix it.

[nickvergessen]

Reverting nextcloud/server#34934 seems to fix it 🙊

cc @juliushaertl any clue about potential side effects of your PR?

[nickvergessen]

PS: only reverting the first commit is enough to fix the problem. 😿

[SystemKeeper]

That PR also got backported to 25.0.2 RC2, so the error should be seen on c.nc.c as well, shouldn’t it?

[juliusknorr]

I still see this happening with the patch reverted (just that the original bug of #8265 happens more often and before that) and at least currently can't imagine what kind of side effect that might be.

One thing I noticed when stepping through the session joining with multiple browsers was that it seems to delete the oc_talk_session entry once the second browser joins the session.

This seems to be triggered by https://github.com/nextcloud/spreed/blob/3d49db3b713b7fd6379b288a379f30401400cd88/lib/Controller/RoomController.php#LL1355C49-L1355C49 returning null, which then fetches any session when getting the previous participant/session in

spreed/lib/Controller/RoomController.php

Line 1369 in 3d49db3

$previousSession = $previousParticipant->getSession();

.

According to PHPdoc null is trying to load any session, which I think is probably wrong here.

Small patch that seemed to work on my testing, but I'm not fully sure if that makes sense:

diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php
index 958cbb23e..35086ad1c 100644
--- a/lib/Controller/RoomController.php
+++ b/lib/Controller/RoomController.php
@@ -1345,6 +1345,7 @@ class RoomController extends AEnvironmentAwareController {
        /**
         * @PublicPage
         * @BruteForceProtection(action=talkRoomPassword)
         *
         * @param string $token
         * @param string $password
@@ -1365,7 +1366,7 @@ class RoomController extends AEnvironmentAwareController {
                $previousSession = null;
                if ($this->userId !== null) {
                        try {
-                               $previousParticipant = $this->participantService->getParticipant($room, $this->userId, $sessionId);
+                               $previousParticipant = $this->participantService->getParticipant($room, $this->userId, $sessionId ?? '');
                                $previousSession = $previousParticipant->getSession();
                        } catch (ParticipantNotFoundException $e) {
                        }

[juliusknorr]

In addition it might make sense to re-add the @UseSession annotation to the joinRoom and leaveRoom controller methods to ensure that the session is locked during those operations. This might help with keeping the session state consistent between multiple concurrent requests.

The original root cause of this was resolved in the server PR.

The only scenario I could reproduce still was that when quickly switching between rooms in one browser window sometimes the signaling API request was still sent with the old room id even though the leave and join room requests already happened, but that sounds more like a frontend bug:

This then leads to a "The conversation does not exist" page.

[nickvergessen]

Did something improve with the latest server changes @SystemKeeper @mahibi ?