From 652dea1ce3e1b3c9ab20f47285f452caa154a4c7 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Mon, 17 Aug 2026 14:20:52 -0500 Subject: [PATCH 1/2] fix: Do not relay a transport the bridge did not restart A transport that carries no ice-generation is not the answer to an ICE restart: the bridge did not restart and described the Agent it already has, which it does when its transport has not connected yet. It was treated as a relayed restart anyway, with three consequences: - lastRelayedIceGeneration was set to GENERATION_UNSPECIFIED, which reset the high-water mark and lost the staleness protection for the next reordered response. A generation that had already been relayed could then be relayed again. - ice_restarts_relayed counted a restart that never happened. - The participant was sent a transport-info it has nothing to do with. It ignores it (an untagged transport does not take the ICE restart path in lib-jitsi-meet), so this was noise rather than a fault. Treat an absent generation as "the bridge kept its existing transport": log it, count it in the new ice_restarts_not_needed, and relay nothing. The staleness check no longer has to exclude untagged transports, since they never reach it. --- .../jitsi/jicofo/metrics/IceRestartMetrics.kt | 11 +++++++++++ .../bridge/colibri/ColibriV2SessionManager.kt | 13 +++++++++++-- .../colibri/ColibriV2SessionManagerTest.kt | 18 ++++++++++-------- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt b/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt index a93facc858..06980ebf2a 100644 --- a/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt +++ b/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt @@ -42,6 +42,17 @@ class IceRestartMetrics { "Number of in-place ICE restarts for which the bridge's rotated transport was signaled to the participant." ) + /** + * The bridge answered with the transport it already had, because it did not need to restart (its own + * transport has not connected yet). Nothing was relayed and the participant keeps the session it has, so + * this is neither a completed restart nor a failed one. + */ + @JvmField + val notNeeded = metricsContainer.registerCounter( + "ice_restarts_not_needed", + "Number of in-place ICE restarts for which the bridge kept its existing transport." + ) + /** * An ICE restart did not complete: the request was rejected (disabled, rate-limited, stale bridge-session ID) * or the bridge's response could not be relayed (no transport in the response, stale generation, participant diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt index bbf9d11eab..aa1faee9fe 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt @@ -793,6 +793,11 @@ class ColibriV2SessionManager @JvmOverloads constructor( * if the participant restarts twice in quick succession the two responses can arrive in either order. The * `ice-generation` the bridge echoes tells us which round each belongs to, and anything older than what we * have already relayed is discarded. + * + * A transport with no `ice-generation` at all means something different: the bridge did not restart and + * answered with the credentials of the Agent it already has. It does this when its transport has not + * connected yet, where there is nothing to preserve and nothing for the participant to do. There is nothing + * to relay in that case, and no restart to record. */ internal fun endpointIceRestarted(endpointId: String, transport: IceUdpTransportPacketExtension) { val generation = transport.iceGeneration @@ -802,8 +807,12 @@ class ColibriV2SessionManager @JvmOverloads constructor( IceRestartMetrics.failed.inc() return } - if (generation != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && - participantInfo.lastRelayedIceGeneration != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && + if (generation == IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED) { + logger.info("ICE restart: the bridge kept its existing transport for $endpointId, not relaying.") + IceRestartMetrics.notNeeded.inc() + return + } + if (participantInfo.lastRelayedIceGeneration != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && generation <= participantInfo.lastRelayedIceGeneration ) { logger.info( diff --git a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt index e0cd9f1a98..88c0bfea0e 100644 --- a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt +++ b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt @@ -45,7 +45,6 @@ import org.jitsi.utils.time.FakeClock import org.jitsi.xmpp.extensions.colibri2.ConferenceModifyIQ import org.jitsi.xmpp.extensions.jingle.DtlsFingerprintPacketExtension import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension -import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED import org.jivesoftware.smack.packet.IQ import org.jxmpp.jid.Jid import org.jxmpp.jid.impl.JidCreate @@ -359,15 +358,18 @@ class ColibriV2SessionManagerTest : ShouldSpec() { } } - context("With a bridge that does not tag generations") { - sessionManager.endpointIceRestarted("p1", transportWithGeneration(null)).also { drain() } + context("With a transport that carries no generation") { + // The bridge kept the Agent it already had instead of restarting, so there is nothing to relay. + sessionManager.endpointIceRestarted("p1", transportWithGeneration(2)).also { drain() } sessionManager.endpointIceRestarted("p1", transportWithGeneration(null)).also { drain() } - should("relay everything (the guard can not order untagged transports)") { - iceRestartedTransports.size shouldBe 2 - iceRestartedTransports.map { - it.second.iceGeneration - } shouldBe listOf(GENERATION_UNSPECIFIED, GENERATION_UNSPECIFIED) + should("not relay it") { + iceRestartedTransports.map { it.second.iceGeneration } shouldBe listOf(2) + } + should("not lose the generation of the last transport that was relayed") { + sessionManager.endpointIceRestarted("p1", transportWithGeneration(1)).also { drain() } + + iceRestartedTransports.map { it.second.iceGeneration } shouldBe listOf(2) } } } From bb364c8115781f9964a6d822e1ef77f887455be0 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Mon, 17 Aug 2026 16:52:45 -0500 Subject: [PATCH 2/2] doc: Correct what a missing transport in an ICE restart response means The list of reasons was wrong in both directions. A transport that is not established yet no longer omits the transport: the bridge answers with the Agent it already has, which arrives as a transport with no ice-generation and is handled by endpointIceRestarted. A failure to create the new Agent does omit it, and was not listed. --- .../org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt index 4246602ea7..74cf358537 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt @@ -216,11 +216,16 @@ class Colibri2Session( sendRequest(request.build(), "restartIce") { response -> val bridgeTransport = response.endpoints.find { it.id == participant.id }?.transport?.iceUdpTransport if (bridgeTransport == null) { - // The bridge declined the restart (disabled in its configuration, transport not established yet, - // or shutting down). It says so by omitting the transport rather than by returning an error. + // The bridge can not restart ICE at all: restarts are disabled or misconfigured, the transport + // is stopped (the endpoint is going away), or it failed to create the new Agent. It says so by + // omitting the transport rather than by returning an error, so that one endpoint's request does + // not fail the whole conference-modify. logger.warn("No transport in the response to an ICE restart request for ${participant.id}") colibriSessionManager.endpointIceRestartFailed(participant.id) } else { + // Either the transport of the new Agent, tagged with its ice-generation, or -- when the bridge + // did not need to restart, because its transport is not established yet -- the transport of the + // Agent it already has, with no generation. See endpointIceRestarted. colibriSessionManager.endpointIceRestarted(participant.id, bridgeTransport) } }