feat(sdk): opt-in peer repair for connected but unusable ICE paths (OPTI-4246) by devin-ai-integration[bot] · Pull Request #544 · millicast/millicast-sdk

devin-ai-integration · 2026-09-04T04:25:40Z

Summary

Viewers in some regions end up with an ICE pair that is succeeded but unusable (RTT of several seconds, 60-90% STUN loss, no decoded video) while a relay pair with good RTT exists in the same peer connection. The browser keeps the bad pair because host candidates have a higher ICE priority than relay candidates. This PR adds an opt-in, experimental peerRepair option to View.connect() that detects this and replaces the connection without stopping media first.

Disabled by default. Nothing changes for existing users when peerRepair.enabled is not set.

How it works

PeerRepairMonitor (new) evaluates every stats event using the raw candidate-pair / transport reports:
- selected pair is bad: RTT > 500 ms or no incoming packets during the first 10 s after connected; RTT > 1 s, loss > 20 % or RTT > 3x the alternative for 3-5 consecutive reports in steady state.
- an alternative pair exists: responsesReceived > 0 and RTT < selected RTT / 2. Without an alternative nothing is done.
- guards: 30 s cooldown, max 3 repairs per View.
On a decision, View.repairConnection() runs the existing migrate path (initConnection({ migrate: true, repair })): new token generator call, new signaling and RTCPeerConnection, old media keeps flowing. The new peer becomes this.webRTCPeer only after it reaches connected; otherwise (state failed/closed or repairTimeoutMs = 10 s) it is closed, the old one is kept and its track events are re-emitted so the app can restore the previous streams.
Candidates are demoted, not removed. SdpParser.demoteCandidates(sdp, candidates) rewrites the priority of the given remote a=candidate lines in the answer to 1 before setRemoteDescription. Pair priority is 2^32 * min(local, remote) + ... (RFC 8445), so those pairs sort last but remain usable as fallback. The selected bad candidate and any candidate that never answered a check (requestsSent >= 5, responsesReceived = 0) are demoted together.
If the alternative is a relay pair and the selected one is not, the replacement peer is created with iceTransportPolicy: 'relay'.
A full connect() creates a new monitor, so a normal reconnect retries all candidates at their original priority. A failed repair also clears the demotions.
New View event peerRepair with state: 'started' | 'completed' | 'failed', the reason, both pairs (type, address, RTT) and the demoted candidates, for telemetry.

Relay-only operation needs no SDK change: connect({ peerConfig: { iceTransportPolicy: 'relay' } }) is passed to the RTCPeerConnection as before. The viewer plugin maps the relayonly and pcrepair query parameters to these options (separate PRs).

Release

Version is set to 0.8.2-pcrepair.0. It must be published with the pnpm-build-publish workflow using dist_tag=pcrepair so latest stays at 0.8.1. If the option proves useful it will be released normally; otherwise the prerelease is left unused.

Tests

tests/features/PeerRepair.feature: SDP demotion (IPv6 and IPv4 lines, empty set), repair with alternative, no repair without alternative or with a non-better alternative, multi-candidate demotion, steady-state hysteresis, cooldown, failed repair clearing state, disabled monitor. pnpm run build, eslint and the full unit suite (30 suites) pass.

Manual validation in Chrome, Firefox and Safari of the rewritten remote priority is still pending (done through the public viewer with ?pcrepair=true).

Link to Devin session: https://dolby.devinenterprise.com/sessions/5a13aa779c4c4f8b8f5fc938cee80dcc
Open in Devin Desktop: https://dolby.devinenterprise.com/desktop/session/5a13aa779c4c4f8b8f5fc938cee80dcc?variant=devin
Requested by: @craig-johnston

…te priority Co-Authored-By: craig.johnston <cjohn@dolby.com>

… peer connections (OPTI-4246) Co-Authored-By: craig.johnston <cjohn@dolby.com>

Co-Authored-By: craig.johnston <cjohn@dolby.com>

devin-ai-integration · 2026-09-04T04:25:43Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

changeset-bot · 2026-09-04T04:25:44Z

⚠️ No Changeset found

Latest commit: 1887abd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

craig-johnston

Reviewed the peerRepair design end-to-end (PeerRepairMonitor, SdpParser.demoteCandidates, and the make-before-break path in View.js). The happy path is sound and the feature is correctly opt-in/off-by-default, but the failure/lifecycle paths have a few bugs that unit tests don't currently exercise (PeerRepair.steps.js only tests the pure decision function, not View orchestration). Left line comments below; the two in PeerRepairMonitor.js are the ones I'd block on since they defeat the ticket's stated recovery behavior ("if there is no alternative / repair fails, the bad connection is kept" implies monitoring should continue, not stop).

craig-johnston · 2026-09-04T05:35:47Z

