fix(client): harden SFU reconnection and ICE-restart recovery#2285
Open
oliverlaz wants to merge 12 commits into
Open
fix(client): harden SFU reconnection and ICE-restart recovery#2285oliverlaz wants to merge 12 commits into
oliverlaz wants to merge 12 commits into
Conversation
Add CandidatePairMonitor, which watches the publisher's BUNDLE-shared ICE transport for selected-candidate-pair changes (e.g. WiFi to LTE) and triggers an ICE restart so the SFU learns the new transport path. A settle window, opened on startup and on every restartIce(), absorbs the restart-induced candidate-pair churn so reconnect-driven restarts (Call.restoreICE) do not loop back into another restart. Covered by unit tests for the monitor and Publisher-level reconnect-interplay tests.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesICE Resilience and Signal-Close Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ignal-close revival Replace the standalone CandidatePairMonitor with a reconnect-time check in BasePeerConnection/Publisher: snapshot the selected candidate pair when ICE drops to `disconnected`, and on recovery keep the scheduled ICE restart only if the path actually migrated (e.g. Wi-Fi to LTE). A same-path recovery cancels the restart, avoiding restart loops. Also harden signal-close revival in StreamSfuClient and Call: - add a one-shot latch so onSignalClose fires at most once per client, even when both the health watchdog and a delayed WebSocket close detect death - drive revival immediately on an unhealthy close instead of waiting for the transport close event, which a wedged socket may delay for seconds - ignore signal closes from a superseded client so a stale socket's late close cannot drive a reconnect after a new client has swapped in
oliverlaz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client/src/rtc/BasePeerConnection.ts (1)
477-482:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale timeout state after the callback runs to prevent duplicate restarts.
At Line 477, the timeout handle is never reset when the callback fires. Later, the recovery path at Line 499 can still treat that stale handle as “scheduled” and call
shouldKeepScheduledIceRestart()/tryRestartIce()again after a restart already ran.Suggested fix
clearTimeout(this.iceRestartTimeout); this.iceRestartTimeout = undefined; this.iceRestartTimeout = setTimeout(() => { + this.iceRestartTimeout = undefined; const currentState = this.pc.iceConnectionState; if (currentState === 'disconnected' || currentState === 'failed') { this.tryRestartIce(); } }, this.iceRestartDelay);Also applies to: 499-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/rtc/BasePeerConnection.ts` around lines 477 - 482, The iceRestartTimeout handle is assigned at line 477 but never cleared after the timeout callback executes, causing the stale handle to appear "scheduled" when checked later at lines 499-505 in the recovery path. This can trigger duplicate ICE restart attempts. Fix this by resetting this.iceRestartTimeout to null after the callback logic completes inside the setTimeout body, ensuring that subsequent calls to shouldKeepScheduledIceRestart() or tryRestartIce() recognize that no timeout is currently active. This same cleanup should be applied wherever iceRestartTimeout callbacks are used to prevent similar stale-state issues.
🧹 Nitpick comments (1)
packages/client/src/rtc/__tests__/Publisher.test.ts (1)
699-742: ⚡ Quick winAdd one regression test for “timer already fired, then connected” behavior.
The new suite is strong, but it doesn’t cover the case where the scheduled restart has already fired and ICE later transitions to
connected/completed. Adding that case would prevent duplicate-restart regressions in this reconnect path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/rtc/__tests__/Publisher.test.ts` around lines 699 - 742, Add a new test case after the existing three tests in the Publisher.test.ts suite that verifies the "timer already fired, then connected" behavior. The test should use vi.useFakeTimers(), call publishVideo() to capture the ICE transport, set up a spy on the restartIce method, transition ICE from connected to disconnected (which schedules a restart), advance timers to allow the scheduled restart to execute, and then transition ICE back to connected or completed. Assert that the restartIce method was called exactly once (from the scheduled timer) and not again when ICE transitions to connected, ensuring no duplicate-restart regression occurs in the reconnect path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/client/src/rtc/BasePeerConnection.ts`:
- Around line 477-482: The iceRestartTimeout handle is assigned at line 477 but
never cleared after the timeout callback executes, causing the stale handle to
appear "scheduled" when checked later at lines 499-505 in the recovery path.
This can trigger duplicate ICE restart attempts. Fix this by resetting
this.iceRestartTimeout to null after the callback logic completes inside the
setTimeout body, ensuring that subsequent calls to
shouldKeepScheduledIceRestart() or tryRestartIce() recognize that no timeout is
currently active. This same cleanup should be applied wherever iceRestartTimeout
callbacks are used to prevent similar stale-state issues.
---
Nitpick comments:
In `@packages/client/src/rtc/__tests__/Publisher.test.ts`:
- Around line 699-742: Add a new test case after the existing three tests in the
Publisher.test.ts suite that verifies the "timer already fired, then connected"
behavior. The test should use vi.useFakeTimers(), call publishVideo() to capture
the ICE transport, set up a spy on the restartIce method, transition ICE from
connected to disconnected (which schedules a restart), advance timers to allow
the scheduled restart to execute, and then transition ICE back to connected or
completed. Assert that the restartIce method was called exactly once (from the
scheduled timer) and not again when ICE transitions to connected, ensuring no
duplicate-restart regression occurs in the reconnect path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e76865-c64d-4dbb-88e3-72d1cb60aa47
📒 Files selected for processing (8)
packages/client/src/Call.tspackages/client/src/StreamSfuClient.tspackages/client/src/__tests__/StreamSfuClient.test.tspackages/client/src/rtc/BasePeerConnection.tspackages/client/src/rtc/Publisher.tspackages/client/src/rtc/__tests__/Call.reconnect.test.tspackages/client/src/rtc/__tests__/Publisher.test.tspackages/client/src/rtc/__tests__/mocks/webrtc.mocks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/client/src/rtc/tests/mocks/webrtc.mocks.ts
Wrap Subscriber.negotiate in try/catch/finally so isIceRestarting is always cleared, even when negotiation throws. Previously the reset was the last statement of the happy path and was skipped on failure, leaving the flag stuck at true and disabling all future automatic ICE restart/reconnect handling on the subscriber. On failure, roll back the remote description when the peer connection is still awaiting an answer (have-remote-offer), and trace the negotiation failure. Add tests covering the success, failure, and rollback branches.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/client/src/rtc/Subscriber.ts (1)
204-211: 💤 Low valueRollback failure could mask the original negotiation error.
If
setRemoteDescription({ type: 'rollback' })throws, the originalerris never rethrown and its context is lost. Consider wrapping the rollback in a nested try/catch to preserve the original error for debugging:Proposed fix to preserve original error
} catch (err) { if (this.pc.signalingState === 'have-remote-offer') { - await this.pc.setRemoteDescription({ type: 'rollback' }); + try { + await this.pc.setRemoteDescription({ type: 'rollback' }); + } catch (rollbackErr) { + this.logger.warn('SDP rollback failed after negotiation error', rollbackErr); + } } throw err; } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/rtc/Subscriber.ts` around lines 204 - 211, The catch block in Subscriber.ts where setRemoteDescription with rollback is called can mask the original negotiation error if the rollback itself throws an exception. Wrap the setRemoteDescription({ type: 'rollback' }) call in a nested try/catch block to handle any rollback failure separately, then ensure the original err is still thrown after the finally block executes. This way, if the rollback fails, its error can be logged or handled without losing the context of the original negotiation error that triggered the rollback attempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/client/src/rtc/Subscriber.ts`:
- Around line 204-211: The catch block in Subscriber.ts where
setRemoteDescription with rollback is called can mask the original negotiation
error if the rollback itself throws an exception. Wrap the
setRemoteDescription({ type: 'rollback' }) call in a nested try/catch block to
handle any rollback failure separately, then ensure the original err is still
thrown after the finally block executes. This way, if the rollback fails, its
error can be logged or handled without losing the context of the original
negotiation error that triggered the rollback attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb45dd31-7db5-4bc4-92ed-4ef3a77065b9
📒 Files selected for processing (2)
packages/client/src/rtc/Subscriber.tspackages/client/src/rtc/__tests__/Subscriber.test.ts
Reverts the organic ICE path migration detection added earlier on this branch: the onIceDisconnected/shouldKeepScheduledIceRestart hooks in BasePeerConnection and the candidate-pair snapshot/compare logic in Publisher. When ICE returns to connected after a disconnect, the scheduled restartICE is once again simply canceled, as on main. Also removes the now-obsolete candidate-pair migration tests and the ICE-transport mock helpers that only those tests used. BasePeerConnection, Publisher, and webrtc.mocks are now identical to main. The signal-close revival hardening in StreamSfuClient and Call is unrelated and stays.
oliverlaz
changed the title
…ver bandwidth Add CandidatePairMonitor, which watches the publisher's BUNDLE-shared ICE transport for selected-candidate-pair changes (e.g. WiFi to LTE) and restarts ICE so libwebrtc's network-route change re-seeds the send-side bandwidth estimate. Without this the estimate craters on a handover and ramps back over ~30s, starving the encoder and collapsing FPS. Validated via webrtc-internals: recovery in ~2-3s instead of ~30s. A change restarts ICE only when it moves to a new interface (keyed by network-id) while ICE is healthy; same-interface churn, unhealthy-ICE changes, and returns to a recently-used path (flap) are ignored. On React Native this consumes the selectedcandidatepairchange event from the GetStream react-native-webrtc fork and is a safe no-op where the API is unavailable.
On a failed subscriber negotiation, escalate via tryRestartIce so a failed ICE restart falls through to a reconnect instead of being silently logged in the dispatcher's catch. Also guard the renegotiation rollback so a rollback failure can no longer mask the original negotiation error; the original error always propagates now. Adds a regression test for the rollback-failure case.
Reverts the CandidatePairMonitor introduced in aed0593. Detecting an organic selected-candidate-pair migration and restarting ICE did not work well in practice: the restart churn and disconnect cascades on flaky handovers outweighed the bandwidth-recovery benefit.
Restore the bf837b1 behavior: on a fast reconnect, restart the publisher's ICE whenever it is publishing instead of skipping it when the publisher PC looks stable. The stability-gated skip could strand a crateted send-side bandwidth estimate, since a 'stable' publisher PC never restarts ICE and the route-change BWE reset never fires. Removes the now-unused isStable() helper and its tests.
oliverlaz
changed the title
The unhealthy-close timer fired at exactly two ping intervals (10s) after the last inbound message, coinciding with the second keep-alive ping. That left only the first probe's response a window to arrive, so a single lost or delayed healthCheckResponse could close a live signal socket and force an unnecessary reconnect. Extend the inactivity timeout to two ping intervals plus a 2s grace period (12s) so the second probe's response has time to land. It now takes two missed responses to trip the watchdog.
jdimovska
approved these changes
Jun 22, 2026
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.
💡 Overview
Three reconnection-robustness changes for the core client.
1. Signal-close revival hardening (
StreamSfuClient,Call). A wedged signal WebSocket can be detected as dead by two independent sources (the health watchdog and the transportcloseevent), which on a stuck socket can arrive seconds apart. Revival is now driven by a one-shot latch so it fires at most once per client, triggers immediately on an unhealthy close instead of waiting for the (possibly long-delayed)closeevent, and ignores closes from a superseded client so a stale socket cannot drive a reconnect after a new client has swapped in.2. Subscriber negotiation-failure recovery (
Subscriber).Subscriber.negotiateresetisIceRestartingonly as the last statement of its happy path, so a negotiation failure left the flag stuck attrueand disabled all future automatic ICE-restart / reconnect handling on the subscriber. Negotiation is now wrapped in try/catch/finally so the flag always clears; on failure the remote description is rolled back when the peer connection is still awaiting an answer (have-remote-offer), the rollback is itself guarded so it can never mask the original error, the failure is traced, and the subscriber escalates viatryRestartIceso a failed restart falls through to a reconnect instead of being silently dropped.3. Revert the stability-gated publisher fast-reconnect skip (
Call,BasePeerConnection).mainskips the publisher's ICE restart on a fast reconnect when the publisher PC still looks stable (isStable()), to avoid SDP/ICE churn on signal-WS-only drops. In practice that skip can strand a cratered send-side bandwidth estimate: a "stable" publisher PC never restarts ICE, so the network-route-change BWE reset never fires and the encoder stays bitrate-starved after a handover. This restores the earlier behavior (on a fast reconnect the publisher's ICE is restarted unconditionally whenever it is publishing) and removes the now-unusedisStable()helper.📝 Implementation notes
StreamSfuClient:handleWebSocketClosebecomesnotifySignalClose, guarded by asignalClosedone-shot latch;close()notifies on a non-clean close without waiting for the transport event.Call.handleSfuSignalCloseignores closes from any client that is no longer the activesfuClient.Callfast reconnect: back torestoreICE(sfuClient, { includeSubscriber: false })(publisher restart governed byincludePublisher'struedefault); theisStable()-gated skip is gone.Subscriber.negotiatewrapped in try/catch/finally; guarded rollback onhave-remote-offer;subscriber.negotiationFailedtrace;tryRestartIceescalation on failure.BasePeerConnection.isStable()removed (no remaining callers).StreamSfuClientrevival,Callsuperseded-client guard,Subscribersuccess / failure / rollback / rollback-failure branches; removed theisStable()and FAST-skip tests.🎫 Ticket: https://linear.app/stream/issue/XYZ-123
📑 Docs: https://github.com/GetStream/docs-content/pull/