fix(room): finish local unpublish cleanup when the transport is gone - #1436
LautaroPetaccio wants to merge 2 commits into
Conversation
`unpublish_track` removed the publication from the participant, then propagated a failure from `rtc_engine.remove_track` with `?`. Everything after that call was skipped, including `publication.set_track(None)` — the only thing that unregisters the track's mute callbacks. The publication holds its track and those callbacks hold the publication back, so the pair kept itself alive, and with it the transceiver and the peer connection the transceiver references. Removing the sender fails on two routine paths, not on exotic ones: - An abnormal disconnect. `EngineEvent::Disconnected` is emitted after the session has closed its transports, so the room's teardown runs against a publisher that is already closed and libwebrtc rejects the removal. - A full reconnect. The new session installs a new publisher before `Restarted` is emitted, so the sender being removed belongs to the previous transport. `close()` discards the result, so neither surfaced anywhere. The local cleanup now always runs and the error is reported afterwards, leaving the return value unchanged. The `local_track_unpublished` callback and the renegotiation stay on the success path exactly as before: that callback dispatches LocalTrackUnpublished, and firing it during the reconnect republish would emit an unpublish immediately before LocalTrackRepublished, which bindings are told to treat as a preserved publication handle. Note this leaves the stale E2EE FrameCryptor for a republished track in place, since that cleanup hangs off the same callback.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if removed.is_ok() { | ||
| if let Some(local_track_unpublished) = | ||
| self.local.events.local_track_unpublished.lock().as_ref() | ||
| { | ||
| local_track_unpublished(self.clone(), publication.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Full reconnects retain stale frame cryptors
When remove_track fails during a full reconnect, local_track_unpublished is skipped and the old frame cryptor remains registered. on_local_track_unpublished is the only per-track path removing the previous SID's cryptor. Each full reconnect retains another native cryptor and its old RTP sender until room shutdown.
Learn more
A full reconnect replaces the RTC session, then republishes every local track. Removing an old sender through the new session fails, so this guard suppresses the callback. That callback invokes on_local_track_unpublished, which removes the frame cryptor stored under the old track SID. The subsequent publish creates a new frame cryptor under the new SID, leaving the old native cryptor registered. The stale cryptor holds the old RTP sender and related native transport resources until the room-wide E2EE cleanup runs at shutdown.
Example: An encrypted track starts under SID TR_old. A full reconnect makes sender removal fail, then republishes it as TR_new. The manager contains cryptors for both TR_old and TR_new; repeated reconnects add another stale entry each time.
Recommended fix: Separate internal E2EE teardown from the externally visible LocalTrackUnpublished event. Always remove the cryptor for the old SID after local unpublish cleanup, while preserving the reconnect event contract for bindings. Add a full-reconnect lifecycle test that verifies the old SID disappears and the cryptor count returns to one.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I implemented this and could not reproduce the leak, so I reverted it.
On unpatched code the cryptor map after a full reconnect holds one entry under the new sid, with the old one already gone:
before: [(ParticipantIdentity("p0"), TrackSid("TR_V5kQJepNxmJEu"))]
after: [(ParticipantIdentity("p0"), TrackSid("TR_VPqmeKjnwF8ub"))]
So remove_track is not failing on the reconnect path the way it does on an abnormal disconnect, and the unpublished callback removes the old sid normally. The other path is covered too: RoomSession::close calls e2ee_manager.cleanup(), which clears every cryptor.
The test I wrote for it passed with and without the change, which makes it worthless as a guard. Measured against a local dev server, so if you have a trace showing two cryptors after a reconnect, send it and I will put the change back.
|
The red macOS job is a SIGSEGV in It passed on the previous run here, and 8/8 locally on the same target. The macOS failure has also moved around between runs, hitting a different test on #1434 earlier while Linux and Windows stayed green both times. I cannot re-run it from a fork. The segfault itself looks worth a separate look, since it points at something unsound in the native reconnect path rather than at any of these PRs. |
1 participant
The problem
LocalParticipant::unpublish_trackremoved the publication from the participant and then propagated a transport failure with?:set_track(None)is the only thing that unregisters the track's mute callbacks. Those callbacks capture the publication (publication/mod.rs:181-216) while the publication holds the track ininfo.track— a cycle, so the pair keeps itself alive, and with it theRtpTransceiverand thePeerConnectionit references.Removing the sender fails on two routine paths, not exotic ones:
EngineEvent::Disconnectedis emitted atrtc_engine/mod.rs:790, aftersession.close()has closed the publisher, so the room's teardown runs against an already-closed transport and libwebrtc rejects the removal.try_restart_connectioninstalls the new session beforeRestartedis emitted, sohandle_restarted's unpublish targets a sender bound to the previous transport. The existing TODO atrtc_engine/mod.rs:387-389says this error "is safe to ignore" — it is not, because the caller's cleanup sits behind the?.close()calls this aslet _ = ... .await, so the error surfaces nowhere.The fix
Run the local cleanup unconditionally and report the failure once local state is consistent. The return value is unchanged.
The
local_track_unpublishedcallback and the renegotiation stay on the success path, exactly as before. That callback dispatchesRoomEvent::LocalTrackUnpublished, and firing it on the reconnect path would emit an unpublish immediately beforeLocalTrackRepublished— whichroom.protoexplicitly tells bindings to treat as a preserved publication handle. Keeping it where it was means this change has no observable behavioural delta.Known follow-up: the E2EE
FrameCryptorcleanup (e2ee/manager.rs:214-220) also hangs off that callback, so a republished track still leaves a stale cryptor keyed on the old sid. Fixing that means accepting the event-stream change above, so I left it out of this PR rather than bundle the decision.Verification
test_unpublish_cleans_up_when_transport_is_gonepublishes a track, deletes the room server-side to force an abnormal disconnect, then drops every reference held outside the SDK — the publication handle, the track helper, and the event receiver — so anything still alive is held only by the cycle.Full e2e suite is green; the only delta from the baseline is the added test.