fix(webrtc): serialize screen-share publication and capture restarts - #91
Merged
Merged
Conversation
Fixes #70. On macOS the app terminated in ScreenCaptureKit/ReplayKit with "Collection <__NSArrayM> was mutated while being enumerated" after repeatedly publishing the same screen_share track while the room recovered. The publish path decided replace-vs-publish by reading trackPublications and then awaited publishTrack. livekit only inserts the publication once that await resolves, so two overlapping callers both read "nothing published yet" and both published: "publishing a second track with the same source" and "TrackInvalidError: a track with the same ID has already been published". Surfacing that rejection drove another retry, and the retries restarted capture underneath the running stream. Overlap was easy to reach: CapturePreview's publish effect re-runs on five dependencies, and isHosting alone flaps false/true on every reconnect, while a publish ending in "publication of local track timed out" stays in flight for seconds. - Serialize publishStream/unpublishStream onto one promise chain so the replace-vs-publish decision and the publish are atomic, and so a stop/start straddling a reconnect cannot unpublish the track the restart just aired. - Treat "already been published" as success instead of an error, so a duplicate that slips through cannot start a retry loop. - Give publishStream an isStale callback, checked once it reaches the front of the queue; CapturePreview's effect uses it to abandon a publish whose capture session has been superseded. - Guard the three capture entry points with a ref instead of isCapturing state, which is not visible to another handler in the same tick and so let two concurrent ScreenCaptureKit sessions start. - Snapshot trackPublications in stopHosting before unpublishing; unpublishing deletes from the Map being walked. - Log publish/replace/unpublish with source and track id. apps/web's SFU host never received the replace-or-publish fix from #80, so it published unconditionally on every call. Brought it to parity with the same serialization, dedupe and already-published handling. Regression tests cover the overlap, the publish/unpublish interleave, the already-published error and the stale-publish skip; the first two fail without the queue. The existing mock resolved publishes synchronously, which is why it never caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfEYUb5EtniuCgViazw7Vm
ThreatCrush Security Scan72 finding(s) HIGH/CRITICAL: 13 | MEDIUM: 41 | LOW: 18
…and 22 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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
1 participant
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.
Fixes #70.
The crash
macOS terminated with an uncaught
NSGenericExceptionfrom ScreenCaptureKit / ReplayKit —Collection <__NSArrayM> was mutated while being enumerated— after the app repeatedly published the samescreen_sharetrack while the room was recovering.Two independent races produced it.
1. The publish path was not single-flight.
publishStreamdecided replace-vs-publish by readingtrackPublications, then awaitedpublishTrack. livekit only inserts the publication once that await resolves, so two overlapping callers both read "nothing published yet" and both published — exactly the reported sequence:Surfacing that rejection drove the caller into another retry, and the retries restarted capture underneath the running stream.
Overlap was easy to reach:
CapturePreview's publish effect re-runs on five dependencies,isHostingalone flaps false/true on every reconnect, and a publish ending in "publication of local track timed out" stays in flight for seconds while it does.2. The capture guard could not exclude. All three capture entry points guarded on
isCapturing, which is React state — not visible to another handler running in the same tick. Two clicks, or a click racing a recovery restart, both readfalseand both opened a capture session, giving two concurrentcontentPickerDidSelectFilter:forStream:callbacks mutating one stream collection. That is the direct crash signature.Changes
publishStream/unpublishStreamonto one promise chain, so the replace-vs-publish decision and the publish are atomic, and a stop/start straddling a reconnect cannot unpublish the track the restart just put on the air.publishStreamtakes anisStalecallback checked once the call reaches the front of the queue;CapturePreview's effect uses its cleanup to abandon a publish whose capture session has been superseded.runExclusiveCapture) replacing the stale-state check on all three entry points.trackPublicationsinstopHostingbefore unpublishing — unpublishing deletes from the Map being walked.unpublishStreamalready did this;stopHostinghad been missed, in both apps.apps/web's SFU host never received the replace-or-publish fix from #80 and published unconditionally on every call, so it is brought to parity with the same serialization, dedupe and already-published handling.The bulk of the
home.tsxdiff is re-indentation from wrapping each handler body; the logic inside is unchanged.Tests
Four regression tests in
useWebRTCHostSFUAPI.test.ts: the overlapping publish, the publish/unpublish interleave, the already-published error, and the stale-publish skip. The first two fail without the queue and pass with it — verified by temporarily reducing the queue to a direct call.The existing mock resolved publishes synchronously, which is precisely why it never caught this; the new helper holds the screen-share publish open (and only that one —
startHostingawaits the host-mic publish inline).Checks
Run individually, since this repo's pre-commit hook gets OOM-killed on my machine (commit used
--no-verify):vitest run apps/desktop apps/web— 1483 passed / 141 files, 0 failurestypecheck— desktop and web, both cleanlint— desktop clean; web has 2 warnings, both pre-existing in files this branch does not touchpnpm format:check(repo-wide) — cleanNot covered
The acceptance criterion "exercise repeated reconnect/recovery on macOS" needs a macOS box, which I do not have — worth a manual pass before release. Note also that merging does not ship the desktop app; it needs a
v*tag.