Stop orphaned publisher transceivers that trigger SFU force-rejoin - #1757
Merged
aleksandar-apostolov merged 1 commit intoAug 3, 2026
Merged
Conversation
… loop Three transceiver-lifecycle bugs in Publisher could strand live sendonly m-lines that SetPublisher.tracks never announces, causing the SFU to force-rejoin the publisher (VID-1376): - syncPublishOptions: the cleanup loop compared the cache against itself (always true), so every transceiver was torn down on each ChangePublishOptions event. Now keep transceivers whose option is still requested by the SFU. - publishStreamInternal fallback: replaced the old transceiver without stopping it, leaving an orphaned m-line. Now stop() the old one first. - addTransceiver: silently overwrote the cache entry for the same [track_type, publish_option_id], stranding the old transceiver. Now stop() any pre-existing transceiver before adding. All paths use stop() only (never dispose()) on a live PeerConnection; the PC owns the native transceiver/sender and frees it safely at teardown. Disposing mid-session is a use-after-free on network_thread (SIGSEGV). Adds unit tests covering all three cases. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
PratimMallick
changed the title
Contributor
WalkthroughPublisher now prevents orphaned live transceivers during track replacement and cache updates, avoids disposing transceivers still associated with the peer connection, and adds regression tests for these lifecycle behaviors. ChangesPublisher transceiver lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt (1)
470-477: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover removal with stop-only cleanup.
This verifies retained options only. Add an unrequested cached option and assert
stop()once,dispose()never, and cache removal once; otherwise the directsyncPublishOptionsno-dispose guarantee can regress untested.🤖 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 `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt` around lines 470 - 477, Add an unrequested option to the cached transceivers in the PublisherTest syncPublishOptions scenario, then verify its stop() is called exactly once, dispose() is never called, and the cache removes it exactly once. Keep the existing assertions for retained options to preserve coverage of the no-dispose guarantee.
🤖 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
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Publisher.kt`:
- Around line 295-302: The transceiver cache is removed even when stopping the
existing transceiver fails, allowing the same live m-line to be recreated. In
Publisher.kt at lines 295-302, 367-374, and 432-440, update the stop handling in
the fallback and removed-track paths to detect stop success, retain the
TransceiverCache entry on failure, and only remove or replace/add a transceiver
after a successful stop or explicit recovery.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt`:
- Around line 435-574: Update PublisherTest to extend and use the existing
TestBase harness instead of maintaining its own mocked setup and coroutine test
context. Migrate the lifecycle tests around syncPublishOptions,
publishStreamInternal, and addTransceiver to the TestBase conventions while
preserving their current assertions and behavior.
---
Nitpick comments:
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt`:
- Around line 470-477: Add an unrequested option to the cached transceivers in
the PublisherTest syncPublishOptions scenario, then verify its stop() is called
exactly once, dispose() is never called, and the cache removes it exactly once.
Keep the existing assertions for retained options to preserve coverage of the
no-dispose guarantee.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43e2de77-3efc-4237-99bc-6976d1d9d672
📒 Files selected for processing (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Publisher.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt
Contributor
SDK Size Comparison 📏
|
|
aleksandar-apostolov
changed the title
aleksandar-apostolov
deleted the
fix/publisher-transceiver-orphans-vid-1376
branch
August 3, 2026 08:14
27 tasks
Collaborator
|
🚀 Available in v1.30.0 |
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
3 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.
Goal
Fixes AND-1351 linked to VID-1367
Fixes three publisher transceiver-lifecycle bugs that can leave a live
sendonlym-line on the publisherPeerConnectionwhichSetPublisher.tracksnever announces. The SFU then rejects the offer withtrack ... not announced by userand force-rejoins the publisher — a disruptive reconnect for the user.The three bugs this PR solves:
syncPublishOptionstears down every transceiver — the cleanup loop compared the transceiver cache against itself, so it always matched and stopped/disposed all transceivers (audio + video) on each SFUChangePublishOptionsevent, dropping tracks (and crashing native WebRTC viadispose()on a live PC).publishStreamInternalfallback orphans the old transceiver — the sender-reuse fallback created a replacement transceiver but dropped the old one from the cache without stopping it, leaving an orphaned live m-line that is never announced.addTransceiverstrands the previous transceiver — for the same[track_type, publish_option_id]the cache entry was silently overwritten, orphaning the earlier transceiver on thePeerConnection.Implementation
syncPublishOptionscleanup predicate — the cleanup loop compared the transceiver cache against itself (transceiverCache.has(option)), which was alwaystrue, so every transceiver was torn down on eachChangePublishOptionsevent (dropping audio + video). It now keeps transceivers whose[id, track_type]option is still requested by the SFU and only removes the ones that are actually gone.publishStreamInternalfallback — when the sender can't be reused, the fallback created a replacement transceiver but dropped the old one from the cache without stopping it, leaving an orphaned live m-line. It nowstop()s the old transceiver before replacing it.addTransceiverguard — the cache is keyed by[track_type, publish_option_id], so adding a second transceiver for the same tuple silently overwrote the entry and stranded the previous transceiver. A guard nowstop()s any pre-existing transceiver for the same option before adding the new one.In all three paths we call
stop()only, neverdispose(), on a livePeerConnection:stop()marks the m-line inactive so it is recycled in the next offer, while the PC retains ownership of the native transceiver/sender and frees it safely at teardown. Disposing mid-session is a use-after-free onnetwork_threadand hard-crashes native WebRTC (SIGSEGV), which we observed and fixed during on-device reproduction.Testing
./gradlew :stream-video-android-core:testDebugUnitTest --tests "io.getstream.video.android.core.call.connection.PublisherTest"— BUILD SUCCESSFUL./gradlew :stream-video-android-core:spotlessApply— BUILD SUCCESSFULChangePublishOptionscodec swap VP9→H264, sender fallback, duplicate transceiver). With the fix: old video transceiver isstop()ed, audio is untouched, no orphaned m-line, nonot announced by userforce-rejoin, and no nativeSIGSEGV. Debug hooks removed before this PR.New unit tests added to
PublisherTest:syncPublishOptions keeps transceivers whose options are still requestedpublishStreamInternal fallback stops the orphaned transceiveraddTransceiver stops existing transceiver for the same publish option before adding☑️Contributor Checklist
General
developbranchCode & documentation
stream-video-examples)☑️Reviewer Checklist
🎉 GIF
Skipped — pure RTC logic fix, no UI change.