fix(room): release remote participants when the room is closed - #1435
LautaroPetaccio wants to merge 1 commit into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
44d8d9a to
6788a0d
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| let remote_participants = std::mem::take(&mut *self.remote_participants.write()); | ||
| for participant in remote_participants.into_values() { | ||
| for (sid, publication) in participant.track_publications() { | ||
| participant.remove_publication(&sid); | ||
| publication.set_track(None); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Cancelled close leaks racing participants
When room_task inserts a participant after release_remote_participants, cancelling close skips the second drain. Publication callbacks then retain the participant and engine indefinitely.
Learn more
The first drain is not mutually exclusive with participant creation. room_task remains active until close_tx is signalled later, and create_participant can therefore repopulate the map while close awaits local unpublishing or engine shutdown. Cancellation drops the remaining future, so the post-join drain never runs and the new participant's callback cycle survives.
Example: A participant update starts while close performs its first drain. The update inserts Alice, then the caller's timeout cancels close during local track unpublishing. Alice remains in remote_participants, and her publication callbacks retain both Alice and RtcEngine.
Recommended fix: Make shutdown and participant insertion mutually exclusive before the cancellation-safe drain. A closing state checked under the same synchronization as insertion can reject or immediately unlink late participants. Add a lifecycle test that blocks an in-flight participant update between the first drain and cancellation, then verifies both participant and room-session destruction.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed. The teardown is now an idempotent helper called twice: once before close can suspend, and again after the task joins.
Running it only after the joins was wrong for exactly the reason you give. close takes the handle first, so a dropped future cannot retry and the cleanup would be skipped for good. Running it up front covers cancellation; running it again afterwards still catches anything room_task inserted in the meantime.
Added test_cancelled_close_releases_remote_participants, which cancels the close at its first suspension point with a subscribed remote track. Dropping the early call makes it fail, so it does guard this.
`RoomSession::close` unpublished the local participant's tracks and then dropped the remote participant map. Dropping the map does not release the participants: every remote publication registers callbacks that hold the participant which owns them — `on_subscribed`/`on_unsubscribed` capture the `RemoteParticipant`, and `on_muted`/`on_unmuted` capture it through the publication map — so each remote participant is kept alive by a cycle through its own publications. A leaked participant holds `Arc<RtcEngine>`, so it pins the whole engine, its peer connections and the WebRTC runtime: exactly what commit 16530d2 set out to release. Each connect/close cycle leaked another one. `handle_participant_disconnect` already unregisters these when a peer actually leaves, but it also emits ParticipantDisconnected and TrackUnpublished. The peers have not left here — the room is closing — so unregister directly instead, leaving the event stream unchanged. Two ordering constraints make this correct: - The teardown runs after `room_handle` has been joined. `room_task` is what inserts remote participants, so draining the map any earlier would let an engine event queued behind the close repopulate it, and those participants would never be visited. - Each publication is unregistered before its track is detached. `RemoteTrackPublication::set_track(None)` invokes the unsubscribe handler *before* clearing it, and that handler dispatches TrackUnsubscribed carrying strong clones of the participant and track. Clearing the handlers first keeps teardown silent, so a receiver the application has stopped draining cannot pin the participant through a queued event. `test_close_releases_room_session` did not catch any of this because it runs with a single participant and no tracks, so neither cycle is ever formed. The new test uses two participants with a published, subscribed track, keeps its event receiver alive and undrained across the close, and asserts via a drop probe that the remote participant's internals are released. The teardown is idempotent and runs twice: once before `close` can suspend, and again after the task joins. `close` takes the room handle first, so a caller that bounds it with a timeout and drops the future cannot retry, and a teardown placed only after the joins would be skipped for good. Running it up front keeps a cancelled close correct, and running it again afterwards catches anything `room_task` inserted in between.
6dd253b to
1357589
Compare
1 participant
The problem
RoomSession::closeunpublished the local participant's tracks and then let the remote participant map drop. Dropping the map does not release the participants, because every remote publication registers callbacks that hold the participant which owns them:on_subscribed/on_unsubscribedcaptureself.clone()(remote_participant.rs:384-402), stored inRemoteTrackPublication.remote.events.on_muted/on_unmutedcapture the participant (participant/mod.rs:450-500), stored inTrackPublicationInner.events.Both publications live in the participant's own
track_publicationsmap, so each one is a cycle:Arc<ParticipantInner>→ map → publication → closure →Arc<ParticipantInner>.ParticipantInnerholdsArc<RtcEngine>, so a single leaked remote participant pins the entire engine, its peer connections and the WebRTC runtime — the same graph commit 16530d2 set out to release. Every connect/close cycle leaked another one.handle_participant_disconnectalready unregisters these when a peer actually leaves, butclose()never walked the remote participants: grepping every use ofremote_participantsshows inserts atmod.rs:2273and a single remove at:2285, with everything else read-only.The fix
Tear the remote participants down in
close(), unregistering the callbacks that form the cycles.This deliberately does not reuse
handle_participant_disconnect, which also emitsParticipantDisconnectedandTrackUnpublished. Those peers have not left — the room is closing at the caller's request — so emitting departures for them would be a behaviour change for anyone still draining the event stream. The teardown here unregisters directly and leaves the event stream untouched.Verification
test_close_releases_remote_participantsconnects two participants, publishes and subscribes a track (the cycles only exist once a publication exists), takes a drop probe on the remote participant, closes the room, and asserts the internals are released.Full e2e suite is green; the only delta from the baseline is the added test.
Worth noting why this was not already caught:
test_close_releases_room_sessionruns withtest_rooms(1)— one participant, no remote participants, no tracks — so neither cycle is ever formed.