Repository navigation
fix(tts): don't fail the session while FallbackAdapter is falling back - #2568
Merged
Merged
Conversation
FallbackAdapter re-emitted every child TTS `error` event verbatim. A non-retryable provider failure (an ElevenLabs 401, say) therefore reached AgentSession as an unrecoverable `tts_error` and closed the session even though the next instance was already serving audio. The recovery probe made it terminal rather than occasional: it re-raises the same error every `recoveryDelayMs`, so a downed provider exhausts `maxUnrecoverableErrors` no matter how it is tuned. Absorb child errors instead, matching stt.FallbackAdapter and Python's tts/fallback_adapter.py, which forwards only `metrics_collected`. A listener stays attached rather than being dropped entirely: a TTS with no `error` listener throws ERR_UNHANDLED_ERROR on emit, and ChunkedStream's main task has no catch, so the recovery probe would escape as an unhandled rejection. Terminal failure still reaches the session — once every instance has failed the adapter's own stream throws APIConnectionError and the base class emits that here. close() now removes only the listeners this adapter registered. It called removeAllListeners on instances the caller owns and constructed, deleting their own subscriptions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 700dfac The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin flagged an unhandled rejection reachable from this adapter: a non-streaming child is synthesized through a StreamAdapter, which re-emits the child's errors onto itself with nothing listening, and Node turns that into a synchronous throw inside the child's emitError. The suggested fix — attaching an error sink to the temporary wrapper — does not resolve it. Measured on the non-streaming fallback path: PR as-is 2 rejections (ERR_UNHANDLED_ERROR) + sink on the wrapper 2 rejections (APIError) + ChunkedStream catch, no sink 0 rejections The sink only changes which error escapes. Both escape from the same place: ChunkedStream's main task is detached in the constructor with no catch, unlike SynthesizeStream's, which has swallowed its rejection since it was written. Give ChunkedStream the same treatment and the same comment, rather than a sink per wrapper at every construction site. Regression covered by the non-streaming fallback test, which now asserts no unhandled rejection escapes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tinalenguyen
left a comment
tinalenguyen
left a comment
Member
There was a problem hiding this comment.
thanks for the PR, left a small comment!
| * this adapter's own stream throws `APIConnectionError` and the base class | ||
| * emits that here. | ||
| */ | ||
| private readonly absorbChildError = () => {}; |
Member
There was a problem hiding this comment.
let's log provider errors, also for parity with python
A failing child stream reports its provider's error on the TTS `error` event and then ends without throwing, so FallbackAdapter's failover and recovery logs only ever carried its own stand-in — "TTS stream completed but no audio was received" — never the provider's 401. Before child errors were absorbed, the real error surfaced only as AgentSession shut down. Record the error each attempt's instance reports and log that instead, matching Python's "error, switching to next TTS" and "recovery failed" lines. The absorbing sink stays silent: it cannot tell a failover from a recovery probe, and probes re-fail every recoveryDelayMs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous commit recorded provider errors from the TTS-wide `error` event for the duration of each attempt. That event is shared by every stream of a TTS and doesn't identify its source, so when two requests used one instance, a request whose attempt ended silently was logged with the other request's 401. A stream now keeps the error it failed with (`error`, @internal, beside `startedTime`), set in emitError. Each attempt reads its own stream's error when it finds no audio, so attribution holds under concurrency and the listener plumbing goes away. The attempt still throws the declared APIConnectionError; only the logged error changes. The recovery probe, which has no Throws<> contract and doesn't branch on the error, throws its stream's error directly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A non-streaming TTS reaches FallbackAdapter's streaming path through a StreamAdapter, whose wrapper synthesizes each sentence as a ChunkedStream. A failed sentence ends that iteration without throwing and the wrapper never read the sentence stream's error, so its own `error` stayed undefined and a non-streaming provider's 401 was logged only as "TTS stream completed but no audio was received". The wrapper now keeps the first sentence failure and reports it through `error` when it didn't fail itself. It still skips a failed sentence rather than failing: propagating would emit a second `tts_error` for the same provider failure on the wrapped TTS, which AgentSession counts toward maxUnrecoverableErrors. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ting StreamAdapterWrapper reported a skipped sentence's error even after a later sentence delivered audio, contradicting the `error` contract (the error the stream failed with). It now reports a sentence error only when no sentence delivered audio: a wrapper that delivered anything didn't fail, whatever it skipped. Two more failures went unreported once child errors were absorbed: - A provider that fails after delivering some audio was treated as a success and its error was dropped, so a cut-off reply left no log at all. Both attempt paths now log it with the provider's error, reusing the adapter's existing mid-utterance message. - The recovery probe consulted its stream's error only when no audio arrived, so a provider that sent a little audio and then failed was marked recovered. A probe that errors now fails recovery, as in Python. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolve StreamAdapterWrapper against livekit#2576, which added an `expressive` snapshot beside this branch's `error` getter; both are kept. The API report is regenerated from the merged tree and differs from main only by this branch's three internal `error` getters. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous commit logged a provider failure that cut off delivered audio but completed the utterance, so AgentSession never learned speech was lost; on main it did, through the child error this PR stops forwarding. The adapter now fails its own stream in that case, without retrying since another instance would replay the audio, so the session hears about lost speech exactly once. The chunked path gains the mid-utterance guard the streaming path already had instead of falling back. A sentence StreamAdapterWrapper skipped is lost speech too, so the wrapper reports it even when other sentences delivered audio, restoring the contract the previous round narrowed. getStreamingInstance created a StreamAdapter for every attempt, skipped ones included, and nothing closed it; close()'s removeAllListeners used to sweep them up. Create it only for an attempt that runs and close it when the attempt ends, as livekit#2468 does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@tinalenguyen back for re-review, thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2399.
Problem
FallbackAdapterre-emitted every child TTSerrorverbatim. A non-retryable child failure (ElevenLabs401in the report) reachedAgentSessionasrecoverable: falseand closed the session while the adapter was successfully failing over. The recovery probe re-raises it everyrecoveryDelayMs(default 1 s), so nomaxUnrecoverableErrorsvalue survives a downed provider.Fix
The adapter absorbs a child's failure only when it recovers from it, matching
stt.FallbackAdapterand Python'stts/fallback_adapter.py. What it can't recover from still reaches the session once, as its own error: every instance failing, or a failure after audio went out, which fails without retrying since another instance would replay it.errorevent doesn't say which stream failed. Streams now keep the error they failed with (error,@internal, likestartedTime), including a sentence aStreamAdapterskipped, so failover, recovery and mid-utterance logs carry the provider's own error.close()removes only the adapter's own, and theStreamAdaptercreated for a non-streaming instance is released after each attempt (overlaps Expose TTS fallback controls and activation events (LKINF-495) #2468).ChunkedStream's detached main task no longer rejects unhandled (overlaps fix(agents): handle rejections from .finally(); enable no-floating-promises #2552 / fix(agents): handle rejections from .finally() on background tasks #2519).Tests
14 new cases in
fallback_adapter.test.ts; 13 fail onmain, and the 14th guards the wrapper cleanup thatclose()'sremoveAllListenersused to do by accident.vitest run agents/src: 2768 pass. Typecheck, lint, prettier and throws-check clean.🤖 Generated with Claude Code