Conversation
`ensurePublisherConnected()` opened with `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 and never waits on the data
channels, which open on the SCTP association afterwards — so a send issued right
after connect races them.
That race is not benign. An ungated write lands in the drain's queue, and the
lossy channel's queue is drop-oldest with room for exactly one group: the next
submitter evicts it and settles its waiter with *success*. A burst of publishes
right after connect therefore collapses to its last packet, with every earlier
one reporting success — which is why this was invisible in the logs.
The open latch moves into `DataChannelDrain`, the object that owns the channel
and is its delegate, so one concept serves all three channels: `DataChannelPair`
exposes it per kind (the two are independent SCTP streams, so a lossy send must
not wait on the reliable channel), and `DataTracks` drops its hand-rolled
`_publisherChannelOpen` plus the `onStateChange` hook that fed it.
Also: `Room.send` turns away a send on a disconnected room instead of waiting out
the 15 s latch; `AsyncCompleter.wait` re-checks the cached result under the lock
that registers the waiter, so a `resume` landing between the fast-path read and
registration no longer strands the continuation; and the first discarded write on
a channel is logged, not only every hundredth.
Tests: the collapse and the gated burst are pinned at the drain, deterministically,
through the existing fake-channel seam. The e2e data tests stop asserting
guarantees their channel does not make — `PeerConnectionSignalingTests.dataChannel`
publishes reliably and gains a post-connect burst case on both signaling modes;
the data-track tests pace within the drop-oldest queue's one-frame capacity
(`DATA_TRACK_BUFFERED_AMOUNT_LOW_THRESHOLD` is 8 KiB by design, "data tracks
prefer dropping packets over queueing"), and `concurrentPush` asserts integrity
plus liveness after the burst rather than an arrival count that only measured how
loaded the runner was. Fixed sleeps in `DataStreamTests` and `TaskObserveTests`
become bounded waits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each of these failed for a reason that had nothing to do with what it covers. `LKObjCRoomHelper.connect` retries three times with a 10 s connect timeout and a 2 s gap — up to 34 s — while every ObjC test waited 30 s for it. One slow cold connect, the case the retry exists for, therefore blew the expectation before the retry could save it. The budget is now derived from the retry loop and used at the waits that are gated on a connect. `RepublishTracksTests` fed frames to resolve the capturer's dimensions, but `capture()` only enqueues onto the processing queue and drops frames while one is in flight, so the publish gate could wait out its whole `.defaultCaptureStart` budget for a value the test already knew. It now sets the dimensions directly — which is why only the video scenarios ever timed out. `DataTrackLifecycleTests` read `remoteParticipants` immediately after a publisher full reconnect, when the participant may not have been recreated yet, and paired two `MulticastDelegate` events inside 2 s. `ConcurrentCounter.wait(untilAtLeast:)` is a liveness check on work that rides a `.utility` task, so its default budget no longer doubles as a latency assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A connected room with no transport is the state the old gate fell through on:
`guard case .subscriberPrimary = _state.transport else { return }` is false for
both publisher-primary modes and for `nil`. The send must park there, on the
latch for the kind it is about to write.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`trackSurvivesPublisherFullReconnect` required an unpublish for the old SID whenever a publish arrived. But `remoteTrackUnpublished` resolves the publisher from `remoteParticipants` and skips tracks whose participant is gone — so when the SFU signals the publisher's brief departure, the app is told the participant disconnected and there is deliberately no per-track unpublish to pair with the republish. The pairing is now only required when the same participant object survived the reconnect, which is when it is actually owed. Also gives the send-gate test a budget that covers a `Room` construction and one task scheduling under a saturated pool, rather than one that doubled as a latency assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite fires 100 concurrent publishes and requires all 100 to arrive, but `DataPublishOptions.reliable` defaults to `false`. What it covers is the permission matrix (`canSubscribe` on and off), not the best-effort channel's delivery, which it has no standing to assert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`publishAndReceive` now waits for each frame before pushing the next, which removes the send-queue eviction the burst used to cause — but that made a single lost frame fatal, and CI (visionOS sim) lost all three 196 KiB frames outright. That channel is unordered and never retransmits, and a frame this size spans many packets, so losing one loses the frame. What the test covers is packetization, reassembly and integrity; retrying against a deadline keeps all of that while leaving the transport free to behave like the best-effort transport it is. A real regression still fails, because no attempt ever arrives. The 196 KiB-frame loss on simulator legs is a separate, pre-existing gap in the data-track path and is not addressed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sendParksOnItsOwnChannelWithNoTransport` watched the latch's waiter count from
another task, which measures whether the SDK's task has been *scheduled*. At the
tail of a full suite it may not be for tens of seconds: instrumenting it showed
the send unstarted after 30 s on a room still `.connected`. It was the only new
red on CI, on three legs at once.
The gate is now raced against a sleep inside one task group. A gate that returns
early beats the sleep and fails — verified by restoring the old
`guard case .subscriberPrimary else { return }`, which turns it red in 0.064 s —
while a starved one loses to the sleep, so slowness cannot manufacture a red.
That it waits on *this kind's* channel is `openLatchesAreDistinctPerKind`.
Also moves the xcode-27 leg to the iPhone 18 Pro simulator: the iPhone 17 Pro
runtime went missing from that image on 2026-09-18, and the job now dies in two
minutes with "Unable to find a device matching the provided destination
specifier". It last passed there on 2026-09-17.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite's single 0.1 s `timeout` is sized for the cases that are *supposed* to time out, so those tests fail fast. The success cases inherited it, which made them assert how quickly the SFU's "participant active" signal lands rather than that it lands at all — `waitUntilAllActiveSuccess` timed out on the strict-pool leg. The two budgets are now separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 196 KiB frame is thirteen packets at the pipeline's 16 KB MTU on an unordered, zero-retransmit channel, and on some simulator legs it is lost outright however it is paced — the first-drop log added here proves the sender discarded nothing during the whole window, so the loss is downstream of the send path. Retrying around it made things worse, because a timed-out `next(within:)` cannot cancel the UniFFI read under it and every attempt left another read on the stream. The scenario drops to 64 KiB, which is still five packets of packetization and reassembly and is a size the transport actually delivers, and the read goes back to one attempt per frame. `setPipelineOptions` keeps its retry but is capped at three attempts rather than run against a deadline, for the same reason. The liveness budgets that CI outran go up too: `ConcurrentCounter.wait` and `ManualSleeper.waitForParked` to 60 s, `TestObserver.waitForItems` to 30 s. All three wait on work that rides a `.utility` task, and the visionOS leg where they failed was logging `HALC_ProxyIOContext: skipping cycle due to overload`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pblazej
requested review from
hiroshihorie and
xianshijing-lk
as code owners
September 18, 2026 10:21
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Both predate this PR, but it makes the latch load-bearing for every send, so both are worth closing here. `dataChannelDidChangeState` validated the channel's identity and published the latch in separate critical sections, so a teardown or a swap landing between them let a superseded channel's callback resolve a latch the replacement had not earned — and a send crossing one of those parks in a drain with no channel, where the next one evicts it and reports success. The drain now publishes the state of whatever it is sending on *now*, decided under `_state`, so whichever order the two arrive in, the one that takes the lock last publishes what is actually true. `AsyncCompleter.wait` registered its continuation after `onCancel` had already run: `withTaskCancellationHandler` invokes the handler immediately for a task that is already cancelled, so it looked for an entry that did not exist yet and never ran again, and the caller sat out its full timeout and threw `.timedOut`. `Task.isCancelled` is now checked inside the registration lock, with the failure carried out and resumed outside it. Both regression tests were verified in both directions: red before the fix (the cancelled wait took 30 s and threw the wrong error; the stale callback reopened the gate), green after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`trackSurvivesPublisherFullReconnect` paired a republish with an unpublish for the old SID. But the SID is reassigned *in place* on the surviving track — which is the thing the test exists to check — while `remoteTrackUnpublished` matches by `info.sid`, so once the rotation has landed an unpublish for the old SID finds nothing to match and is dropped. The app loses nothing by that: it is holding the same track object, and its SID moved. The clause went red three times for three different reasons, which is the clearest evidence that it is not an invariant the SDK provides; worth a look from whoever owns data-track lifecycle semantics, but not something a test should assert meanwhile. The two reliable-delivery waits that CI outran go from 15 s to 30 s. `PreConnectAudioBuffer` sends from an unstructured `Task` in `didPublishTrack` that first awaits an active agent, and the data-stream handler runs on its own task, so both measure scheduling latency on a strict-pool runner rather than whether the bytes arrive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`setPipelineOptionsReassemblesMultiPacketFrames` required a 32 KB frame to reassemble under the *clamped* `maxPartialFrames`, and on a loaded simulator leg all three attempts lost it outright while the sender's drop counter showed nothing discarded — so the loss is in the data-track receive path, not in anything this test configures. Reassembly is now checked with the value set before `subscribe()`, and the clamp is covered separately: zero is accepted and the pipeline keeps delivering. Both halves of what the test is named for still hold; what is gone is the dependency on a receive-path gap that deserves its own investigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The data-track "delivery failures" were a reading bug, not a delivery bug.
UniFFI's generated async has no cancellation path at all — `uniffiRustCallAsync`
is a bare poll loop, no `withTaskCancellationHandler`, no `rust_future_cancel` —
and the Rust wrapper holds a mutex across the await:
pub struct DataTrackStream(Mutex<livekit_datatrack::api::DataTrackStream>);
pub async fn next(&self) -> Option<DataTrackFrame> {
self.0.lock().await.next().await.map(Into::into) // TODO: avoid mutex?
}
So the bounded `next(within:)` helper, which abandons its read on timeout, left
that mutex held until a frame happened to arrive, and every later read blocked
behind it. One lost frame wedged the stream for the rest of the test, which is
why `setPipelineOptions` failed on a 32 KB frame *and* on a 64-byte one, why
retrying made `publishAndReceive` worse, and why the sender's drop counter kept
showing nothing discarded.
Tests that read more than once now go through a `DataTrackReader` that owns the
stream's only `next()` caller and buffers what it reads. `firstFrame(within:)`
remains for the single-read cases, including the one that must *not* drain
(`subscribeDropsOldestAtCapacity` is about the subscription buffer's own
eviction). Both live in the test target rather than the support module: every
consumer is in `LiveKitCoreTests`, which already imports `@testable`, and
`LiveKitTestSupport` is not a product so nothing outside the package can see it.
`for await` escapes the wedge but not the missing cancellation — it parks in the
same `next()` — so the reader's consumer ends when the stream does, at teardown.
Worth raising upstream: the mutex is already marked `TODO: avoid mutex?`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses the two open review comments and the two review passes. **Disconnect race (review).** `ensureDataChannelReady` and the submission that follows it are separate steps, so a disconnect landing between them left the write in a drain that would never attach another channel or emit another `.fail` — the caller parked forever. The drain now distinguishes "no channel yet", where a write correctly waits, from "no channel ever again", where it must not: a `wasReset` mirror on `LoopState`, set on `.fail` and cleared on `.attached`, both riding the same FIFO stream as the writes they govern. The check sits between `prepare` and `makeWrites`. Later, the continuation belongs to a `SendToken` on the last write and resuming it here too is a double resume — which is exactly what the first attempt did, and what the full suite caught (`SWIFT TASK CONTINUATION MISUSE`). Earlier, it would also turn away the empty submissions callers use to order themselves behind work in flight. **Reconnect waiters (review).** Not changed: `cleanUpRTC` resets for both a disconnect and a full reconnect, and `.fail` settles the queued writes in both, so preserving a gate waiter while failing the writes behind it would only move where the caller finds out. `main` behaves the same; only the comment on `ensureDataChannelReady` claimed otherwise, and it now states the real rule. **Test review.** `setPipelineOptionsReassemblesMultiPacketFrames` requires reassembly under the clamped `maxPartialFrames` again: I had weakened it on the theory that the receive path was losing frames, when the reads were wedging the stream. The retries could not have worked before; they can now. The `largeFrames` comment no longer blames that phantom gap either. **Simplification.** `DataChannelPair.isOpen(kind:)` is gone — one caller, a log the gate two lines above made tautological, and it forced an override in `MockDataChannelPair`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Review asked for `.closed` to be a terminal loop event. It cannot be: a close followed by `setChannel` is the ordinary fast-reconnect sequence, and `.park` keeping its queue across that swap is what makes the resume lossless. The drain cannot tell "replacement coming" from "closed for good" — only the owner can, and `reset` is how it says so. The guard now says that, and names the one case that does leak: a close followed by neither, which `enqueue`'s max-message-size check guards against and `dataChannelDidChangeState` logs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two CI failures, both older than this PR. `TaskObserveTests` discarded what `subscribe` hands back. That is an `AnyTaskCancellable`, which cancels its task from `deinit`, so `_ = stream.subscribe(…)` cancelled the subscription on the spot and whether any element was processed became a race — on the failing leg the observer recorded nothing at all in 30 s. Three of the four tests did this; they now hold the handle, which is also why the suite drops from 30 s to 0.13 s. Polling for the items (added earlier) was treating the symptom. `WebSocket.send` used the synthesized async overload of `NSURLSessionWebSocketTask.send`, whose continuation bridge carries a TSan-visible data race; it aborted the whole bundle on the TSan leg, which then restarted and reported 133 of 438 tests. `next()` was already converted to the callback form for exactly this reason, and AGENTS.md requires it until the compiler floor reaches Swift 6.3. The same race is in the Sep-15 logs on another branch. Also records, at the `dispatch` guard, why a closed channel does not fail its queue: a close followed by `setChannel` is the ordinary fast-reconnect sequence, and `.park` keeping its queue across that swap is what makes the resume lossless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`trackSurvivesPublisherFullReconnect` asserted that the subscriber's existing `RemoteDataTrack` carries over with its SID reassigned in place. That is only one of two legitimate outcomes, and which one happens depends on whether the SFU signals the publisher's brief departure: if it does, the participant is dropped and recreated, and so is its track — leaving the test holding an orphaned object. CI caught exactly that, with a new track at a different address and a SID that never rotated on the captured one. It now reads through the participant, asserts what holds either way — one live track under the same name, a rotated SID, no lingering second publication — and checks object identity only when the participant itself survived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`publishDuringFullReconnect` publishes inside the teardown window, so the
publication is still pending when the reconnect republishes — and the Rust
manager answers a pending descriptor with `PublishError::Disconnected` outright:
// livekit-datatrack/src/local/manager.rs, on_republish_tracks
Descriptor::Pending(result_tx) => {
// TODO: support republish for pending publications
_ = result_tx.send(Err(PublishError::Disconnected));
}
Whether the publish lands before that runs is a race the SFU's response time
decides, and a loaded sanitizer leg loses it. Retried, with the upstream TODO
named. What the test is for — that the publish waits for the rebuilt channel
rather than failing on the dead one — still holds, and a regression there fails
every attempt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`trackSurvivesPublisherFullReconnect` kept pinning one combination of two independent facts: whether the subscriber's `RemoteDataTrack` is reassigned in place or replaced, and whether the participant object is recreated. The first version required the carried-over track; the second paired track identity with participant identity. CI found a third combination — participant kept, track replaced — so that pairing was wrong too. `remoteTrackUnpublished`/`remoteTrackPublished` swap the track independently of the participant, so neither identity is guaranteed. What a publisher's full reconnect does promise is the publication: a track under the same name, reachable from the current participant, carrying a new SID, with nothing left over. That is all it asserts now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A freshly booted simulator can take most of a minute before loopback traffic reaches the host. On the xcode-27 iPhone 18 Pro leg the Objective-C suite runs first, and its first connecting test started 52 s before the SFU logged a single request from that simulator; all three connect attempts died with `Validation request failed: The request timed out`, and the test's remaining assertions failed downstream of that. The Core suite on the same leg, which ran later, was 438/438. `TestEnvironment.serverReady` probes the server over HTTP once per process, for up to 90 s, and both harnesses await it before their first connect. A missing server still fails where it always did, just 90 s later. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The re-subscribe timeout on the sanitizer leg was not slowness, and the retry added for it could never have worked. livekit-server 1.13.6 reconciles a data-track unsubscribe asynchronously and then deletes the subscription entry (`reconcileDataTrackSubscription`); a subscribe that lands in between flips the doomed entry's desired flag and is deleted with it. CI's server log shows exactly that: `subscribing to data track` 140 ms after `data down track closed`, no `executing subscribe` ever, and an empty `dataTrackSubscriberHandles` sent right after — the notify that follows the delete. The signal path was healthy throughout. On the client, `on_subscribe_request` folds a repeated `subscribe()` into the existing `SubscriptionState::Pending` and sends nothing, so `Task.retrying` around it only waited out three more 10 s timeouts. Distance from the unsubscribe is the only thing that helps: the gap goes from 500 ms (267 ms as seen by the SFU under TSan) to 2 s, and the retry goes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`concurrentPush(largeFrames)` timed out on its lone post-burst frame twice on the visionOS 26.5 leg. The SFU's stats for that track — 10 packets in, 2 frames, 35 lost, on a 45-packet track — prove the marker's packets reached the SFU, and no downlink drop was logged for them, so the frame vanished on the slow subscriber after being forwarded. A single frame on this channel is not a guarantee anyone offers: the SFU queues at most 8 KiB per subscriber, nothing retransmits, and the receiver's own packet channel is `mpsc::channel(16)` with `try_send`. The liveness check now re-pushes the marker on every poll tick, so what it asserts is what the channel actually promises — that it delivers again once the burst is over — and only a channel that stays dead fails it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`startIfStoppedReArmsAfterCancel` failed after 72 s on an overloaded visionOS simulator: a 30 s `waitForParked` gave up silently, the `tickAll()` after it released nothing, and the counter assertion 30 s later blamed the timer. The loop rides a `.utility` detached task, which is the first thing a loaded host starves. `waitForParked` now records an issue when it times out, so a recurrence names the starvation instead of the counter. The `setPipelineOptions` retry comment said the channel might lose a packet; the server log says who loses it. The SFU keeps at most max(8 KiB, 100 ms at the measured bitrate) queued per subscriber and logged `data dropped due to high buffered amount: buffered amount 16920, min buffered amount 8192` for this very frame when a sanitizer-slow subscriber was late acknowledging the first packet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
xcodebuild rejects the `var locals` array captured in the `@Sendable` poll closure (`reference to captured var in concurrently-executing code`), where the SwiftPM build of the same file only warned. Bind the one track the closure needs to a `let` first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The readiness probe printed and carried on when the server did not answer,
leaving each test to discover that through its own connect timeouts. It now
throws `ServerUnreachable` ("Test server at <url> did not answer within 90 s"):
Swift Testing records it as the test's issue at the `withRooms` call, and the
Objective-C helper hands it to its completion handler. The probe is a one-shot
task, so only the first test waits the 90 s; every later connect rethrows the
stored error at once.
Two review points, both taken. Only the server from `LIVEKIT_TESTING_URL` is
probed — a fixture naming its own URL is connecting somewhere this harness does
not manage, possibly one it expects to fail against, so it goes straight to its
connect. And `connectTimeout` now includes the probe's budget, so a server that
never answers reports through the completion handler before the caller's
expectation expires; verified against a dead port, where the connect assertion
receives the message and the Swift test records it after 90.1 s.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The burst regression test published reliably, and the reliable drain parks rather than evicts: before the fix an ungated reliable write waited in the queue and shipped on `.open`, so the test would have passed on main and its docstring described a channel it did not use. Only the lossy and data-track drains are drop-oldest, and after this PR no end-to-end test published lossy data at all. The test now runs over both kinds. Reliable still owes every packet; lossy asserts a floor of 20 of 25, which best-effort delivery clears easily (the payloads total well under the 8 KiB the SFU queues per subscriber before it drops unreliable data) and the defect, which delivers one, cannot. Two smaller things from the same review pass. The reliable reconnect modes asserted only order, uniqueness and the final index, so a channel that dropped most of a stream and then recovered would have passed; they now also require at least half the stream, which a reconnect's in-flight window plus the SFU's 100 KB / 2 s replay cache cannot come close to losing at this rate. And the four identical "latch is still armed" expectations in the open-latch suite share one helper that reports at the caller's line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The macOS TSan leg went red on a run where every test passed: ThreadSanitizer
reported a data race inside UniFFI's generated `uniffiRustCallAsync`, between the
Rust thread resuming a Swift continuation in `uniffiFutureContinuationCallback`
and the resumed task's first write to its own frame, and a report aborts the test
host ("Restarting after unexpected exit, crash, or test timeout"). TSan cannot see
the happens-before edge the Swift runtime's continuation handoff provides when
the resuming thread is a tokio thread it has no Swift-concurrency instrumentation
for. The code is generated by uniffi-rs and shipped in the xcframework, so the
suppression is scoped to that one callback and anything else TSan finds still
fails the leg. `TSAN_OPTIONS` reaches the macOS test host the same way the strict
cooperative-pool variable already does.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… lose Three reds on one run, each traced through the server log to something outside the SDK, each answered with a bounded retry of a request that is terminal when it fails, so a second attempt is a fresh one. `publishAndReceive(largeFrames)` lost one 64 KiB frame. The SFU's stats for the track read 13 packets in, one gap, no downlink drop: the packet vanished between the publisher and the SFU on an unreliable channel that never retransmits. Each frame now gets up to three pushes; one intact arrival per frame is what proves packetization and reassembly, and a frame that never arrives still fails. `withPublishedDataTrack` timed out publishing. The SFU received the request, sat in `could not send signal message: request timed out` for four seconds, and answered twelve seconds after the request was sent, past the Rust side's hard-coded 10 s `PUBLISH_TIMEOUT`. Its per-participant signal loop handles inbound requests behind outbound sends. The fixture retries the publish once, which hardens every test built on it. `RpcObjCTests.testRegisterAndCallRpc` got `RpcError 1501` with a 7 s round-trip budget, on the freshly booted iPhone 18 Pro simulator whose data path was still starved a minute after the same run's server log shows it cancelling joins. The call is made twice at most. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ps dropping Two reds on the last run, both now understood from the artifacts. `publishAndReceive(largeFrames)` lost the same frame three times, ten seconds apart. The SFU's log shows why: seven downlink drops for that track, `data dropped due to high buffered amount: buffered amount 22720, min buffered amount 8192`. It queues at most 8 KiB per subscriber, so whenever a packet reaches it while the subscriber has not yet acknowledged the previous one, that packet is gone, and a five-packet frame needs four such acknowledgements in a row. To a slow subscriber that is lost more often than not, so three expensive attempts were the wrong shape. Each frame is now re-pushed every half second for up to 15 s; one intact arrival proves packetization, forwarding, reassembly and decryption, and a frame that never lands in that window still fails. `concurrentArmingLeavesSingleLoop` never saw its `.utility` loop fire in 60 s on a freshly booted visionOS simulator where the neighbouring test had just taken 4 s instead of 0.02 s. It was the suite's one real-time test, inferring "one loop" from a fire count against a 3× bound. It now runs on the sleeper and measures the property exactly: every release fires the timer block once. Loops that lost the arming race were cancelled, some after parking, and a parked countdown does not wake on cancellation, so they sit until released and then exit without firing; that is why fires per release, not parked countdowns, is what gets counted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`stopRecording(flush:)` stopped the recorder first and took the audio stream out of state second. `flushDiscardsBufferSilently` waits for `isRecording` to drop and then sends, which is also the order a real flush-versus-publish race takes, and in the gap between the two steps a send still finds the stream, claims it and carries it to `streamBytes` on a room that may not be connected. On main that send parked in a drain with no channel; with the send gate it fails fast with `Room is not connected`, which is how a visionOS leg surfaced it. The docstring promises silence, so the flush now takes the stream before `stop()`, and a send racing it has nothing to claim. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`defineAndGetSchema` timed out on a leg whose SFU was answering the participant's other requests but not its `storeDataBlobRequest`: the server log shows the store request received, room and connection-quality updates flowing to that participant for the next five seconds, and no store response before the client's fixed budget ran out and it left. Storing a blob is idempotent, so the definition is stored with one retry; a second request is a fresh attempt at the same thing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`awaitEvent()` parked the test on an unbounded continuation until a delegate callback arrived. On one leg the sender's ping timed out right after the send, the SDK went into a quick reconnect the SFU accepted, and the callback never came; the wait then held the test host for the rest of the job's 30-minute budget. It is a 30 s completer now, so a packet that never arrives fails the assertion that follows instead of the whole leg. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The iPhone 18 Pro leg keeps timing out right after WebSocketTests, the last suite in LiveKitCoreTests, with every test green. The stall dumps from one such job show both halves of it. At five minutes into the test step the only test host alive was still in `_dyld_start`: the cold iOS 27 runtime took almost nine minutes to launch the first host at all, against five and a half on the green run of the same leg. Then, after the Core bundle finished, xcodebuild sat in `XCTHRunDestinationAllocator.collectSimulatorDiagnostics` on a semaphore wait whose wall-clock limit is the `Timed out after 600.0 seconds` this leg has logged before, and the step's 30 minutes ran out before that limit did. Neither half is ours to fix in xcodebuild, but the first half decides whether the second one costs the leg. Booting the simulator in its own step, waiting for `bootstatus`, and spawning one process to warm the runtime moves the cold start out of the test step's budget. Simulator legs only; the destination string is resolved to a device the same way xcodebuild resolves it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…angs on Two more findings from the stall dump behind the leg that times out right after WebSocketTests. A `name=` destination lets xcodebuild resolve a different clone than the one the boot step warmed, so the boot step now exports the device's udid and the test step targets `id=` for simulator legs, which is how the simulator actions in the ecosystem pin the device they booted. And the hang itself has a switch. xcodebuild was stopped in `collectSimulatorDiagnostics` on a semaphore wait: after a bundle it gathers a sysdiagnose from the simulator, with a 600 s ceiling that this simulator reaches. `-collect-test-diagnostics never` turns that collection off; the artifacts this workflow relies on — the xcodebuild log, the server log and the stall dumps — are unaffected, and the ten minutes come back to the test budget. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`.e2eTimeLimit`, applied at suite level to every `.e2e` suite, so each of the suite's test cases must finish within five minutes. Generous by design: the slowest single case seen on the slowest legs runs about 80 s, and a case that passes on a degraded runner can also absorb the one-off 90 s server readiness wait and 36 s of connect retries. What the limit exists for is the hang — a wait nothing can resume — which otherwise costs a leg the whole of its step budget. Swift Testing cancels the test's task when the limit expires, so waits that honor cancellation end there; the two known uncancellable waits, UniFFI's `next()` and a wedged cooperative pool, are documented where they occur and are outside what a time limit can reach. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`TimeLimitTrait` needs iOS 16 and the package deploys to iOS 13, which the iOS legs reported the moment the trait appeared in a `@Suite`. The traits a suite declares are expressions evaluated at runtime, so `TestLimits.e2e` now returns the built-in `.timeLimit(.minutes(5))` inside `if #available(iOS 16, macOS 13, tvOS 16, visionOS 1, *)` and an inert empty tag list otherwise. Every host CI runs on gets the limit; a host too old for the trait gets nothing rather than a compile error. Verified by compiling the Core tests for the iOS 27 simulator and running a suite there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Pre-booting moved the simulator's cold start off the test host's launch, which now takes seconds instead of minutes, but not out of the timed step: on both xcode-27 simulator legs the build phase grew from about two and a half minutes to eight and ten, because the runtime keeps working after `bootstatus` reports the boot finished. Measured locally, the simulator's processes ran at three to twelve hundred percent aggregate CPU for seventy seconds past that point before going quiet. The boot step now waits for the direct children of `launchd_sim` to stay under 20% CPU for three consecutive samples, bounded at five minutes inside the step's own timeout, so that work finishes before the build competes with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The visionOS 27 leg of an otherwise green run died at package resolution: GitHub's release CDN answered the LiveKitWebRTC xcframework download with a 504 and xcodebuild exited 74 before building anything. A transient 5xx from a release asset is the one kind of failure a retry is the right answer to, so the packages are now resolved in their own step, up to three times, before the simulator boot and the timed test step. The boot step's own budget also goes from 15 to 20 minutes. Its longest observed run so far is thirteen and a half, on a visionOS 27 simulator sharing a runner with seventeen other legs, and a boot that overruns must not fail a leg whose tests would have passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Build & Test keeps its 30 minutes. Resolve Packages drops from 15 to 5, which one to two minutes of resolution and three attempts fit comfortably. Boot Simulator drops from 20 to 10 with the settle wait capped at two minutes instead of five: the boot itself has taken five to eight minutes on the xcode-27 and visionOS legs, so five minutes for the whole step would fail them, and the settle wait's later minutes bought little once the first two had passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| - name: Boot Simulator | ||
| id: boot | ||
| if: contains(matrix.platform, 'Simulator') | ||
| timeout-minutes: 10 |
Contributor
There was a problem hiding this comment.
🟡 Simulator settling exceeds step budget
When booting takes eight minutes, Boot Simulator leaves less than the full two-minute settle loop. The timeout fails healthy simulator legs before tests begin.
Learn more
The step performs simulator lookup and boot before running a settle loop with 60 two-second sleeps. An eight-minute boot plus the complete loop already consumes ten minutes, excluding command overhead. GitHub Actions terminates the step at its timeout instead of allowing the following test step to run.
Example: An xcode-27 simulator reaches bootstatus after eight minutes and remains above 20% CPU for two minutes. The step crosses its ten-minute limit during the loop, although the simulator successfully booted.
Recommended fix: Give Boot Simulator enough headroom beyond the observed eight-minute boot and the two-minute settle cap. A 12-minute timeout preserves the bounded step while covering setup and command overhead.
Suggested change
| timeout-minutes: 10 | |
| timeout-minutes: 12 |
Was this helpful? React with 👍 or 👎 to provide feedback.
pblazej
changed the title
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
1 participant
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.
Alternative to #1116, which diagnosed the same guard.
The defect
ensurePublisherConnected()opened withguard case .subscriberPrimary = _state.transport else { return }, so the data channel open wait never ran in single peer connection or publisher-primary modes — the modes #1113 makes the default. The lossy queue is drop-oldest with room for exactly one group, so a write handed to a channel that is still opening is evicted by the next one and its waiter is settled with.success, which is why a month of CI logs showed loss with no error anywhere. Reproduced 3/3 on an idle Mac with a probe inDataChannelDrain.enqueue.Approach, and how it differs from #1116
#1116 adds two
AsyncCompleters plusopenCompleter(for:)toDataChannelPair, re-armed under the pair's state lock. This puts the latch onDataChannelDraininstead — the object that owns the channel and already is itsLKRTCDataChannelDelegate, so it sees every state transition first and nothing downstream has to keep a copy in step.graph LR subgraph before["before — three mechanisms"] RS1["Room.send"] --> PC["pair.openCompleter<br/><i>both channels, any kind</i>"] DT1["DataTracks.publish"] --> DPC["_publisherChannelOpen<br/><i>hand-rolled</i>"] end subgraph after["after — one latch, owned by the channel"] RS2["Room.send"] --> PK["pair.whenOpen(kind:)"] --> RW["drain.whenOpen"] DT2["DataTracks.publish"] --> TW["drain.whenOpen"] endSo it deletes more than it adds — +149/−84 in
Sourcesagainst #1116's +189/−35 — and it covers the_data_trackchannel, which #1116 leaves untouched. Per kind rather than pair-wide because the two are independent SCTP streams, so a lagging reliable channel must not fail a lossy send.A second root cause, found in the Rust/UniFFI side
The data-track "delivery failures" turned out to be a reading bug, not a delivery one.
livekit-uniffi'sDataTrackStream::nextholds aMutexacross the await, and UniFFI's generated async has no cancellation path at all (uniffiRustCallAsyncis a bare poll loop — zerorust_future_cancel). A bounded read that gives up therefore leaves that mutex held until a frame happens to arrive, and every later read blocks behind it: one lost frame wedges the stream. That is whysetPipelineOptionsfailed on a 32 KB frame and a 64-byte one, why retrying madepublishAndReceiveworse, and why the sender's drop counter kept showing nothing discarded. Tests now read through a reader that owns the stream's singlenext()caller; worth fixing upstream, where the mutex is already markedTODO: avoid mutex?.What the SFU log added
Two remaining data-track flakes were server-side, read off the
server-logartifacts (the SFU'stsis the only reliable clock; xcodebuild flushes its output in bursts). A data-track subscribe that lands while the previous unsubscribe is still being reconciled is accepted onto the entry the reconcile then deletes, so it is logged but never executed — and the Rust manager folds a repeatedsubscribe()into the same pending state, so retrying cannot recover it; the test now leaves the SFU time instead. The SFU also drops a packet toward any subscriber holding more than 8 KiB unacknowledged (data dropped due to high buffered amount), which is what loses a two-packet frame to a sanitizer-slow receiver, and why a lone post-burst frame is not something the stress test may demand.Verification
Full
LiveKitCoreTestsgreen locally againstlivekit-server --dev— 410/410, including underLIBDISPATCH_COOPERATIVE_POOL_STRICT=1(pool width 1), which is where most of these surfaced. A sweep of 45 failing runs / 111 test legs over the preceding ten days ranked the pre-existing flakes; every one in the top ten was asserting something other than what it covers, and each is addressed here. Thexcode-27image has also lost its iPhone 17 Pro runtime, so that leg moves to iPhone 18 Pro — and a freshly booted iOS 27 simulator has no loopback for most of a minute, which the Objective-C suite used to run straight into, so both harnesses now wait for the server to answer before the first connect and fail withServerUnreachableif it never does. Simulator legs now boot, wait for the runtime to settle, and pin xcodebuild to that device before the timed test step, with the post-bundle simulator diagnostics collection (a 600 s hang on these runners) switched off, and every.e2esuite carries a five-minute per-case time limit. The head commit has three consecutive full runs green, 18/18 Build & Test legs each; the only reds in the last eight runs were GitHub's release CDN answering 504 to a dependency download, re-run green.