fix(rtc_engine): escalate a PeerConnection that stays disconnected - #1332
Conversation
| // Already reported the moment it happened, by the `Failed` branch in the connection | ||
| // change handler; escalating again here would double-report the same failure. | ||
| PeerConnectionState::Failed => false, | ||
| // Still `Disconnected` — or never got back past `Connecting` — after a window that a |
There was a problem hiding this comment.
The ICE_CONNECTION_TIMEOUT is currently 15s.
As this check here also acts on connecting state now, we would basically never allow for PC reconnections to take longer than 5s. Is that desired behaviour?
4bfb607 to
7cdd933
Compare
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}
| // Still `Disconnected` — or never got back past `Connecting` — after a window that a | ||
| // healthy blip would have recovered inside. Treat it as a dead transport. | ||
| _ => true, | ||
| } |
There was a problem hiding this comment.
🟡 Slow but healthy reconnections get upgraded to a heavier full reconnect after five seconds
A connection that is still re-establishing itself is judged dead and reported as failed (should_escalate_after_grace at livekit/src/rtc_engine/rtc_session.rs:2783-2797) after only five seconds, even though the lightweight recovery path is allowed fifteen seconds to finish, so recoveries that take longer are always turned into the heavier full rebuild.
Impact: Users on slower or lossy networks lose the fast in-place recovery and instead get a full reconnect, which tears down and republishes all their tracks.
Grace-period escalation races the resume's own ICE_CONNECT_TIMEOUT budget
Sequence: a transport drops to Disconnected, which arms the 5s countdown (arm_disconnected_grace, livekit/src/rtc_engine/rtc_session.rs:2048-2085). Independently the engine starts a resume, which waits up to ICE_CONNECT_TIMEOUT (15s, livekit/src/rtc_engine/rtc_session.rs:2484) for the transports to come back, and additionally applies PC_RECONNECT_SETTLE_DELAY (3s, livekit/src/rtc_engine/mod.rs:77).
While the ICE restart / renegotiation is in flight the PeerConnection reads Connecting, and should_escalate_after_grace maps everything except Connected/Closed/Failed to true — so at t=5s the countdown fires on_session_disconnected(...). RtcEngineInner::reconnection_needed sees running_handle.reconnecting == true and sets running_handle.full_reconnect = true (livekit/src/rtc_engine/mod.rs:824-835), which is explicitly sticky and cannot be downgraded. The in-flight resume is therefore converted into a full reconnect (new session, new PCs, tracks unpublished/republished) at 5s, even though it had 10 more seconds of budget and might well have succeeded.
The unit test transport_still_down_after_grace_is_escalated asserts Connecting escalates, so this is deliberate in the decision function, but it conflicts with the resume path's own timeout budget. Distinguishing Disconnected (genuinely dead) from Connecting/New (actively re-establishing), or aligning the grace window with the resume budget, would avoid it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
that's the same issue I refered to in this comment
7cdd933 to
80ccb82
Compare
64b0e51 to
1cb2a4e
Compare
Only `PeerConnectionState::Failed` drove recovery. libwebrtc does not reach `Failed` until ICE consent expires, tens of seconds after a transport actually stops working; `Disconnected`, which it reports within seconds, was ignored entirely. Nothing acted in between, so a session whose media plane had died stayed "connected" and silently deaf for the whole consent window before anything began recovering. This is most visible in steady state -- a NAT rebind or a network handover with no signalling failure at all -- where no resume is in flight to notice. `Disconnected` is not on its own a reason to reconnect: ordinary network disturbance produces brief disconnects that recover unaided, and tearing down a session for one would be worse than the disturbance. So a transport entering `Disconnected` now starts a grace period and is judged on its state when that elapses, rather than on the transition that started it. A connection that recovered needs no cancellation bookkeeping -- it simply reads as connected and the countdown lapses silently. Repeated transitions collapse onto the running countdown instead of each spawning their own. Escalation is left to the engine, which already interprets a transport failure in context: outside a reconnect it starts one, and during a reconnect it sticks a full-reconnect escalation onto the cycle rather than looping on resume. For reference, client-sdk-js does not act on `disconnected` either -- its `PCTransportManager.updateState` has no branch for it, so the state silently retains its previous value and `verifyTransport` keeps accepting it. Its actual net for this case is the server-driven `ConnectionQuality::LOST` signal (`scheduleLostQualityReconnect`), which catches a publisher the server has stopped receiving but not a subscriber that has stopped receiving the server. Acting locally on `disconnected` covers both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1cb2a4e to
213407c
Compare
2 participants
Before you submit your PR
PR description
Only
PeerConnectionState::Faileddrove recovery. libwebrtc does not reachFaileduntil ICE consent expires — tens of seconds after a transport actually stops working — whileDisconnected, which it reports within a couple of seconds, was ignored entirely. Nothing acted in between, so a session whose media plane had died stayed "connected" and silently deaf for the whole consent window before anything began recovering.The clearest case is steady state, with no signalling failure at all: a NAT rebind, a Wi-Fi→cellular handover, a route change. There is no resume in flight to notice,
Disconnectedis dropped on the floor, and recovery does not begin untilFailedfinally arrives.It also lengthened node-failure recovery, though #1331 already removes the worst of that: with the resume no longer accepting a stale
Connected, it rejects the dead transport and escalates at its own bound. This PR shortens that further and, more importantly, covers the case where nothing is waiting.Approach
Disconnectedis deliberately not treated as an immediate failure: brief disconnects during ordinary network disturbance are normal and self-healing, and tearing down a session for one would be worse than the disturbance itself. So a transport enteringDisconnectedstarts a grace period (PC_DISCONNECTED_GRACE, 5s — comfortably past a transient blip, far short of consent expiry) and is judged on its state when the countdown elapses, not on the transition that started it.Judging on the final state rather than tracking the transition has a nice property: a connection that recovered on its own needs no cancellation bookkeeping at all. It simply reads as connected when the countdown lapses, and nothing happens. Repeated
Disconnectedtransitions collapse onto the countdown already running rather than each spawning their own, so a flapping transport cannot accumulate timers that all fire and re-report the same failure.Escalation is delegated to the engine rather than decided locally, because the engine already interprets a transport failure in context: outside a reconnect it starts one; during a reconnect its existing sticky-escalation logic converts the cycle to a full reconnect instead of looping on resume.
Failedis left to the existing branch, so it is still reported the instant it occurs and is not double-reported by the countdown.Comparison with client-sdk-js
Worth recording, because it is not a straight port: client-sdk-js does not act on
disconnectedeither.PCTransportManager.updateStatehas no branch for it — none of its conditions match, sothis.statesilently retains its previous value — which meansverifyTransportkeeps accepting the transport and the connection-reconcile loop cannot catch it.JS's actual net for this case is server-driven:
ConnectionQuality::LOSTon the local participant →scheduleLostQualityReconnect(RTCEngine.ts:1207), which is the same arm/re-check/guard shape used here. That signal catches a publisher the server has stopped receiving, but not a subscriber that has stopped receiving the server. Acting locally ondisconnectedcovers both directions.Rust currently forwards
ConnectionQualityto the application and does nothing with it (rtc_engine/mod.rs), so adopting the JS-style LOST trigger as well would be a reasonable complementary follow-up — the two are not redundant.Breaking changes
None. No public API change;
PC_DISCONNECTED_GRACEis new and additive.Behaviourally, a transport that stays disconnected for 5s now triggers recovery where previously it was ignored until
Failed. That is the point of the change, but it does mean sessions on genuinely marginal links may now reconnect where they previously sat in a degraded-but-quiet state. The grace period is the tuning point if that proves too eager in the field.MSRV
Unchanged.
Testing
cargo test -p livekit --lib— 88 passed.The escalation decision is split into
should_escalate_after_grace(state)so each branch is asserted directly, without waiting out a real countdown:transport_still_down_after_grace_is_escalated— the defect itself:Disconnected(andConnecting, i.e. never got back) at the end of the window must escalate rather than wait forFailed.transport_that_recovered_during_grace_is_left_alone— the reason this is a countdown and not an immediate reaction; guards against regressing to "escalate onDisconnected".failed_is_not_double_reported_after_grace—Failedis already reported by the existing branch.closed_transport_is_not_escalated_after_grace— a deliberate teardown (session close, or a full reconnect replacing this session) must not be fought.Plus, against a real
PeerConnection:disconnect_grace_admits_one_countdown_at_a_time— asserts repeated transitions collapse onto one countdown, and that a fresh one can be armed afterwards, so a transport that recovers and dies again is still escalated. The second half is the one that matters: a dedupe flag that is never released would silently disable all subsequent detection.Not covered by unit tests: the wall-clock behaviour of the timer itself, and the engine-side escalation it triggers (already covered by the existing reconnect tests and the
__lk-e2e-testfault injection). End-to-end confirmation needs a live multi-node deployment; I have not run it — I do not have the credentials.Async
Uses
livekit_runtime::spawnandlivekit_runtime::sleep, no direct runtime dependency.on_rtc_eventtakesself: &Arc<Self>so the countdown can keep the session alive for its duration; it checks the session'sclosedflag on wake and bails, so a closed session does not escalate.The delay here is not waiting for state to "catch up" — it is the substance of the fix. Reacting to
Disconnectedwithout it would reconnect on every transient blip. The two new unit tests are synchronous; the existing#[tokio::test]convention in these modules is unchanged.