TEL-886: Media Port rework - #802
Conversation
11f4b21 to
dd06f29
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if c.lkRoomIn != nil { | ||
| if err := c.lkRoomIn.Close(); err != nil { | ||
| log.Warnw("error closing livekit room audio input", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Outbound call closes the room audio track twice on teardown
When an outbound call ends, the room-facing audio track is closed once by the media port teardown (c.media.Close() at pkg/sip/outbound.go:382, which propagates through the media port's inbound-audio switch) and then closed a second time explicitly (c.lkRoomIn.Close() at pkg/sip/outbound.go:390), so the same audio writer is torn down twice.
Impact: The call-teardown path double-releases the outbound audio track, which can surface as spurious error logs or, if the underlying encoder is not close-idempotent, resource corruption during shutdown.
Why the same object is closed on two paths
In connectMedia the (possibly processor-wrapped) c.lkRoomIn is handed to the media port via c.media.WriteInboundAudioTo(c.lkRoomIn) (pkg/sip/outbound.go:559), which stores it as the inner writer of the port's audioIn switch (pkg/sip/media_port.go:734-735).
On mediaPort.Close(), p.audioIn.Close() is called with the explicit comment "Propagate Close() to onwards to room" (pkg/sip/media_port.go:693), closing c.lkRoomIn.
Then outboundCall.close closes it again at pkg/sip/outbound.go:389-393. The inbound path (inboundCall.closeMedia at pkg/sip/inbound.go:1578-1585) only closes it once via c.media.Close(), so the outbound explicit close is redundant and asymmetric.
Prompt for agents
In outboundCall.close (pkg/sip/outbound.go around lines 389-393), c.lkRoomIn.Close() is called explicitly, but c.media.Close() (called just above at line 382) already propagates Close() to the inbound-audio writer stored via connectMedia's c.media.WriteInboundAudioTo(c.lkRoomIn), which is c.lkRoomIn. This results in a double Close() of the room participant track / opus encoder on every outbound call teardown. The inbound path (inboundCall.closeMedia) relies solely on media.Close() and does not double-close. Decide on a single ownership model: either drop the explicit c.lkRoomIn.Close() here (relying on media.Close to close it), or ensure the explicit close only runs when connectMedia never wired c.lkRoomIn into the media port (e.g. early-failure paths). Make outbound consistent with inbound.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed that this is actually a thing: I'm seeing the log warning a lot in staging. Will address and fix.
|
Huh, the race in CI is real, and is apparently a problem in media-sdk (yay zerocopy) that previous tests simply did not expose. |
Will fix this in media-sdk. |
2 participants
This change is intended to: