Skip to content

Fix CI flakiness: data-stream races, QoS inversions, deterministic tests - #1071

Open
pblazej wants to merge 34 commits into
mainfrom
blaze/ci-flaky
Open

Fix CI flakiness: data-stream races, QoS inversions, deterministic tests#1071
pblazej wants to merge 34 commits into
mainfrom
blaze/ci-flaky

Conversation

@pblazej

@pblazej pblazej commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Deep investigation of CI flakiness (61 failed runs sampled across Jun–Jul, plus per-run test/server log capture). Every fix below is root-caused from CI evidence; several turned out to be production bugs rather than test issues.

Production code changes

IncomingStreamManager — dropped and reordered incoming data streams
Senders reuse a stream ID for consecutive streams (transcription does); descriptor cleanup ran in a per-reader task that raced the reopening header and silently dropped the new stream (confirmed 2 of 5 in CI). Cleanup is now synchronous in the trailer path and generation-guarded so a stale cleanup can never remove a successor. Cross-stream handler ordering was also a scheduler race (one detached task per stream); topics registered with ordered: true now run handlers through a per-topic FIFO — opt-in, used by the transcription receiver.

AsyncCompleter — timeout timers could be deferred indefinitely
Timeout blocks ran on a .background queue, the QoS tier the system may defer without bound under CPU pressure. A waiter whose only resume path is the timeout then never wakes: two CI legs burned full 30-minute attempts hung this way, and RPC callers saw 1501 instead of 1502 when the 0.05 s timer lost to a 2 s watchdog. Now .utility.

DeviceManager — same QoS inversion on camera start
Device-discovery KVO registration ran on the .background global queue and is the only resumer of the completers CameraCapturer.startCapture awaits. Now .utility, matching the completer timers.

AsyncTimer — injectable sleep
Internal sleep: parameter (defaulted, all call sites unchanged) so timing tests drive ticks deterministically instead of asserting against the scheduler.

SwiftLint: no_background_qos — errors on any .background queue QoS or task priority to keep the inversion class out.

Test changes

withRooms teardown on every exit path — a thrown body previously skipped disconnect(), leaking live Rooms (WebSocket, peer connections, timers) into every later suite in the process; this was the amplifier that turned one flake into a run-wide cascade.

Deterministic waits instead of fixed sleeps — AsyncTimer tests drive ticks via the injected sleep (ManualSleeper); transcription tests wait for expected update counts; reliable-data tests wait for delivery inside withRooms (waiting after teardown lost in-flight packets: 56–127 of 128 delivered).

RPC tests inject via the awaited afterPublish hook — unstructured Task {} injection raced response timeouts under load (1502 flakes).

ObjC tests retry connects (like withRooms always has) — the first Room in a fresh process pays one-time WebRTC/audio init that can exceed the connect timeout on simulators (testSendFile, 40 historical failures).

Buffer-track tests resolve dimensions directly — captured frames resolve dimensions on the capture queue, which lags past the publish timeout under load.

WebSocketTests disconnect cancelled connects — a cancelled connect can still complete the server-side join; 21 leaked participants per run otherwise.

New regression tests — stream-ID reuse delivery and ordered-topic FIFO (unit-level, drive the manager's event loop directly; the reuse test reproduces the drop deterministically without the fix), transcription burst-ordering, AsyncTimer semantics.

Guard against process aborts — transcription validation indexed past #expect count mismatches, crashing the whole test process (52 aborts historically).

CI changes

  • Job-level retries replaced with in-run test retries: the nick-fields/retry wrapper masked every flake above behind full rebuild-and-rerun attempts, with failures buried in superseded logs. Now -retry-tests-on-failure -test-iterations 2 re-runs failures inside the same xcodebuild invocation (XCTest re-runs only the failed cases; Swift Testing runs a second iteration) — a flaky leg costs one extra test pass, and per-iteration results stay visible in the log and xcresult.
  • Failure artifacts: raw xcodebuild output (xcbeautify swallows parameterized-test failure details) and the dev-server log (close reasons prove client vs server fault) upload on failure.
  • Xcode 26.6 on macos-26 legs; new xcode-27 preview legs (macOS, Catalyst, iOS/tvOS/visionOS 27.0 sims).

@github-actions

Copy link
Copy Markdown

⚠️ This PR does not contain any files in the .changes directory.

pblazej and others added 28 commits July 30, 2026 12:32
This reverts commit f2ab598. The larger runners made no difference to the
flake rate — the failures are scheduling- and teardown-related, not
throughput-related.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`withRooms` only disconnected its rooms on the success path. A `Room` keeps
itself alive through its signaling read loop and transport tasks and has no
deinit-time teardown, so any test whose body threw — or whose connect failed —
left a fully live Room behind: WebSocket, both peer connections, timers, and a
still-registered delegate, for the remainder of the host process.

That is the amplifier behind most of the observed CI failures. Suites run
sequentially in a single test host, so one flaky failure leaked live rooms into
every later suite; the historical annotations show unrelated timeouts piling up
after the first failure, ending with the ObjC bundle (which runs last) failing
its 30s connect expectations 40 times over the sampled runs.

Connect and participant discovery move into a helper so the teardown covers
them too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`validateTranscriptionResults` indexed `updates` and `messages` right after a
soft `#expect` on their counts. `#expect` doesn't stop execution, so a short
result set trapped on the subscript and took the whole test *process* down —
every later suite included. This accounts for 52 "Fatal error: Index out of
range" aborts across the sampled failed runs, each one costing a full 25-minute
retry attempt.

Record the mismatch and return instead, so a transcription flake fails only the
transcription test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AsyncTimerTests was the single largest flake source in the sampled runs: 36
suite failures spanning all ten tests. Every one of them slept a fixed multiple
of a 50ms interval and then asserted a count.

`AsyncTimer` runs its loop from `Task.detached(priority: .utility)`, so it only
guarantees "fires no earlier than the interval" — a `.utility` wake-up on a
loaded CI host (simulators especially) is routinely deferred well past its
deadline. The tests were asserting a scheduling guarantee the platform doesn't
make, which is also why bigger runners didn't help.

Each "did fire" assertion now waits for the event with a generous deadline via
`ConcurrentCounter.wait(untilAtLeast:)`; each "did not fire" assertion is
bounded so sleep overshoot can't manufacture a fire, and the
one-loop-only bound in `concurrentArmingLeavesSingleLoop` is normalized by
measured elapsed time rather than the requested sleep. Suite runtime is
unchanged (~4.3s).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`performRpc()` (34 failures) and `performRpcConcurrentRequestsHaveDistinctIds()`
(18) injected their ack/response from an unstructured `Task` spawned inside the
mock data channel's packet handler, so passing depended on that task being
scheduled before the call's 15s response timeout — hence the 52
`RpcError(1502, "Response timeout")` failures against a mock that never touches
the network. `v2ResponseStreamFromWrongSenderIsIgnored` had the same problem
against the 2s ack watchdog, surfacing as 1501 where the test asserts 1502.

`RpcClientManager.performRpc` already awaits its `afterPublish` hook, and the
tests in this file that use it have never flaked. Move all three over.

`performRpc()` also asserted its wire format via an early `return` inside the
mock — a format regression showed up as an opaque timeout. The request is now
captured and asserted directly after the call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xcbeautify's github-actions renderer collapses a parameterized Swift Testing
failure to "Recorded an issue (1) argument(s)" — no message, no failing
argument. That is 138 of the failure annotations in the sampled runs, i.e. the
flakiest tests in the repo are also the ones whose failures can't be read.

xcodebuild's own output carries the full message and the failing argument, so
tee it to a file and upload it when the job fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`large_tuple` on the extracted fixture type, and the now-superfluous
`function_body_length` disable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client-side logs can't distinguish "we gave up" from "the SFU hung up on us".
The e2e failure on the TSan leg shows both rooms connecting in 25ms, then 18s of
silence, then `Broken pipe` / `Connection reset by peer` and both data channels
closing "unexpectedly" — i.e. not SDK-initiated. Nothing on the client says why
the session ended.

`dev-server-action` already writes JSONL to a path it exposes as `log-path`, and
the server records the participant close reason. Capture it alongside the test
log so these are diagnosable without another CI round trip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
26.6 is the newest Xcode on the macos-26 image (26.5 was the image default).

Simulator runtimes stay at OS=26.5 deliberately: that image ships iOS/tvOS/
visionOS runtimes 26.2, 26.4 and 26.5 only, so 26.5 is already the latest
available and Xcode 26.6 drives it fine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plain coverage of every supported platform on the xcode-27 preview image
(macOS, Mac Catalyst, iOS/tvOS/visionOS simulators at OS=27.0), deliberately
without the asan/tsan/symbol-graph/extension-api variants — these legs answer
"does the next toolchain build and pass at all".

Uses `xcode-version: latest`, which per setup-xcode's docs includes beta
releases (`latest-stable` would exclude them and pick nothing here).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With 3 attempts a flake that passes on retry leaves the job green, so
`if: failure()` never fires and neither log artifact uploads — the most
informative failures were the ones we couldn't see. One attempt makes every
flake a job failure so the test and server logs are captured.

Revert to 3 before merging.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`reliableDelivery` polled for received packets *after* the `withRooms` body
returned, by which point both rooms are disconnected and the SCTP association
is gone — anything still in flight is unrecoverable, so the poll could never do
what its comment claimed.

That is the partial-delivery flake on loaded simulator legs: 117/128 with no
reconnect at all, 127/128 for a late dual reconnect, 56/128 when a receiver
reconnect left more packets queued at teardown. It passes locally only because a
fast host drains the queue before teardown.

`concurrentReliableSendsDeliverExactlyOnce` in the same file already waits inside
`withRooms` and documents why; do the same here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`v2CallerV1FallbackErrorResponse` and `v2ResponseStreamReaderFailureFailsPending`
were the two remaining tests injecting their ack/response from an unstructured
`Task` inside the mock packet handler, so passing depended on that task being
scheduled before the call's timeout. The first failed in CI with
`1502 "Response timeout"` instead of the injected error 101, and Xcode 27's new
`#NoUseUnstructuredThrowingTask` diagnostic flags the second (its `try #require`
throws into a discarded task).

Both move to the awaited `afterPublish` hook, matching the rest of the file.

Also trims the CI narrative out of the comments added by the earlier commits in
this series — that context belongs here, not in the source.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cancelled `Room.connect` can still have completed the server-side join, so
skipping `disconnect()` on the failure path leaves a live participant behind.
`WebSocketTests` cancels 21 connects across three tests and only disconnected the
ones that happened to win the race, leaving that many participants on the SFU
until the test process died — the server only reaps them 8-10s later, which lands
in the middle of whatever runs next.

Disconnect unconditionally; it's a no-op on an already-disconnected room.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`runTranscriptionTest` returned from the confirmation block 10ms after the last
`sendText`, with nothing waiting for the stream-close finalizations that arrive
afterwards — so `updates()` and `replace()` (the two 5-message tests) reported
`confirmed 4 of 5` whenever the last message was still in flight.

`MessageCollector.waitForUpdates(count:)` polls to a deadline. Also replaces the
two 500ms fixed settle sleeps in `streamCloseFinalizes` and
`attributeBasedIsFinal` with the same helper: same protection, and they now
return as soon as the messages land rather than always burning 500ms.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`testSendFile` is the first ObjC test to connect, and it is the first `Room` in
that bundle's process — the ObjC bundle runs in its own PID, separate from the
Swift phase. Server logs show the SFU sending the subscriber offer and five ICE
candidates within 1ms of the join while the SDK never answers; the client gives
up 10.1s later and the next Room in the same process answers in 45ms. One-time
WebRTC and audio-stack init exceeds the connect timeout on a simulator.

`TestEnvironment.withRooms` has always retried connects, which is why no Swift
test is exposed to this. `LKObjCRoomHelper.connectWithRoom:url:token:` gives the
ObjC tests the same 3 attempts, disconnecting between them so a timed-out attempt
doesn't leave a participant on the server.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`publishManyTracks` captured a single frame before each `publish(videoTrack:)`
and discarded `CVPixelBufferCreate`'s result, so a failed allocation captured
nothing at all. `BufferCapturer` documents both halves of what that breaks:
`capture(_:)` is meant to be called repeatedly, and dimensions must resolve
before publishing or the publish times out — they come only from a real frame,
not from `BufferCaptureOptions(dimensions:)`.

Server logs show the effect: five of six tracks published in 2-6ms each, then
video-2 sat for 10.6s and failed with `Timed out` without any `add_track`
reaching the SFU.

Keep frames flowing until publish returns, matching how
`PublishBufferCapturerTests` drives its buffer track. A fresh buffer per capture
keeps anything non-Sendable out of the feeding task, and `#require` replaces the
discarded allocation result.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
61b3df2 replaced the single pre-publish capture with a detached feeding task,
which made dimension resolution depend on that task being scheduled. On loaded
simulator legs it isn't: the same run shows three AsyncTimerTests failing because
a detached loop produced nothing in 10s, and publishManyTracks still timed out
after 10.0s — this time in V1 (Single PC), while V0 published all six tracks in
13ms each.

Capture synchronously before publish so the dimension wait can't be starved, and
keep the feeding task for liveness. The buffer is now allocated once via
`#require` and reused rather than reallocated per frame.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e test

Three AsyncTimerTests failed again on a visionOS leg with 10s waits expiring at a
50ms interval — a 200x miss, which carries no information about the code under
test. The same run showed the frame-feeding task in publishManyTracks producing
nothing in 10s at a different priority, so this is detached tasks being starved
broadly, not a `.utility` problem.

`AsyncTimer` gains a defaulted `sleep` parameter, so the tests can decide when a
countdown elapses instead of asserting against a wall clock. `AsyncTimer` is
internal, so this is invisible outside the module, and all six call sites keep
working via the default. `Clock`/`TestClock` would be the idiomatic seam but need
macOS 13/iOS 16, well above this package's floor.

`ManualSleeper` parks each countdown on a continuation the test releases. Two
tests become deterministic, and `startIfStoppedFiresWhileRepeatedlyArmed` gets
stronger: it now asserts directly that re-arming leaves exactly one countdown
parked, rather than inferring it from a fire count.
`concurrentArmingLeavesSingleLoop` keeps real time (it is about concurrent loops)
but at 5ms so its liveness check can't be starved out.

Suite goes from 4.3s to 3.2s and passed three consecutive local runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
52adeb7 converted three tests to the injected sleep; the two that kept real
timing, `continuesFiringAfterBlockThrows` and `startIfStoppedReArmsAfterCancel`,
failed the very next run on a visionOS leg with their 10s waits expiring. Finish
the job for every test whose assertion is "it fired".

`updatesBlockOnNextCycle` gets sharper in the process: it now swaps the block
while a countdown is parked and asserts the in-flight cycle still runs the old
one, then that the next cycle picks up the new one — both halves of the
documented behaviour instead of just the second.

Three tests keep real time deliberately: the two that assert a timer did *not*
fire, and the orphan-loop test, where a shared gate can't tell a live loop from a
cancelled-but-parked one.

Suite is 4.3s -> 1.6s and passed four consecutive local runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third iteration on this failure, with the mechanism finally pinned: captured
frames resolve dimensions inside `processingQueue.async` (VideoCapturer._process),
so even a capture issued synchronously before publish only enqueues a GCD block —
on a loaded host it ran later than the 10s publish timeout, and the tvOS leg
failed with the `add_track` request never reaching the SFU while the host logged
"skipping cycle due to overload".

The dimensions gate is not this test's subject, so resolve it directly via the
internal `set(dimensions:)` and keep the feeding task purely for media, which the
subscriber-visibility assertion does need.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two CI legs in one run burned their full 30-minute attempt hung inside
`waitUntillAnyActiveTimeout` — the one test whose completion depends solely on
an AsyncCompleter timeout firing (every remote is disconnected, so the expected
outcome is the timed-out throw; nothing else ever resumes the waiter).

The timeout block is a DispatchWorkItem on a `.background` queue, the QoS tier
the system may defer indefinitely under CPU pressure. A default-priority waiter
suspended on a continuation that only a background-QoS queue can resume is a
QoS inversion with no donation edge, so on a saturated runner the deadline
simply never arrives.

This also explains the recurring RPC flake where callers saw 1501 instead of
1502: the 0.05s response-timeout block at background QoS raced the 2s
ack-watchdog `Task.sleep` at default QoS and lost by 40x.

These deadlines gate connect, publish, RPC, and participant-active waits, so
run them at `.default`; the blocks themselves are microseconds of work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 8099290: `.utility` for the completer timer queue — it clears the
indefinite-deferral hazard of `.background` without over-elevating, and measured
indistinguishable from `.default` under 16x CPU oversubscription.

`DeviceManager` had the same inversion one level up: its KVO registrations ran
on the `.background` global queue, and their `.initial` callbacks are the only
resumers of the device completers that `CameraCapturer.startCapture` awaits —
so camera publishing raced a queue the system may never schedule under load.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two QoS inversions shipped this way (AsyncCompleter's timeout timers,
DeviceManager's discovery registration): work at `.background` may be deferred
indefinitely under CPU pressure, so any waiter depending on it can stall
forever. Covers DispatchQueue qos:, Task/Task.detached priority:, DispatchQoS/
TaskPriority references, and QOS_CLASS_BACKGROUND.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two races in IncomingStreamManager behind the recurring TranscriptionTests
failures:

Dropped streams (the recurring "confirmed 2 of 5" / "4 of 5"): senders reuse a
stream ID for consecutive streams — each transcription `sendText` does — and
descriptor cleanup ran in the reader's `onTermination` task. When the reopening
header beat that task to the event loop, `openStream`'s already-open guard
silently dropped the whole stream. Remove the descriptor synchronously in the
trailer handler instead; the new unit test reproduces the drop deterministically
without the fix (only 1 of 3 payloads delivered).

Reordering (observed once: a segment's final state regressed to a stale
partial): each stream's handler ran in its own detached task, so cross-stream
delivery order was a scheduler race even though the wire is ordered. Topics
registered with `ordered: true` now push handler invocations through a per-topic
AsyncStream drained by a single task — the same FIFO shape as the manager's own
event loop. Opt-in because serializing would hurt concurrent consumers (RPC),
and a stream that never closes would block those behind it; the transcription
receiver opts in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create the queue and its consumer eagerly at registration instead of tracking
ordered topics in a separate set and materializing the queue on first stream —
a topic having a queue entry is what makes it ordered, so the set and the lazy
accessor both go away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The synchronous trailer removal in 25cb496 was not enough: the reader's
`onTermination` cleanup task still ran at an arbitrary later point, and when a
sender had already reused the stream ID it deleted the successor's descriptor.
Its chunks were then silently ignored, the reader never finished, and with an
ordered topic the hung handler blocked every stream behind it — CI showed both
shapes, the new unit test dropping one payload on four legs and the transcription
e2e test delivering 0 of 5.

Descriptors now carry a generation token and the termination cleanup removes
only its own generation, so stale cleanup can never affect a successor under any
interleaving. `onTermination` is assigned after the descriptor is stored; nothing
can have finished the continuation before that within the same isolated scope.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retries were masking every flake this branch fixed — a failed attempt that
passed on retry left the job green and the logs unread. Run the tests once and
let failures surface.

Also removes a dead helper left behind by 1af1227, a redundant explicit sleep
argument in AsyncTimerTests, and trims verbose comments across the branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pblazej pblazej changed the title ci: flakiness Fix CI flakiness: data-stream races, QoS inversions, deterministic tests Jul 31, 2026
`-retry-tests-on-failure -test-iterations 2` re-runs failures inside the same
xcodebuild invocation: XCTest re-runs only the failed cases; Swift Testing
re-runs the selection as a second iteration. Verified that a test failing on
iteration 1 and passing on iteration 2 yields a green run.

Unlike the job-level retry wrapper this branch removed, a flaky leg costs one
extra test pass instead of a full rebuild-and-rerun, and per-iteration results
stay visible in the log and xcresult instead of being buried in a superseded
attempt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pblazej
pblazej marked this pull request as ready for review July 31, 2026 12:32
devin-ai-integration[bot]

This comment was marked as resolved.

pblazej and others added 3 commits July 31, 2026 14:37
Matches the AsyncCompleter timer queue: `.utility` clears the indefinite-deferral
hazard of `.background` without over-elevating one-shot discovery work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…errors

Review follow-ups to the ordered-delivery change:

A stream whose trailer never arrives (sender disconnects mid-stream) stayed in
`openStreams` forever, and on an ordered topic its blocked handler stalled the
FIFO consumer, silently dropping every later stream on that topic for the rest
of the Room's lifetime. Open streams are now failed with
`StreamError.terminated` when their sender disconnects
(`closeStreams(from:)` via `Room._onParticipantDidDisconnect`) and on
`Room.cleanUp` (`reset()`, which keeps handlers and their ordered queues
registered so streams after a reconnect are still handled).

The chunk error paths (encryption mismatch, length exceeded) finished the
continuation but left the descriptor in place, relying on the async
generation-guarded cleanup — the same race the trailer path already fixes
synchronously. They now remove the descriptor before finishing.

Unit tests cover all three: ID reuse immediately after a chunk error, ordered
queues draining after `closeStreams(from:)` and after `reset()`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 6 additional findings in Devin Review.

Open in Devin Review

// so the receiver works even if the `Room.reservedTopicPrefix` guard is
// later widened to cover non-RPC `lk.*` topics like this one.
try await room.incomingStreamManager.registerTextStreamHandler(for: topic) { [weak self] reader, participantIdentity in
try await room.incomingStreamManager.registerTextStreamHandler(for: topic, ordered: true) { [weak self] reader, participantIdentity in

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Live transcripts stop being delivered while an earlier transcript is still arriving

Incoming transcripts are now handled strictly one at a time (ordered: true at Sources/LiveKit/Agent/Chat/Receive/TranscriptionStreamReceiver.swift:97) instead of side by side, so a transcript that is still arriving holds back every transcript that started after it.

Impact: A speaker's live transcript can be withheld for the entire duration of another participant's in-progress transcript (and indefinitely if that one never ends), so transcripts appear late or seem to stall.

Per-topic FIFO serializes handlers for the whole lifetime of each stream

registerTextStreamHandler(for:ordered:) creates one FIFO per topic (Sources/LiveKit/DataStream/Incoming/IncomingStreamManager.swift:105-113) whose single consumer runs each enqueued handler to completion before starting the next (Sources/LiveKit/DataStream/Incoming/IncomingStreamManager.swift:107-110, enqueued at Sources/LiveKit/DataStream/Incoming/IncomingStreamManager.swift:183-190).

The transcription handler does not return until its reader finishes, i.e. until that stream's trailer arrives — see the for try await message in reader loop at Sources/LiveKit/Agent/Chat/Receive/TranscriptionStreamReceiver.swift:98-113. Per this file's own documentation (lines 23-24) an agent message is one stream that stays open and delivers chunks until the message is finalized. So while an agent message stream is open, any transcription stream opened afterwards — e.g. the user's own transcript during barge-in, or a following segment — sits in the queue and is not delivered at all, even though its chunks are already buffered in its continuation.

The FIFO is per topic, not per participant or per segment, so the blocking is cross-participant. If a sender stops without a trailer, the head handler only unblocks when the sender disconnects (IncomingStreamManager.closeStreams(from:)) or the room cleans up (reset()); there is no per-stream timeout.

A per-segment / per-participant ordering key, or ordering only the dispatch of handlers rather than serializing their full execution, would preserve wire order without head-of-line blocking.

Prompt for agents
The new per-topic ordered FIFO in IncomingStreamManager (registerTextStreamHandler(for:ordered:), the orderedQueues dictionary and its single-consumer task) runs each stream handler to completion before starting the next handler for that topic. TranscriptionStreamReceiver opts into it with ordered: true. Because the transcription handler only returns when its reader finishes (i.e. when the stream's trailer arrives), and because an agent message is a single long-lived stream that streams chunks until the message is finalized, any transcription stream that opens while an earlier one is still open is not handled at all until the earlier one closes. The FIFO is keyed by topic only, so this blocks across participants and segments (for example a user's live transcript during barge-in is withheld until the agent's message stream closes), and there is no per-stream timeout — only participant disconnect (closeStreams(from:)) or room cleanUp (reset()) unblocks the head.

Consider whether ordering needs to serialize the whole handler execution, or only the point at which handlers observe/emit updates. Options worth evaluating: order per participant and/or per segment id rather than per topic; or keep handlers concurrent and instead assign a sequence number at header time and serialize only the emission of results into the message stream. Whatever approach is chosen should keep the stream-ID-reuse ordering guarantee this PR adds while ensuring a still-open stream cannot delay unrelated streams.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants