Expose TTS fallback controls and activation events (LKINF-495) - #2468
Expose TTS fallback controls and activation events (LKINF-495)#2468russellmartin-livekit wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: d133fbc The changes in this PR will be included in the next version bump. This PR includes changesets to release 38 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 |
203175f to
6578d87
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| for (const tts of this.ttsInstances) { | ||
| tts.removeAllListeners('metrics_collected'); | ||
| tts.removeAllListeners('error'); | ||
| tts.removeAllListeners('system_default_fallback'); |
There was a problem hiding this comment.
🟡 Adapter close erases child listeners
When FallbackAdapter.close() runs, removeAllListeners deletes consumer-installed system_default_fallback listeners from every child. Reused children then silently lose fallback notices.
Learn more
The adapter attaches an anonymous forwarding callback to every child. Because it retains no callback reference, shutdown cannot remove only its own listener and instead removes all listeners for the event. EventEmitter listeners belong to their registrants, so closing one wrapper must not alter unrelated subscriptions on a shared or later-reused child.
Example: An application subscribes directly to inferenceTts.on('system_default_fallback', auditNotice), then wraps that instance in a FallbackAdapter. After adapter.close(), reusing inferenceTts no longer invokes auditNotice, although the application never removed it.
Recommended fix: Store each forwarding callback per child, as StreamAdapter does, and call off('system_default_fallback', storedCallback) during close. Apply the same ownership-safe cleanup pattern to the existing metrics and error forwarders instead of using removeAllListeners.
Was this helpful? React with 👍 or 👎 to provide feedback.
6578d87 to
9a94c2b
Compare
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
🟨 Gateway errors bypass PII redaction
A gateway error event can contain provider or customer content. Logging it as serverEvent lacks the required pii marker, preventing reliable redaction.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| this.#tts.on('metrics_collected', this.#forwardMetrics); | ||
| this.#tts.on('error', this.#forwardError); | ||
| this.#tts.on('fallback_activated', this.#forwardFallbackActivated); |
There was a problem hiding this comment.
🟡 Non-streaming fallback notices leak listeners
Each non-streaming fallback utterance adds #forwardFallbackActivated permanently. getStreamingInstance creates a new adapter, but its stream never closes it. Listener counts grow without bound and trigger EventEmitter warnings.
Learn more
A StreamAdapter subscribes to its wrapped TTS in its constructor and unsubscribes only in close(). The fallback streaming path creates a fresh adapter for every attempt through getStreamingInstance, then retains only the returned stream. Nothing closes that temporary adapter after success, failure, cancellation, or shutdown, so this added subscription remains on the child TTS.
Example: With one non-streaming child, eleven utterances create eleven permanent fallback_activated listeners on that child. Node then emits a MaxListenersExceededWarning, and later utterances keep increasing the listener array.
Recommended fix: Give the fallback stream explicit ownership of temporary StreamAdapter instances and close each one in the attempt's finally block. Do not close adapters that represent caller-owned streaming TTS instances.
Was this helpful? React with 👍 or 👎 to provide feedback.
russellmartin-livekit
changed the title
77f73ec to
5afdad3
Compare
5afdad3 to
d133fbc
Compare
1 participant
Summary
disableSystemDefaultFallbackand exact opt-out-only session serialization without changing existing fallback payloadsfallback_activatedevent with idiomaticsessionIdandfallbackTypeFallbackAdapterandStreamAdapterunknownwithout interrupting audiofallback_activatedis declared on the shared JSTTSCallbackssurface because the base TTS emitter is not generic. Scoping it to inference as Python does would require a broader emitter redesign; the adapters forward the event so wrapping an inference TTS does not create a dead listener.Testing
pnpm exec vitest run agents/src/inference/tts.test.ts agents/src/inference/api_protos.test.ts agents/src/inference/tts.type.test.ts agents/src/tts/fallback_adapter.test.tspnpm --filter @livekit/agents typecheckpnpm --filter @livekit/agents build:typespnpm --filter @livekit/agents api:checkpnpm --filter @livekit/agents lintpnpm exec prettier --check agents/src/inference/api_protos.ts agents/src/inference/api_protos.test.ts agents/src/inference/tts.test.ts agents/src/inference/tts.type.test.ts agents/src/tts/tts.ts agents/etc/agents.api.mdCoordinated PRs
LKINF-495