Wait for the data channel before every send by xianshijing-lk · Pull Request #1116 · livekit/client-sdk-swift

xianshijing-lk · 2026-09-08T18:55:07Z

Two independent defects that presented as one symptom: PeerConnectionSignalingTests.dataChannel timing out, which is what blocks the single-PC default flip in #1113.

1. Data sends were not gated on the channel being open (`49cc465d`)

ensurePublisherConnected() opened with a guard that returned early for any transport mode other than .subscriberPrimary:

// Only needed when subscriber is primary in dual PC mode
guard case .subscriberPrimary = _state.transport else { return }

So in single peer connection and publisher-primary modes the data channel open wait never ran. connect() returns once the primary transport is connected; the channels open afterwards on the SCTP association, so a send issued right after connect races them. Under .dropOldest a lossy write parked through that window resolves its waiter even when evicted, so the send reported success for a packet that never shipped.

The guard conflated two questions. Only the on-demand negotiation is specific to subscriber-primary; the channel wait applies to every mode. This splits them — the wait runs unconditionally, negotiation stays gated as before.

Not caused by the single-PC work: the guard read guard _state.isSubscriberPrimary else { return } before #919, which only reshaped it for the TransportMode enum. .publisherPrimary dual PC has always had the same hole.

How rust-sdks handles it, and how this aligns

rust has never had this bug and needed no fix for it. ensure_publisher_connected(kind) (rtc_session.rs:2415) resolves the channel for the requested kind on every call and polls until it reports open, with no transport-mode branch anywhere on the path:

let required_dc = self.data_channel(SignalTarget::Publisher, kind).unwrap();
if required_dc.state() == DataChannelState::Open { return Ok(()) }
while !self.publisher_pc.is_connected() || required_dc.state() != DataChannelState::Open {
    tokio::time::sleep(Duration::from_millis(50)).await;
}

The revision immediately before rust added single-PC support (8b3f35a7^) already had this function unconditional and per-kind — adding single PC did not require touching it. rust also keeps the two concerns separate the way this PR now does: negotiation gated on has_published, channel wait unconditional.

	rust-sdks	Swift (this PR)
gate runs in all transport modes	yes	yes
gate is per channel kind	`data_channel(Publisher, kind)`	`openCompleter(for: kind)`
kind comes from the packet	`publish_data(kind)`	`send(dataPacket:)` passes `packet.kind`
negotiation decoupled from the wait	`has_published`	`.subscriberPrimary` branch only

Per-kind rather than pair-wide because reliable and lossy are independent SCTP streams — waiting on both would let a lagging reliable channel stall or fail a send the lossy channel was ready to take. This is not academic here: the failing test publishes lossy, so a pair-wide gate would have been waiting on the wrong channel.

client-sdk-js is not a good reference for this. It gates from the same place (sendDataPacket → ensurePublisherConnected(kind), RTCEngine.ts:1574) but memoizes a single connection-scoped publisherConnectionPromise rather than one per kind, cleared only when the publisher PC goes closed/disconnected/failed. The first kind to call wins for the life of the connection, so a later send of the other kind awaits an already-resolved promise without its own channel ever being checked — the exact case this gate prevents. The docstring records this so the next reader doesn't "align" Swift back to it.

Two deliberate divergences from rust, flagged for review: the non-subscriber-primary branch waits on the channel opening but not additionally on the PC being connected (an open SCTP channel should already imply DTLS and ICE are up), and it doesn't kick negotiation from inside the gate, since the publisher is already negotiated at connect in those modes.

2. The test asserted delivery on a best-effort channel (`40d13d7b`)

dataChannel published with a default-constructed DataPublishOptions, whose reliable defaults to false, and then asserted the payload arrived. A lossy channel does not retransmit and does not guarantee delivery, so the test asserted a guarantee the transport does not make.

This — not the send race — is what actually made the test time out, and it was never single-PC specific. Dual PC failed too.

Verification

All against staging Cloud (1.13.6).

Stress, 12 runs of dataChannel per configuration:

configuration	pass rate
this branch, test as written (lossy)	5 / 12
this branch, test reliable	12 / 12

