fix(rtc_engine): require evidence of PeerConnection recovery on resume

[xianshijing-lk]

Before you submit your PR

• I have read the contributing guidelines and validated that this PR will be accepted.
• I have read and followed the principles regarding breaking changes, testing, and code quality.

PR description

A resume decided whether a PeerConnection 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 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 therefore RoomEvent::Reconnected with ConnectionState::Connected, for a session whose subscriber transport was dead. An application had no signal that it had stopped receiving media. The transport's eventual Failed (~30s later) then started a fresh cycle — as a resume again, since no escalation had been recorded — which burned the full ICE_CONNECT_TIMEOUT before escalating to a full reconnect.

livekit/src/rtc_engine/mod.rs already described this race in the doc comment on PC_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 — with a single node there is nothing to move to. Two participants publish video to each other, then one calls Room::simulate_scenario(SimulateScenario::NodeFailure).

Roughly 2 runs in 3: RoomEvent::Reconnected fires, connection_state() returns Connected, no inbound RTP arrives within 45s, and the SDK logs resuming 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. The same scenario driven as SimulateScenario::Migration recovers consistently (10–14s) because the server-driven Leave{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 in PeerTransport::set_remote_description, i.e. whenever a negotiation round-trip completes. (The rollback inside create_and_send_offer calls the PeerConnection directly rather than this wrapper, so re-applying an existing description correctly does not count.)
• disconnect_generation — incremented from the existing RtcEvent::ConnectionChange handler whenever the PC leaves Connected, 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:

• its negotiation generation advanced — a fresh offer/answer completed since the resume began, which is positive proof of a live path (the publisher's ICE-restart answer; the subscriber offer from the node we landed on); or
• it never left Connected for the whole settle window — nothing broke, so the pre-existing connection is still good.

A transport that left Connected and has not renegotiated since is rejected however it currently reports itself. The initial-connect path is unchanged: it starts from New, has no earlier state to be confused by, and still takes Connected at 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 already MigrateStateComplete and 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. Mirrors PCTransportManager.triggerIceRestart in 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 a server-driven migration already 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_negotiation never advances during a node-failure resume, the node we landed on never offered, which points at send_sync_state using current_local_description()/current_remote_description() where client-sdk-js uses pc.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_reconnected gains a snapshot parameter, but rtc_engine is pub(crate)-facing and the only caller is the resume path.

One behavioural change worth flagging: PC_RECONNECT_SETTLE_DELAY goes 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 reporting Connected, whose disconnect generation moved and which has not renegotiated, is not recovered. The previous logic was is_connected() alone, which returns true here, 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 existing renegotiation_does_not_deadlock pattern:

• 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 staying Connected is not a disconnect, and that returning to Connected leaves the record standing, which is the case a polling observer would otherwise miss.

End-to-end coverage of the node-failure scenario lives outside this repo and needs a live multi-node deployment, so I have not run it — I do not have the credentials. The expected result is that it now recovers deterministically, but via escalation to full reconnect rather than via a working resume, so enabling that scenario should be a separate change once someone has confirmed it against a real deployment.

Async

No new runtime dependencies. The settle window is measured with std::time::Instant and elapses concurrently with polling rather than as an upfront sleep, which is what lets a renegotiating transport be accepted the moment it renegotiates. Waiting is still driven by the existing pc_state_notify event 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 in livekit/specs/signalling-reconnection.allium. The two new unit tests use #[tokio::test], consistent with the existing tests in those modules.

[github-actions]

Changeset ✓

This PR includes a changeset covering all affected packages:

Package	Bump
livekit	patch
livekit-ffi	patch

[lukasIO]

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?

[xianshijing-lk]

good catch. subscriber_pc is Some only if !single_pc_mode, so in that else case is reached only when !single_pc_mode && subscriber_primary, which means the None arm is unreachable.

Fixed the code.

[lukasIO]

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.

[xianshijing-lk]

Good catch — this is a real leak I introduced, and your reasoning is exactly right. Fixed in b2f300a.

The premise is confirmed by the server: the SFU only re-offers the subscriber when the resume actually moved the participant to a different node (the new node starts it in MigrateStateInit, which drives the migration-sync re-offer). On a same-node signal-only resume the participant is already MigrateStateComplete, so no offer is coming, and set_remote_description — the only thing that clears the flag — never runs.

The asymmetry I missed: the publisher sets restarting_ice alongside an offer it will certainly get an answer to, so it always clears. The subscriber's is speculative — we're only expecting the SFU to re-offer — so it needs an explicit close.

Fix: PeerTransport::finish_restarting_ice() clears the flag and applies whatever queued behind it; 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 it's a no-op. Two details worth flagging:

• Called on the resume's failure path too (the result is captured, the window closed, then ?), so a transport we may yet keep is never left stranded.
• Candidates queued because there is no remote description yet stay queued — there is still nothing to apply them against, so it returns early rather than blindly draining.
• A candidate that fails to apply logs a warning instead of failing the resume; escalating to a full reconnect over one bad candidate would be worse than the problem.

Tests: finishing_ice_restart_without_a_new_offer_resumes_applying_candidates drives the no-re-offer sequence against a real PeerConnection with a remote description — mark, queue, finish, then assert the queue drained and that later candidates go straight through (the second part is the actual user-visible symptom you described). I verified it fails against the unfixed code, with "the window must not outlive the resume that opened it". Plus finishing_ice_restart_is_a_noop_when_not_restarting for the no-description case.

Worth noting for the record: client-sdk-js has the same latent leak — triggerIceRestart() sets subscriber.restartingIce = true and only setRemoteDescription clears it, with no close on the no-re-offer path. So this is not a divergence from the reference; it's a bug in both, now fixed here.

One alternative I considered and rejected: dropping the subscriber mark_restarting_ice entirely. It is arguably unnecessary — the SFU buffers its candidates in clearLocalDescriptionSent() and only flushes them in localDescriptionSent(), after the offer has been sent, so offer-before-candidates is structurally guaranteed for a new generation and the flag guards a window the server cannot produce. I kept it as defence in depth against a race that would be miserable to debug, now that it is properly bounded. Happy to remove it instead if you'd rather not carry the state.

[xianshijing-lk]

Audited every path that opens the window. The previous fix had a gap — now closed in 28a6d6e.

Path	Before 28a6d6e	Now
wait_pc_reconnected succeeds	✅ closed	✅
wait_pc_reconnected fails	✅ closed	✅
SFU does re-offer	✅ closed by set_remote_description (close is a no-op)	✅
restart_publisher itself fails	❌ leaked	✅
resume task cancelled mid-flight (engine closing)	❌ leaked	❌ leaked, benign — see below

The miss was that restart_publisher opened the window as its first act and could then fail sending the publisher offer; the caller's ? skipped the close. Same class of bug as the original review comment, one layer down.

It was survivable, but only by a three-hop argument: any resume failure sets full_reconnect → the next attempt is a full reconnect → try_restart_connection closes the old session before building the new one → the stranded flag lands on a dead transport. True today, but it makes a local invariant depend on distant escalation policy. If a failed resume were ever retried as a resume — which client-sdk-js does for signal-level errors — the leak would go live.

So I made the pairing structural rather than fixing the one path. Opening the window moved out of restart_publisher into the resume, directly adjacent to the close, with both steps wrapped so every exit passes through it:

session.begin_subscriber_ice_restart().await;
let recovered = async {
    session.restart_publisher().await?;
    session.wait_pc_reconnected(pc_snapshot, PC_RECONNECT_SETTLE_DELAY).await
}
.await;
session.finish_subscriber_ice_restart().await;
recovered?;

restart_publisher goes back to doing only what its name says.

The one remaining hole, deliberately: if the engine closes while the resume is awaiting (the reconnect_task future is dropped by the close_notifier select arm), the close never runs. It can't be fixed with a guard — Drop cannot await, and the flag lives behind an async mutex. It is benign because the engine is closed at that point and the session is finished; a stranded flag on a transport nobody will use again has no effect. Flagging it rather than papering over it.

Concurrency: reconnection_needed gates on running_handle.reconnecting and the reconnect holds the reconnecting_lock write guard throughout, so two resumes cannot overlap and open/close cannot interleave. If the SFU's offer arrives concurrently, both paths take the same inner lock and the close is idempotent.

Added finishing_ice_restart_is_unconditional_and_idempotent, which covers the shape of the missed path (open, then close with nothing in between) plus double-close. 91 tests pass; fmt and clippy clean.

Stacked #1332 rebased on this.

[xianshijing-lk]

Nothing can — you're right, that path is unreachable. Fixed in 5c5e34b.

subscriber_pc is Some exactly when single_pc_mode is false. Both are set once in RtcSession::connect and neither is ever reassigned (single construction site; subscriber_pc: Option<PeerTransport> and single_pc_mode: bool, no interior mutability):

// In single PC mode, subscriber_pc is None
let mut subscriber_pc = if single_pc_mode { None } else { Some(PeerTransport::new(..)) };

So the else — reached only when !single_pc_mode && subscriber_primary — could only ever see Some.

And your instinct about requiring it goes further than unreachability: None => true is fail-open, so had it somehow been reached it would have reported a non-existent subscriber as recovered. That is exactly the pattern this PR exists to remove — the whole point is to stop treating absence of evidence as evidence of recovery, so a default doing precisely that in the same predicate is the wrong shape regardless of reachability. It came from me mechanically preserving the .map(|pc| pc.is_connected()).unwrap_or(true) that was there before.

Rather than add a runtime requirement check for a state that cannot occur, I keyed the match on the Option itself, since that is the source of truth for whether a second transport exists:

let subscriber_ok = match self.subscriber_pc.as_ref() {
    None => true,
    Some(_) if !self.subscriber_primary => true,
    Some(pc) => Self::transport_recovered(pc, .., settled),
};

Now None honestly means single-PC mode — the publisher is the only transport, so there is nothing else to wait for — instead of standing in for an impossible state. No default to fall open through, no unreachable arm, and the single_pc_mode test drops out of the condition as redundant (it was exactly subscriber_pc.is_none()).

I also recorded on the field that the equivalence is now load-bearing rather than incidental, since this wait reads absence as "nothing more to wait for" — so anything that could leave it None in dual-PC mode would silently stop a resume waiting on the subscriber.

cargo fmt clean, 86 tests pass. #1332 rebased.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

[devin-ai-integration]

🟡 Queued network candidates are silently thrown away when one of them fails to apply

When the saved-up network candidates are replayed at the end of a resume, a single failing candidate aborts the replay (? at livekit/src/rtc_engine/peer_transport.rs:156) and the remaining ones are discarded rather than tried, so the connection can lose usable network paths.
Impact: After a resume, some server-proposed network paths are dropped entirely, which can leave media unable to recover on a changed network.

Vec::drain semantics discard the untraversed remainder on early return

for ic in inner.pending_candidates.drain(..) creates a Drain iterator; an early ? return drops the iterator, and Drain's Drop removes the entire drained range from the vector regardless of how far iteration progressed. So candidates after the failing one are neither applied nor left queued.

The caller at livekit/src/rtc_engine/rtc_session.rs:2296-2300 swallows the error with the comment "The rest still drain", which is not what happens.

A resilient replay would log and continue on a per-candidate error instead of propagating with ?.

---

Was this helpful? React with 👍 or 👎 to provide feedback.