fix(rtc_engine): hold a weak reference to the engine from its own task - #1434
Open
LautaroPetaccio wants to merge 2 commits into
Open
LautaroPetaccio wants to merge 2 commits into
LautaroPetaccio wants to merge 2 commits into
Conversation
`engine_task` took an `Arc<EngineInner>`, but the task's `JoinHandle` and the oneshot sender that stops it both live in `EngineHandle`, inside that same `EngineInner`. The task therefore kept alive the only thing that could ever stop it. An engine dropped without an explicit `close()` released nothing: the session, both peer connections, the signal client with its open websocket, and the task itself stayed resident for the lifetime of the process, and the SFU kept its half of the session because the socket was never closed. `RtcSession` already avoids this by keeping `close_tx` in `SessionHandle`, outside the `Arc`'d inner. Rather than restructure the handle here, take a `Weak` and upgrade per event, which is what AGENTS.md prescribes for a task whose owner stores it. The task now also stops on its own once the engine is gone, instead of depending solely on the close signal. Adds an e2e test asserting the internals are released after a drop without `close()`, and a `test_engine()` harness helper so engine lifecycle can be observed without the room's own ownership edges masking it.
Contributor
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
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
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.
The problem
engine_tasktook anArc<EngineInner>:But the task's
JoinHandleand theoneshot::Senderthat stops it both live inEngineHandle(rtc_engine/mod.rs:266), which lives inrunning_handleinside that sameEngineInner. The task therefore kept alive the only thing that could stop it — its own stop signal — andsession_events' emitter is likewise reachable only through the session it pins.An engine dropped without an explicit
close()released nothing: the session, both peer connections, theArc<LkRuntime>(and so thePeerConnectionFactoryand its WebRTC threads), the signal client with its open websocket, and the task itself all stayed resident for the lifetime of the process. The SFU also kept its half of the session, since the socket was never closed.There is no
DroponRtcEngine,EngineInner,RtcSessionorSessionInner, so nothing else could recover it.The fix
RtcSessionalready avoids exactly this by keepingclose_txinSessionHandle, outside theArc'd inner, so dropping it stops its tasks. Rather than restructureEngineHandle, the task now takes aWeakand upgrades per event.This is what AGENTS.md already requires:
A side benefit is that the task now stops on its own once the engine is gone, instead of depending solely on the close signal.
Verification
test_drop_without_close_releases_engineconnects an engine, takes a drop probe, drops it withoutclose(), and waits for the internals to be released.Full e2e suite against
livekit-server --devis green, including all six reconnection tests. The only delta from the baseline is the added test.The test targets
RtcEnginedirectly rather than going throughRoom, because the room layer has the same structural issue with its ownclose_txand would mask an engine-level fix.test_engine()is added to the e2e harness for that purpose.