fix(llc): connection flickering rejoin#1236
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (2)
📝 WalkthroughWalkthroughThis PR adds a 3s stability window for rejoin/migrate reconnection flows, refactors _awaitNetworkAvailable into a bounded, retrying wait that enforces stability and an overall availability budget, and adds tests validating fast/rejoin/migrate behaviors and budgeted failure. ChangesNetwork Stability on Reconnection
Sequence DiagramsequenceDiagram
participant Reconnect as _reconnect
participant AwaitNet as _awaitNetworkAvailable
participant Monitor as NetworkMonitor
participant Status as InternetStatus
participant Lifecycle as Call Lifecycle
Reconnect->>Reconnect: Compute stabilityWindow (3s for rejoin/migrate)
Reconnect->>AwaitNet: Call with stabilityWindow
AwaitNet->>Monitor: Adjust polling to offline-check interval
AwaitNet->>AwaitNet: Compute total time budget
loop until budget exhausted or stable
AwaitNet->>Status: Wait for InternetStatus.connected
Status->>AwaitNet: Connected / NotConnected
alt Connected
AwaitNet->>Status: Observe stability for stabilityWindow
Status->>AwaitNet: Stable / Disconnect observed
alt Stable
AwaitNet->>Reconnect: Return connected
else Disconnect
AwaitNet->>AwaitNet: Log instability and retry
end
end
AwaitNet->>Lifecycle: Race against call-leave completion
end
AwaitNet->>Monitor: Restore original interval
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Actionable comments posted: 0 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1236 +/- ##
========================================
+ Coverage 9.01% 9.10% +0.09%
========================================
Files 676 676
Lines 49585 49607 +22
========================================
+ Hits 4469 4518 +49
+ Misses 45116 45089 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/stream_video/test/src/call/call_reconnect_stability_test.dart (2)
73-75: ⚡ Quick winConsider adding explicit Call cleanup in tearDown.
While the test framework may handle cleanup automatically, explicitly calling
call.leave()on any created Call instances would ensure proper resource disposal and prevent potential memory leaks in the test suite.♻️ Suggested enhancement
+ late Call? call; + setUp(() { internetStatusController = BehaviorSubject<InternetStatus>.seeded( InternetStatus.connected, ); + call = null; // ... rest of setup }); tearDown(() async { + await call?.leave(); await internetStatusController.close(); });Then in each test, assign
call = buildCall(...)so tearDown can clean it up.🤖 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/stream_video/test/src/call/call_reconnect_stability_test.dart` around lines 73 - 75, Add explicit Call cleanup in the tearDown by tracking the Call instance created in tests (assign call = buildCall(...) inside each test) and invoking await call.leave() (or call.dispose() if applicable) before closing other resources; update the tearDown to check if the tracked Call is non-null and call await call.leave() then await internetStatusController.close() so Call resources are deterministically released.
337-375: ⚡ Quick winConsider adding a test verifying migrate proceeds after the stability window.
Test coverage for
migratecurrently only validates that it doesn't proceed early (within 500ms), but there's no corresponding test verifying it eventually proceeds after the full 3s stability window elapses. Therejoinstrategy has this coverage via the pairing of test 2 (doesn't proceed early) and test 3 (proceeds after window). Adding a similar positive-case test formigratewould ensure symmetric coverage.✅ Suggested test to add
test( 'migrate reconnect proceeds once the network stays connected for the ' 'stability window', () async { final call = buildCall(); await call.join(); expect(capturedCallback, isNotNull); capturedCallback!(mockPc, SfuReconnectionStrategy.migrate); // Wait past the 3s stability window with the network steady. await Future<void>.delayed(const Duration(milliseconds: 3500)); // Initial join + migrate = 2 joinCall invocations. // For migrate, migratingFrom should be set to the current SFU name. verify( () => coordinatorClient.joinCall( callCid: any(named: 'callCid'), ringing: any(named: 'ringing'), create: any(named: 'create'), migratingFrom: any(named: 'migratingFrom'), migratingFromList: any(named: 'migratingFromList'), video: any(named: 'video'), membersLimit: any(named: 'membersLimit'), ), ).called(2); }, timeout: const Timeout(Duration(seconds: 10)), );🤖 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/stream_video/test/src/call/call_reconnect_stability_test.dart` around lines 337 - 375, Add a positive-case test mirroring the rejoin stability-window test: create a test (e.g. "migrate reconnect proceeds once the network stays connected for the stability window") that builds the call via buildCall(), awaits call.join(), asserts capturedCallback is non-null, invokes capturedCallback!(mockPc, SfuReconnectionStrategy.migrate), then awaits past the 3s stability window (e.g. Future.delayed 3500ms) and verifies coordinatorClient.joinCall was called twice (initial join + migrate) and that migratingFrom is set as expected; include a longer test timeout (e.g. Timeout(Duration(seconds:10))).
🤖 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/stream_video/test/src/call/call_reconnect_stability_test.dart`:
- Around line 73-75: Add explicit Call cleanup in the tearDown by tracking the
Call instance created in tests (assign call = buildCall(...) inside each test)
and invoking await call.leave() (or call.dispose() if applicable) before closing
other resources; update the tearDown to check if the tracked Call is non-null
and call await call.leave() then await internetStatusController.close() so Call
resources are deterministically released.
- Around line 337-375: Add a positive-case test mirroring the rejoin
stability-window test: create a test (e.g. "migrate reconnect proceeds once the
network stays connected for the stability window") that builds the call via
buildCall(), awaits call.join(), asserts capturedCallback is non-null, invokes
capturedCallback!(mockPc, SfuReconnectionStrategy.migrate), then awaits past the
3s stability window (e.g. Future.delayed 3500ms) and verifies
coordinatorClient.joinCall was called twice (initial join + migrate) and that
migratingFrom is set as expected; include a longer test timeout (e.g.
Timeout(Duration(seconds:10))).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28464513-7de9-40a4-abd2-a78080af6d38
📒 Files selected for processing (1)
packages/stream_video/test/src/call/call_reconnect_stability_test.dart
2 participants
Summary by CodeRabbit
Bug Fixes
Tests