Skip to content

fix(core): keep the cached text stream observable across a room disconnect - #1425

Open
daniel1014 wants to merge 1 commit into
livekit:mainfrom
daniel1014:fix/text-stream-cache-survives-disconnect
Open

fix(core): keep the cached text stream observable across a room disconnect#1425
daniel1014 wants to merge 1 commit into
livekit:mainfrom
daniel1014:fix/text-stream-cache-survives-disconnect

Conversation

@daniel1014

Copy link
Copy Markdown

Summary

setupTextStream caches one observable per room:topic, registers the handler in tap({ subscribe }) and unregisters it in finalize (refcounted via share({ resetOnRefCountZero: true })). But the RoomEvent.Disconnected listener still deletes the cache entry:

room.on(RoomEvent.Disconnected, () => {
  getObservableCache().delete(cacheKey);   // <-- leftover
  textStreams = [];
  textStreamsSubject.next([]);
});

Deleting the entry does not invalidate consumers that already hold the observable. So on a reused Room instance:

  1. Consumer A subscribes while connected — handler registered.
  2. The room disconnects. A unsubscribes (useTextStream passes undefined when disconnected), the handler is unregistered, and the cache entry is dropped — but A still holds observable wip #1.
  3. Consumer B mounts, or an existing consumer changes its topic (useMemo(..., [room, topic]) rebuilds without a remount), while disconnected → cache miss → observable Restructure disconnect #2 for the same topic.
  4. The room reconnects. Both observables' tap({ subscribe }) fire → registerTextStreamHandler is called twice → livekit-client throws:
DataStreamError: A text stream handler for topic "lk.transcription" has already been set.

The second subscription dies permanently: that topic never delivers text again for the life of the page, while everything else on the room (tracks, other topics) keeps working — which makes it look like a UI bug rather than a stream-handler collision.

This is a leftover from #1188 ("don't unregister stream handler on disconnect"), whose stated goal is exactly this scenario — "This ensures that a room instance can be reused and the transcription handler stays registered even after a disconnect." That PR correctly introduced the refcount pattern, but kept the getObservableCache().delete(cacheKey) line from the pre-refactor version (where the subject was also completed, so eviction was correct). With the subject now long-lived, the eviction is what breaks reuse.

Fix

Drop the cache eviction; keep the buffer reset. The subject is never completed and registration is refcounted, so the cached observable stays usable across any number of connect/disconnect cycles on the same Room.

It also stops a listener leak as a side effect: #1188 changed room.onceroom.on without a matching off, so every cache-key rebuild added another permanent Disconnected listener. With no rebuild, there is no accumulation.

Repro

Reproduced in an app that mounts useTextStream consumers under a <LiveKitRoom connect={...}> toggle: connect → disconnect → change a consumer's topic (or mount a new one) → reconnect. The regression test in this PR is the minimal version of that, using a fake Room that mirrors livekit-client's one-handler-per-topic contract.

Test plan

New packages/core/src/components/textStream.test.ts:

  • keeps one observable per topic across a disconnect, so a later subscriber shares the handler — red on main, and it fails with the production error rather than a bare identity mismatch:
    Error: A text stream handler for topic "lk.transcription" has already been set.
    Test Files  1 failed (1)
    Tests  1 failed | 1 passed (2)
    
    Green with the fix:
    Test Files  1 passed (1)
    Tests  2 passed (2)
    
  • clears buffered streams on disconnect — guards the behaviour that is intentionally kept.

Suites:

  • packages/core: pnpm testTest Files 10 passed (10) / Tests 100 passed (100); tsc --noEmit exit 0; pnpm lint reports only the 14 pre-existing warnings, none in the touched files.
  • packages/react: pnpm testTest Files 4 passed (4) / Tests 14 passed (14).

Changeset included (patch on @livekit/components-core).

@vercel

vercel Bot commented Aug 27, 2026

Copy link
Copy Markdown

@daniel1014 is attempting to deploy a commit to the LiveKit Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot

changeset-bot Bot commented Aug 27, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ec1a50a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@livekit/components-core Patch
@livekit/components-react Patch
@livekit/agents-ui Patch
@livekit/component-example-next Patch
@livekit/components-js-docs Patch
@livekit/component-docs-storybook Patch
@livekit/components-docs-gen Patch

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

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@daniel1014

daniel1014 commented Aug 27, 2026

Copy link
Copy Markdown
Author

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Btw I just tried this link to sign the required CLA but the link appears broke blocking me to sign anything on it. It will be really appreciated to have this PR being reviewed and merged as it's a blocking issue around livekit cache (see the details above) requiring manual workaround patch.

@lukasIO

lukasIO commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR!
Confirmed it's a bug.

However the fix as you suggest would keep the cache entries around indefinitely.

I'd suggest instead to use a WeakMap keyed on the Room instances like

const cache: WeakMap<Room, Map<string, Observable<TextStreamData[]>>>

so that the entries can get dropped if the Room instance isn't used anymore. This also allows us to drop the RoomEvent.Disconnected handler entirely (buffer reset can move into tap({ subscribe }).

Let me know if you want to tackle that yourself or if I should create a PR with the fix.

@lukasIO

lukasIO commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Btw I just tried this link to sign the required CLA but the link appears broke

This is unexpected, do you get an error message that I can use to investigate what might be wrong?

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.

3 participants