Repository navigation
A close frame from the server fails the signalling stream, so the client reconnects at once - #1179
Draft
TimofeyIvanenko wants to merge 12 commits into
Draft
TimofeyIvanenko wants to merge 12 commits into
TimofeyIvanenko wants to merge 12 commits into
Conversation
…ebSocket session after it is closed
…mework callbacks on a global queue instead of a new one
…uation The opening wait and the upgrade-status probe each kept a continuation in a private `Once` class, with a separate `asyncAfter` block as the deadline. The SDK's `AsyncCompleter` already does both: it resumes once, times out on its own, and throws `LiveKitError(.cancelled)` when the waiting task is cancelled, so the `catch` below it closes the connection in every failure case. `resume(with:)` rather than `resume(throwing:)`, as the latter logs a late `.cancelled` at error. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Network.framework reports a bare TCP FIN as a completed receive with no metadata. Treating that as the end of the stream, like a close frame, made the signal client wait out its ping timeout before reconnecting. A FIN now fails the receive with a network error so the reconnect starts at once; only a close frame (or our own cancel) ends the sequence cleanly. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The Network.framework transport sends its upgrade headers through `NWProtocolWebSocket.Options.setAdditionalHeaders`; the `URLRequest` builder was left over from the URLSession transport and had no caller apart from the test that asserted its header merging. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`NWProtocolWebSocket.Options.maximumMessageSize` already defaults to 0, no limit; the assignment restated the default. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SwiftLint's optional_data_string_conversion flags the lossy `String(decoding:as:)` on an optional payload. The failable form keeps every valid frame as it was, an absent payload included, and fails the receive on a frame that is not UTF-8 instead of handing the signal client replacement characters. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A loopback server that answers the upgrade by hand and then misbehaves on purpose: a close frame, a bare FIN, a reset, an HTTP status, or no answer. The suite covers how the socket opens and fails to open, how its message sequence ends for each way a peer can go away, and the retention the transport change exists for: twenty sockets opened and closed leave no socket and no NWConnection behind, where the URLSession transport kept its session and task for the life of the process. The socket's connection is internal rather than private so the release test can hold a weak reference to it. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…queue, deterministic peer closure Review findings on the lifecycle suite. Every test now cancels its fake server in a `defer`, so a throw no longer leaves the listener, and the server its handler captures, alive; the awaited `stop()` stays where the port has to refuse connections. The server delivers on the shared global queue, as the transport under test does, instead of a serial queue of its own. The cancellation test waits for the listener to accept the TCP connection before cancelling, so the cancel lands during the connect rather than before it. The peer-closure cases were flaky: the server went away straight after the 101, racing the client's handshake, and a close frame that arrives while no receive is pending is consumed by Network.framework's close handshake, after which the next receive fails with ENOTCONN. The server now goes away only after a frame from the client, and the close-frame case accepts either a clean end or that ENOTCONN. Twenty consecutive runs pass. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A raw TCP receive can return part of a frame. The server's first receive after the upgrade could then leave the rest of the client's trigger frame in the socket, and the receive meant for the close reply would take those bytes for it and cancel too early. The server now parses the client frame header, the masked 7-bit or 16-bit length and the mask key, and keeps receiving until exactly that many payload bytes are in, both for the trigger frame and for the close reply. Co-authored-by: omnigent <noreply@omnigent.ai> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…HTTP or PAC proxy
…e code, so the signal client reconnects at once
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
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.
Stacked on #1173: until it merges, this diff includes its commits. The change of its own is the last commit. It is independent of #1178.
Problem
A close frame from the server ended the signalling stream with
nil, which never reachesonFailure. The signal client stayed.connectedand reconnected only after the ping timeout. When the frame arrived while no receive was pending, Network.framework consumed it and the next receive failed withENOTCONN, so the client reconnected at once. Both outcomes happened. The JS, Android, Flutter and Rust SDKs treat every server close as a reconnect trigger.Change
LiveKitError(.network). ItsinternalErrorisWebSocketClosedByPeer, which carries the close code (1005 when the peer sent none). Our ownclose()still ends the sequence withnil(ECANCELED).asTransportErrormapsWebSocketClosedByPeerto.Closed, so livekit-net still reads a close as the end of the stream.Tests
WebSocketLifecycleTests.peerClosure: the close-frame case now requires a network error, with close code 1000, orENOTCONNwhen Network.framework consumed the frame.TransportErrorMappingTests:WebSocketClosedByPeer→.Closed.ServerRemovalTests.aRemovedParticipantDisconnectsWithoutReconnecting(e2e):RoomService.RemoveParticipantsends a leave and then closes the socket. The room disconnects and never starts a reconnect. It passed 7 of 7 runs. In these runs the close landed while no receive was pending, so the stream failed withENOTCONN. That is the leave-then-failure order this change makes universal.livekit-server --dev1.13.9:WebSocketLifecycleTests|TransportErrorMappingTests|WebSocketTests|RustTransportTests|SignalClientTests|TransportCloseTests|ReconnectTests|RoomTests|ParticipantTests, 47 tests in 11 suites, all pass.LiveKitCoreTeststarget againstlivekit-server --dev1.13.9, with CI'sdeparture_timeout: 1andenable_participant_data_blob: true: 595 tests in 109 suites, all pass.🤖 Generated with Claude Code