Bound SignalStream::close so a dead signal link cannot hang Room::close() - #1422
github-is-great wants to merge 1 commit into
Conversation
|
github-is-great seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…se() A signal WebSocket can stop delivering data while the socket stays open: no FIN, no RST, no ICMP error, which is what a lost cellular or Wi-Fi uplink looks like. The read task waits in conn.recv(), and nothing could wake it — NativeConnection::close closes the writer only, and the reader keeps polling a socket that never delivers. Since SignalStream::close joined the read task's handle, it never returned. That took the room down with it. The reconnect never reached SignalStream::connect because restart was waiting on the stream lock, and Room::close() never returned from the Leave send. Devices stayed in a room for hours after the SDK had reported them disconnected, and a caller-side timeout was no help because close() does not return at all. read_task now selects on a shutdown channel owned by the SignalStream, so close can stop it. The sender is held by value, so dropping a SignalStream without closing it stops its tasks too, and no Drop impl is needed. A CLOSE_DRAIN_TIMEOUT backstop bounds the shutdown if a transport stalls some other way — a send waiting on TCP retransmission, say — and aborts the tasks rather than leaving them parked on a socket the caller has given up on. That also bounds the capacity-8 channel backpressure case from livekit#1335. SignalInner::close now takes the stream out of the slot before closing it, so the write lock is no longer held across the shutdown. Three tests cover it: close(true), close(false) (the restart path, which has no Close message to unblock the write task) and a stream dropped without closing. All three fail against the previous code. issue-1407-test-results.md records the run, including the failures against the unpatched code. Fixes livekit#1407
a95b567 to
e68459e
Compare
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| /// Dropped to stop `read_task`, which is otherwise parked in a `recv()` that a dead | ||
| /// link never completes. Held by value, so dropping the `SignalStream` without | ||
| /// calling [`SignalStream::close`] stops the reader too. | ||
| shutdown_tx: oneshot::Sender<()>, | ||
| read_handle: JoinHandle<()>, | ||
| write_handle: JoinHandle<()>, |
There was a problem hiding this comment.
🔴 Stalled writer survives stream drop
When SignalStream drops during a stalled send, shutdown_tx only wakes the reader. The detached write_handle retains its task and socket indefinitely.
Learn more
Dropping a Tokio JoinHandle detaches its task; it does not cancel it. The new shutdown channel only participates in read_task, while write_task has no shutdown arm around conn.send() or conn.close(). A cancelled close future also drops these handles, producing the same leak before its timeout can call abort. The writer then keeps its Arc<dyn WsConnection> and native socket for as long as the transport operation remains pending.
Example: A full internal channel blocks close(true) behind a writer awaiting TCP retransmission. If room teardown is cancelled, SignalStream drops; the reader exits, but the writer and socket remain detached forever.
Recommended fix: Give both handles an abort-on-drop owner that remains armed whenever SignalStream::close is cancelled or the stream is dropped. On normal close, cancel and join both tasks within one deadline before disarming that owner. Add a transport whose send remains pending, then assert task completion and observable connection destruction after both direct drop and cancelled close.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let _ = | ||
| tokio::time::timeout(CLOSE_DRAIN_TIMEOUT, internal_tx.send(InternalMessage::Close)) | ||
| .await; | ||
| } | ||
|
|
||
| // Stop the read task first: it holds a clone of `internal_tx`, so the write | ||
| // task's channel only closes once the read task is gone. | ||
| drop(shutdown_tx); | ||
| drop(internal_tx); | ||
|
|
||
| let drained = tokio::time::timeout(CLOSE_DRAIN_TIMEOUT, async { | ||
| let _ = (&mut read_handle).await; | ||
| let _ = (&mut write_handle).await; | ||
| }) |
There was a problem hiding this comment.
🟡 Close deadline runs twice
When the channel is full, close(true) spends two consecutive CLOSE_DRAIN_TIMEOUT periods. Its documented two-second bound therefore becomes four seconds.
Learn more
The notification enqueue and task drain each receive a fresh two-second timeout. A stalled writer can keep the bounded channel full, so the first timeout expires completely before the second starts. This affects room teardown because it calls close(true).
Example: Eight queued messages fill the channel while conn.send() remains pending. Enqueuing Close times out after two seconds, then joining the writer times out after another two seconds; close returns after roughly four seconds instead of two.
Recommended fix: Compute one deadline at entry and use timeout_at with that deadline for both phases, or wrap the complete notify-and-drain sequence in one timeout. Preserve access to both handles so expiry still aborts them.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes #1407.
The bug
A signal WebSocket can stop delivering data while the socket stays open — no FIN, no RST, no ICMP error, which is what a lost cellular or Wi-Fi uplink looks like.
read_taskwaits inconn.recv(), and nothing could wake it:NativeConnection::closecloses the writer only, and the reader keeps polling a socket that never delivers. SinceSignalStream::closejoined the read task's handle, it never returned.That took the room down with it:
SignalInner::restartnever reachedSignalStream::connect, because it was waiting on the stream write lock that the hungclosewas holding. The engine sat inReconnectingand logged no connect attempt.Room::close()never returned from theLeavesend, which is a pass-through signal and so waits on the same lock.A caller-side timeout is not a workaround, because
close()does not return at all. The reporter saw devices stay in a room for hours after their SDK had reported a disconnect.The fix
read_taskselects on a shutdown channel owned by theSignalStream, soclosecan stop it. The sender is held by value, so dropping aSignalStreamwithout closing it stops its tasks too — noDropimpl needed.CLOSE_DRAIN_TIMEOUT(2s) bounds the shutdown if a transport stalls some other way, and aborts the tasks rather than detaching them. A write task parked inconn.send()on a half-open socket holds the last references to the connection; detaching would keep the socket alive long after the caller gave up. This also bounds the capacity-8 channel backpressure case from Close peer connections before awaiting signal teardown #1335.SignalInner::closetakes the stream out of the slot before closing it, so the write lock is not held across a network operation. This is the one path where the lock protects nothing, since the stream is already on its way out.SignalClient::closealso picked up a doc note on its upper bound, which the reporter asked for: a close racing an in-flight reconnect still waits behind that reconnect's drain,SIGNAL_CONNECT_TIMEOUTandJOIN_RESPONSE_TIMEOUT. Callers who need a deadline should impose their own.Tests
Three tests over a transport that never delivers data and whose
closeaffects only the writer, mirroringNativeConnection:close_returns_on_a_dead_link— the reporter's reproductionclose_without_notify_returns_on_a_dead_link— therestartpath, which has noClosemessage to unblock the write taskdropping_the_stream_stops_its_tasks— a caller that abandonscloseand drops the room insteadAll three fail against the previous code, at their 10s guard, with
SignalStream::close() did not return on a dead link. Verified by temporarily reverting the two production changes while keeping the tests.With the fix, the full
livekit-signalingsuite is 42 passed / 0 failed in 0.03s — nothing reaches a timeout, soclose()returns on its own rather than being rescued by the backstop.cargo fmt --checkandcargo clippy --all-targetsare clean (the crate's 7 remaining clippy warnings are all pre-existing and outside the changed code). Toolchain: the repo's pinned 1.97.1.Not addressed
Two things the issue lists under Effect are left alone, as the reporter suggested — they are the reason a slow
close()became an unleavable room, but they are a separate fix:Roomhas noDropimpl, so a caller that abandonsclose()cannot drop the room instead.RoomSession::closetakes its task handles on entry, so a cancelledclose()cannot be retried; the second call returnsAlreadyClosedand shuts nothing down.The reporter also offered an end-to-end test that puts a TCP proxy in front of the signal WebSocket and stops it forwarding while holding both sockets open. That exercises the real transport rather than a mock and would be worth taking them up on.