fix(react): recreate Krisp processor per track#1321
fix(react): recreate Krisp processor per track#1321Topherhindman wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 34b330a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
size-limit report 📦
|
The useKrispNoiseFilter hook stored the KrispNoiseFilterProcessor in React state and only created it once, reusing the same instance across sessions. A KrispNoiseFilterProcessor's internal audio graph is bound to the AudioContext it was init'd against; after Room.disconnect() closes that context and session.start() republishes the mic on a fresh one, reattaching the old processor throws "InvalidAccessError: cannot connect to an AudioNode belonging to a different audio context". Drop the duplicated React state and treat LocalAudioTrack.getProcessor as the source of truth. The effect now creates a fresh processor per track — immune to track swaps because nothing is cached across them.
Topherhindman
force-pushed
the
fix/krisp-processor-stale-audio-context
branch
from
7108c30 to
60a7d50
Compare
| await track.setProcessor(processor); | ||
| if (cancelled) return; |
There was a problem hiding this comment.
question: Is it possible that track.setProcessor could run here, the effect could rerun (cancelled could be set to true), the bailout would occur, and if the effect didn't run again (maybe due to a higher level component unmount / etc), the track could be left with the processor set?
Is it worth doing something in the effect cleanup function to teardown the track processor and leave the track in the state that it started in?
There was a problem hiding this comment.
Ahh yeah! Good catch. This was a real race. I went back and forth on where to put the detach.
The constraint I ran into: at the moment the cleanup function fires, it can't reliably know whether track.setProcessor has actually landed, because the await may still be outstanding. The cleanup is what causes the bailout, it doesn't happen after it. Cleanup also can't await stopProcessor(), so the best it could do is fire and forget. And because cleanup runs on any dep change (including shouldEnable toggling), detaching unconditionally there would destroy the processor on every toggle and re-pay Krisp's wasm + model init on re-enable.
The spot that can do exactly what you described is the IIFE itself, after the await track.setProcessor(processor) resolves, since it knows the attach landed and can
await track.stopProcessor() cleanly. That's what the if (cancelled) { await track.stopProcessor(); return; } branches handle do. Same contract as "teardown on cancel, leave the track as you found it," just placed where the async state is legible.
The one case that isn't covered is "effect succeeded fully, then the component unmounts with the track still alive." For the default micPublication path i dont believe that can happen, as LocalAudioTrack.stop() destroys the processor during the room's own teardown, so the track can't outlive us. It could happen if a caller points trackRef at a track they own and keep alive past this hook's lifetime though. I looked at covering that with either a fire and forget detach in the cleanup function or a two effect split (attach lifecycle + enable sync), but both introduced worse problems. The cleanup function version broke toggle reuse by destroying the processor on every shouldEnable flip, and the two effect split regressed isNoiseFilterPending handling, made the returned processor a stale state value instead of the live track.getProcessor(), and created a processorRequested latch that reattached on future track swaps even while the filter was off.
Happy to revisit the caller owned trackRef case if you think it's common enough to justify the complexity. Maybe as a follow up? Otherwise I think I'd leave this PR scoped to the race you caught.
There was a problem hiding this comment.
I think what you've done makes sense to me - I also get the sense this isn't common.
One other possible idea that came to mind just now - you could put the await stopProcessor() in an unrelated effect which would have the benefit of at least cleaning it up when the component the hook is in unmounts (or that secondary effect could be given a different set of dependencies, unlinked to the set passed into the effect that calls await startProcessor()).
If the effect was cancelled while `track.setProcessor(processor)` was still in flight — e.g. the component unmounted with the track still alive via a caller-owned `trackRef` — setProcessor could resolve after the bailout, leaving the track with a Krisp processor attached that the hook no longer manages. When the track outlives the hook nothing was going to clean that up. Tear down the attachment inside the IIFE at each post-setProcessor cancellation check, so the track is left in the state the hook found it in.
|
|
||
| const processor = | ||
| micPublication?.track instanceof LocalAudioTrack | ||
| ? (micPublication.track.getProcessor() as KrispNoiseFilterProcessor | undefined) |
There was a problem hiding this comment.
issue: this cast assumes that no other processor has been set on the track by some other module.
Topherhindman
force-pushed
the
fix/krisp-processor-stale-audio-context
branch
from
d9522d5 to
86cca01
Compare
Topherhindman
force-pushed
the
fix/krisp-processor-stale-audio-context
branch
from
86cca01 to
6b6e4c2
Compare
Two correctness fixes from PR review: - Guard the `KrispNoiseFilterProcessor` cast in the returned `processor` value with a `name === 'livekit-noise-filter'` check. The unconditional cast assumed nothing else could attach a `TrackProcessor` to the track; if anything did, the hook handed consumers a value typed as a Krisp processor that wasn't one. - Add a separate cleanup effect that detaches the processor we own when the underlying track changes or the hook unmounts. Tracks the attached processor + its track via refs so cleanup can verify identity (`track.getProcessor() === processor`) before calling `stopProcessor()` — avoids accidentally detaching a Krisp processor that some other module owns. Refs are populated only after `setProcessor` lands, so a cancelled mid-attach leaves them clear. Closes the gap where the track outlives the hook (caller-owned `trackRef` unmount or in-place swap to a different live track), while leaving the existing toggle-reuse path on `shouldEnable` unchanged.
Topherhindman
force-pushed
the
fix/krisp-processor-stale-audio-context
branch
from
6b6e4c2 to
34b330a
Compare
4 participants
Summary
Noticed with our homepage agent in www that
useKrispNoiseFiltercurrently throwsInvalidAccessError: Failed to execute 'connect' on 'AudioNode': cannot connect to an AudioNode belonging to a different audio context.on the second session any time a consumer callsRoom.disconnect()followed by a newsession.start()without a page reload.Root cause
The hook stored the
KrispNoiseFilterProcessorinReact.useStateand only created it once (guarded byif (!krispProcessor)), reusing the same instance across mic republishes. AKrispNoiseFilterProcessor's internal audio graph is bound to theAudioContextit wasinit()-ed against, andRoom.disconnect()closes that context. When the mic republishes on a freshAudioContext, attaching the cached processor to the new track callsAudioNode.connect()on nodes from the old (closed) context — hence the error.Duplicating the processor into React state while
LocalAudioTrackalready owns it was the underlying design mistake; the state copy had no mechanism to track when the real processor had been destroyed out from under it.Fix
Three commits:
fix(react): recreate Krisp processor per track— remove the duplicated state and treatLocalAudioTrack.getProcessor()as the source of truth. A single effect handles both attach and enable/disable sync:setEnabled(shouldEnable).KrispNoiseFilterProcessorbound to this track'sAudioContextand attach it.Because nothing is cached across tracks, mic republishes can't produce a stale-processor-from-a-closed-context scenario.
fix(react): detach Krisp processor on cancel— handle the cancel-mid-attach race wheresetProcessorresolves after the effect was cancelled (e.g. unmount during attach). The IIFE checkscancelledafter eachawaitand undoes the attach viatrack.stopProcessor()so the track is left as we found it.fix(react): guard processor cast and detach on cleanup— guard the returnedprocessorvalue with aname === 'livekit-noise-filter'check before casting (prevents misrepresenting a non-KrispTrackProcessorset by another module), and add a separate cleanup effect keyed on the track that detaches the processor on track change or unmount. Uses identity (track.getProcessor() === processor) so it only detaches the instance this hook owns. Closes the gap where a caller-ownedtrackRefoutlives the hook's component or swaps to a different live track.Test plan
session.start()→session.end()→session.start()errors on second start.setNoiseFilterEnabledflipping false → true does not destroy and recreate the processor.pnpm --filter @livekit/components-react build/lint/testall pass (no new lint warnings).