subscription_thread_dispatcher: guard against duplicate room events

[stephen-derosa]

Overview

• Hardens SubscriptionThreadDispatcher and Room remote track callback handling against duplicate subscription events and unsafe callback replacement
• Readers track the subscribed track SID and skip redundant restarts when the same publication is re-subscribed
• Data track teardown marks in-flight subscriptions as cancelled so republish/unpublish does not leave stale readers or duplicate FFI subscriptions behind
• setOn*FrameCallback is deprecated in favor of [[nodiscard]] trySetOn*FrameCallback, which returns false when a reader is already active; callers must clearOn*FrameCallback before replacing a running callback
• Deprecated setOn* overloads remain as thin forwards for backwards compatibility
• Room::trySetOn* registers with the dispatcher and starts a reader immediately when the matching remote track is already subscribed; otherwise the reader starts on the next subscribe event

Public API Changes

Added:

• Room::trySetOnAudioFrameCallback, trySetOnVideoFrameCallback, trySetOnVideoFrameEventCallback — return bool
• Matching SubscriptionThreadDispatcher::trySetOn* methods

Deprecated:

• Room::setOnAudioFrameCallback, setOnVideoFrameCallback, setOnVideoFrameEventCallback
• Matching SubscriptionThreadDispatcher::setOn* methods

Test-only:

• RemoteDataTrack::testFfiHandleId() removed; tests use internal RemoteDataTrackTestAccess instead

README and Doxygen updated to document the deprecation and replacement pattern.

Testing

• Unit tests cover duplicate-event SID guards, trySetOn* rejection while a reader is active, clear-then-replace flows, and data reader cancellation/republish behavior
• Integration tests verify room lifecycle delegate callbacks fire exactly once and that auto-wired data callbacks follow unpublish/republish under the same track name
• Existing audio, video metadata, and room callback tests updated to use trySetOn*

[Copilot]

Pull request overview

This PR refines SubscriptionThreadDispatcher’s reader-thread lifecycle handling to avoid redundant restarts and to ensure data reader threads reliably tear down (especially when a data-track subscription is still in flight), aligning with the SDK’s threading model by preventing teardown hangs.

Changes:

• Add cancellation signaling for active data readers and ensure cancellation is set before closing/joining during unpublish and teardown.
• Introduce a self-cleanup path (eraseDataReaderIfCurrent) so data reader threads can safely remove stale slots when they exit.
• Skip redundant audio/video/data reader restarts when the same track SID is already active.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/tests/unit/test_subscription_thread_dispatcher.cpp	Adds unit tests covering data reader cancellation semantics and self-erase behavior.
src/subscription_thread_dispatcher.cpp	Implements cancellation-before-close, self-erase cleanup, and redundant reader-start suppression.
include/livekit/subscription_thread_dispatcher.h	Adds per-reader state (track_sid, cancelled) and declares the new cleanup helper.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[devin-ai-integration]

Devin Review found 1 new potential issue.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

[alan-george-lk]

Not sure if better or worse, but I typically use GTest's FRIEND_TEST macro so you can just give a single test suite/case access to what it needs. May break down a bit between unit and integration test binaries if they have different suite names (could unify name). Basically decays down to a friend class <blah> anyways, just looks a bit cleaner: https://google.github.io/googletest/reference/testing.html#FRIEND_TEST

[alan-george-lk]

From what I can tell, these new "try" methods have the same inputs as the deprecated prior ones, just a new return type (and name).

This is actually a situation where I think maintaining the API and using exceptions is cleaner than deprecation/new API just to get the bool return type.

Thoughts on this? Could we not move the new implementations from room.cpp into the older versions and throw instead of returning false?

[stephen-derosa]

isnt that still changing the public API though? maybe im wrong but i would think adding a throw to a function that previously didnt would require a major bump?