fix: recover broken publish paths and stuck reconnects - #2030
Conversation
Adds several connection-recovery improvements aimed at the class of failures where publishing is broken even though the peer connection still reports connected: - Act on local `ConnectionQuality.Lost`: when the server reports it isn't receiving our media for a sustained period while connected and publishing, force a full reconnect. - Verify the ICE restart actually landed during a resume (wait for `restartingIce` to clear via a matching-offerId answer) instead of only waiting for `connected`, and escalate a rejected publisher answer during a resume to a full reconnect. - Recreate the peer connection (via escalation) when an ICE restart is needed but there is no remote description to restart on, rather than stalling on `renegotiate`. - Add outbound-RTP liveness to `verifyTransport()` (bytesSent must advance with active senders) and bound how long a transport may stay CONNECTING. - Preserve a full-reconnect request that arrives mid-resume so a successful resume no longer clears it. - On a detected connection state mismatch, attempt a full reconnect (keeping the room alive) instead of tearing the session down. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: dcad734 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
Reverts the resumeConnection change that blocked the resume until the publisher's ICE restart offer was answered (`restartingIce` cleared). When only the signal blips but the media path is fine, the server may never answer the resume's ICE-restart offer, so gating on it hangs the resume and it never emits `resumed` (regressed the signalDisconnectDuringResume e2e test). A stale-but-connected publisher is instead caught at runtime by the local `ConnectionQuality.Lost` handler and the outbound-RTP liveness check in `verifyTransport()`, without breaking resume semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (this.hasActivePublisherSenders()) { | ||
| const bytesSent = await this.getPublisherBytesSent(); | ||
| if (bytesSent !== undefined) { | ||
| const advanced = | ||
| this.lastPublisherBytesSent === undefined || bytesSent > this.lastPublisherBytesSent; | ||
| this.lastPublisherBytesSent = bytesSent; | ||
| if (!advanced) { | ||
| this.log.warn('publisher outbound bytes not advancing while senders active', { | ||
| ...this.logContext, | ||
| bytesSent, | ||
| }); | ||
| return false; | ||
| } | ||
| } | ||
| } else { | ||
| this.lastPublisherBytesSent = undefined; | ||
| } |
There was a problem hiding this comment.
🔴 Sessions that legitimately send no video for a few seconds get forcibly reconnected
A connected session is judged broken (bytesSent > this.lastPublisherBytesSent at src/room/RTCEngine.ts:1729-1741) purely because outbound bytes didn't grow between health checks, so a perfectly healthy session that momentarily has nothing to send is torn down and re-established.
Impact: Users sharing a static screen or holding a paused/muted (but still attached) camera can be kicked into a full reconnect, briefly interrupting their published media.
Why a healthy publisher can flatline outbound bytes
hasActivePublisherSenders() (src/room/RTCEngine.ts:1228-1234) only requires a sender whose MediaStreamTrack.readyState === 'live'; it does not consider whether the track is currently producing frames. Two common cases keep a sender "live" while producing no RTP:
- A screen share of unchanging content: the capturer delivers no new frames, so the encoder emits nothing and
bytesSentstays flat. - A user-provided or screen-share track muted via
enabled = false(LiveKit only callsstop()for non-user-provided camera/mic tracks — seeLocalVideoTrack.mute()andLocalAudioTrack.mute()), which stayslivebut can produce (near-)zero traffic.
The reconcile in src/room/Room.ts:2587-2628 runs every 4s and escalates after 3 consecutive failures, so ~12s of a static/paused publisher (with no other advancing sender, e.g. mic stopped on mute) is enough to trigger engine.reconnect().
A tolerance (e.g. require several consecutive stalled samples only when a track is unmuted/expected to send, or compare against framesEncoded/packetsSent plus an allowance for idle encoders) would avoid the false positive.
Prompt for agents
verifyTransport() in src/room/RTCEngine.ts treats a non-advancing summed outbound-rtp bytesSent as a broken publish path whenever hasActivePublisherSenders() is true. hasActivePublisherSenders() only checks that sender.track.readyState === 'live', which is also true for tracks that legitimately produce no media: a screen share of static content (capturer emits no new frames), or a muted user-provided/screen-share track (LiveKit only stops camera/mic tracks that are not user-provided, see LocalVideoTrack.mute / LocalAudioTrack.mute). Combined with Room's reconcile (4s interval, 3 consecutive failures) this can force a full reconnect on a perfectly healthy session after ~12 seconds of idleness. Consider restricting the liveness assertion to senders whose track is enabled/unmuted and whose source is expected to emit, or requiring corroborating evidence (e.g. ICE/DTLS state, RTCP round trip, or a much longer stall window) before declaring the transport unhealthy.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is also an interesting question too 🤔
| private clearLostQualityTimeout() { | ||
| if (this.lostQualityTimeout) { | ||
| clearTimeout(this.lostQualityTimeout); | ||
| this.lostQualityTimeout = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Pending connection-quality reconnect timer may not be cancellable on some platforms
The countdown that forces a reconnect is cancelled with the global timer-clearing call (clearTimeout(this.lostQualityTimeout) at src/room/RTCEngine.ts:1222) instead of the platform-overridable one used to start it, so on platforms that swap in their own timers the countdown can keep running and reconnect anyway.
Impact: On platforms with custom timers (e.g. React Native background-safe timers), a connection-quality alarm that should have been cancelled can still fire and needlessly restart the connection.
Timer API mismatch
The timeout is created with CriticalTimers.setTimeout (src/room/RTCEngine.ts:1203), whose implementation can be replaced at runtime (src/room/timers.ts) with a platform-specific one whose handle is not a plain setTimeout id. Every other site in the codebase pairs it with CriticalTimers.clearTimeout (e.g. clearReconnectTimeout at src/room/RTCEngine.ts:1962-1966, clearPingTimeout in src/api/SignalClient.ts). Using the global clearTimeout here can silently fail to cancel the handle.
| private clearLostQualityTimeout() { | |
| if (this.lostQualityTimeout) { | |
| clearTimeout(this.lostQualityTimeout); | |
| this.lostQualityTimeout = undefined; | |
| } | |
| } | |
| private clearLostQualityTimeout() { | |
| if (this.lostQualityTimeout) { | |
| CriticalTimers.clearTimeout(this.lostQualityTimeout); | |
| this.lostQualityTimeout = undefined; | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
^ I was also wondering this, I think this change as is is today would work on web, but would fail to clean itself up properly on react native.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
I think ideally it would be good to get some test coverage for these paths - I know that's challenging since this is fairly intermixed in the existing room connection logic. Maybe some e2e test(s)?
| private clearLostQualityTimeout() { | ||
| if (this.lostQualityTimeout) { | ||
| clearTimeout(this.lostQualityTimeout); | ||
| this.lostQualityTimeout = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
^ I was also wondering this, I think this change as is is today would work on web, but would fail to clean itself up properly on react native.
| if (this.hasActivePublisherSenders()) { | ||
| const bytesSent = await this.getPublisherBytesSent(); | ||
| if (bytesSent !== undefined) { | ||
| const advanced = | ||
| this.lastPublisherBytesSent === undefined || bytesSent > this.lastPublisherBytesSent; | ||
| this.lastPublisherBytesSent = bytesSent; | ||
| if (!advanced) { | ||
| this.log.warn('publisher outbound bytes not advancing while senders active', { | ||
| ...this.logContext, | ||
| bytesSent, | ||
| }); | ||
| return false; | ||
| } | ||
| } | ||
| } else { | ||
| this.lastPublisherBytesSent = undefined; | ||
| } |
There was a problem hiding this comment.
This is also an interesting question too 🤔
2 participants
What & why
A set of connection-recovery improvements targeting the class of failures where publishing is broken even though the peer connection still reports
connected— cases the current logic doesn't notice, so the client sits on a dead session without reconnecting.Changes
Act on local
ConnectionQuality.Lost(RTCEngine) —LOSTis the server's verdict that it isn't receiving our media. If it staysLOSTfor a sustained window (5s) while connected and actively publishing, force a full reconnect. Catches the whole class regardless of root cause. Any non-LOSTupdate cancels the pending trigger.Recreate the PC when an ICE restart has no remote description (
PCTransport) — the old TODO fell through torenegotiate = true, which stalls (the pending offer is never answered). Now it throws so the caller escalates to a full reconnect (which rebuilds the peer connections). Only reachable on the resume path.Outbound-RTP liveness in
verifyTransport()(RTCEngine,Room) —verifyTransport()is now async: it checks the publisher's summedbytesSentis advancing when there are active senders, and bounds how long a transport may sit inCONNECTING(previously treated as healthy indefinitely). Room's reconcile awaits it once per tick.Don't clobber a full-reconnect request that arrives mid-resume (
RTCEngine) — a successful resume no longer clearsfullReconnectOnNextit didn't act on (e.g. a serverRECONNECTleave that arrived during the resume); it's dispatched after the attempt settles.Reconnect on a detected state mismatch instead of tearing down (
Room,RTCEngine) — when the connection-reconcile safety net trips (transport silently died while we looked connected), it now triggers a full reconnect via the existing engine (reusing saved credentials, keeping the room alive) rather than terminally disconnecting. Terminal teardown remains the fallback when there's no usable engine or the reconnect ultimately fails.Dropped
An earlier revision gated the resume on the ICE restart formally landing (waiting for
restartingIceto clear via a matching-offerId answer). This was reverted: when only the signal blips but the media path is fine, the server may never answer the resume's ICE-restart offer, so gating on it hangs the resume and it never emitsresumed(regressed thesignalDisconnectDuringResumee2e test). A stale-but-connected publisher is instead caught at runtime by change (1) and the liveness check in (3), without changing resume semantics.Reviewer notes
DisconnectReason.STATE_MISMATCH) previously emittedRoomEvent.Disconnectedimmediately. It now goesReconnecting → (Reconnected | Disconnected), matching how every other transport failure is handled. Apps that watched for an immediateSTATE_MISMATCHdisconnect to trigger their own reconnect will see the reconnecting flow instead.verifyTransport()changed from syncbooleantoasync Promise<boolean>— it's@internal, andRoomis the only caller.Testing
pnpm type:check,pnpm lint(0 errors),pnpm throws:check, andpnpm test(663 tests) all pass.🤖 Generated with Claude Code