fix(core): keep abnormally ended text streams from rethrowing globally - #1426
fix(core): keep abnormally ended text streams from rethrowing globally#1426anzemur wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: ebbd8d0 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. |
| const attachments = new Map( | ||
| (attachedStreamIds ?? []).map((id) => [ | ||
| id, | ||
| new Future<{ fileName: string; mimeType: string; buffer: Array<Uint8Array> }, never>(), | ||
| new Future<{ fileName: string; mimeType: string; buffer: Array<Uint8Array> }, Error>(), | ||
| ]), | ||
| ); |
There was a problem hiding this comment.
I suspect here you want something like the below, otherwise if you have multiple attachments which are included on chat message, there's a narrow period of time in the concatMap in chat.ts line 134 where if a supposed attachment index > 0 were to reject, it would get raised as an unhandled rejection. I think to work around this, you'd want something like the below here:
| const attachments = new Map( | |
| (attachedStreamIds ?? []).map((id) => [ | |
| id, | |
| new Future<{ fileName: string; mimeType: string; buffer: Array<Uint8Array> }, never>(), | |
| new Future<{ fileName: string; mimeType: string; buffer: Array<Uint8Array> }, Error>(), | |
| ]), | |
| ); | |
| const attachments = new Map( | |
| (attachedStreamIds ?? []).map((id) => { | |
| const future = new Future<{ fileName: string; mimeType: string; buffer: Array<Uint8Array> }, Error>(); | |
| future.promise.catch(() => {}); // Ignore emitting `unhandledRejection` if the promise rejects before the attachments `concatMap` switches to this promise | |
| return [id, future]; | |
| }), | |
| ); |
Example test which I think should exercise this case:
const byteReader = (id: string, body: () => AsyncGenerator<Uint8Array>): ByteReader => ({
info: { id, name: `${id}.bin`, mimeType: 'application/octet-stream' },
[Symbol.asyncIterator]: body,
});
it('does not reject an attachment future before anything subscribes to it (multiple attachments)', async () => {
const rejections: Array<string> = [];
const onUnhandled = () => rejections.push('unhandledRejection');
process.on('unhandledRejection', onUnhandled);
vi.spyOn(log, 'debug').mockImplementation(() => {});
vi.spyOn(log, 'warn').mockImplementation(() => {});
try {
const { room, textHandlers, byteHandlers } = makeRoom();
const chat = setupChat(room);
const subscription = chat.messageObservable.subscribe(() => {});
const textHandler = textHandlers.get('lk.chat')!;
const byteHandler = byteHandlers.get('lk.chat')!;
await textHandler(textReader('msg-1', 'two attachments', ['att-1', 'att-2']), {
identity: 'sender',
});
await settle();
// `concatMap` subscribes to the attachment futures one at a time, so nothing is
// listening to att-2 while att-1 is still in flight. Rejecting it here surfaces
// globally as an unhandled rejection until att-1 settles.
await byteHandler(
byteReader('att-2', async function* () {
throw new DataStreamError(
'Participant sender unexpectedly disconnected in the middle of sending data',
DataStreamErrorReason.AbnormalEnd,
);
}),
);
await settle();
expect(rejections).toEqual([]);
subscription.unsubscribe();
} finally {
process.off('unhandledRejection', onUnhandled);
vi.restoreAllMocks();
}
});| mergeMap((chunk: string) => { | ||
| if (attachments.size === 0) { | ||
| return of({ chunk, attachedFiles: [] }); |
There was a problem hiding this comment.
Another case that I don't think your current solution handles properly - a byte stream fails before the text stream yields its first chunk which results in an unhandled rejection.
The attachment observable is only constructed inside mergeMap here, which doesn't fire until a text chunk arrives. Similarly to my other comment, if the downstream attachment is rejected before that and nobody upstream is listening with a `.catch()1, then you will get an unhandledRejection.
I think a test which should assert this:
const byteReader = (id: string, body: () => AsyncGenerator<Uint8Array>): ByteReader => ({
info: { id, name: `${id}.bin`, mimeType: 'application/octet-stream' },
[Symbol.asyncIterator]: body,
});
it('settles the message when an attachment byte stream never arrives at all', async () => {
vi.spyOn(log, 'debug').mockImplementation(() => {});
vi.spyOn(log, 'warn').mockImplementation(() => {});
try {
const { room, textHandlers } = makeRoom();
const chat = setupChat(room);
const emissions: ReceivedChatMessage[][] = [];
const subscription = chat.messageObservable.subscribe((me
emissions.push(messages);
});
const textHandler = textHandlers.get('lk.chat')!;
// The text arrives complete, but the sender drops before
// byte stream, so its future never settles. Nothing errors, so no error callback
// can rescue this: the pipeline never completes, `finali
// `streamIdToAttachments` entry is retained for the lifetime of the page.
await textHandler(textReader('msg-3', 'text arrived, atta']), {
identity: 'sender',
});
await settle();
// The received text must not be silently swallowed. (If the preferred behaviour is
// to drop the message instead, assert that the pipeline
// that it must reach *some* terminal state rather than hanging forever.)
expect(emissions.flat().map((message) => message.message)
'text arrived, attachment never did',
]);
subscription.unsubscribe();
} finally {
vi.restoreAllMocks();
}
});
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.