Repository navigation
Send a rejected signalling upgrade once: livekit-net's status comes from the validation endpoint - #1178
Draft
TimofeyIvanenko wants to merge 12 commits into
Draft
Send a rejected signalling upgrade once: livekit-net's status comes from the validation endpoint#1178TimofeyIvanenko 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
…comes from the validation endpoint
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.
Problem
#1173 moved the signalling WebSocket to Network.framework. A rejected upgrade then arrives as
ECONNABORTED, without its HTTP status. To keepTransportError.Http(status:)for livekit-net,WebSocket.upgradeStatussent the same upgrade a second time over a raw TCP/TLS connection and read the status line. That is a second rejected upgrade on every refusal, which no other SDK sends. It also carries the race Devin found in theAsyncCompleterit waits on: a reset after the status line can replace 401 with nil.Change
WebSockethas no probe and noWebSocketUpgradeFailureany more. A rejected upgrade is aLiveKitError(.network)that carries theECONNABORTEDPOSIX error.WsClientAdapter.connectasks the validation endpoint when the connect fails withECONNABORTED, asSignalClient.connectdoes after any failed connect:HTTP.requestValidationonUtils.toValidateUrl(url), with the bearer token from theAuthorizationheader, or none when the URL carriesaccess_token. A status from it becomes.Http(status:). Any other failure, or a validation that passes, keeps the mapping it had.HTTP.requestValidationtakes the token as optional. The 5xx and 429 errors now carry theirstatusCodeas well, so a rejected upgrade's 503 still reaches livekit-net as.Http(status: 503). The only reader ofstatusCode,isRetryableForRegionFailover, reads it on.validationonly, so nothing else changes.Tests
TransportErrorMappingTests: the twoWebSocketUpgradeFailurecases are gone. New: "A rejected upgrade's status comes from the validation endpoint" (the fake server rejects the upgrade, then the adapter reads 401 off the validation request) and "Any other failed connect asks the validation endpoint nothing".WebSocketLifecycleTests.rejectedUpgradeIsAnAbortreplacesrejectedUpgradeCarriesTheStatus.RustTransportTests"A rejected upgrade keeps its HTTP status" (through the FFI against livekit-server 1.13.9) passes unchanged.swift test --no-parallel --filter 'WebSocketLifecycleTests|TransportErrorMappingTests|WebSocketTests|RustTransportTests|SignalClientTests|RegionManagerTests|TransportCloseTests': 38 tests in 8 suites pass againstlivekit-server --dev.LiveKitCoreTeststarget againstlivekit-server --dev1.13.9, with CI'sdeparture_timeout: 1andenable_participant_data_blob: true: 596 tests in 108 suites, all pass.🤖 Generated with Claude Code