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 1 commit 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}
| // The subscriber's ICE is restarted by the SFU, which will send us a fresh offer. | ||
| // Queue any remote candidates until it arrives rather than applying them to the | ||
| // outgoing generation. | ||
| if let Some(ref sub_pc) = self.subscriber_pc { | ||
| sub_pc.mark_restarting_ice().await; | ||
| } |
There was a problem hiding this comment.
🟡 Subscriber stops applying new network candidates after a resume where the server never re-offers
The subscriber transport is put into a "waiting for a fresh offer" mode (mark_restarting_ice() at livekit/src/rtc_engine/rtc_session.rs:2275) on every resume, but nothing ever takes it back out unless the server actually sends a new offer, so after the common signal-only resume the client silently buffers every new network path the server offers instead of using it.
Impact: After a reconnect where the media path was never broken, the subscriber can no longer pick up new network routes the server proposes, so media can stall until some unrelated event triggers a new offer.
Flag is only cleared by an incoming remote description, which the signal-only resume path never produces
PeerTransport::mark_restarting_ice (livekit/src/rtc_engine/peer_transport.rs:115-117) sets inner.restarting_ice = true. The only place that resets it is PeerTransport::set_remote_description (livekit/src/rtc_engine/peer_transport.rs:188), i.e. it requires the SFU to send a subscriber offer. The PR description itself states that on a same-node signal blip the participant is already MigrateStateComplete and "no re-offer happens" — precisely the case where wait_pc_reconnected accepts recovery through the "never broke" branch. In that case the flag stays set indefinitely.
While it is set, add_ice_candidate (livekit/src/rtc_engine/peer_transport.rs:149-160) takes the queueing branch for every trickled remote candidate, pushing them into pending_candidates, which is drained only from set_remote_description. The queue is unbounded and the candidates are eventually applied in a batch whenever some later subscriber offer arrives.
A fix would clear the flag when the resume completes without a subscriber renegotiation (or scope it with a timeout), e.g. in resume_finalize / at the end of wait_pc_reconnected_with_snapshot.
Prompt for agents
restart_publisher sets the subscriber transport's restarting_ice flag for the whole resume, but PeerTransport only clears that flag inside set_remote_description. On resumes where the SFU does not re-offer the subscriber (the same-node signal blip case, which the new recovery logic explicitly accepts via the 'never broke' branch), the flag stays true after the resume finishes. From then on every trickled remote ICE candidate for the subscriber is pushed into pending_candidates instead of being applied, and that queue is only drained by the next remote description — which may never arrive. Consider clearing the flag once the resume concludes (success or failure) when no subscriber renegotiation was observed — the resume already knows this, since wait_pc_reconnected_with_snapshot compares the subscriber's negotiation generation against the snapshot — or add an explicit PeerTransport::clear_restarting_ice that also flushes any queued candidates.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
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.