+  }
+
+  onRepairFailed () {
+    this.clearDemotions()


Bug: monitor is permanently disabled after the first failed repair.

onRepairStarted (line 116) calls this.reset(), which sets connectedAt = null. onRepairFailed only calls clearDemotions() — it never restores connectedAt. evaluate() short-circuits whenever connectedAt === null:

if (!this.options.enabled || !stats?.raw || this.connectedAt === null) return null

So the very first repair attempt that fails (Director token error, signaling failure, repairTimeoutMs timeout) silently turns off monitoring for the rest of the View session, even though the old (bad) connection is kept and the intent per the PR description is to keep watching. Suggest restoring connectedAt here, e.g.:

onRepairFailed () { this.clearDemotions() this.onConnected() }

This path isn't covered by PeerRepair.feature — the "Failed repair clears demotions" scenario only asserts demotedCandidates/relayOnly/attempts, not that evaluate() still produces decisions afterward.

Fixed in dd699d6. onRepairFailed() now calls clearDemotions() and then onConnected(), so evaluate() keeps producing decisions for the kept connection. Note: this restarts the startup window for that peer; the 30 s cooldown still prevents an immediate second attempt. Added scenario "Monitoring continues after a failed repair", which asserts that a decision is produced after a failed repair.

craig-johnston · 2026-09-04T05:35:47Z

+      this.demotedCandidates.add(candidate)
+    }
+    if (decision.relayOnly) {
+      this.relayOnly = true


Bug: relayOnly is sticky and can't go back to false.

if (decision.relayOnly) this.relayOnly = true never assigns false when a later decision picks a non-relay alternative. Once one repair goes through a relay pair, every subsequent repair in the same View session is forced to iceTransportPolicy: 'relay' (see the read of this.peerRepairMonitor?.relayOnly in View.js initConnection), even if the monitor would otherwise prefer a direct pair. The only way to clear it today is onRepairFailed() or a brand-new connect(). Suggest assigning directly: this.relayOnly = decision.relayOnly.

Fixed in dd699d6: this.relayOnly = decision.relayOnly === true. Added scenario "Relay only follows the latest repair decision" (relay repair followed by a direct-alternative repair turns it off; demotions are kept).

craig-johnston · 2026-09-04T05:35:47Z

+     */
+    this.emit('peerRepair', { state: 'started', ...decision })
+    try {
+      await this.initConnection({ migrate: true, repair: decision })


Bug: replacement peer/signaling leak when initConnection({repair}) throws before finishRepair is reached.

If createRTCPeer, signalingInstance.connect(), subscribe(), setLocalDescription(), or setRTCRemoteSDP() throws anywhere inside initConnection, the exception propagates here and the catch below only resets flags and calls onRepairFailed(). The half-created webRTCPeerInstance/signalingInstance (already registered in this.trackEventsByPeer and, in some cases, already listening via monitorPeerRepair) are never closed or removed. Only failures after finishRepair is reached get cleaned up via its rollback(). Suggest wrapping the body of initConnection (from where webRTCPeerInstance/signalingInstance are created onward) in a try/finally that closes them on the repair path, or having this catch close whatever was created before rethrowing/swallowing.

Fixed in dd699d6. initConnection stores the replacement pair in this.repairPeer right after createRTCPeer. The catch in repairConnection and the rollback in finishRepair both call closeRepairPeer(), which closes the signaling and peer and removes the trackEventsByPeer entry. stop() calls it too.

craig-johnston · 2026-09-04T05:35:48Z

+
  stop () {
    super.stop()
+    this.repairInProgress = false


Risk: stop() doesn't cancel an in-flight repair.

stop() resets repairInProgress and closes this.webRTCPeer/this.signaling, but finishRepair's complete()/rollback() closures (below, ~line 516) only guard on a local done flag — they don't check whether the View was stopped in the meantime. If stop() runs while a repair is in flight, complete() can still fire afterward and reassign this.webRTCPeer/this.signaling to the replacement, effectively reopening a connection the caller just asked to close. Relatedly, BaseWebRTC.reconnect() isn't aware a repair is in progress, so a disconnect on the old peer during a repair could race a full reconnect against finishRepair. Suggest a guard (e.g. checking repairInProgress/a generation counter, or an explicit stopped flag captured by finishRepair's closures) before complete()/rollback() touch this.webRTCPeer/this.signaling.

Fixed in dd699d6 with a connectionGeneration counter. stop() increments it (so reconnect(), which calls stop(), does too) and closes any in-flight replacement via closeRepairPeer(). repairConnection and finishRepair capture the generation; if it changed, finishRepair.onState closes the replacement and returns without touching this.webRTCPeer/this.signaling, and repairConnection's catch does not emit failed for a stopped View.

craig-johnston · 2026-09-04T05:35:48Z

+
+  monitorPeerRepair (webRTCPeerInstance) {
+    webRTCPeerInstance.on(webRTCEvents.connectionStateChange, (state) => {
+      if (state === 'connected' && this.webRTCPeer === webRTCPeerInstance) {


Risk: doesn't account for browsers that report completed instead of connected.

PeerConnection.js's addPeerEvents (the oniceconnectionstatechange branch, used when peer.connectionState doesn't exist, e.g. Firefox) emits the raw peer.iceConnectionState value here, which can be the literal string 'completed'. This listener only calls onConnected() for state === 'connected', so on a browser/timing path where the ICE agent's state transitions coalesce straight to completed without a separate connected event being observed, connectedAt stays null and the monitor silently never starts evaluating. Note getConnectionState() in PeerConnection.js already normalizes completed → connected for getRTCPeerStatus()/finishRepair's onState check — this listener should probably use the same normalization (e.g. call webRTCPeerInstance.getRTCPeerStatus() === 'connected' instead of comparing the raw emitted state to 'connected').

Fixed in dd699d6. Added isConnectedState(state) (connected or completed) and used it in monitorPeerRepair and in finishRepair.onState, so Firefox's raw iceConnectionState is handled. I did not call getRTCPeerStatus() because it logs at info level on every call.

craig-johnston · 2026-09-04T05:35:48Z

    }

    webRTCPeerInstance.on(webRTCEvents.track, (trackEvent) => {
+      this.trackEventsByPeer.get(webRTCPeerInstance)?.push(trackEvent)


Risk: old peer's track events aren't filtered out after finishRepair switches this.webRTCPeer.

This listener is attached per-peer-instance (old and new) and unconditionally calls this.onTrackEvent(trackEvent) (via line 407) once isMainStreamActive, regardless of whether webRTCPeerInstance is still this.webRTCPeer. After finishRepair's complete() reassigns this.webRTCPeer to the replacement, the old peer is kept alive for 1s before closeRTCPeer() (line ~528) and can still emit late track events, which would overwrite this.tracksMidValues and re-emit track to the application — potentially flipping the video element back to the old stream briefly right after a successful repair. Suggest gating this callback on this.webRTCPeer === webRTCPeerInstance (mirroring the check already used in monitorPeerRepair), at least for the repair case.

Fixed in dd699d6. When peer repair is enabled, the track listener ignores events from a peer that is neither this.webRTCPeer nor the pending replacement (this.repairPeer.webRTCPeer). So after complete() swaps the peers, late track events from the old peer are dropped. Behavior without peerRepair is unchanged.

- keep monitoring after a failed repair (restore connectedAt) - relayOnly follows the latest decision instead of being sticky - close the replacement peer/signaling when initConnection throws - stop()/reconnect invalidate an in-flight repair (connection generation) - treat 'completed' as connected (Firefox iceConnectionState) - ignore track events from peers that are neither current nor the replacement Co-Authored-By: craig.johnston <cjohn@dolby.com>

devin-ai-integration · 2026-09-04T05:39:43Z

All six review comments are addressed in dd699d6 (replies in each thread). Unit tests: 185 pass, with 2 new monitor scenarios. The View orchestration paths (stop during repair, cleanup on throw, old-peer track events) are still only covered by the manual browser test, not unit tests.

…only-pcrepair

devin-ai-integration Bot and others added 4 commits September 4, 2026 04:24

feat(sdk): add SdpParser.demoteCandidates to lower remote ICE candida…

085bda9

…te priority Co-Authored-By: craig.johnston <cjohn@dolby.com>

feat(sdk): opt-in peerRepair option to replace connected but unusable…

8bedb8c

… peer connections (OPTI-4246) Co-Authored-By: craig.johnston <cjohn@dolby.com>

test(sdk): peer repair monitor and candidate demotion

b12703d

Co-Authored-By: craig.johnston <cjohn@dolby.com>

chore(sdk): prerelease version 0.8.2-pcrepair.0

d2f87e2

Co-Authored-By: craig.johnston <cjohn@dolby.com>

devin-ai-integration Bot assigned craig-johnston Sep 4, 2026

devin-ai-integration Bot mentioned this pull request Sep 4, 2026

feat: relayonly and pcrepair options mapped to SDK peerConfig/peerRepair (OPTI-4246) millicast/vue-viewer-plugin#198

Draft

craig-johnston requested changes Sep 4, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into devin/OPTI-4246-relay…

1887abd

…only-pcrepair

Uh oh!

Conversation

devin-ai-integration Bot commented Sep 4, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How it works

Release

Tests

Uh oh!

devin-ai-integration Bot commented Sep 4, 2026

🤖 Devin AI Engineer

Uh oh!

changeset-bot Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

craig-johnston left a comment

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

craig-johnston Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

devin-ai-integration Bot commented Sep 4, 2026 •

edited by atlassian Bot

Loading

changeset-bot Bot commented Sep 4, 2026 •

edited

Loading