Fixed full reconnect republish racing concurrent publishes of the same track - #990
Conversation
🦋 Changeset detectedLatest commit: f22a18c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c856263 to
e242bd9
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } | ||
| if (!success) { | ||
| if (!success && !isTrackPublished(track)) { | ||
| track.stop() | ||
| } |
There was a problem hiding this comment.
🟡 Microphone keeps recording after a failed track restore following reconnect
The microphone is re-armed for recording (prewarm() at livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt:1403) but is never turned back off when the restore fails, so the device keeps capturing audio indefinitely.
Impact: After a failed reconnect restore, the microphone stays active and the system recording indicator remains on even though nothing is being sent.
Prewarm is started in restartTrackIfStopped but the republish failure path only calls track.stop()
restartTrackIfStopped calls track.prewarm() + track.start() for LocalAudioTrack (livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt:1402-1405). prewarm() forwards to JavaAudioRecordPrewarmer.prewarm which starts recording on the audio device module and is only cancelled by stopPrewarm() (livekit-android-sdk/src/main/java/io/livekit/android/room/track/LocalAudioTrack.kt:104-110).
In republishTrack, when the republish fails and the track is still unpublished, only track.stop() is called (line 1377-1379), unlike the equivalent failure path in setTrackEnabled which calls both track.stop() and track.stopPrewarm() (livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt:387-389). The prewarmed recording therefore leaks.
| } | |
| if (!success) { | |
| if (!success && !isTrackPublished(track)) { | |
| track.stop() | |
| } | |
| if (!success && !isTrackPublished(track)) { | |
| track.stop() | |
| (track as? LocalAudioTrack)?.stopPrewarm() | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed. The failure path now mirrors setTrackEnabled and stops the prewarm.
| is LocalVideoTrack -> { | ||
| track.start() | ||
| track.startCapture() | ||
| } |
There was a problem hiding this comment.
🟡 Screen share that the user ended during a reconnect is restarted, which can crash the app
A screen share that the user already ended is forcibly restarted (startCapture() at livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt:1409) while restoring tracks after a reconnect, so the app can crash or resume a dead screen share.
Impact: If the user stops screen sharing while the connection is being re-established, the app may crash or end up publishing a broken screen share, and other tracks are not restored.
restartTrackIfStopped does not distinguish screencast tracks, whose projection and foreground service are already gone
When the user stops screen capture through the system UI, MediaProjectionCallback invokes LocalScreencastVideoTrack.stop(), which stops the capturer, sets enabled = false and shuts down the capture foreground service (livekit-android-sdk/src/main/java/io/livekit/android/room/track/LocalScreencastVideoTrack.kt:186-190). The participant's own onStop callback calls unpublishTrack, which early-returns during a full reconnect because trackPublications was already cleared by prepareForFullReconnect (livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt:996-1001).
The accumulated republish snapshot therefore still holds an unmuted screen share publication whose track is disabled. republishTrack then calls restartTrackIfStopped, whose is LocalVideoTrack branch matches LocalScreencastVideoTrack and calls start() + startCapture(). The media projection permission result has already been consumed and the foreground service is stopped, so ScreenCapturerAndroid.startCapture fails (throwing on modern Android). The exception escapes republishTracks() into Room.onPostReconnect and the RTCEngine reconnect coroutine, which has no exception handler (livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt:213, :693), aborting the republish of any remaining tracks. AGENTS.md also forbids crashing consumer code via unchecked exceptions.
Consider skipping restart (and republish) for screencast tracks that are no longer live, and guarding the restart/publish loop against exceptions.
Prompt for agents
restartTrackIfStopped in LocalParticipant.kt restarts any disabled LocalVideoTrack before republishing, including LocalScreencastVideoTrack instances. A screen share whose MediaProjection was ended by the user during a full reconnect ends up in the republish snapshot with muted=false and enabled=false (unpublishTrack early-returns during reconnect because trackPublications was cleared). Restarting such a track calls ScreenCapturerAndroid.startCapture with an already-consumed projection result and without the capture foreground service, which fails/throws; the exception propagates out of republishTracks into Room.onPostReconnect and the RTCEngine reconnect coroutine (no exception handler), aborting republishing of the remaining tracks. Consider excluding screencast tracks from the restart path (and from republishing when their capture is dead), and making the per-track republish loop resilient to exceptions so one track cannot abort the rest.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, a stopped MediaProjection cannot be reused. Fixed: republishTrack skips a stopped screencast since the share has ended, restartTrackIfStopped no longer touches screencasts, and the republish loop catches per-track failures so one entry cannot abort the rest.
49c99ef to
14231aa
Compare
14231aa to
f22a18c
Compare
1 participant
Fixes #989.
republishTracks()published outside the per-source publish lock, so a full reconnect could overlapsetMicrophoneEnabledor a directpublishAudioTrackon the same default track instance. The losing path's duplicate publish failed and its failure branch stopped the shared track, leaving the publication unmuted but sending silence. When the pre-reconnect publication was muted, republish also unpublished a publication the app had just created mid-reconnect and skipped republishing it.All publishes now serialize on the per-source lock:
publishAudioTrack/publishVideoTrackacquire it, whilesetTrackEnabledandrepublishTrackscall the unlocked internals under the lock they hold.republishTracksskips snapshot entries whose track already has a live publication instead of unpublishing them, and only stops a track after a failed republish if it is not published.Two adjacent holes in the same reconnect window are closed as well: the republish snapshot now accumulates across reconnect attempts with the newest entry per track winning, so a publication created between failed attempts is restored instead of dropped and a mute during the reconnect is preserved, and a track that a failed concurrent enable stopped (e.g. on the add track timeout) is restarted before republishing rather than republished silent. Only tracks the SDK itself stopped are restarted; a track the consumer stopped through the public Track API stays stopped.
Added MockE2ETest coverage for the reconnect interleavings, the direct-publish overlap, the multi-attempt cases, and the stopped-track restart, driven by withholding the
TrackPublishedresponse.