[yummybomb]

Summary

A client that asks for media and leaves before WebRTC connects left the video and audio pipelines running for the life of the process. A peer that has not connected is now destroyed when its session's websocket goes away.

Why it happened

signal/request creates the peer and subscribes its tracks to the capture streams, which starts their pipelines. From then on the peer only goes away through its connection state: disconnected and failed destroy it, closed removes its listeners. A peer whose offer is never answered stays in new, where none of that happens. Deleting or disconnecting the session does not help, because both only destroy a peer that is watching.

The streams stop when their last listener leaves, so one such peer keeps both pipelines encoding. Viewers that come and go afterwards do not stop them.

The legacy /ws endpoint requests media as soon as a client connects, so opening a page that uses it and closing or reloading it before the stream starts is enough.

What changes

• DisconnectWebSocketPeer destroys the session's WebRTC peer if it has not connected. A connected peer is left alone as before, and a delayed disconnect still gives the client its time to come back first.
• SetWebRTCPeer does the same when the websocket is already gone. Creating a peer takes long enough for the client to leave in the meantime, and the legacy endpoint logs the session out as soon as its client disconnects. The disconnect then had no peer to destroy.
• The session records whether its current peer has connected, and both checks above read that. The session's watching state cannot serve here: it is left over from a peer that was replaced, because the closing of a replaced peer is ignored. A client that was watching, asked for media again and left before it answered would still have left its peer behind. A page reload on /api/ws does this when the new page asks before the server has noticed that the old peer is gone.
• Track.SetStream returns ErrTrackClosed after Shutdown. A peer destroyed between CreatePeer and the SetVideo that follows it would otherwise add a listener that nothing removes, and its samples would be written to a closed channel.

Testing

• internal/session: a peer that never connected is destroyed when the websocket closes, when the session is deleted and when it is disconnected. It is kept while a dropped websocket may still reconnect, and a streaming peer is not destroyed by a closing websocket. A peer set after the websocket left is destroyed. A peer that replaced a streaming one is destroyed when the websocket closes, unless it has connected itself.
• internal/webrtc: a track stops listening on shutdown and does not listen to a stream after it.
• Removing any one of the five parts of the change (the cleanup on disconnect, the check that the peer is not streaming, the check in SetWebRTCPeer, the reset of the connected state when a peer is replaced, the guard in SetStream) fails at least one of these tests.
• On a VM with a hardware encoder I ran clients that leave at different points against the server built from release tag v3.0.8-v1.6.3, with and without this change, and looked 9 s after each client had left:

Client	Without	With
/api/ws: requests media, leaves after the offer	both pipelines still running	stopped
/api/ws: requests media, leaves at once or 300 ms later	still running	stopped
/ws: leaves after the offer	still running	stopped
/ws: leaves 0, 20, 100, 300, 1000 or 3000 ms after connecting	still running, all six	stopped, all six
eight /ws clients at once, leaving after 0 to 600 ms	still running	stopped

Without the change each of these left GPU memory at about 885 MiB against 370 MiB idle and the server at 65 to 72% of a core against 0, unchanged 40 s later; the server was restarted between cases so that each one started clean. With it, memory and CPU were back at idle in every case, the server never restarted or logged a panic, and a real viewer afterwards got its first frame 1.4 s after connecting.

• The replaced peer was checked with the real WebRTC manager, a pion client over loopback and a capture manager that only counts listeners; this test is not part of the change. A peer connects, the websocket closes, a second request replaces the peer and the websocket closes again. Reading the watching state, both listeners were still there 9 s later; with the connected state per peer, none were. The VM runs above were made before that commit and were not repeated.

Two things this does not do: it does not time out a peer whose client keeps its websocket open and never answers, and it leaves the existing unsynchronized reads of session state as they are.