From fe1bfbaa99b983eb6ed80313ece208e1acb0eb44 Mon Sep 17 00:00:00 2001 From: avosa Date: Sun, 13 Sep 2026 19:50:31 -0700 Subject: [PATCH 1/4] Stop the subscriber entering an ice restart state on a resume --- .changeset/quiet-pandas-marry.md | 13 +++++++ .../android/room/PeerConnectionTransport.kt | 4 -- .../java/io/livekit/android/room/RTCEngine.kt | 5 ++- .../android/test/mock/MockPeerConnection.kt | 4 ++ .../android/room/RTCEngineMockE2ETest.kt | 38 +++++++++++++++++++ 5 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 .changeset/quiet-pandas-marry.md diff --git a/.changeset/quiet-pandas-marry.md b/.changeset/quiet-pandas-marry.md new file mode 100644 index 00000000..f8406c96 --- /dev/null +++ b/.changeset/quiet-pandas-marry.md @@ -0,0 +1,13 @@ +--- +"client-sdk-android": patch +--- + +Fix the subscriber silently buffering remote ICE candidates after a resume + +A soft reconnect put the subscriber into an ice restart state, but only `setRemoteDescription` +clears that and the server re-offers the subscriber only when the reconnect moved the participant +to another node. After an ordinary resume no offer arrives, so the flag stayed set for the life of +the transport and every later remote candidate was queued instead of applied, leaving the +subscriber unable to adopt any new path the server proposed. The subscriber no longer enters that +state: the server does not send candidates ahead of the offer that introduces them, so queueing +them gains nothing. Matches the same fix in client-sdk-js. diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt index efae279a..07634c2f 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt @@ -298,10 +298,6 @@ constructor( return sdp } - fun prepareForIceRestart() { - restartingIce = true - } - fun isClosed() = isClosed.get() fun closeBlocking() { diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt index e0e64e7d..a3536261 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt @@ -637,7 +637,10 @@ internal constructor( } connectionState = ConnectionState.RESUMING LKLog.v { "Attempting soft reconnect." } - subscriber?.prepareForIceRestart() + // The subscriber deliberately does not enter an ice restart state here. Only a + // remote description clears that, and the server re-offers the subscriber only + // when the reconnect moved us to another node, so on an ordinary resume it + // would never clear and every later candidate would be queued and never added. try { val response = client.reconnect(url!!, token, participantSid) if (response is Either.Left) { diff --git a/livekit-android-test/src/main/java/io/livekit/android/test/mock/MockPeerConnection.kt b/livekit-android-test/src/main/java/io/livekit/android/test/mock/MockPeerConnection.kt index 6a51cfeb..28c2e1d2 100644 --- a/livekit-android-test/src/main/java/io/livekit/android/test/mock/MockPeerConnection.kt +++ b/livekit-android-test/src/main/java/io/livekit/android/test/mock/MockPeerConnection.kt @@ -129,7 +129,11 @@ class MockPeerConnection( return true } + /** Every candidate actually handed to the connection, so a test can tell added from queued. */ + val addedIceCandidates = mutableListOf() + override fun addIceCandidate(candidate: IceCandidate?): Boolean { + candidate?.let { addedIceCandidates.add(it) } return true } diff --git a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt index 6225fb1e..f0d87998 100644 --- a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt +++ b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt @@ -683,4 +683,42 @@ class RTCEngineMockE2ETest : MockE2ETest() { ) } } + + /** + * A resume must leave the subscriber able to take the paths the server proposes. It used to + * enter an ice restart state that only a remote description clears, and the server re-offers + * the subscriber only when the reconnect moved us to another node, so after an ordinary resume + * every later candidate was queued and never added. Same defect as client-sdk-js#2054. + */ + @Test + fun softReconnectKeepsSubscriberApplyingRemoteCandidates() = runTest { + room.setReconnectionType(ReconnectType.FORCE_SOFT_RECONNECT) + connect() + + disconnectPeerConnection() + testScheduler.advanceTimeBy(1000) + wsFactory.listener.onOpen(wsFactory.ws, createOpenResponse(wsFactory.request)) + simulateMessageFromServer(TestData.RECONNECT) + connectPeerConnection() + advanceUntilIdle() + + val subPeerConnection = getSubscriberPeerConnection() + val before = subPeerConnection.addedIceCandidates.size + simulateMessageFromServer(subscriberTrickle()) + advanceUntilIdle() + + assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) + } + + private fun subscriberTrickle(): LivekitRtc.SignalResponse { + val trickle = LivekitRtc.TrickleRequest.newBuilder() + .setCandidateInit( + """{"candidate":"candidate:1 1 UDP 1 127.0.0.1 9 typ host","sdpMLineIndex":0,"sdpMid":"0"}""", + ) + .setTarget(LivekitRtc.SignalTarget.SUBSCRIBER) + .build() + return LivekitRtc.SignalResponse.newBuilder() + .setTrickle(trickle) + .build() + } } From 9e4b3382bb9190c4ae9982d449b211ad368b838e Mon Sep 17 00:00:00 2001 From: avosa Date: Sun, 13 Sep 2026 21:56:24 -0700 Subject: [PATCH 2/4] Hold candidates for the offer that is coming, not for the whole resume --- .../android/room/PeerConnectionTransport.kt | 18 ++++++++++++---- .../java/io/livekit/android/room/RTCEngine.kt | 11 ++++++---- .../android/room/RTCEngineMockE2ETest.kt | 21 +++++++++++++++++++ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt index 07634c2f..dbdf576b 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt @@ -89,7 +89,9 @@ constructor( ) ?: throw IllegalStateException("peer connection creation failed?") }!! private val pendingCandidates = mutableListOf() - private var restartingIce: Boolean = false + + /** Holds remote candidates until the remote description they belong to has been applied. */ + private var awaitingRemoteDescription: Boolean = false private var renegotiate = false @@ -104,7 +106,7 @@ constructor( fun addIceCandidate(candidate: IceCandidate) { executeRTCIfNotClosed { - if (peerConnection.remoteDescription != null && !restartingIce) { + if (peerConnection.remoteDescription != null && !awaitingRemoteDescription) { peerConnection.addIceCandidate(candidate) } else { pendingCandidates.add(candidate) @@ -130,7 +132,7 @@ constructor( peerConnection.addIceCandidate(pending) } pendingCandidates.clear() - restartingIce = false + awaitingRemoteDescription = false } return@launchRTCIfNotClosed result } ?: Either.Right("PCT is closed.") @@ -167,7 +169,7 @@ constructor( constraints.findConstraint(MediaConstraintKeys.ICE_RESTART) == MediaConstraintKeys.TRUE if (iceRestart) { LKLog.d { "restarting ice" } - restartingIce = true + awaitingRemoteDescription = true } if (peerConnection.signalingState() == SignalingState.HAVE_LOCAL_OFFER) { @@ -298,6 +300,14 @@ constructor( return sdp } + /** + * Says a remote description is on its way, so candidates for it wait rather than land against + * the description it replaces. Called before the work that applies it is scheduled. + */ + fun expectRemoteDescription() { + awaitingRemoteDescription = true + } + fun isClosed() = isClosed.get() fun closeBlocking() { diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt index a3536261..3c04ff34 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt @@ -637,10 +637,10 @@ internal constructor( } connectionState = ConnectionState.RESUMING LKLog.v { "Attempting soft reconnect." } - // The subscriber deliberately does not enter an ice restart state here. Only a - // remote description clears that, and the server re-offers the subscriber only - // when the reconnect moved us to another node, so on an ordinary resume it - // would never clear and every later candidate would be queued and never added. + // The subscriber is not told to hold candidates here. The server re-offers it + // only when the reconnect moved us to another node, so a resume that stays put + // would never clear the hold. onServerOffer sets it instead, which covers + // exactly the offer it arrives with and nothing after. try { val response = client.reconnect(url!!, token, participantSid) if (response is Either.Left) { @@ -1131,6 +1131,9 @@ internal constructor( override fun onServerOffer(sessionDescription: SessionDescription, offerId: Int) { LKLog.v { "received server offer: ${sessionDescription.type}, ${runBlocking { publisher?.signalingState() }}" } + // Set before the work is scheduled: responses keep their order but this coroutine does not + // run inline, so a candidate for this offer can arrive while the old description is still on. + subscriber?.expectRemoteDescription() coroutineScope.launch { run { when (val outcome = subscriber?.setRemoteDescription(sessionDescription, offerId).nullSafe()) { diff --git a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt index f0d87998..e621841f 100644 --- a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt +++ b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt @@ -710,6 +710,27 @@ class RTCEngineMockE2ETest : MockE2ETest() { assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) } + /** + * A candidate belonging to a server offer waits for that offer to be applied. Offer handling is + * launched rather than run inline, so without the hold the candidate would land against the + * description the offer replaces and be rejected against its ice credentials. + */ + @Test + fun candidateArrivingWithAServerOfferWaitsForItsDescription() = runTest { + connect() + + val subPeerConnection = getSubscriberPeerConnection() + val before = subPeerConnection.addedIceCandidates.size + simulateMessageFromServer(TestData.OFFER) + simulateMessageFromServer(subscriberTrickle()) + + assertEquals(before, subPeerConnection.addedIceCandidates.size) + + advanceUntilIdle() + + assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) + } + private fun subscriberTrickle(): LivekitRtc.SignalResponse { val trickle = LivekitRtc.TrickleRequest.newBuilder() .setCandidateInit( From e479784a537fba73ec9f952311e75a66ddb107d7 Mon Sep 17 00:00:00 2001 From: avosa Date: Sun, 13 Sep 2026 22:07:40 -0700 Subject: [PATCH 3/4] End the candidate hold when the description attempt ends --- .changeset/quiet-pandas-marry.md | 11 ++-- .../android/room/PeerConnectionTransport.kt | 54 ++++++++++++++----- .../android/room/RTCEngineMockE2ETest.kt | 32 +++++++++++ 3 files changed, 80 insertions(+), 17 deletions(-) diff --git a/.changeset/quiet-pandas-marry.md b/.changeset/quiet-pandas-marry.md index f8406c96..c3bd84da 100644 --- a/.changeset/quiet-pandas-marry.md +++ b/.changeset/quiet-pandas-marry.md @@ -6,8 +6,11 @@ Fix the subscriber silently buffering remote ICE candidates after a resume A soft reconnect put the subscriber into an ice restart state, but only `setRemoteDescription` clears that and the server re-offers the subscriber only when the reconnect moved the participant -to another node. After an ordinary resume no offer arrives, so the flag stayed set for the life of +to another node. After an ordinary resume no offer arrives, so the state stayed set for the life of the transport and every later remote candidate was queued instead of applied, leaving the -subscriber unable to adopt any new path the server proposed. The subscriber no longer enters that -state: the server does not send candidates ahead of the offer that introduces them, so queueing -them gains nothing. Matches the same fix in client-sdk-js. +subscriber unable to adopt any new path the server proposed. + +Candidates now wait on the description they belong to rather than on the reconnect. `onServerOffer` +says a description is coming before it schedules the work that applies it, so a candidate that +arrives in between is held rather than tried against the description being replaced, and the wait +ends when that attempt ends whether the description lands or is refused. diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt index dbdf576b..1037e829 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt @@ -90,8 +90,12 @@ constructor( }!! private val pendingCandidates = mutableListOf() - /** Holds remote candidates until the remote description they belong to has been applied. */ - private var awaitingRemoteDescription: Boolean = false + /** + * How many remote descriptions are on their way. Candidates wait while any is outstanding, so + * one belonging to a description that has not landed is never tried against the description it + * replaces. Only touched on the RTC thread. + */ + private var awaitedDescriptions = 0 private var renegotiate = false @@ -106,7 +110,7 @@ constructor( fun addIceCandidate(candidate: IceCandidate) { executeRTCIfNotClosed { - if (peerConnection.remoteDescription != null && !awaitingRemoteDescription) { + if (peerConnection.remoteDescription != null && awaitedDescriptions == 0) { peerConnection.addIceCandidate(candidate) } else { pendingCandidates.add(candidate) @@ -124,16 +128,13 @@ constructor( val result = launchRTCIfNotClosed { val currentOfferId = latestOfferId.get() if (sd.type == SessionDescription.Type.ANSWER && currentOfferId > 0 && offerId > 0 && currentOfferId > offerId) { + // The offer this answers has been superseded, so its wait ends here; the offer that + // replaced it holds its own. + descriptionSettled(applied = false) return@launchRTCIfNotClosed Either.Right("Old offer, ignoring. Expected: $currentOfferId, actual: $offerId") } val result = peerConnection.setRemoteDescription(sd) - if (result is Either.Left) { - pendingCandidates.forEach { pending -> - peerConnection.addIceCandidate(pending) - } - pendingCandidates.clear() - awaitingRemoteDescription = false - } + descriptionSettled(applied = result is Either.Left) return@launchRTCIfNotClosed result } ?: Either.Right("PCT is closed.") @@ -169,7 +170,7 @@ constructor( constraints.findConstraint(MediaConstraintKeys.ICE_RESTART) == MediaConstraintKeys.TRUE if (iceRestart) { LKLog.d { "restarting ice" } - awaitingRemoteDescription = true + awaitedDescriptions++ } if (peerConnection.signalingState() == SignalingState.HAVE_LOCAL_OFFER) { @@ -181,6 +182,10 @@ constructor( // the best thing to do is to recreate the peerconnection peerConnection.setRemoteDescription(curSd) } else { + // No offer goes out, so the answer it would have waited for is not coming. + if (iceRestart) { + descriptionSettled(applied = false) + } renegotiate = true return@launchRTCIfNotClosed } @@ -197,6 +202,9 @@ constructor( is Either.Left -> outcome.value is Either.Right -> { LKLog.d { "error creating offer: ${outcome.value}" } + if (iceRestart) { + descriptionSettled(applied = false) + } return@launchRTCIfNotClosed } } @@ -302,10 +310,30 @@ constructor( /** * Says a remote description is on its way, so candidates for it wait rather than land against - * the description it replaces. Called before the work that applies it is scheduled. + * the description it replaces. Called before the work that applies it is scheduled, and + * answered by the attempt to set that description whether it lands or is refused. */ fun expectRemoteDescription() { - awaitingRemoteDescription = true + executeRTCIfNotClosed { awaitedDescriptions++ } + } + + /** + * Ends one wait, however it ended. Only a description that landed takes the candidates held for + * it, and only once nothing else is awaited. A wait that ended without one leaves them queued + * for the next description rather than dropping them, since one that no longer fits is refused + * by the connection anyway and one that still fits would otherwise be lost. + */ + private fun descriptionSettled(applied: Boolean) { + if (awaitedDescriptions > 0) { + awaitedDescriptions-- + } + if (!applied || awaitedDescriptions > 0) { + return + } + pendingCandidates.forEach { pending -> + peerConnection.addIceCandidate(pending) + } + pendingCandidates.clear() } fun isClosed() = isClosed.get() diff --git a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt index e621841f..0d5275bb 100644 --- a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt +++ b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt @@ -731,6 +731,38 @@ class RTCEngineMockE2ETest : MockE2ETest() { assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) } + /** + * A refused offer ends its own wait. The description it would have installed never came into + * being, so the connection carries on with the one in force and later candidates reach it + * rather than piling up behind an offer that is never going to land. + */ + @Test + fun aRefusedOfferStopsHoldingCandidates() = runTest { + connect() + val subPeerConnection = getSubscriberPeerConnection() + + simulateMessageFromServer(refusedOffer()) + advanceUntilIdle() + + val before = subPeerConnection.addedIceCandidates.size + simulateMessageFromServer(subscriberTrickle()) + advanceUntilIdle() + + assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) + } + + /** An empty description is the one the mock connection refuses. */ + private fun refusedOffer(): LivekitRtc.SignalResponse { + val offer = LivekitRtc.SessionDescription.newBuilder() + .setSdp("") + .setType("offer") + .setId(100) + .build() + return LivekitRtc.SignalResponse.newBuilder() + .setOffer(offer) + .build() + } + private fun subscriberTrickle(): LivekitRtc.SignalResponse { val trickle = LivekitRtc.TrickleRequest.newBuilder() .setCandidateInit( From 52f0c8b9c5c71ce9d8ebfa35bf713507c5538e66 Mon Sep 17 00:00:00 2001 From: avosa Date: Sun, 13 Sep 2026 22:15:06 -0700 Subject: [PATCH 4/4] Let each wait be ended only by what answers it --- .changeset/quiet-pandas-marry.md | 6 +- .../android/room/PeerConnectionTransport.kt | 63 +++++++++++-------- .../android/room/RTCEngineMockE2ETest.kt | 55 ++++++++++++++++ 3 files changed, 96 insertions(+), 28 deletions(-) diff --git a/.changeset/quiet-pandas-marry.md b/.changeset/quiet-pandas-marry.md index c3bd84da..5c52144c 100644 --- a/.changeset/quiet-pandas-marry.md +++ b/.changeset/quiet-pandas-marry.md @@ -12,5 +12,7 @@ subscriber unable to adopt any new path the server proposed. Candidates now wait on the description they belong to rather than on the reconnect. `onServerOffer` says a description is coming before it schedules the work that applies it, so a candidate that -arrives in between is held rather than tried against the description being replaced, and the wait -ends when that attempt ends whether the description lands or is refused. +arrives in between is held rather than tried against the description being replaced. Each wait is +owned by what answers it, an ice restart offer by its own offer id and a server offer by its own +turn, and it ends when that attempt ends whether the description lands, is refused, or answers an +offer that has since been superseded. diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt index 1037e829..e93f32ef 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/PeerConnectionTransport.kt @@ -91,11 +91,14 @@ constructor( private val pendingCandidates = mutableListOf() /** - * How many remote descriptions are on their way. Candidates wait while any is outstanding, so - * one belonging to a description that has not landed is never tried against the description it - * replaces. Only touched on the RTC thread. + * The offers whose answers are still owed, by offer id, and how many server offers are still to + * be applied. Candidates wait while either stands, so one belonging to a description that has + * not landed is never tried against the description it replaces. Only touched on the RTC thread. */ - private var awaitedDescriptions = 0 + private val awaitedAnswers = mutableSetOf() + private var awaitedOffers = 0 + + private fun awaitingDescription() = awaitedAnswers.isNotEmpty() || awaitedOffers > 0 private var renegotiate = false @@ -110,7 +113,7 @@ constructor( fun addIceCandidate(candidate: IceCandidate) { executeRTCIfNotClosed { - if (peerConnection.remoteDescription != null && awaitedDescriptions == 0) { + if (peerConnection.remoteDescription != null && !awaitingDescription()) { peerConnection.addIceCandidate(candidate) } else { pendingCandidates.add(candidate) @@ -128,13 +131,13 @@ constructor( val result = launchRTCIfNotClosed { val currentOfferId = latestOfferId.get() if (sd.type == SessionDescription.Type.ANSWER && currentOfferId > 0 && offerId > 0 && currentOfferId > offerId) { - // The offer this answers has been superseded, so its wait ends here; the offer that - // replaced it holds its own. - descriptionSettled(applied = false) + // The offer this answers has been superseded, so only that offer's wait ends here; + // the offer that replaced it keeps its own. + descriptionSettled(sd, offerId, applied = false) return@launchRTCIfNotClosed Either.Right("Old offer, ignoring. Expected: $currentOfferId, actual: $offerId") } val result = peerConnection.setRemoteDescription(sd) - descriptionSettled(applied = result is Either.Left) + descriptionSettled(sd, offerId, applied = result is Either.Left) return@launchRTCIfNotClosed result } ?: Either.Right("PCT is closed.") @@ -170,7 +173,6 @@ constructor( constraints.findConstraint(MediaConstraintKeys.ICE_RESTART) == MediaConstraintKeys.TRUE if (iceRestart) { LKLog.d { "restarting ice" } - awaitedDescriptions++ } if (peerConnection.signalingState() == SignalingState.HAVE_LOCAL_OFFER) { @@ -182,10 +184,6 @@ constructor( // the best thing to do is to recreate the peerconnection peerConnection.setRemoteDescription(curSd) } else { - // No offer goes out, so the answer it would have waited for is not coming. - if (iceRestart) { - descriptionSettled(applied = false) - } renegotiate = true return@launchRTCIfNotClosed } @@ -198,13 +196,18 @@ constructor( // this may skip some ids, but is not an issue. offerId = latestOfferId.incrementAndGet() + // An ice restart mints new local credentials, so candidates wait for the answer + // that carries the far side's. The wait is owned by this offer id alone. + if (iceRestart) { + awaitedAnswers.add(offerId) + } + val sdpOffer = when (val outcome = peerConnection.createOffer(constraints)) { is Either.Left -> outcome.value is Either.Right -> { LKLog.d { "error creating offer: ${outcome.value}" } - if (iceRestart) { - descriptionSettled(applied = false) - } + // No offer goes out, so the answer it would have waited for is not coming. + awaitedAnswers.remove(offerId) return@launchRTCIfNotClosed } } @@ -314,20 +317,28 @@ constructor( * answered by the attempt to set that description whether it lands or is refused. */ fun expectRemoteDescription() { - executeRTCIfNotClosed { awaitedDescriptions++ } + executeRTCIfNotClosed { awaitedOffers++ } } /** - * Ends one wait, however it ended. Only a description that landed takes the candidates held for - * it, and only once nothing else is awaited. A wait that ended without one leaves them queued - * for the next description rather than dropping them, since one that no longer fits is refused - * by the connection anyway and one that still fits would otherwise be lost. + * Ends the wait this description answers, however it ended: an answer ends the wait its own + * offer opened and no other, a server offer ends one of the waits opened for them. Only a + * description that landed takes the candidates held, and only once nothing else is awaited. A + * wait that ended without one leaves them queued rather than dropping them, since one that no + * longer fits is refused by the connection anyway and one that still fits would be lost. */ - private fun descriptionSettled(applied: Boolean) { - if (awaitedDescriptions > 0) { - awaitedDescriptions-- + private fun descriptionSettled(sd: SessionDescription, offerId: Int, applied: Boolean) { + if (sd.type == SessionDescription.Type.ANSWER) { + // A legacy answer carries no id to match itself to, so it ends whatever was owed. + if (offerId > 0) { + awaitedAnswers.remove(offerId) + } else { + awaitedAnswers.clear() + } + } else if (awaitedOffers > 0) { + awaitedOffers-- } - if (!applied || awaitedDescriptions > 0) { + if (!applied || awaitingDescription()) { return } pendingCandidates.forEach { pending -> diff --git a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt index 0d5275bb..b1de4f88 100644 --- a/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt +++ b/livekit-android-test/src/test/java/io/livekit/android/room/RTCEngineMockE2ETest.kt @@ -751,6 +751,61 @@ class RTCEngineMockE2ETest : MockE2ETest() { assertEquals(before + 1, subPeerConnection.addedIceCandidates.size) } + /** + * A stale answer ends only its own offer's wait. An ordinary offer opens none, so an answer to + * it arriving late must not release the wait an ice restart opened afterwards, or candidates + * for the restart land against the credentials it replaced. + */ + @Test + fun aStaleAnswerDoesNotEndAnotherOffersWait() = runTest { + room.setReconnectionType(ReconnectType.FORCE_SOFT_RECONNECT) + connect() + + // Offers go unanswered from here, so the restart's wait is still outstanding below. + val heldAnswers: SignalRequestHandler = { request -> request.hasOffer() } + wsFactory.registerSignalRequestHandler(heldAnswers) + val pubPeerConnection = getPublisherPeerConnection() + + disconnectPeerConnection() + testScheduler.advanceTimeBy(1000) + wsFactory.listener.onOpen(wsFactory.ws, createOpenResponse(wsFactory.request)) + simulateMessageFromServer(TestData.RECONNECT) + connectPeerConnection() + advanceUntilIdle() + + val before = pubPeerConnection.addedIceCandidates.size + simulateMessageFromServer(staleAnswer()) + simulateMessageFromServer(publisherTrickle()) + advanceUntilIdle() + + assertEquals(before, pubPeerConnection.addedIceCandidates.size) + wsFactory.unregisterSignalRequestHandler(heldAnswers) + } + + /** An answer to the first offer, long since replaced by the ones after it. */ + private fun staleAnswer(): LivekitRtc.SignalResponse { + val answer = LivekitRtc.SessionDescription.newBuilder() + .setSdp("remote_answer") + .setType("answer") + .setId(1) + .build() + return LivekitRtc.SignalResponse.newBuilder() + .setAnswer(answer) + .build() + } + + private fun publisherTrickle(): LivekitRtc.SignalResponse { + val trickle = LivekitRtc.TrickleRequest.newBuilder() + .setCandidateInit( + """{"candidate":"candidate:2 1 UDP 1 127.0.0.1 9 typ host","sdpMLineIndex":0,"sdpMid":"0"}""", + ) + .setTarget(LivekitRtc.SignalTarget.PUBLISHER) + .build() + return LivekitRtc.SignalResponse.newBuilder() + .setTrickle(trickle) + .build() + } + /** An empty description is the one the mock connection refuses. */ private fun refusedOffer(): LivekitRtc.SignalResponse { val offer = LivekitRtc.SessionDescription.newBuilder()