Skip to content

Room lifecycle memory fixes - #1405

Merged
alan-george-lk merged 6 commits into
alan/bugfix-video-source-lifecyclefrom
alan/bugfix-room-lifecycle
Sep 8, 2026
Merged

Room lifecycle memory fixes#1405
alan-george-lk merged 6 commits into
alan/bugfix-video-source-lifecyclefrom
alan/bugfix-room-lifecycle

Conversation

@alan-george-lk

@alan-george-lk alan-george-lk commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Break the RoomSession ownership cycle by capturing it weakly from the E2EE callback
  • Clear data-channel callbacks during RTC teardown, breaking the observer/callback self-cycle
  • Add regression tests verifying both room-session destruction and data-channel callback cleanup

Memory impact

Previously, repeated room joins retained room sessions, SDP/codec allocations, peer connections, and WebRTC threads. After these fixes, 1,000 connect/disconnect cycles dropped from approximately 488 MiB RSS to 44 MiB, with stable thread and file-descriptor counts.

Testing

  • New room lifecycle E2E regression passed
  • C++ 1,000-cycle memory lifecycle tester (representative of robotics use case) passed

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Changeset ✓

This PR includes a changeset covering all affected packages:

Package Bump
libwebrtc patch
livekit patch
livekit-ffi patch

@alan-george-lk alan-george-lk changed the title WIP: Room lifecycle memory fixes Room lifecycle memory fixes Sep 4, 2026
@alan-george-lk
alan-george-lk marked this pull request as ready for review September 4, 2026 15:51

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@raparanjpe

Copy link
Copy Markdown
Contributor

Hi @alan-george-lk! Thank you for proposing this fix; I believe I followed a similar approach to weakly capturing the RoomSession in #1377. For the data channel I was going to follow a similar approach to you here, but noticed we can avoid capturing the data channel in its own closure altogether by changing the interface slightly. Feel free to use that implementation if it's easier than manually unsetting the closures.

@alan-george-lk
alan-george-lk force-pushed the alan/bugfix-room-lifecycle branch from 33891b7 to d539b63 Compare September 4, 2026 18:16
@alan-george-lk
alan-george-lk force-pushed the alan/bugfix-room-lifecycle branch from e99174b to 7c3310a Compare September 7, 2026 20:56
@alan-george-lk

alan-george-lk commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi @alan-george-lk! Thank you for proposing this fix; I believe I followed a similar approach to weakly capturing the RoomSession in #1377. For the data channel I was going to follow a similar approach to you here, but noticed we can avoid capturing the data channel in its own closure altogether by changing the interface slightly. Feel free to use that implementation if it's easier than manually unsetting the closures.

Hey @raparanjpe, great minds think alike -- I just reviewed your PR. I think removing amount is cleaner, but there are test items/a PR stack here addressing a few related changes.

Can you retarget your PR to this one in the next day or two, keeping my changes except for the data channel related ones (taking your approach instead)? Then I can re-test everything with a memory tracker util I have on a separate SDK that brings in these changes.

I have pulled in your commit such that you get credit. We may merge this before you have a chance to review, but I'll make sure your name is in the author credits.

@raparanjpe

Copy link
Copy Markdown
Contributor

@alan-george-lk no problem at all!

Comment thread livekit/src/rtc_engine/rtc_session.rs Outdated
// closing here — before the future can suspend — is what stops a cancelled
// `close()` leaving the ICE sockets bound for the process's lifetime. The
// signalling socket is unaffected, so the Leave below still goes out.
self.lossy_dc.on_message(None);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to set these to None anymore as the closure isn't capturing the arc reference on these data channels!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch -- meant to remove these, thanks. Done now.

@alan-george-lk
alan-george-lk force-pushed the alan/bugfix-room-lifecycle branch from ded4ec4 to 9a14150 Compare September 8, 2026 13:58
@alan-george-lk
alan-george-lk merged commit 16530d2 into main Sep 8, 2026
22 checks passed
@alan-george-lk
alan-george-lk deleted the alan/bugfix-room-lifecycle branch September 8, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants