From 9d3283960c7aebcffee62790729f8fa6734ac3d7 Mon Sep 17 00:00:00 2001 From: Alain Lauzon Date: Sun, 21 Jun 2026 13:52:05 -0400 Subject: [PATCH] Fix #6169: elect call offerer using same-namespace session IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In HPB (external signaling) mode the remote participant sessionId passed to PeerConnectionWrapper is a signaling-layer session ID, while the local session used for the offerer election (callSession) is the Nextcloud session ID. The two peers then compare unrelated strings and can both compute the same role: both "not offerer" (no offer is created — the call deadlocks) or both "offerer" (offer collision). The requestoffer fallback only applies in MCU mode, so direct 2-party calls deadlock outright. Pass the local signaling session (webSocketClient.sessionId when external signaling is used, callSession otherwise — matching the existing currentSessionId resolution) so both peers compare the same namespace. Also extract the election into PeerConnectionWrapper.isOfferer() and add PeerConnectionWrapperOffererRoleTest covering the contract and the cross-namespace failure mode. Signed-off-by: Alain Lauzon Co-Authored-By: Claude Opus 4.8 (1M context) --- .../nextcloud/talk/activities/CallActivity.kt | 6 +- .../talk/webrtc/PeerConnectionWrapper.java | 26 +++++++- .../PeerConnectionWrapperOffererRoleTest.kt | 64 +++++++++++++++++++ 3 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperOffererRoleTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt b/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt index c6af2d4462..7a45a2e1b3 100644 --- a/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt @@ -2485,7 +2485,11 @@ class CallActivity : CallBaseActivity() { iceServers, tempSdpConstraints, sessionId, - callSession, + // #6169: the remote sessionId is a signaling-layer session ID when external signaling + // is used, so the offerer election must compare same-namespace IDs. Pass the local + // signaling session (matching the currentSessionId resolution used elsewhere); with + // internal signaling webSocketClient == null, so callSession is used as before. + if (webSocketClient != null) webSocketClient!!.sessionId else callSession, tempLocalStream, tempIsMCUPublisher, tempHasMCU, diff --git a/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java b/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java index ba726753d1..484ad34460 100644 --- a/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java +++ b/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java @@ -116,7 +116,7 @@ public PeerConnectionWrapper(PeerConnectionFactory peerConnectionFactory, this.mediaConstraints = mediaConstraints; sdpObserver = new SdpObserver(); - boolean hasInitiated = sessionId.compareTo(localSession) < 0; + boolean hasInitiated = isOfferer(localSession, sessionId); this.isMCUPublisher = isMCUPublisher; PeerConnection.RTCConfiguration configuration = new PeerConnection.RTCConfiguration(iceServerList); @@ -165,6 +165,30 @@ public PeerConnectionWrapper(PeerConnectionFactory peerConnectionFactory, } } + /** + * Decides whether this client takes the offerer role for a peer connection. + * + *

The role is resolved purely by lexicographic comparison of the two session IDs: + * the client whose remote session ID sorts before its own local session ID becomes the + * offerer. This yields exactly one offerer per pair only if both peers compare the + * same ordered pair of strings. + * + *

Bug #6169: with the High-Performance Backend the remote {@code sessionId} is a + * signaling-layer session ID. Callers must therefore pass a {@code localSession} from the + * same namespace (the local signaling session, not the Nextcloud session); otherwise + * the two peers compare unrelated strings and can both compute {@code false} (neither offers — + * deadlock) or both {@code true} (offer collision). The {@code requestoffer} fallback only + * applies in MCU mode, so a direct (no-MCU) 2-party call deadlocks outright. The deadlock is + * exercised by {@code PeerConnectionWrapperOffererRoleTest}. + * + * @param localSession this client's own session ID (same namespace as {@code remoteSession}) + * @param remoteSession the remote peer's session ID + * @return true if this client should create the initial offer + */ + static boolean isOfferer(String localSession, String remoteSession) { + return remoteSession.compareTo(localSession) < 0; + } + public void raiseHand(Boolean raise) { NCMessagePayload ncMessagePayload = new NCMessagePayload(); ncMessagePayload.setState(raise); diff --git a/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperOffererRoleTest.kt b/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperOffererRoleTest.kt new file mode 100644 index 0000000000..897433920e --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperOffererRoleTest.kt @@ -0,0 +1,64 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.webrtc + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Test + +/** + * Tests for the offerer-role election ([PeerConnectionWrapper.isOfferer]) — bug #6169. + * + * Contract: for a pair of peers, exactly one must create the initial offer. The role is resolved + * by a lexicographic comparison of the two session IDs, which yields exactly one offerer **only + * if both peers compare the same ordered pair of strings** — i.e. the local and remote session + * IDs are in the same namespace. + * + * #6169: in High-Performance Backend mode the remote `sessionId` is a signaling-layer session ID. + * The fix passes a local signaling session ID (not the Nextcloud session ID) at the construction + * site so both peers compare the same pair. These tests pin both the desired contract and the + * failure mode that occurs if cross-namespace IDs are ever passed again. + */ +class PeerConnectionWrapperOffererRoleTest { + + /** Roles computed by BOTH peers for one consistent (same-namespace) pair of session IDs. */ + private fun rolesForConsistentPair(sessionA: String, sessionB: String): Pair { + val aIsOfferer = PeerConnectionWrapper.isOfferer(sessionA, sessionB) + val bIsOfferer = PeerConnectionWrapper.isOfferer(sessionB, sessionA) + return aIsOfferer to bIsOfferer + } + + @Test + fun exactlyOneOfferer_whenBothPeersCompareTheSameSessionIdPair() { + val (a, b) = rolesForConsistentPair("session-aaa", "session-bbb") + assertNotEquals("exactly one of the two peers must be the offerer", a, b) + } + + @Test + fun offererRoleIsComplementary_forAnyConsistentPair_regardlessOfInsertionOrder() { + assertEquals(rolesForConsistentPair("x", "y"), rolesForConsistentPair("x", "y")) + val (a, b) = rolesForConsistentPair("alpha", "omega") + assertNotEquals(a, b) + } + + /** + * Regression guard for the failure mode of #6169: if cross-namespace IDs are passed (each peer + * comparing its own Nextcloud session against the other's HPB session — four unrelated + * strings), both peers can compute the same role. Here both compute "not offerer" → nobody + * offers → deadlock. The fix prevents this by passing same-namespace IDs at the call site. + */ + @Test + fun bothPeersComputeNotOfferer_whenCrossNamespaceSessionIdsArePassed() { + val aIsOfferer = PeerConnectionWrapper.isOfferer("nc-A", "sig-B") + val bIsOfferer = PeerConnectionWrapper.isOfferer("nc-B", "sig-A") + + assertFalse("peer A does not take the offerer role", aIsOfferer) + assertFalse("peer B does not take the offerer role", bIsOfferer) + assertEquals("both peers compute the same role = deadlock (#6169)", aIsOfferer, bIsOfferer) + } +}