fix(rtc_engine): require evidence of PeerConnection recovery on resume - #1331
fix(rtc_engine): require evidence of PeerConnection recovery on resume#1331xianshijing-lk wants to merge 2 commits into
Conversation
A resume decided that a transport had recovered by reading `PeerConnectionState`. That state keeps reporting `Connected` for tens of seconds after the far end goes away -- ICE only leaves `Connected` after its receiving timeout, and only reaches `Failed` after consent expiry -- so the check could not distinguish a transport that recovered from one whose peer had vanished. When it read the stale value, the resume reported success and the engine emitted `Resumed`, and so `RoomEvent::Reconnected` with `ConnectionState::Connected`, for a session whose subscriber transport was dead. Applications had no signal that they had stopped receiving media. The transport's eventual `Failed` then started a fresh cycle -- as a resume again, since no escalation had been recorded -- which burned the full `ICE_CONNECT_TIMEOUT` before escalating. Track two per-transport generations instead, both bumped from existing seams: `negotiation_generation` on every applied remote description, and `disconnect_generation` on every transition away from `Connected`. A resume samples both before touching the signalling link, and then accepts a transport only when it is connected and either renegotiated since the resume began -- positive proof of a live path -- or never left `Connected` for the settle window. A transport that broke and has not renegotiated is rejected regardless of what it currently reports. Because a renegotiation is accepted immediately, genuine recovery no longer waits out a fixed delay; the settle window now bounds only the ambiguous case and is raised to 3s so it exceeds ICE's receiving timeout. Also mark the subscriber as restarting ICE for the duration of a resume. It never issues its own offer, so it had no `create_and_send_offer(ice_restart)` call to set the flag, and remote candidates for the new generation arriving before the SFU's offer were applied against the old remote description instead of being queued. Mirrors `PCTransportManager.triggerIceRestart` in client-sdk-js. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Changeset incompleteThis PR's changeset is missing version bumps for packages that are affected by the change. The following packages still require a bump:
Already covered:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`restart_publisher` marked the subscriber as awaiting a fresh ICE generation on every resume, but only `set_remote_description` cleared it -- and the SFU re-offers the subscriber only when the resume actually moved the participant to another node. After the far more common signal-only resume no offer arrives, so the flag stayed set for the lifetime of the transport and every subsequent remote candidate was buffered instead of applied, leaving the subscriber unable to adopt any new network path the server proposed. The publisher never had this problem because it sets the flag alongside an offer it will certainly receive an answer to. The subscriber's is speculative, so it needs an explicit close: `finish_restarting_ice` clears the flag and applies whatever queued behind it, and the resume calls it once it can no longer expect an offer. Where the SFU did re-offer, the description has already cleared the flag and this is a no-op. Called on the resume's failure path as well, so a transport we may yet keep is never left stranded. Candidates queued because no remote description exists yet stay queued: there is still nothing to apply them against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| session.restart_publisher().await?; | ||
| session.wait_pc_reconnected(PC_RECONNECT_SETTLE_DELAY).await?; | ||
| let reconnected = session.wait_pc_reconnected(pc_snapshot, PC_RECONNECT_SETTLE_DELAY).await; | ||
|
|
||
| // The SFU re-offers the subscriber only when the resume moved us to a different | ||
| // node; on an ordinary signal-only resume no offer is coming, so close the window | ||
| // `restart_publisher` opened and apply whatever queued behind it. Done on the | ||
| // failure path too: leaving it open would strand every later remote candidate on a | ||
| // transport we may yet keep. | ||
| session.finish_subscriber_ice_restart().await; | ||
| reconnected?; |
There was a problem hiding this comment.
🟡 A failed publisher re-offer leaves the media receiver ignoring new network paths
The subscriber's "waiting for a fresh offer" window is opened (mark_restarting_ice() at livekit/src/rtc_engine/rtc_session.rs:2296) but is skipped by the early exit when the publisher re-offer fails, so it is never closed and later network-path information for that receiver is buffered instead of used.
Impact: If a resume aborts at the publisher re-offer step, the still-running session's receiver silently stops adopting any new network path the server proposes for as long as that session is kept alive.
How the early `?` bypasses the window-closing call
SessionInner::restart_publisher (livekit/src/rtc_engine/rtc_session.rs:2290-2307) sets restarting_ice = true on the subscriber transport before issuing the publisher ICE-restart offer. In try_resume_connection, session.restart_publisher().await? (livekit/src/rtc_engine/mod.rs:1158) propagates on error and therefore never reaches session.finish_subscriber_ice_restart() (livekit/src/rtc_engine/mod.rs:1166) — despite the comment there claiming the failure path is covered. With restarting_ice stuck true, PeerTransport::add_ice_candidate (livekit/src/rtc_engine/peer_transport.rs:96-108) queues every subsequent remote candidate forever, and only set_remote_description or finish_restarting_ice can clear it. The engine escalates to a full reconnect after the failed resume, but the old session remains in use until a new one succeeds, so the stuck flag persists for that whole period.
| session.restart_publisher().await?; | |
| session.wait_pc_reconnected(PC_RECONNECT_SETTLE_DELAY).await?; | |
| let reconnected = session.wait_pc_reconnected(pc_snapshot, PC_RECONNECT_SETTLE_DELAY).await; | |
| // The SFU re-offers the subscriber only when the resume moved us to a different | |
| // node; on an ordinary signal-only resume no offer is coming, so close the window | |
| // `restart_publisher` opened and apply whatever queued behind it. Done on the | |
| // failure path too: leaving it open would strand every later remote candidate on a | |
| // transport we may yet keep. | |
| session.finish_subscriber_ice_restart().await; | |
| reconnected?; | |
| let republished = session.restart_publisher().await; | |
| let reconnected = match republished { | |
| Ok(()) => session.wait_pc_reconnected(pc_snapshot, PC_RECONNECT_SETTLE_DELAY).await, | |
| Err(err) => Err(err), | |
| }; | |
| // The SFU re-offers the subscriber only when the resume moved us to a different | |
| // node; on an ordinary signal-only resume no offer is coming, so close the window | |
| // `restart_publisher` opened and apply whatever queued behind it. Done on the | |
| // failure path too: leaving it open would strand every later remote candidate on a | |
| // transport we may yet keep. | |
| session.finish_subscriber_ice_restart().await; | |
| reconnected?; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } else { | ||
| self.subscriber_pc.as_ref().map(|pc| pc.is_connected()).unwrap_or(true) | ||
| match self.subscriber_pc.as_ref() { | ||
| None => true, |
There was a problem hiding this comment.
what situation would lead to the subscriber being None in this else path?
It looks to me like we should require a subscriber to be present here?
| /// The generations sampled at the start of a resume, for readability below. | ||
| const SNAPSHOT: Option<(u32, u32)> = Some((7, 3)); | ||
|
|
||
| /// The defect this fix exists for. |
There was a problem hiding this comment.
The comments in this PR appear overly verbose with wording like this one leaking into code comments where it's not even clear what this fix is outside of the context of this PR.
2 participants
Before you submit your PR
PR description
A resume decided whether a PeerConnection had recovered by reading
PeerConnectionState. That state keeps reportingConnectedfor tens of seconds after the far end goes away — ICE only leavesConnectedafter its receiving timeout, and only reachesFailedafter consent expiry — so the check could not tell a transport that recovered from one whose peer had vanished.When it read the stale value, the resume declared success and the engine emitted
Resumed, and thereforeRoomEvent::ReconnectedwithConnectionState::Connected, for a session whose subscriber transport was dead. An application had no signal that it had stopped receiving media. The transport's eventualFailed(~30s later) then started a fresh cycle — as a resume again, since no escalation had been recorded — which burned the fullICE_CONNECT_TIMEOUTbefore escalating to a full reconnect.livekit/src/rtc_engine/mod.rsalready described this race in the doc comment onPC_RECONNECT_SETTLE_DELAY("the resume can return success immediately and the next failure detector then trips the engine into a real disconnect"), and tried to cover it with a 1s sleep. A delay cannot fix it: the predicate is ambiguous, so sleeping only shifts which side of the race you land on.Reproduction
Requires a multi-node deployment; a single node makes migration meaningless. In the private e2e suite:
(
rust/src/tests/migration.rs, currentlyskip()ed.) Two agents publish video to each other, then one triggersSimulateScenario::NodeFailure. Roughly 2 runs in 3:RoomEvent::Reconnectedfires,connection_state()returnsConnected, no inbound RTP arrives within 45s, and the SDK logsresuming connection failed: connection error: wait_pc_connection timed out.The 2-in-3 rate is the race itself. If the poll lands after the PC has dropped to
Disconnected, the wait times out and the engine escalates correctly; if it lands while the state is still stale, the resume falsely succeeds.migrationpasses consistently (3/3, 10–14s) because a server-drivenLeave{RECONNECT}goes straight to a full reconnect and builds new PeerConnections, never exercising this path.Approach
Track two per-transport generations, both bumped from seams that already exist:
negotiation_generation— incremented inPeerTransport::set_remote_description, i.e. whenever a negotiation round-trip completes. (The rollback insidecreate_and_send_offercalls thePeerConnectiondirectly rather than this wrapper, so re-applying an existing description correctly does not count.)disconnect_generation— incremented from the existingRtcEvent::ConnectionChangehandler whenever the PC leavesConnected, recorded before waking waiters so a drop-and-return between two polls cannot be missed.A resume samples both before touching the signalling link, and accepts a transport only when it is connected and either:
Connectedfor the whole settle window — nothing broke, so the pre-existing connection is still good.A transport that left
Connectedand has not renegotiated since is rejected however it currently reports itself. The initial-connect path is unchanged: it starts fromNew, has no earlier state to be confused by, and still takesConnectedat face value.This is checked against server behaviour rather than assumed. After a node failure the client is routed to a fresh node where the participant starts in
MigrateStateInit, which drives the migration-sync path and makes that node re-offer the subscriber. On a same-node signal blip the participant is alreadyMigrateStateCompleteand no re-offer happens — so "did the subscriber renegotiate" is exactly the right discriminator, and it was already flowing through the SDK, just not recorded.Also included: mark the subscriber as restarting ICE for the duration of a resume. It never issues its own offer, so it had no
create_and_send_offer(ice_restart)call to set the flag, and remote candidates for the new generation arriving before the SFU's offer were applied against the old remote description instead of being queued and replayed. MirrorsPCTransportManager.triggerIceRestartin client-sdk-js.Scope
This makes the resume's verdict honest. It does not, by itself, make a subscriber re-establish that otherwise would not: where recovery genuinely fails, the outcome becomes a fast, deterministic escalation to full reconnect (~15s wait + reconnect) instead of a silent 45s+ failure — the same route
migrationalready takes. That is the correct behaviour either way, and it is what applications need in order to react at all.It also instruments the open question. If
subscriber_negotiationnever advances during anodeFailureresume, the node we landed on never offered, which points atsend_sync_stateusingcurrent_local_description()/current_remote_description()where client-sdk-js usespc.localDescription/pc.remoteDescription— the latter fall back to a pending description, so an offer in flight when the node dies would have us hand the new node stale SDP to rebuild from. That is a separate, still-unverified hypothesis and is deliberately not addressed here.Breaking changes
None to the public API.
RtcSession::wait_pc_reconnectedgains a snapshot parameter, butrtc_engineispub(crate)-facing and the only caller is the resume path.One behavioural change worth flagging:
PC_RECONNECT_SETTLE_DELAYgoes 1s → 3s, and must exceed ICE's receiving timeout to do its job. Since a completed renegotiation is now accepted immediately, genuine recovery gets faster than before rather than slower — the window is only reached in the ambiguous "nothing broke and nothing renegotiated" case, i.e. a signal-only blip where the media plane was fine throughout. Those resumes settle in ~3s instead of ~1s. If that latency matters, the follow-up that removes it is confirming liveness from the selected candidate pair's stats, which the session already collects.MSRV
Unchanged.
Testing
cargo test -p livekit --lib— 83 passed.The fix is a decision-logic defect, so the decision is split into a free function
recovery_decision(..)that is exercised directly, without standing up a PeerConnection:stale_connected_after_a_disconnect_is_not_recovery— the regression test. Asserts a transport reportingConnected, whose disconnect generation moved and which has not renegotiated, is not recovered. The previous logic wasis_connected()alone, which returnstruehere, so this test fails against the old code.renegotiation_is_accepted_immediately— a completed renegotiation short-circuits the settle window, so genuine recovery is not slowed.unbroken_connection_is_accepted_only_after_settling— asserts both sides of the boundary, so the settle gate cannot be dropped without failing.disconnected_transport_is_never_recovered,initial_connect_takes_connected_at_face_value— the remaining branches, including that initial connect is not gated on renegotiation.The counters are verified against real
PeerConnections, following the existingrenegotiation_does_not_deadlockpattern:negotiation_generation_advances_on_applied_remote_description— drives a real offer/answer exchange and asserts an unanswered offer does not bump the counter while an applied answer does. Both directions matter: failing to bump would reject a genuine recovery into an unnecessary full reconnect; bumping without a negotiation would accept a dead transport.disconnect_generation_records_leaving_connected— asserts stayingConnectedis not a disconnect, and that returning toConnectedleaves the record standing, which is the case a polling observer would otherwise miss.End-to-end coverage stays in the private e2e suite, where
nodeFailureneeds a multi-node Cloud deployment. I have not run it — that needs staging credentials. Expected result is that it passes deterministically, but via escalation to full reconnect rather than via a working resume; re-enabling it (deletingfn skip()inrust/src/tests/migration.rsand flipping the Rust column indocs/sdk-test-matrix.md) should be a separate change once someone has confirmed that against staging.Async
No new runtime dependencies. The settle window is measured with
std::time::Instantand elapses concurrently with polling rather than as an upfrontsleep, which is what lets a renegotiating transport be accepted the moment it renegotiates. Waiting is still driven by the existingpc_state_notifyevent flow — this adds no artificial delay for state to "catch up"; the window is a protocol requirement (ICE's receiving timeout), documented as such on the constant and inlivekit/specs/signalling-reconnection.allium. The two new unit tests use#[tokio::test], consistent with the existing tests in those modules.