VAPI-3917 fix(signaling): rebuild peer connections and re-publish media on signaling reconnect - #18
Open
smoghe-bw wants to merge 5 commits into
Open
VAPI-3917 fix(signaling): rebuild peer connections and re-publish media on signaling reconnect#18smoghe-bw wants to merge 5 commits into
smoghe-bw wants to merge 5 commits into
Conversation
…aling reconnect The websocket client auto-reconnects on drops (rpc-websockets, unlimited reconnect), and every reconnect re-fires "open" - re-running setMediaPreferences and re-emitting "init". BandwidthRtc.init() reacted to that by building brand-new RTCPeerConnections every time, without closing the stale ones or re-adding any already-published MediaStream tracks. A signaling reconnect therefore silently dropped all media and orphaned the old peer connections, even though the underlying connection is meant to resume the same session. signaling.ts now tracks whether an "open" is the first one or a reconnect and passes that through on the "init" event. bandwidthRtc.ts's init() closes the stale peer connections and resets subscribe-side bookkeeping on a reconnect, then re-adds every currently published stream to the rebuilt publishing connection and re-offers, so the far end keeps receiving media instead of silence. Also fixes a dead-code bug in setupPeerConnection/setupNewPeerConnection: the "disconnected" connection-state handler was being immediately overwritten by the "failed" handler set right after it, so the disconnected-state log could never fire. Merged into one handler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Every handler in setupNewPeerConnection (and the merged onconnectionstatechange handler in setupPeerConnection) caught errors but only logged them when globalThis.window was set, silently discarding them in any non-browser environment. logger.warn has no browser dependency (just console + EventEmitter), so there was nothing this guard was protecting against - it just meant every error here vanished with zero trace outside a browser. Log unconditionally. Also switched retryOffer in retryIceOnFailed to an async/await function with braces instead of an implicit-return arrow expression, matching the project's style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
requestOutboundConnection, hangupConnection, acceptStream, declineStream, offerSdp, and answerSdp only logged the outgoing call. If the gateway rejected the RPC, the rejection propagated with no trace in this SDK's own logs - silent unless the calling application happened to catch and log it itself. Log a warning on rejection and rethrow, so callers still see the same rejected promise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
smoghe-bw
changed the title
… methods requestOutboundConnection, hangupConnection, acceptStream, declineStream, offerSdp, and answerSdp were wrapped in async/try-catch+rethrow, out of scope for this PR (signaling reconnect + ICE restart retry) and a regression: wrapping a plain `this.ws?.call(...)` passthrough in `async` means `await undefined` (when `this.ws` is null) resolves silently instead of leaving the caller with a non-Promise value that blows up immediately on `.then()`. Reverts to the direct passthrough. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
rpc-websocketsauto-reconnects on drops (unlimited reconnect), creating a brand-new underlyingWebSocketeach time against the sameJsonRpcClientinstance. Every reconnect re-firesopen, which re-runssetMediaPreferencesand re-emitsinit(signaling.ts) - not just on the very first connect.BandwidthRtc.init()reacted to everyinitthe same way: build brand-newRTCPeerConnections for publish/subscribe. On a reconnect this silently dropped all media - the new publishing connection has no tracks, since whatever was published viapublish()was only ever added to the old (now orphaned, never closed) connection's transceivers - and leaked the stale peer connections.Also found and fixed a dead-code bug while in this area:
setupNewPeerConnectionsetonconnectionstatechangeto log thedisconnectedstate, butsetupPeerConnectionimmediately overwrote that same handler with afailed-only one right after, so thedisconnectedlog path was unreachable.The fix
signaling.ts:connect()now tracks whetheropenis the session's first firing or a reconnect, and passesisReconnectthrough on theinitevent. Thebeforeunloadlistener registration is guarded to only run once, so it no longer re-accumulates on every reconnect either.bandwidthRtc.ts:init(setMediaPreferencesResponse, isReconnect)- on reconnect, closes the stale peer connections, resets subscribe-side bookkeeping (subscribingPeerConnectionSdpRevision,subscribeTrackMetadata,localDtmfSenders), rebuilds both peer connections, then re-adds every currently publishedMediaStreamto the new publishing connection and re-offers so the far end keeps receiving media instead of silence.onconnectionstatechangehandlers insetupPeerConnection/setupNewPeerConnectioninto one that handles bothdisconnected(log only) andfailed(existing ICE-restart retry).Known limitations (called out, not fixed here - scope creep beyond "stop dropping media")
codecPreferencespassed to the originalpublish()call aren't retained across a reconnect (PublishedStreamnever stored them), so a reconnect re-adds tracks with default codec preferences.onStreamUnavailablefires for pre-reconnect subscribed streams before the fresh ones arrive from the rebuilt subscribing connection - there's no class-level tracking of "currently available subscribed streams" to hang that off today.Test plan
tsc --noEmitcleanjest src/v1- all 65 pass, including:signaling.test.ts:initnow emitsisReconnect=falseon the first open andtrueon subsequent opensbandwidthRtc.test.ts(init on signaling reconnect, new): first init doesn't touch old peer connections or re-publish; a reconnect closes stale connections, resets subscribe state, and re-publishes existing streams; a reconnect with nothing published doesn't re-offer🤖 Generated with Claude Code