The send fix holds independently. send(dataPacket:) logs a diagnostic when it writes to a channel that is not open. Across all 24 stress runs on this branch it fired 0 times; on plain main it fires. So the packet is no longer handed to a closed channel, regardless of the test's reliability setting.

Full suite: PeerConnectionSignalingTests 9/9 passing, both modes, 52s.

Diagnosis method, in case it matters for review: temporary probes on the send path, on DataChannelPair.handle(received:) before any decode, and on server-opened channel routing. On every failure the pattern was identical — send issued with the channel open, and the receiving room logging no inbound data channel message at all. Loss in transit, not a decode, routing or dispatch problem. The probes are not part of this PR.

Not verified: swiftlint is not installed on this machine, so lint is unrun — CI is the first check.

One thing worth a separate conversation

A ~58% loss rate on a single lossy datagram sent right after connect is far above ordinary network loss. The most likely explanation is that the receiving side's data path is not warm at that instant and lossy has no queue to cover the gap. That is inherent to a best-effort channel and fine for applications that stream lossy data continuously, but it does mean "send one lossy packet immediately after connect and expect it to arrive" is unreliable by construction. Flagging rather than burying it under a now-green test.

`ensurePublisherConnected()` opened with a guard that returned early for any transport mode other than `.subscriberPrimary`, so in single peer connection and publisher-primary modes the data channel open wait never ran. `connect()` returns once the primary transport is connected; the channels open afterwards on the SCTP association, so a send issued right after connect races them. Under `.dropOldest` a lossy write parked through that window resolves its waiter even when evicted, so the send reported success for a packet that never shipped. The guard conflated two questions. Only the on-demand *negotiation* is specific to subscriber-primary; the channel wait applies to every mode. Split them: the wait now runs unconditionally and negotiation stays gated as before. Gate per channel kind rather than on the pair. The two are independent SCTP streams, so waiting on both would let a lagging reliable channel stall — or fail — a send the lossy channel was ready to take. `send(dataPacket:)` passes `packet.kind`, the same field `DataChannelPair.send` routes on, so the channel awaited is the one the packet is handed to. This matches rust-sdks, where `ensure_publisher_connected(kind)` resolves `data_channel(Publisher, kind)` on every call with no transport-mode branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`dataChannel` published with a default-constructed `DataPublishOptions`, whose `reliable` defaults to `false`, then asserted the payload arrived. A lossy channel is best-effort — no retransmission, no delivery guarantee — so the test asserted a guarantee the transport does not make, and failed whenever the single datagram was dropped. Measured against staging over 12 runs of the test: 5/12 passed as written, 12/12 with `reliable: true`. Failures were not specific to single PC — dual PC failed too — and on every failure the receiving room logged no inbound data channel message at all, confirming loss in transit rather than a decode or dispatch problem. Reliable is the correct channel for a delivery assertion. The lossy path keeps its coverage in the dedicated data channel suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The channel gate fired in every mode that is not subscriber-primary, which is the mode the mock-based tests run in — `RoomTestingOptions` defaults `canSubscribe` to false, so the server reports `subscriber_primary: false`. `MockDataChannelPair` replaces the whole pair and never sets channels, so nothing drives `handleStateChange` and the latch could never resolve: `RoomTests.sendDataPacket`, `publishDtmfSendsSipDtmfPacket` and most of `RpcClientTests` waited out the full timeout. The mock now reports itself open, which is what it is standing in for. Split the two concerns the gate had conflated. `ensurePublisherConnected()` goes back to transport-only, so the data-track publish path — which writes to its own channel and gates on its own latch — no longer picks up a dependency on the reliable channel through a default argument. `send(dataPacket:)` calls the new `ensureDataChannelReady(kind:)`, which awaits transport and channel concurrently. Switch over the whole transport enum rather than negating a pattern-match. `_state.transport` is optional and `cleanUpRTC` nils it, so `guard case .subscriberPrimary … else` folded "no transport" in with the publisher-primary modes, making a send during the full-reconnect window wait on a channel that cannot open. Re-arm the open latches when a channel leaves `.open`. libwebrtc closes the SCTP channel itself on a max-message-size violation, and a latch that only ever resolves would let the next send through to park in the drain — the same silent drop this change exists to stop. `rearm()` rather than `reset(throwing:)` so a flapping channel does not fail a send the reopened channel can take. Report the diagnostic per kind too, so a lossy send no longer logs an error because the reliable channel is still opening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The re-arm added in the previous commit made ordering matter: `handleStateChange` read `isOpen` outside the mutation, and `onStateChange` fires both from `setChannel` and from WebRTC's delegate thread, so a stale invocation could `resume` a latch that a newer close had already re-armed. The next send would then skip the gate and park in a closed drain — the silent drop this change exists to prevent. Read each channel's state inside the existing state lock, so the read and the latch update are one step. No new synchronization primitive. rust-sdks and client-sdk-js avoid this class structurally by polling live channel state every 50ms rather than holding a sticky latch; the lock is the proportionate fix for the latch design we have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`wait()` read the cached result under one `_lock.sync` and inserted the waiter under another. A `resume` landing between the two cached its result and found no waiter to hand it to, so the continuation waited out its full timeout on an already-resolved completer. The per-send data channel gate calls `wait()` on every send, which makes the window reachable on a hot path. Re-check `_result` inside the block that registers the entry, resuming the continuation directly on a hit. The fast-path read above it stays: the gate calls this per send, so the common already-resolved case should not allocate a continuation. The test is repetition-based rather than deterministic — the window sits between two lock acquisitions and there is no seam to interpose on without adding test-only machinery to the primitive. It fails reliably against the unfixed code with the timeout it predicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Only conflict was adjacency in CompleterTests: `resumeRacingRegistrationNeverStrands` (this branch) and `cancelRacingTimeoutSettles` (#1124) landed next to each other and are independent — both kept. The merge git did not flag: #1124 moved every `AsyncCompleter` resume outside `_lock`, while this branch's re-check resumes inside it. Those auto-merge silently into the lock-order inversion #1124 removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`ensurePublisherConnected` returns immediately when `_state.transport` is nil, but `ensureDataChannelReady` still awaited the channel latch, which has no cached result after `cleanUpRTC` reset it. A `publish(data:)` in the `cleanUpRTC`-to-JOIN window therefore waited the full `.defaultPublisherDataChannelOpen` (measured: 15.011s) and then reported `.timedOut`, which says nothing about why. Nothing can open a channel in that window, so turn the caller away instead. Racy by nature -- the transport can go away right after the check -- but a waiter already registered when it does is failed by `DataChannelPair.reset(throwing:)`, so the remaining window is one hop wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

#1124 moved every resume in this type outside `_lock`: resuming a continuation needs the task's status-record lock, and cancellation takes that lock before running `onCancel`, which takes `_lock`. Resuming under `_lock` inverts the order and wedges. The re-check added here resumed inside the lock, and merging main auto-merged the two without a conflict -- leaving `timeout` and `onCancel` outside the lock and this one inside it. Decide under the lock, carry the result out, resume after. The existing test did not cover the window it was written for: one waiter against one resume either has not started (and takes the cached result on the fast path) or has already registered, so removing the re-check entirely still passed. Fan out to 32 waiters per round so a single resume can land inside one of them -- that fails in under a second without the re-check. `resumeRacingCancellationSettles` covers the inversion itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`_publishDataTest` fires 100 concurrent `publish(data:)` with a default-constructed `DataPublishOptions`, whose `reliable` defaults to false, then asserts every topic arrives. A lossy channel does not retransmit, and under `.dropOldest` a starved drain evicts queued writes -- which is why this timed out in 4 of the last 5 Build & Test runs on #1124 while passing locally. Same call this PR already made for `PeerConnectionSignalingTests.dataChannel`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pblazej · 2026-09-16T09:33:44Z

@xianshijing-lk I'm trying to fix the remaining failures now 💣

Moved to draft as I revisit the root cause(s)

`handleStateChange` samples each drain's readiness and points the latches at it under `_state`, but `reset(throwing:)` cleared the drains and the completers outside that lock. A state callback could read a channel as open, lose the race to a concurrent reset, and then resolve a latch teardown had just cleared -- leaving the next send to skip the gate and park in a drain whose `.fail` event was already consumed, so it never settles. Safe to hold `_state` across those resets: the drain detaches its delegate before closing the channel, so nothing re-enters `handleStateChange`; both the drain and the completers settle continuations outside their own locks; and no cancellation handler takes `_state`. `set(maxMessageSize:)` stays outside -- it only yields onto each drain's event stream, and the `.fail`-then-`.configured` order is unchanged. Reported by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`cleanUpRTC` resets the channel latches before awaiting transport closure and retires the transport only afterwards, so `transport` stays non-nil across a blocking WebRTC teardown. A waiter registered in that window is not covered by the reset that already ran and sat out the full 15s timeout -- the case the previous guard missed. `disconnect()` moves to `.disconnecting` before any of that, so gating there closes the whole window. Also drops a regression the transport check would have introduced: a full reconnect clears the transport too, and a send in that window should park until the rebuilt channel opens rather than fail. `.reconnecting` now waits, as it did before this PR. Reported by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`awaitEvent()` parked on an unbounded `withCheckedContinuation` waiting for a delegate callback that rides on actual delivery. A single dropped packet hung the test forever, so the job hit its 30-minute cap with the log cut off mid-test instead of failing in seconds -- what `encryptionSurvivesFullReconnect` did on the macOS leg of the last run. Also removes a latent trap: both handlers captured the same continuation and each call chains the one before it, so two callbacks firing (data received *and* decryption error), or a second `awaitEvent`, resumed one continuation twice. `AsyncCompleter` tolerates that, and brings the timeout and cancellation handling with it. The chaining stays -- the `confirmation` blocks install their `confirm()` through it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

b62064e made `reset(throwing:)` clear the drains under `_state` so a state callback could not resurrect a latch teardown had just cleared. It held that lock across `DataChannelDrain.reset`, which detaches the channel's delegate -- a proxied `BlockingCall` onto WebRTC's signaling thread. If that thread is concurrently inside `dataChannelDidChangeState`, it is already waiting for `_state` in `handleStateChange`, and the two wait on each other: teardown never completes and the test process wedges until the job's 30 minute cap. Split the drain's teardown so the state transition and the WebRTC release are separate: `detachSendTarget(throwing:)` clears the send target and fails queued writes touching nothing in WebRTC, and returns the channel; `release(_:)` detaches the delegate and parks it. The pair now takes only the state transitions under `_state` -- both drains and all three latches, still atomic against `handleStateChange` -- and releases the two channels after dropping the lock. `readyState` is `BYPASS`, so `handleStateChange` sampling `isOpen` under the lock stays non-blocking; the delegate setter was the only blocking call on the path. Reported by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`reliableDelivery` asserted exact delivery of all 128 packets in every mode, including the reconnect ones. The SDK does not promise that: `startReconnect` escalates to a full reconnect when the resume does not land in time, and a full reconnect clears the publisher's replay set by design -- `ReliableStage.reset()` drops the retained writes along with the sequence counter they were stamped under, since writes from the old counter cannot be replayed into a session whose counter restarted. A packet already handed to `sendData` at that moment is lost, and its `send` has already returned success. That is why this failed under load on CI (86/128 on sender reconnect, 100/128 on dual-late) with no duplicate drops, no send error, and no replay-gap warning -- four negatives that only the escalation path explains. Ordering, dedup and the index range are still asserted in every mode; those are what a regression in the send path would break. `.none` keeps the exact check. Reconnect modes assert the property that does hold: the session recovers and delivers the final packet. The wait loop now waits on the last index rather than a full count, so a mode that legitimately drops the in-flight window no longer burns the whole 15s deadline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`reliableDelivery` ships ~3.9 MB of reliable payload (128 x 30,720 bytes) and then gave the receiver a flat 15s to drain it. On a loaded runner that is not enough: the visionOS leg received 0...98 contiguous and lost 99...127 with no reconnect at all, because the wait expired and `withRooms` tore the room down with those packets still in the transport. They are gone at that point -- `send` resolves when a write reaches `sendData`, not when it is delivered -- so the test failed for a reason that says nothing about the send path. The timeout now applies to *idle* rather than to the whole wait: any progress resets it. A slow drain finishes, a genuine stall still fails within the same window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pblazej · 2026-09-22T09:25:59Z

Closing in favor of #1131

xianshijing-lk requested review from hiroshihorie and pblazej as code owners September 8, 2026 18:55

This comment was marked as resolved.

Sign in to view