fix(core): keep the cached text stream observable across a room disconnect - #1425
fix(core): keep the cached text stream observable across a room disconnect#1425daniel1014 wants to merge 1 commit into
Conversation
|
@daniel1014 is attempting to deploy a commit to the LiveKit Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: ec1a50a 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 |
|
|
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. |
|
Thanks for the PR! 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 Let me know if you want to tackle that yourself or if I should create a PR with the fix. |
This is unexpected, do you get an error message that I can use to investigate what might be wrong? |
Summary
setupTextStreamcaches one observable perroom:topic, registers the handler intap({ subscribe })and unregisters it infinalize(refcounted viashare({ resetOnRefCountZero: true })). But theRoomEvent.Disconnectedlistener still deletes the cache entry:Deleting the entry does not invalidate consumers that already hold the observable. So on a reused
Roominstance:useTextStreampassesundefinedwhen disconnected), the handler is unregistered, and the cache entry is dropped — but A still holds observable wip #1.topic(useMemo(..., [room, topic])rebuilds without a remount), while disconnected → cache miss → observable Restructure disconnect #2 for the same topic.tap({ subscribe })fire →registerTextStreamHandleris called twice → livekit-client throws: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.once→room.onwithout a matchingoff, so every cache-key rebuild added another permanentDisconnectedlistener. With no rebuild, there is no accumulation.Repro
Reproduced in an app that mounts
useTextStreamconsumers 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 fakeRoomthat 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 onmain, and it fails with the production error rather than a bare identity mismatch:clears buffered streams on disconnect— guards the behaviour that is intentionally kept.Suites:
packages/core:pnpm test→Test Files 10 passed (10)/Tests 100 passed (100);tsc --noEmitexit 0;pnpm lintreports only the 14 pre-existing warnings, none in the touched files.packages/react:pnpm test→Test Files 4 passed (4)/Tests 14 passed (14).Changeset included (
patchon@livekit/components-core).