Handle data track SID reassignment#2000
Conversation
🦋 Changeset detectedLatest commit: 14655c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Old routing handle is never removed after track identifier reassignment, causing a stale entry in the packet routing table
The old subscription handle is never deleted from the routing table (this.subscriptionHandles at src/room/data-track/incoming/IncomingDataTrackManager.ts:555) when a new handle is later assigned (registerSubscriberHandle at src/room/data-track/incoming/IncomingDataTrackManager.ts:600-604), so the stale entry persists indefinitely.
Impact: If the server reuses the old handle number for a different track, incoming packets for that track would be misrouted to the wrong subscriber.
Stale handle accumulation mechanism
When handleSidReassigned detects an active subscription, it updates the old handle's mapping to point to the new SID at src/room/data-track/incoming/IncomingDataTrackManager.ts:555:
this.subscriptionHandles.set(descriptor.subscription.subcriptionHandle, newSid);
Later, when the SFU assigns a new subscriber handle, registerSubscriberHandle at src/room/data-track/incoming/IncomingDataTrackManager.ts:600-604 overwrites the descriptor's handle and adds the new handle to the map, but never deletes the old handle:
case 'active': {
descriptor.subscription.subcriptionHandle = assignedHandle;
this.subscriptionHandles.set(assignedHandle, sid);
return;
}
The old handle is now orphaned in subscriptionHandles — no descriptor references it, but it still maps to the new SID. This is both a memory leak and a potential misrouting hazard.
(Refers to lines 600-604)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!this.descriptors.delete(oldSid)) { | ||
| return false; | ||
| } | ||
| descriptor.info.sid = newSid; | ||
|
|
||
| switch (descriptor.subscription.type) { | ||
| case 'none': | ||
| break; | ||
| case 'pending': | ||
| case 'active': | ||
| // The SFU does not carry subscriptions across a publisher's full | ||
| // reconnect; re-request the subscription under the new SID. | ||
| this.emit('sfuUpdateSubscription', { sid: newSid, subscribe: true }); | ||
| break; | ||
| } | ||
| if (descriptor.subscription.type === 'active') { | ||
| // Keep the routing index consistent until the SFU assigns a new handle | ||
| // (see `registerSubscriberHandle`). | ||
| this.subscriptionHandles.set(descriptor.subscription.subcriptionHandle, newSid); | ||
| } | ||
| this.descriptors.set(newSid, descriptor); |
There was a problem hiding this comment.
🔴 Stream cleanup and abort handlers break after track identifier reassignment because they reference the old identifier
After a track identifier is reassigned (handleSidReassigned at src/room/data-track/incoming/IncomingDataTrackManager.ts:537-540), existing stream closures still look up the descriptor using the original identifier (this.descriptors.get(sid) at src/room/data-track/incoming/IncomingDataTrackManager.ts:161,183), which no longer exists in the map, so cleanup silently fails.
Impact: After a publisher reconnects, cancelling or aborting a subscription stream will not properly unsubscribe from the server, leaking the subscription.
Closure capture and stale lookup mechanism
The openSubscriptionStream method at src/room/data-track/incoming/IncomingDataTrackManager.ts:142 captures the sid parameter as a string constant in the cleanup (line 161) and onAbort (line 183) closures.
When handleSidReassigned runs, it deletes the descriptor from this.descriptors under the old SID (line 537) and re-inserts it under the new SID (line 557). The closures still hold the old SID string.
When the user later cancels the ReadableStream (triggering cleanup at line 235-237) or aborts via signal (triggering onAbort at line 179):
this.descriptors.get(sid)returnsundefinedbecausesidis the old valuecleanuplogs a warning and returns early at line 162-164- The stream controller is never removed from
descriptor.subscription.streamControllers unSubscribeRequestis never called, so the SFU subscription is never torn down
A similar issue affects the subscribeRequest cancel closure at src/room/data-track/incoming/IncomingDataTrackManager.ts:333, which would emit an unsubscribe event with the old SID if a pending subscription times out after reassignment.
Prompt for agents
The handleSidReassigned method deletes the old SID from this.descriptors and inserts the descriptor under the new SID, but existing closures in openSubscriptionStream (cleanup at line 161, onAbort at line 183) and subscribeRequest (cancel at line 333) have captured the old SID string by value. After reassignment, those closures look up the old SID and get undefined, so cleanup silently fails.
Possible approaches:
1. Instead of looking up the descriptor by SID in the closures, capture a reference to the descriptor object itself (which remains the same object across reassignment). The closures can then use descriptor.info.sid to get the current SID.
2. Alternatively, use a mutable wrapper object (e.g. { current: sid }) that handleSidReassigned can update, so all closures see the new SID.
3. When handleSidReassigned detects active stream controllers, it could re-wire them by updating whatever state the closures depend on.
Affected closures:
- openSubscriptionStream cleanup closure (line 154-177): uses this.descriptors.get(sid)
- openSubscriptionStream onAbort closure (line 179-192): uses this.descriptors.get(sid)
- subscribeRequest cancel closure (line 328-343): emits sfuUpdateSubscription with old sid
Was this helpful? React with 👍 or 👎 to provide feedback.
| case 'active': | ||
| // The SFU does not carry subscriptions across a publisher's full | ||
| // reconnect; re-request the subscription under the new SID. | ||
| this.emit('sfuUpdateSubscription', { sid: newSid, subscribe: true }); |
There was a problem hiding this comment.
is there a scenario in which the user might have unsubscribed to the data track and we're re-forcing it to subscribe here?
There was a problem hiding this comment.
One possibility that comes to mind:
- A user completes their final subscription for a data track, which calls
IncomingDataTrackManager.unSubscribeRequest(see here). but otherwise leaves the descriptor inactivetype - A SFU full reconnect occurs, and the client must re subscribe via the new SFU to all remote data tracks
- Because of the
activetype on the descriptor, the unsubscribed data track from 1 is resubscribed
@ladvoc Given this, maybe it's worth introducing a unsubscribing or similar descriptor type which can mark that the descriptor is on its way out and should be dropped during a reconnect. Or maybe another way to do this could be to only republish if descriptor.streamControllers.size > 0. Thoughts? I think this would also need to be done on the rust one too, as that also only has the three none / pending / active states.
3 participants
Equivalent to: livekit/rust-sdks#1228
Closes BOT-457