fix(client): harden SFU signal-close revival and subscriber ICE-restart recovery#2285
fix(client): harden SFU signal-close revival and subscriber ICE-restart recovery#2285oliverlaz wants to merge 6 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.
|
|
Warning Review limit reached
More reviews will be available in 12 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThree independent improvements to reconnect resilience: (1) ChangesICE Candidate-Pair Path Migration Detection
Subscriber Negotiation Resilience and Error Recovery
SFU Signal-Close Deduplication and Superseded-Client Guard
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}
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.
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.
1 participant
💡 Overview
Two reconnection-robustness fixes for the core client.
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.Subscriber ICE-restart recovery (
Subscriber).Subscriber.negotiateresetisIceRestartingas 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), and the failure is traced.📝 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.Subscriber.negotiatewrapped in try/catch/finally; rollback onhave-remote-offer; asubscriber.negotiationFailedtrace added.🎫 Ticket: https://linear.app/stream/issue/XYZ-123
📑 Docs: https://github.com/GetStream/docs-content/pull/