feat: Support in-place ICE restart with a new Agent - #2438
Conversation
IceTransport.icePassword was the local ICE password read live off the ice4j Agent, and it doubled as the auth token baked into the colibri WebSocket URL handed to the peer. Those are separate concerns: the peer holds that URL and re-dials it on every WebSocket reconnect, so the value must stay fixed for the lifetime of the transport. Rename it to webSocketPassword and pin it at construction. No behaviour change today (there is exactly one Agent per transport), but it stops the WebSocket URL from moving once the ICE credentials can rotate.
When an endpoint's network changes, jicofo can now ask the bridge for an
in-place ICE restart instead of tearing down and re-inviting the whole
session. The bridge answers with a second ice4j Agent while the
established one keeps carrying media, and cuts over once the new one
connects (make-before-break).
The trigger is explicit: the colibri2 <transport ice-restart="true"/>
attribute, read in Colibri2ConferenceHandler. The bridge does not infer a
restart from a changed remote ufrag.
The sequence, per endpoint:
1. On the request, create a pending Agent with freshly rotated local
credentials (RFC 8445 section 9) and the next ice-generation.
Connectivity checks are deliberately NOT started: we are the
controlling agent, so our checks need the endpoint's new remote
credentials to build their USERNAME and MESSAGE-INTEGRITY. The new
Agent does answer incoming checks in the meantime - ice4j starts its
check server when the component is created and queues pre-RUNNING
checks.
2. Return the pending Agent's transport (candidates, credentials and
ice-generation) in the conference-modified, so jicofo can relay it
to the endpoint. describe() prefers the pending bundle for this.
3. When the endpoint's own new credentials arrive over the existing
transport-info path, match them to the pending bundle by
ice-generation, apply them, and start connectivity checks then.
4. On COMPLETED, cut over: the pending bundle becomes current. The old
one is freed after a transition window so old-generation checks
still in flight are answered rather than dropped.
5. On failure or timeout, abandon the pending bundle and keep the
established Agent. The transport does not fail.
IceTransport is restructured around a private AgentBundle (agent, stream,
component and listeners) with a current/pending pair guarded by a lock.
send() always uses the current bundle. Only the current bundle drives
transport-level state, so a retired or pending Agent can no longer report
writeability, refresh consent, or fail the transport.
Gated by videobridge.ice.restart.enabled (default true); a request is
rejected and logged when the feature is disabled or the transport is not
established yet. New config: videobridge.ice.restart.transition-window
(2s) and .timeout (10s). New metrics: ice_restarts_started, _succeeded,
_failed and _rejected.
Requires the colibri2 ice-restart and Jingle ice-generation attributes
from jitsi-xmpp-extensions; the pom dependency will be bumped once those
are released.
Picks up the ice-generation and ice-restart attributes used by the in-place ICE restart.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2438 +/- ##
============================================
+ Coverage 48.85% 49.29% +0.44%
- Complexity 2798 2838 +40
============================================
Files 381 382 +1
Lines 22674 22923 +249
Branches 3458 3507 +49
============================================
+ Hits 11077 11301 +224
+ Misses 10417 10402 -15
- Partials 1180 1220 +40
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The WebSocket password was pinned to the initial ICE Agent's local password. Now that it no longer rotates with the ICE credentials, it is not an ICE value at all, and IceTransport has no reason to own it. Generate it independently in Endpoint and Relay, which own the WebSocket it authenticates. The peer treats the URL as opaque: lib-jitsi-meet stores the url attribute of the signaled <web-socket> element and re-dials that string, jicofo only strips the element, and a remote bridge uses the signaled URL as-is. So the value can be any URL-safe random string. Also stop logging the expected password on a mismatch.
| /** | ||
| * Whether the ice4j [Agent]s created by this transport take the 'controlling' role. | ||
| */ | ||
| private val controlling = controlling |
There was a problem hiding this comment.
In general it's possible for the controlling role to differ across different ICE restarts. I think we've structured things so that's not the case but would be a good idea to leave this flexible? How hard would it be?
There was a problem hiding this comment.
Leaving this as it is for now. The role is fixed for the lifetime of the transport: every AgentBundle reads the same field, and nothing in colibri2 signals a per-restart role today, so a settable role would be unreachable. It stays easy to change if we ever need it -- make the field a var and set it before the bundle is created.
JonathanLennox
left a comment
JonathanLennox
left a comment
There was a problem hiding this comment.
Note: this review was generated by AI (Claude Code), from reading this PR against the pinned ice4j 3.2-15-g6da2b08 sources and the jicofo / lib-jitsi-meet sides of the change. It has not been validated by running the code, so please treat each point as something to check rather than as established fact.
The overall design holds up well. The generation counter really does handle overlapping restarts and reordered colibri2 responses; the pendingBundle !== bundle identity guards make a double free() impossible on every path I traced (timeout vs. FAILED vs. supersede vs. cutover); and removing the listeners before agent.free() correctly suppresses the spurious RUNNING -> TERMINATED "failure". The comments below are about things around that core.
The two I'd consider blocking are the inline notes on cutOver's demultiplexing rationale (ice4j routes by remote address, not by ufrag, which makes the restart quietly dependent on the peer's 5-tuple changing) and on applyRemoteCredentialsToPendingRestart, where one untagged <transport> can poison a restart round irrecoverably.
Two further points that don't attach to a specific line:
No prompt recovery from an abandoned restart. An established ice4j Agent never transitions to FAILED -- ConnectivityCheckClient.updateCheckListAndTimerStates bails out on getState().isEstablished() -- so consent-freshness failure only stops refreshing lastIceActivityInstant. After abandonPendingRestart keeps the old Agent, iceTransport.hasFailed() stays false and Endpoint.shouldExpire() falls through to the inactivity path: entity-expiration.timeout is 1 minute with a 1-minute check-interval, i.e. up to ~2 minutes of a live endpoint carrying no media. Real recovery therefore rests entirely on the client noticing its own ICE failure and falling back to session-terminate restart="true". Worth stating explicitly in the PR description, and it's an argument for surfacing the abandonment to jicofo rather than only bumping a counter.
No tests. jvb/src/test has nothing covering IceTransport, and this adds a real state machine (supersede, abandon-vs-cutover, generation mismatch, late credentials). clock is already injected; the ice4j Agent is the obstacle. It would be worth covering at least the pure transition logic if it can be prised apart from Agent construction.
While a restart was pending, every transport update was applied to the pending Agent. The peer stamps ice-generation on the restart answer and on nothing else, so an untagged update is an ordinary transport update (a trickled candidate, or an old one still in flight). Applying it to the pending Agent gave that Agent the peer's old password, started its checks with credentials the peer rejects, and consumed the round, so the real credentials were later dropped as a repeat and the restart burned its full timeout. Route untagged updates to the established bundle, and claim the round (checksStarted) together with the credentials under the lock, so an unusable update can no longer consume it. A generation mismatch is now logged without incrementing ice_restarts_rejected: it is the normal outcome of a superseded round, not a rejected request.
An ICE restart request for a transport that has not connected yet was answered with no transport, which is the signal jicofo uses to escalate to a full re-invite. That is too heavy here: there is no media to preserve, the endpoint only needs to keep the Agent it is already checking against. Return a three-way result instead of a boolean. A bridge that can not restart ICE at all (the feature is disabled, or the transport is stopped) still signals no transport, so a full re-invite follows. A transport that is merely not established yet now signals its unchanged credentials, so the endpoint keeps the connection it has.
Agent.createComponent() declares IOException and BindException. Kotlin does not force us to handle them, so a harvest or bind failure propagated out of requestIceRestart() into handleColibri2Endpoint(), where no catch clause matches it. That fails the whole conference-modify and takes every other endpoint's updates with it, which is what the soft-rejection design exists to avoid. It also leaked: an exception from createComponent() left the Agent and its stream unreferenced but not freed, so the ufrag stayed in the single port harvester for the life of the process. Move the construction of the ice4j objects into createAgentBundle(), which catches, frees whatever it already created, and returns null. A restart that can not create its Agent now keeps the established one. Create the new Agent before superseding an in-flight restart, so a failure leaves that restart alone instead of freeing a bundle the peer may already be checking against.
A second request for a restart that had not started its checks yet rolled a new generation, freed the pending bundle the peer may already have been checking against, discarded its progress and restarted the timeout. Clients do fire duplicate network-change events. Re-describe the pending bundle instead. A request that arrives after the checks have started is a new round and still supersedes.
SCHEDULED_POOL is a single thread shared by the whole bridge, and Agent.free() shuts down the StunStack, closes sockets and joins threads. The other two free sites already hop to IO_POOL.
stop() collected only the current and pending bundles. A bundle inside its transition window is neither: it is reachable only from the task that frees it, so it outlived the transport until that task fired. With the default 2 second window this heals itself, but it scales with the configured window. stop() also left both scheduled tasks to run. Hold the retiring bundle and the two tasks in fields, so stop() frees everything and cancels both. A second cutover inside a transition window now frees the bundle retiring from the first one instead of letting two old Agents linger. AgentBundle.free() is idempotent, so a task that fires anyway (it can already have started when it is cancelled) does not free twice.
A superseded restart incremented ice_restarts_started and then nothing else, so started never reconciled with succeeded plus failed. Add ice_restarts_superseded for it. ice_restarts_rejected no longer doubles as a count of stale-generation credential drops (that stopped in an earlier commit), so it now counts exactly the requests for which no new Agent was created.
A zero or negative videobridge.ice.restart.timeout scheduled the abandon task to fire immediately, after describe() had already handed the pending credentials to the endpoint. The endpoint was then left addressing its checks to an Agent that had been freed. Treat a non-positive timeout the same as the feature being disabled, so the endpoint falls back to a full re-invite.
describe() stamped the generation whenever the bundle had one, so after a cutover the established bundle's transport carried a generation that the endpoint has already seen and would reject as not newer. Nothing hit this yet, because the only caller describes a transport on create or on a restart, but it is a trap for the next caller. It also matters now that a restart which keeps the existing Agent describes that Agent.
requestIceRestart() rotates the credentials we advertise and starts the restart timeout, and describe() then prefers the pending Agent. Both ran before the rest of the endpoint update, so an IqProcessingException raised later in the same request (the visitor-sources check) left the bridge advertising credentials the endpoint never received, diverting its transport updates to a bundle that can not connect, for the whole timeout. Move the restart handling to the end of handleColibri2Endpoint(), after everything that can throw.
The comments said the two Agents accept each other's generation of connectivity checks and that ice4j routes each check to the right Agent by its local ufrag. It does not: the single port harvester looks the remote address up in its socket map first and parses the ufrag only for an address it has never seen. What the transition window actually does is answer the endpoint's old-generation checks, which arrive from the old address and so reach the old Agent's socket. State that, note that both Agents keep accepting media as well as checks, and record the assumption this leaves on the restart path: the new Agent is only reachable because the endpoint's address changes.
The contract was only written down on the jicofo side: a refusal is the absence of a <transport> in the conference-modified for that endpoint, not an error, so that one endpoint's rejected request does not fail the whole conference-modify.
The restart timeout covers the whole signaling round trip, the endpoint's re-gathering and ICE itself, on an endpoint whose network has just changed, so 10 seconds was tight. Raise it to 20, and the transition window from 2 seconds to 3.
Take the creation of the ice4j Agent through a factory, so a test can supply a mock. Real Agents bind ports, start threads and only change state by running ICE against a peer, none of which a test of this state machine needs. The tests cover which Agent is described, which one gets the peer's credentials, and which ones are freed: the not-established and disabled and non-positive-timeout outcomes, a repeated request, a superseded round, credentials tagged with another generation, an untagged update, repeated credentials, cutover, abandonment, stop(), and a failure to create the Agent. The tests of the four fixes that changed behaviour silently (the untagged update, the repeated request, the generation stamped after a cutover, and stop() freeing a retiring Agent) were checked by reverting each fix and watching the test fail.
|
(Also partially AI generated)
On the two points that did not attach to a line:
One behaviour change worth calling out, from the comment about rejections triggering a reconnect: a restart requested before ICE is established no longer answers with no |
JonathanLennox
left a comment
JonathanLennox
left a comment
There was a problem hiding this comment.
Note: this review was generated by AI (Claude Code), like the previous one. I re-read all 15 commits and ran the new test class in a worktree: 22 tests, all passing, BUILD SUCCESS. As before, please treat each point as something to check rather than as established fact.
Every point from the earlier review looks addressed, and several of the fixes go further than what was raised:
- Demux -- corrected in
cutOver,requestIceRestart,IceConfigandreference.conf, andrequestIceRestart's KDoc now states the consequence outright (a peer whose address does not change cannot connect the new Agent, and the restart is abandoned). Documenting rather than fixing seems like the right call here. - Untagged updates -- both halves fixed: only generation-tagged updates route to the pending bundle, and the
checksStartedclaim moved inside the lock. - Arming order -- the restart block moved past everything in
handleColibri2Endpointthat can throw, with a comment saying why. - Timeouts,
free()on the scheduled pool,stop()missing the retiring bundle, Agent-creation failure, idempotency, metrics, timeout validation, thedescribe()generation stamp -- all fixed. The new task bookkeeping (restartTimeoutTask/transitionWindowTask, each with an "unless it's already over" re-check after scheduling) holds up under every interleaving I could trace, and makingfree()idempotent covers the newstop()-vs-transition-task overlap thatretiringBundleintroduces.
The colibri WebSocket password rework goes beyond what was raised -- decoupling it from ICE entirely rather than just pinning it, and dropping the expected secret from the failure log, which was a small real leak.
A few things left, inline below. The only one I'd call a defect is the credential/CAS ordering in applyRemoteCredentialsToPendingRestart; the rest are questions and test coverage.
One further note that doesn't attach to a line: createAgentBundle still harvests candidates while holding restartLock, which stop() and cutOver contend for. The comment explains the choice (a failure then leaves an in-flight restart alone), and it looks like a fair trade -- just flagging that binding a port now happens under a lock on the cutover path.
| ) | ||
| return | ||
| } | ||
| pending.stream.remoteUfrag = ufrag |
There was a problem hiding this comment.
A duplicate update is applied, then reported as ignored.
pending.stream.remoteUfrag = ufrag
pending.stream.remotePassword = password
if (!pending.checksStarted.compareAndSet(false, true)) { ...ignoring a repeated transport update... ; return }A second tagged update for the same generation overwrites the remote credentials of an Agent whose checks are already running -- ice4j reads stream.getRemotePassword() live in Agent.getRemoteKey, so this changes the credentials used for both outgoing checks and validating incoming ones -- while the log line says it was ignored.
Everything that can make an update unusable is already checked before this point (null ufrag/pwd above, wrong generation above, no longer pending inside the lock), so the concern in the comment above is satisfied with the CAS moved before the two assignments. That also makes the behaviour match the log message.
| // for one that kept the existing Agent (its unchanged credentials, so the endpoint keeps the connection | ||
| // it has, with no re-invite). Only IceRestartResult.UNAVAILABLE signals nothing. | ||
| if (c2endpoint.create || iceRestartResult == IceRestartResult.STARTED || | ||
| iceRestartResult == IceRestartResult.KEEP_EXISTING |
There was a problem hiding this comment.
KEEP_EXISTING is signalled as a transport the client discards -- worth confirming the far end agrees.
The transport described here for KEEP_EXISTING carries no ice-generation (correct -- describe() only stamps a pending bundle). Following it through jicofo's ColibriV2SessionManager.endpointIceRestarted:
- It passes the staleness guard, then hits
participantInfo.lastRelayedIceGeneration = generationunconditionally, which resets the high-water mark toGENERATION_UNSPECIFIEDand loses staleness protection for the next reordered response. - jicofo counts it as a relayed restart, so
IceRestartMetricswill over-count in-place restarts that never happened. - It is relayed to the client, whose generation guard then drops it -- which is what actually implements "keep what you have". The lib-jitsi-meet tests cover a non-numeric generation and
0; worth confirming an absent attribute takes the same path.
The reasoning behind KEEP_EXISTING itself looks right for the not-established case: the client has not rotated anything, so the initial Agent can still pick up its new address peer-reflexively. It's just that "keep existing" is currently expressed as "send something the peer ignores" rather than signalled explicitly.
| // The established Agent is untouched and still carrying media, so keep using it. | ||
| logger.warn("Not restarting ICE: failed to create the new Agent. Keeping the existing one.") | ||
| iceRestartsRejected.inc() | ||
| return IceRestartResult.KEEP_EXISTING |
There was a problem hiding this comment.
This is the other caller of KEEP_EXISTING, and it seems less clear-cut than the not-established one. Failing to create an Agent is a real resource problem (a bind failure, exhausted ports), and answering with the established transport deliberately bypasses jicofo's new "re-invite the participant if the bridge declines" fallback -- the endpoint's only remaining recovery is its own ICE failure detection. UNAVAILABLE would escalate immediately, which for a resource failure may be what you want. Genuinely a judgement call, but worth being deliberate about, since the two paths returning the same value have quite different causes.
| * The total number of ICE restart requests for which no new Agent was created: the feature is | ||
| * disabled, the transport is not running, or it is not established yet. | ||
| */ | ||
| val iceRestartsRejected = VideobridgeMetricsContainer.instance.registerCounter( |
There was a problem hiding this comment.
Now that KEEP_EXISTING exists, this counter merges two quite different events: the routine "not established yet" case (normal traffic, the endpoint keeps a working session) and a failure to create the Agent (a resource problem you would want to alert on). Splitting them, or at least noting the difference in the help text, would make the metric actionable.
| iceGeneration shouldBe IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED | ||
| } | ||
| } | ||
| should("free the old Agent only after the transition window") { |
There was a problem hiding this comment.
This asserts only the "not yet" half -- that the old Agent survives the cutover. Neither timer path is exercised anywhere in the class: the transition window elapsing (which frees the bundle, clears retiringBundle and hops to the IO pool) and the restart timeout firing (abandonPendingRestart via the scheduled task, as opposed to the FAILED route that is covered) both go untested.
Both are reachable: TaskPools.SCHEDULED_POOL is a public mutable field and VideobridgeTest already substitutes a fake executor for exactly this. Given that this is where the new task-cancellation bookkeeping lives, it seems like the part most worth pinning down.
| val password = "password-$index" | ||
|
|
||
| var state: IceProcessingState = IceProcessingState.WAITING | ||
| var freed = false |
There was a problem hiding this comment.
freed is written from the IO pool (free() is submitted there) and spun on from the test thread in awaitTrue, with no synchronisation and no happens-before. It will almost always be observed promptly on x86, but it can in principle spin out the full 5 s and fail spuriously in CI. @Volatile here (and on created's usage, if an Agent is ever created off the test thread) would make it sound.
| } | ||
| should("supersede a restart whose checks have started") { | ||
| transport.requestIceRestart() | ||
| transport.startConnectivityEstablishment(remoteTransport(generation = 1)) |
There was a problem hiding this comment.
Nothing in the class covers the ordinary, non-restart path through startConnectivityEstablishment -- every call is inside With ICE established, where the current bundle short-circuits on "Connection already established". Since the routing at the top of that method was restructured for this PR, a leaf under Before ICE is established asserting that an untagged update still reaches the initial Agent (sets its remote credentials, starts its checks) would guard the main negotiation path against a future change to the pending-bundle condition.
| transport.describe().ufrag shouldBe initial.ufrag | ||
| awaitTrue { agents.created[1].freed } | ||
| } | ||
| should("not restart the transport's own failure handling") { |
There was a problem hiding this comment.
Reads like it is missing a word -- "not trigger the transport's own failure handling"?
A repeated transport update for the generation of a restart whose checks are already running overwrote the remote credentials of that Agent, and then logged that it had ignored the update. ice4j reads the credentials off the stream every time it signs a connectivity check or validates an incoming one (ConnectivityCheckServer.getRemoteKey), so the overwrite took effect immediately. Claim the round before applying the credentials rather than after. Everything that can make an update unusable is checked before this point, so claiming first can not burn the round on an update we then reject, which is what the previous order was for.
Failing to create the Agent is a resource problem: ice4j could not harvest or bind. Keeping the endpoint on the established transport in that case bypassed the re-invite fallback and left it with nothing but its own ICE failure detection, even though it had just told us its network changed. Answer UNAVAILABLE instead, so the endpoint is re-invited. KEEP_EXISTING now means only "the transport is not established yet", which is a routine outcome. The two are counted separately, along with the requests this bridge rejects outright, because they call for different responses: ice_restarts_rejected is configuration, ice_restarts_not_established is ordinary traffic, and ice_restarts_agent_creation_failed is worth alerting on.
Neither scheduled task was exercised, which is where the task cancellation bookkeeping lives. Substitute a FakeScheduledExecutorService for TaskPools.SCHEDULED_POOL, as other tests here do, and cover the transition window elapsing, the timeout abandoning a restart, a cutover cancelling the timeout, and stop() cancelling both tasks. The last two were checked by removing the cancellations and watching them fail. Also cover the ordinary, non-restart path through startConnectivityEstablishment, which nothing reached because every existing case had the transport already established. It guards the routing that now sits in front of it. Make FakeAgent.freed @volatile: it is written on the IO pool and spun on from the test thread. Fix the name of the failure-handling test.
2 participants
Implements the bridge side of an in-place ICE restart. When a client's network changes, the current recovery is a full session teardown and re-invite. This adds a lighter path that keeps the same DTLS session, SSRCs and RTP state, so media keeps flowing throughout.
On an
ice-restartrequest from jicofo the bridge creates a new ICE agent with fresh credentials and returns its transport, while the existing agent keeps carrying media. Only once the new agent connects does the bridge cut over, freeing the old one after a transition window so late checks from the old generation are still answered. If the new agent does not connect, the restart is abandoned and the established agent is kept, so a failed restart never breaks a working transport.How it works
IceTransportis restructured around anAgentBundle(agent + stream + component + listeners + generation).currentBundleis what we send on and describe;pendingBundleis a restart in flight.requestIceRestart()creates the pending bundle but deliberately does not start connectivity checks. The bridge is the controlling agent, and the checks it sends are authenticated with the peer's password, so it waits for the peer's new credentials first. Checks arriving from the peer in the meantime are queued by ice4j and replayed when establishment starts.applyRemoteCredentialsToPendingRestart()runs when a transport update arrives with a pending bundle: it matches theice-generation, applies the credentials, and starts checks.cutOver()swaps it in and frees the old bundle aftervideobridge.ice.restart.transition-window.abandonPendingRestart()frees the pending bundle and keeps the established one.The colibri WebSocket password is pinned to the initial agent (first commit). The ICE password doubles as the WebSocket auth token baked into the URL the client holds, and the client re-dials that URL on exactly the network change that triggers a restart, so it must not rotate with the ICE credentials.
Config
videobridge.ice.restart.enabled(default true)videobridge.ice.restart.transition-window(default 2s)videobridge.ice.restart.timeout(default 10s)Metrics:
ice_restarts_started,_succeeded,_failed,_rejected.Notes
No ice4j change is needed: this creates a fresh agent rather than re-arming an existing one.
A rejected restart is soft — it logs, bumps
ice_restarts_rejectedand returns no<transport>, rather than failing the whole conference-modify and taking other endpoints' updates with it.Verified end-to-end against a real client: cut-over 656ms after the request, with outbound video non-zero in every 100ms sample across the cut-over, zero freezes and zero dropped frames.
Part of a multi-repo change; all PRs share the
in-place-ice-restartbranch name so CI tests them together: