Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 9219315 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@anzemur is attempting to deploy a commit to the LiveKit Team on Vercel. A member of the Team first needs to authorize it. |
|
@anzemur Thanks for the contribution! There's a few issues though which I think would need to be addressed prior to being merged: 1. The change does not correct the problem for data stream attachments.For context, the way data streams works, the sender transmits each attachment as a separate data stream forming a "tree". The If the sender disconnects during an attachment stream, the This means the attachment So, I don't think a top-level catch is sufficient. Since no error bubbles up to the top level, you first need to settle the attachment 2. Errors in the text stream are safe, but errors in an attachment stream are notIf the text stream fails during a read, finalize runs for all error types and deletes all state in any persistent maps. This is correct today. However, if an attachment stream fails during a read, the state stays. This is true for all error types, not only for a disconnection. So I think you would need to make this case disconenction error specific, which you can do by reading the General thoughts on these issues? I think as part of this change it might be worthwhile to introduce at least one more (if not more) test(s) with a data stream that has an attachment and validate that errors propegate properly in the above situations. |
3469a61 to
50fbfb6
Compare
…ope them to their room A sender that leaves before opening an attachment's byte stream gives livekit-client nothing to error, so the future is settled on ParticipantDisconnected. Do that with an AttachmentNotReceivedError instead of a DataStreamError built outside the SDK, and only for attachments of the room the participant left: identities are unique per room, not globally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| /** | ||
| * Settles an attachment whose byte stream never opened because its sender | ||
| * left first. livekit-client raises nothing for a stream that never started, | ||
| * so there is no SDK error to pass along. | ||
| */ | ||
| class AttachmentNotReceivedError extends Error { | ||
| readonly participantIdentity: string; | ||
|
|
||
| readonly attachmentStreamId: string; | ||
|
|
||
| constructor(participantIdentity: string, attachmentStreamId: string) { | ||
| super( | ||
| `Participant ${participantIdentity} disconnected before sending attachment ${attachmentStreamId}`, | ||
| ); | ||
| this.name = 'AttachmentNotReceivedError'; | ||
| this.participantIdentity = participantIdentity; | ||
| this.attachmentStreamId = attachmentStreamId; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This new error type would I think be the first instance of a components-js introduced error type which livekit-client doesn't expose. I'm not 100% opposed to it but I think my strong preference would be to fix this issue at the livekit-client level and then let components-js inherit that fix.
Alternatively, if there isn't a way to nicely encapsulate this into livekit-client, maybe something like the below could be an option:
- Build a per-sender "participant left" observable from
RoomEvent.ParticipantDisconnected, filtered to this room andparticipantInfo.identity. - Add
takeUntil(senderLeft$)to streamObservable, placed beforefinalize.
The pipeline would then complete instead of erroring, finalize still cleans up, and the message is dropped just as it is now. If the text stream was still mid-flight it errors on its own anyway, so takeUntil changes nothing there. This would remove the class, the reject loop and the instanceof AttachmentNotReceivedError branch.
There was a problem hiding this comment.
Went with your takeUntil alternative in 9219315. The text stream handler builds senderLeft$ from RoomEvent.ParticipantDisconnected on this room, filtered to participantInfo.identity, and the pipeline has takeUntil(senderLeft$) right before finalize. The class, the reject loop and the instanceof branch are removed; the message is still dropped, and finalize cleans up.
One nuance on the mid-flight case: livekit-client errors the open controller and emits ParticipantDisconnected in the same tick, so takeUntil completes the pipeline before the read error reaches it, and RxJS drops that late error on the closed subscriber without rethrowing. The result is the same, the text already emitted stays and nothing is uncaught; only the debug log is skipped. Covered by "keeps the text already delivered when the sender leaves mid-stream", which mirrors that ordering.
If you'd still prefer the never-opened case to live in livekit-client, I'm happy to open that as a follow-up there and drop senderLeft$ here once it lands.
There was a problem hiding this comment.
I think I'd really prefer this be fixed upstream in livekit-client. The additional suggestion I provided was more in case there plain isn't a way to upstream this, but I would like to avoid a situation where a user runs into a variation of the same issue on livekit-client and we have to figure out how to reconcile the state between the two layers and "back out" hacks at this level.
…connects Replace AttachmentNotReceivedError and its reject loop with takeUntil on the message pipeline, as suggested in review: one notifier is the sender's ParticipantDisconnected (this room, this identity), the other is the room's own disconnect, because a local disconnect clears open stream controllers without erroring them. The pipeline completes instead of erroring, finalize removes the attachment entry, and the entry no longer references the Room. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
When a participant disconnects while one of its text streams is still open,
livekit-clienterrors the open stream's controller with aDataStreamError(AbnormalEnd) — by design, invalidateParticipantHasNoActiveDataStreams.setupTextStreamreads each incoming stream withfrom(reader).pipe(scan(...)).subscribe(next)— no error handler. RxJS then reports the unhandled observable error by rethrowing it globally from a timer (reportUnhandledError→setTimeout(() => { throw err })), so every abnormally ended transcription stream surfaces as an uncaughtDataStreamError: Participant agent-… unexpectedly disconnected in the middle of sending datathat no application code can catch.For voice-agent apps this fires whenever the agent leaves the room mid-segment (interrupted farewell, agent process exit, crash). In our production app it is the #1 error by users impacted (~940 users / week) while being functionally benign — the transcript had already been delivered chunk by chunk and the session is over.
The chat handler's
streamObservable.subscribe({ next })has the same gap.Fix
Give both subscribes an
errorcallback: keep the already-accumulated text (every chunk was emitted throughnext; on abnormal end there is nothing left to deliver) and log the error at debug level. For chat,finalizealready cleans up attachment state on error.Tests
New test drives
setupTextStreamwith a reader that yields chunks and then throws the abnormal-end error: the accumulated text survives, the error is logged, and nothing rethrows globally (without the fix, the test run fails with the uncaught error). Full@livekit/components-coresuite passes (99 tests).Live verification
Reproduced end to end against a real
livekit-server(Docker,--dev) with real Chromium on both sides (Playwright): a sender participant streams four chunks onlk.transcriptionwithout closing the writer, then goes away; the receiver page runssetupTextStreamand records all uncaught errors plus emissions. 8/8 deterministic runs across 2 iterations x {unfixed0.12.15, this branch} x {tab hard-killed, cleanroom.disconnect()with the stream open}:DataStreamError: Participant … unexpectedly disconnected in the middle of sending data(surfaced viawindow.onerror/ Playwrightpageerror— no application code can catch it)."Hello from the agent"), so nothing is lost by handling the error.The only delta between the two receiver bundles is
@livekit/components-core; every other dependency is alias-pinned identically. The live test drivestextStream.ts; the identical handler added inchat.tsshares the same mechanism but was not separately driven.Update: attachment (byte) streams
Review feedback pointed out the fix did not cover attachment streams: a disconnect mid-attachment left the
for awaitrejection unhandled, the attachment'sFuturepending forever (it was typedFuture<…, never>and could not reject), the message pipeline hanging, and thestreamIdToAttachmentsentry leaked.Addressed by settling the future at the source: the byte stream handler now catches the failed read and rejects the attachment's
Future(error type widened toError). The rejection propagates through the existing message pipeline, so the subscribeerrorcallback records it andfinalizecleans up the attachment state — no hang, no leak, no unhandled rejection, and the pipeline stays healthy for subsequent messages. The error callback is also reason-aware now:DataStreamErrorReason.AbnormalEndlogs at debug (expected disconnect churn) while any other failure logs a visible warning.New tests cover both: an attachment ending abnormally (no unhandled rejection, message dropped, a later message with a completing attachment still delivers) and a non-disconnect attachment failure hitting the warn path.
Update 2: early rejections and attachments that never start
Two more gaps from review, both fixed:
concatMapsubscribes to the attachment futures one at a time, so a future rejecting before its turn (a later attachment failing first, or any attachment failing before the first text chunk) surfaced as an unhandled rejection. Each future now pre-attaches a no-opcatchat creation (as suggested), keeping the rejection handled whileconcatMap's later subscription still observes it.takeUntilon the sender'sRoomEvent.ParticipantDisconnectedin that room, and on the room's own disconnect, since a local disconnect clears stream controllers without erroring them.finalizethen removes the attachment entry.On the behavior question: the message is dropped, terminally rather than delivered text-only - consistent with a mid-transfer attachment failure, so partial failure behaves uniformly (error callback logs,
finalizecleans up, later messages unaffected). Happy to switch to text-only delivery if that's preferred.Tests added for both: a later attachment rejecting while the first is still in flight (no unhandled rejection), and a never-starting attachment settled by the disconnect (terminal drop, pipeline healthy for the next message).
Update 3:
takeUntilinstead of a local errorFollowing review, the never-started attachment case no longer rejects with an error built in components-js. The pipeline completes via
takeUntil(senderLeft$), placed beforefinalize, and viatakeUntilon the room's disconnect. The attachment map no longer stores theRoom. New tests: a message still waiting on an attachment ends on a local room disconnect, and chat text already delivered is kept when the sender leaves mid-stream.