Fixed setTrackEnabled orphaning track resources when cancelled before publish completes - #986
Open
adrian-niculescu wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: e9a347a 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 |
adrian-niculescu
force-pushed
the
fix/screen-share-enable-cancellation-cleanup
branch
3 times, most recently
from
July 27, 2026 18:33
62dabc8 to
413face
Compare
adrian-niculescu
marked this pull request as ready for review
July 27, 2026 18:52
adrian-niculescu
requested review from
MaxHeimbrock,
davidliu and
xianshijing-lk
as code owners
July 27, 2026 18:52
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
adrian-niculescu
force-pushed
the
fix/screen-share-enable-cancellation-cleanup
branch
3 times, most recently
from
July 27, 2026 21:19
ba87fa2 to
25fd158
Compare
adrian-niculescu
marked this pull request as draft
July 27, 2026 21:30
adrian-niculescu
force-pushed
the
fix/screen-share-enable-cancellation-cleanup
branch
2 times, most recently
from
July 27, 2026 21:47
549ca9a to
f5218d0
Compare
adrian-niculescu
marked this pull request as ready for review
July 27, 2026 22:03
Contributor
Author
|
Exercised this on a few devices and refined it over a couple of passes since opening. It's ready for review now. |
Contributor
Author
|
#988 landed first. Cleaning this up. |
… publish completes A cancellation landing in startForegroundService or publishVideoTrack unwound out of setTrackEnabled without any cleanup, abandoning the newly created screencast track and, when the bind had already succeeded, leaving ScreenCaptureService bound for the lifetime of the context. Each enable path now tears the unpublished track down in a finally block, so cancellation and publish failures release capture, track resources, and the service binding, and invoke the onStop callback exactly once, even when the platform ended the projection while publishing was in flight. A projection stop already dispatched on its callback thread can reach the track after that cleanup disposed it, where ScreenCapturerAndroid throws from stopCapture. LocalScreencastVideoTrack.stop() now tolerates losing that race instead of crashing the callback thread. publishTrackImpl negotiates the sender transceiver concurrently with the add track request, so a failed or cancelled request could leave a negotiated transceiver holding its sender and SDP m-section with no publication to unpublish. The sender is now detached whenever no publication is created. The rollback targets exactly the sender this attempt created on the transport that created it, so it cannot detach a concurrent publish of the same track, and the handle is recorded non-cancellably so a cancellation cannot discard the RTC-thread result. It runs only while that publisher is connected with no reconnect in flight, since a publish usually fails because the signal connection died and that same failure starts the reconnect which renegotiates the publisher. Neither connectionState nor the reconnect job has moved at the point a dying session fails its pending publishes, and a resume leave fails them without starting a reconnect at all, so a session identity is replaced at every boundary and each sender handle keeps the one that owned its transceiver. Matching that identity is what makes the decision independent of when a failed publish resumes: a soft reconnect keeps the publisher, so cleanup arriving after the next session is serving would otherwise mutate it. The check runs on the RTC thread immediately ahead of the mutation it guards. Stopping the transceiver and releasing its m-section is reserved for video, matching unpublishTrack. LocalScreencastVideoTrack also never released its SurfaceTextureHelper, so its capture thread outlived dispose() even on the normal teardown path. The helper is now registered with the track's CloseableManager, matching LocalVideoTrack. Fixes livekit#985
adrian-niculescu
force-pushed
the
fix/screen-share-enable-cancellation-cleanup
branch
from
July 31, 2026 08:17
f5218d0 to
13364fd
Compare
davidliu
approved these changes
Jul 31, 2026
| private fun abortPendingPublishTracks() { | ||
| // Ordered ahead of the resumes: each one unwinds a publish into cleanup that reads this, | ||
| // and on some paths the reconnect is only triggered later, by the socket closing. | ||
| signalSessionState = SignalSessionState(ended = true) |
Contributor
There was a problem hiding this comment.
nit: this probably be a separate method or just added to the calling sites. it is at the appropriate place in the signal session boundary, but piggybacking this method seems awkward.
Contributor
Author
There was a problem hiding this comment.
Agreed. Pulled it out into endSignalSession(), called at the three sites ahead of abortPendingPublishTracks() and reused in reconnect() for the standalone write. The ordering rule now lives on the new method rather than as a comment inside the abort.
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.
Fixes #985.
setTrackEnabledhad no cleanup around its suspend points: a cancellation landing instartForegroundServiceorpublishVideoTrackdropped the freshly created screencast track without stopping or disposing it, and once the bind had succeeded,ScreenCaptureServicestayed bound for the lifetime of the context (the case #983 could not reach from insideScreenCaptureConnection). Each enable path now ends in afinallythat tears down a track it could not publish: capture stops, the track is stopped and disposed, the service binding is released, andScreenCaptureParams.onStopis invoked. The camera and microphone paths get the same treatment, with one extra condition. They share the default track with publishers that do not take the source pub lock, such asrepublishTracks, so their cleanup skips a track that is already published: #988 keeps a duplicate publish from stopping a track a concurrent publisher owns, and that has to hold when a cancellation unwinds too, not just when publish returns false.publishTrackImplnegotiates the sender transceiver concurrently with the add track request, so a cancelled or failed request could leave a negotiated transceiver holding its sender and SDP m-section with no publication to unpublish. The sender is now detached whenever no publication is created, which also covers the publish-timeout path. The rollback is deliberately narrow: a publish usually fails because the signal connection died, and that same failure starts the reconnect that renegotiates the publisher, so the rollback runs only while its publisher is connected and idle. NeitherconnectionStatenor the reconnect job has moved by the time a dying session fails its pending publishes, so idleness is tracked with a session identity replaced at every boundary; cleanup resuming after a soft reconnect no longer matches and leaves the publisher alone. The check runs on the RTC thread immediately ahead of the mutation it guards. Stopping the transceiver to release its m-section is reserved for video, matchingunpublishTrack.Two smaller fixes ride along:
LocalScreencastVideoTracknever registered itsSurfaceTextureHelperwith the track'sCloseableManagerthe wayLocalVideoTrack.createTrackdoes, so the capture thread surviveddispose()even on normal teardown; and a projection stop already dispatched on its callback thread can reach the track after cleanup has disposed it, whereScreenCapturerAndroid.stopCapturethrows, soLocalScreencastVideoTrack.stop()tolerates losing that race instead of crashing the callback thread.Two behavioral notes:
onStopon the publish-failure path now runs after teardown rather than before, so a throwing callback cannot skip the cleanup, and it is delivered at most once per enable attempt (the platform ending the projection mid-publish already delivers it; the failure path delivered it again).Eleven tests cover cancellation at each suspend point, the rollback and each of its limits, single
onStopdelivery, the late projection stop, helper disposal, and the surviving happy path; six fail on main../gradlew test, spotless, and:livekit-android-sdk:detektReleasepass (repository-wide detekt is red on pre-existing findings).