fix(llc): renegotiate unacknowledged transceivers - #1288
Merged
Conversation
📝 WalkthroughWalkthroughCached publisher transceivers now track SFU negotiation acknowledgement. Publish and unmute flows request renegotiation for unacknowledged active transceivers, while renegotiation failures roll back explicitly and stalled publisher recovery uses fast reconnect. ChangesTransceiver renegotiation tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RtcManager
participant CallSession
participant SFU
participant TransceiverManager
RtcManager->>CallSession: request publisher renegotiation
CallSession->>SFU: send publisher offer
SFU-->>CallSession: return publisher answer
CallSession->>TransceiverManager: mark active tracks negotiated
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1288 +/- ##
==========================================
+ Coverage 11.22% 11.35% +0.13%
==========================================
Files 686 686
Lines 50361 50401 +40
==========================================
+ Hits 5652 5724 +72
+ Misses 44709 44677 -32 ☔ View full report in Codecov by Harness. 🚀 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: 1
🤖 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.
Inline comments:
In `@packages/stream_video/lib/src/call/session/call_session.dart`:
- Line 1145: Move rtcManager?.transceiversManager.markNegotiated() into the
successful pc.setRemoteAnswer() path, ensuring it is not executed when remote
answer application throws or fails. Preserve the existing recovery behavior for
failed negotiation so later republishing can force renegotiation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5afa25b4-a2c3-495e-8e98-d959af656ff2
📒 Files selected for processing (5)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/lib/src/webrtc/transceiver_cache.dartpackages/stream_video/test/src/webrtc/transceiver_cache_test.dart
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/stream_video/lib/src/call/session/call_session.dart (1)
1140-1147: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRollback after a failed remote answer.
A failed
setRemoteAnswerreturns without rolling back the local offer, leaving the publisher potentially stuck inhave-local-offer; the subsequent forced renegotiation can then fail instead of recovering.Proposed fix
if (ansResult is! Success<void>) { _logger.w( () => '[negotiate] `#setRemoteAnswer`; failed: $ansResult', ); + await pc.rollbackLocalDescription(); return Result<void>.error( 'Failed to set remote answer: ${ansResult.getErrorOrNull()}', ); }🤖 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/lib/src/call/session/call_session.dart` around lines 1140 - 1147, Update the failed setRemoteAnswer branch in the negotiation flow to roll back the local offer before returning the error result. Use the existing RTCPeerConnection rollback mechanism and ensure it completes before the return, while preserving the current warning and error propagation behavior.
🤖 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/stream_video/lib/src/call/session/call_session.dart`:
- Around line 1140-1147: Update the failed setRemoteAnswer branch in the
negotiation flow to roll back the local offer before returning the error result.
Use the existing RTCPeerConnection rollback mechanism and ensure it completes
before the return, while preserving the current warning and error propagation
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6814a368-a3ad-4c75-9b9c-1771e51a65ab
📒 Files selected for processing (5)
packages/stream_video/lib/src/call/call.dartpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/webrtc/peer_connection.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/test/src/webrtc/rtc_manager_unmute_renegotiation_test.dart
renefloor
reviewed
Jul 28, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video/lib/src/webrtc/transceiver_cache.dart (1)
26-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset acknowledgement when the cached media track changes.
A previously acknowledged entry remains
negotiated == trueafterupdate(..., track:)replaces itsmediaTrack.id. Reuse can then skip renegotiation although the SFU never received the replacement track.
packages/stream_video/lib/src/webrtc/transceiver_cache.dart#L26-L26: clearnegotiatedwhenupdatereceives a track with a different media-track ID.packages/stream_video/test/src/webrtc/transceiver_cache_test.dart#L168-L182: first mark the old track negotiated, replace it throughupdate, then assert the cached entry becomes unnegotiated.Proposed fix
if (track != null) { + final mediaTrackChanged = + bundle.track.mediaTrack.id != track.mediaTrack.id; bundle.track = track; + if (mediaTrackChanged) { + bundle.negotiated = false; + } }🤖 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/lib/src/webrtc/transceiver_cache.dart` at line 26, Reset the cached entry’s negotiated state in update when the supplied track has a different media-track ID, so replacement tracks require renegotiation; preserve the state when the ID is unchanged. In packages/stream_video/lib/src/webrtc/transceiver_cache.dart:26, update the negotiated handling for the cache entry, and in packages/stream_video/test/src/webrtc/transceiver_cache_test.dart:168-182, mark the original track negotiated, replace it via update, and assert the entry is unnegotiated.
🧹 Nitpick comments (1)
packages/stream_video/test/src/call/session/call_session_reconnect_safety_test.dart (1)
249-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a real renegotiation failure.
Because the test has no SFU connection,
_onRenegotiationNeededexits beforecreateOffer, rollback,SetPublisher, orsetRemoteAnswer. It therefore validates only the disconnected-WebSocket failure, not the cleanup and fallback behavior changed by this PR. Add a connected fake-SFU case that fails after offer creation and asserts rollback plusfastreconnect.Also applies to: 276-282
🤖 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/session/call_session_reconnect_safety_test.dart` around lines 249 - 250, Extend the test covering `_onRenegotiationNeeded` so it uses a connected fake SFU and reaches the post-`createOffer` failure path. Configure the fake response to fail during renegotiation, then assert that rollback and publisher reset occur and that the reconnect request uses the `fast` mode, rather than only validating the disconnected-WebSocket 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.
Inline comments:
In `@packages/stream_video/lib/src/call/session/call_session.dart`:
- Around line 1118-1120: Update the recovery paths in CallSession around the
setRemoteAnswer failure handling and existing rollbackLocalDescription calls to
always attempt rollback when answer application fails, including the path that
currently returns immediately. Do not discard rollback results: propagate or
compose rollback failures so the empty-track path cannot report success when
rollback fails, while preserving successful recovery behavior.
---
Outside diff comments:
In `@packages/stream_video/lib/src/webrtc/transceiver_cache.dart`:
- Line 26: Reset the cached entry’s negotiated state in update when the supplied
track has a different media-track ID, so replacement tracks require
renegotiation; preserve the state when the ID is unchanged. In
packages/stream_video/lib/src/webrtc/transceiver_cache.dart:26, update the
negotiated handling for the cache entry, and in
packages/stream_video/test/src/webrtc/transceiver_cache_test.dart:168-182, mark
the original track negotiated, replace it via update, and assert the entry is
unnegotiated.
---
Nitpick comments:
In
`@packages/stream_video/test/src/call/session/call_session_reconnect_safety_test.dart`:
- Around line 249-250: Extend the test covering `_onRenegotiationNeeded` so it
uses a connected fake SFU and reaches the post-`createOffer` failure path.
Configure the fake response to fail during renegotiation, then assert that
rollback and publisher reset occur and that the reconnect request uses the
`fast` mode, rather than only validating the disconnected-WebSocket path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d51b9ec1-55ec-4cae-8237-39f9c9ee2084
📒 Files selected for processing (5)
packages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/webrtc/transceiver_cache.dartpackages/stream_video/test/src/call/session/call_session_reconnect_safety_test.dartpackages/stream_video/test/src/webrtc/rtc_manager_unmute_renegotiation_test.dartpackages/stream_video/test/src/webrtc/transceiver_cache_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/test/src/webrtc/rtc_manager_unmute_renegotiation_test.dart
renefloor
approved these changes
Jul 30, 2026
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/stream_video/lib/src/call/session/call_session.dart (1)
1147-1152: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winComplete rollback handling on every negotiation failure.
SetPublisherfailure and the catch path still discard rollback failures, whilesetRemoteAnswerfailure returns without rolling back. This can leave the peer connection inhave-local-offer; the new fast fallback reuses that connection and may remain wedged. Roll back on answer failure and propagate or compose rollback failures in all three paths.Also applies to: 1155-1163, 1170-1173
🤖 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/lib/src/call/session/call_session.dart` around lines 1147 - 1152, Update the negotiation flow around setPublisher, setRemoteAnswer, and its catch handler to always attempt pc.rollbackLocalDescription() on failure, including the setRemoteAnswer failure path. Do not discard rollback errors: propagate or compose each rollback failure with the original negotiation error while preserving the existing error context and return behavior.
🤖 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/stream_video/lib/src/call/session/call_session.dart`:
- Around line 1147-1152: Update the negotiation flow around setPublisher,
setRemoteAnswer, and its catch handler to always attempt
pc.rollbackLocalDescription() on failure, including the setRemoteAnswer failure
path. Do not discard rollback errors: propagate or compose each rollback failure
with the original negotiation error while preserving the existing error context
and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5020ef38-c08b-493e-b528-8e25fc577ae8
📒 Files selected for processing (2)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/session/call_session.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/CHANGELOG.md
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.
This PR fixes an issue where republishing could reuse a cached publisher transceiver without renegotiating, even if the previous SetPublisher negotiation failed. Publisher transceivers are now marked as negotiated only after a successful SFU negotiation. If a cached transceiver was never acknowledged by the SFU republishing reuses it but triggers a new negotiation so the track is properly announced.
Summary by CodeRabbit
FormatExceptioncaused by non-ASCII client/device info by sanitizingX-Stream-Clientheader values.