Skip to content

Fix subscription events issuing new subscription requests - #11

Merged
tiredIsa merged 1 commit into
native-v2from
feat/131-subscription-event-state
Sep 9, 2026
Merged

tiredIsa merged 1 commit into
native-v2from
feat/131-subscription-event-state

Conversation

@tiredIsa

@tiredIsa tiredIsa commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Room handles TrackSubscribed and TrackUnsubscribed by calling the public subscription setter, which sends another request to Rust. A delayed event can therefore overwrite a newer application subscription request; a failed request also prevents the event from reaching the delegate.

Use a private state update for those two Room events. Public subscription commands and the class layout remain unchanged. A regression test dispatches 100 subscribe/unsubscribe event pairs through the real FFI callback entry point, using zero publication handles to reject any accidental outbound request and checking publication state and delegate delivery.

Validation: the regression failed against the pinned parent implementation with setSubscribed: invalid FFI handle and missing delegate callbacks. With the fix, all 34 targeted Room/publication tests pass, including the 100 event pairs. Release SDK and test binaries build; clang-format passes and clang-tidy completes without errors (existing const-correctness warnings remain). No server integration test is claimed from this offline suite.

Supports syrnike13/syrnike13-app#131. The application run with SDK commit d966cee and app commit b3fa3ecf passed 100 renderer reloads, 100 crashes and 100 withheld texture-release cycles. Incoming video recovered on every replacement; four outgoing publications retained their identities, with zero observer reconnects or reader failures. Maximum outgoing gaps were 122/114/114 ms; publisher cleanup and normal process exit passed. SDK DLL hashes and source identities were verified unchanged before and after the run. This supports integration of the fix but does not conclusively attribute the earlier intermittent incoming-video stall to this defect.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b36e25ba-78c4-4a0f-bbb6-9cb72dba1db6

📥 Commits

Reviewing files that changed from the base of the PR and between ed1c398 and d966cee.

📒 Files selected for processing (3)
  • include/livekit/remote_track_publication.h
  • src/room.cpp
  • src/tests/unit/test_room.cpp

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

События Room теперь напрямую обновляют состояние RemoteTrackPublication без повторной отправки FFI-запросов. Добавлен модульный тест для событий подписки и отписки.

Changes

Состояние подписки удалённого трека

Layer / File(s) Summary
Обновление состояния без FFI-запроса
include/livekit/remote_track_publication.h, src/room.cpp
RemoteTrackPublication получил приватный метод updateSubscriptionState. Обработчики kTrackSubscribed и kTrackUnsubscribed используют этот метод вместо setSubscribed.
Проверка событий подписки
src/tests/unit/test_room.cpp
Добавлены делегат для проверки состояния публикации и тест, который обрабатывает события подписки и отписки 100 раз без исходящих запросов.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to d966c

Subscription events now update remote publication state without sending duplicate subscription requests, preserving delegate delivery and preventing stale event handling from overwriting newer application intent. The covered behavior is ready to merge.

Sequence Diagram(s)

sequenceDiagram
  participant FfiEvents
  participant Room
  participant RemoteTrackPublication
  participant RoomDelegate
  FfiEvents->>Room: track_subscribed или track_unsubscribed
  Room->>RemoteTrackPublication: updateSubscriptionState(true или false)
  Room->>RoomDelegate: onTrackSubscribed или onTrackUnsubscribed
  RoomDelegate-->>Room: проверка состояния публикации
Loading

Suggested reviewers: alan-george-lk, xianshijing-lk, stephen-derosa

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Заголовок точно описывает основное изменение: исправление обработки событий подписки, чтобы они не создавали новые запросы подписки.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/131-subscription-event-state

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Кролик видит: состояние в пути,
Room его обновил без лишней суеты.
Подписка включилась, отписка пришла,
Проверка сто раз их подряд провела.
FFI-запросы уснули в тиши.

Comment @coderabbitai help to get the list of available commands.

@tiredIsa
tiredIsa merged commit 0906793 into native-v2 Sep 9, 2026
46 of 48 checks passed
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.

1 participant