fix(voice): propagate hard tail receive errors - #851
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 2 remain after this review. WalkthroughThis change standardizes receive timeout errors across voice adapters. Voice draining now propagates hard failures and records separate timeout and error telemetry. Tests cover adapter contracts, transport failures, timeout variants, and terminal chunks. ChangesVoice drain timeout classification
Sequence Diagram(s)sequenceDiagram
participant DrainAgentResponse
participant VoiceAdapter
participant TimeoutClassifier
participant VoiceTelemetry
DrainAgentResponse->>VoiceAdapter: call receiveAudio(timeout)
VoiceAdapter-->>DrainAgentResponse: return timeout or receive failure
DrainAgentResponse->>TimeoutClassifier: classify failure
TimeoutClassifier-->>DrainAgentResponse: return timeout or hard-error classification
DrainAgentResponse->>VoiceTelemetry: record termination or error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Merge Risk: ⚪ Minimal · up to The change propagates genuine voice receive failures while preserving expected timeout and terminal-end behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a9b8748 to
7a97f5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javascript/src/voice/adapters/elevenlabs.ts (1)
829-838: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReject parked receives when the session reports an error.
Line 829 correctly classifies an elapsed deadline. It does not cover
onSessionError()at Lines 570-581. That handler callsdrainPendingWaiters(), which resolves the activereceiveAudio()with an empty chunk.drainInner()then recordsterminal_chunkand returns a successful truncated turn.Split the paths. In
onSessionError(), reject each pending receive with the originalerr. Keep empty chunks for clean terminal conditions only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/voice/adapters/elevenlabs.ts` around lines 829 - 838, Update onSessionError() to reject every pending receive waiter with the original session error instead of resolving it with an empty chunk through drainPendingWaiters(). Preserve empty-chunk resolution for clean terminal conditions, and keep the receiveAudio timeout handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/src/voice/adapters/__tests__/receive-timeout-contract.test.ts`:
- Around line 71-119: Expand the shared ADAPTERS contract coverage to include
the composable, OpenAI Realtime, and Twilio adapters, configuring each to
establish its connection and park receiveAudio(DEADLINE_S) with appropriate
teardown. Add success and timeout/failure assertions for every newly covered
adapter, preserving the existing contract behavior and test structure.
In `@javascript/src/voice/adapters/openai-realtime.ts`:
- Line 37: Update _drainSpokenTurn so its receiveAudio error handling catches
only errors identified by isReceiveTimeoutError, preserving the timeout behavior
while rethrowing socket closures and all other producer failures instead of
returning partial or empty audio as a successful turn. Reuse the imported
ReceiveTimeoutError-related helper or established timeout check already present
in the module.
---
Outside diff comments:
In `@javascript/src/voice/adapters/elevenlabs.ts`:
- Around line 829-838: Update onSessionError() to reject every pending receive
waiter with the original session error instead of resolving it with an empty
chunk through drainPendingWaiters(). Preserve empty-chunk resolution for clean
terminal conditions, and keep the receiveAudio timeout handling unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9578f829-5707-4298-a168-4c5d9646c282
📒 Files selected for processing (13)
javascript/src/voice/__tests__/drain-tail-error-propagation.test.tsjavascript/src/voice/__tests__/voice-spans.test.tsjavascript/src/voice/adapter.runtime.tsjavascript/src/voice/adapter.tsjavascript/src/voice/adapters/__tests__/receive-timeout-contract.test.tsjavascript/src/voice/adapters/composable.tsjavascript/src/voice/adapters/elevenlabs.tsjavascript/src/voice/adapters/gemini-live.tsjavascript/src/voice/adapters/openai-realtime.tsjavascript/src/voice/adapters/pipecat.tsjavascript/src/voice/adapters/twilio.tsjavascript/src/voice/receive-timeout-error.tsspecs/voice-drain-error-propagation.feature
|
On the outside-diff note about It resolved parked receives with the empty terminal chunk, so a broken session read as a clean end of turn and the drain reported the partial audio as a complete one. Parked receives are now rejected with the underlying error, and the failure is recorded so every later receive keeps failing rather than the next probe finding a quiet socket. The clean path stays as it was, deliberately: a session that ENDS still resolves with the terminal chunk, and now keeps returning it once the stream has ended. That last part matters beyond this comment. The tail probe usually lands after the close rather than during it, and it used to hit Same split applied to Pipecat in 9438a94, which had the same problem and was the last adapter still rejecting on a closed stream. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
javascript/src/voice/adapters/pipecat.ts (1)
408-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
receiveAudiocontract with JSDoc.Document
timeout, non-empty audio results, zero-length terminal chunks,ReceiveTimeoutError, and propagated transport or session failures.
javascript/src/voice/adapters/pipecat.ts#L408-L423: add JSDoc for Pipecat clean-close, timeout, and socket-failure behavior.javascript/src/voice/adapters/elevenlabs.ts#L826-L902: add JSDoc for ElevenLabs clean-end, timeout, and session-failure behavior.As per coding guidelines: “Use JSDoc comments for all public APIs in TypeScript” and “Document error conditions in code documentation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/voice/adapters/pipecat.ts` around lines 408 - 423, Document the public receiveAudio contract with JSDoc at javascript/src/voice/adapters/pipecat.ts:408-423 and javascript/src/voice/adapters/elevenlabs.ts:826-902, covering the timeout parameter, non-empty audio results, zero-length chunks for clean stream/session termination, ReceiveTimeoutError on timeout, and propagation of transport or session failures. Update both receiveAudio implementations’ documentation; no behavior changes are required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/src/voice/adapters/__tests__/receive-timeout-contract.test.ts`:
- Around line 344-367: Strengthen the assertion in the real call test around
adapter.call(makeAgentInput()) so it verifies the returned AgentReturnTypes
contains a non-empty audio result matching the emitted payload, rather than only
checking that messages is truthy. Keep the test focused on retaining audio sent
before the stream closes.
- Around line 327-342: The test “still fails every receive after a socket error”
must also cover an in-flight receive: start and retain adapter.receiveAudio(30)
before emitting the socket error, then assert that promise rejects with an Error
containing “ECONNRESET” and is not a receive-timeout error. Preserve the
existing assertions for subsequent receives after the socket error.
In `@javascript/src/voice/adapters/pipecat.ts`:
- Around line 579-582: Preserve the original transport error by assigning err
directly to inbox.failure in javascript/src/voice/adapters/pipecat.ts lines
579-582, rather than creating a wrapper Error. Apply the same change to
sessionFailure in javascript/src/voice/adapters/elevenlabs.ts lines 598-601 so
subsequent receive calls retain the original error identity, message, and stack.
---
Nitpick comments:
In `@javascript/src/voice/adapters/pipecat.ts`:
- Around line 408-423: Document the public receiveAudio contract with JSDoc at
javascript/src/voice/adapters/pipecat.ts:408-423 and
javascript/src/voice/adapters/elevenlabs.ts:826-902, covering the timeout
parameter, non-empty audio results, zero-length chunks for clean stream/session
termination, ReceiveTimeoutError on timeout, and propagation of transport or
session failures. Update both receiveAudio implementations’ documentation; no
behavior changes are required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44167138-1ccb-4c25-b341-b291aab3a6b1
📒 Files selected for processing (6)
javascript/src/voice/adapters/__tests__/elevenlabs.test.tsjavascript/src/voice/adapters/__tests__/openai-realtime-spans.test.tsjavascript/src/voice/adapters/__tests__/receive-timeout-contract.test.tsjavascript/src/voice/adapters/elevenlabs.tsjavascript/src/voice/adapters/openai-realtime.tsjavascript/src/voice/adapters/pipecat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- javascript/src/voice/adapters/openai-realtime.ts
|
Worth flagging separately: this PR had never actually run CI. It comes from a fork, so Approved now, and the real jobs are running. Anyone reviewing a fork PR here should check |
|
Status: CI is green for real now (javascript-complete, python-complete, docs-ci all pass) and CodeRabbit has completed its review. Waiting on two things before this can land.
Happy to do the rebase the moment #895 is in. |
langwatch-agent
left a comment
There was a problem hiding this comment.
No findings from static review of the external contribution. I checked the changed receive/error paths and their tests; I did not execute contributor code or install dependencies.
LangWatch-Review: verdict=clean sha=a1f14942acf1007fb6961ba0600cb94abe7751b3 p0=0 p1=0 p2=0 p3=0
…k mislabel The tail-drain fix rested on one assertion that a hard error rejects with a given message. That leaves the two things worth proving untested: that the error reaches the caller intact rather than as a truncated turn, and that the built-in adapters still produce a rejection the drain reads as a deadline. The second gap is the dangerous one. An adapter that rejects its deadline with a plain Error is now read as a hard failure, so a normal end of turn aborts the run, and nothing in the suite notices because each adapter test asserts its own message rather than the class. - Add specs/voice-drain-error-propagation.feature. - Assert semantics on the tail split: the original error object reaches the caller with its class and fields, no agent messages are produced, the receive span is ERROR and not labelled tail_silence, a deadline still keeps the audio collected so far, and a deliberate agent hangup stays a clean end of turn. - Add an adapter contract test driving ElevenLabs, Pipecat and Gemini Live to their real deadlines, faked at the network-client boundary. - Label first_chunk_timeout only on an actual deadline, so a transport that dies before the first chunk is no longer traced as a quiet agent. Matches Python, and the comment above the catch already claimed this behaviour. - Classify by error name alone, so the DOMException from AbortSignal.timeout() qualifies on any runtime and a doubly loaded module cannot defeat instanceof. - Document both turn-ending signals on receiveAudio: a TimeoutError-named rejection, or an empty chunk for a terminal condition that is not a failure.
Narrowing the drain's catch turned an ordinary end of call into a failed run. Pipecat rejected every receive once its socket closed, and the broad catch used to absorb that. Now it propagates, so a bot that hangs up after speaking fails the scenario with "socket closed, no audio available". ElevenLabs (langwatch#648), OpenAI Realtime (langwatch#646) and Twilio (langwatch#695) all converged on the same answer: a stream that has ended yields the empty terminal chunk, and the shared drain exits cleanly. Pipecat was the last one still rejecting. - Resolve a parked receive with the terminal chunk on socket close, and keep returning it once the stream has ended. - Wake a parked receive on disconnect() the same way, matching langwatch#849. - Record a socket ERROR on the inbox so it keeps failing every later receive, carrying the underlying message and cause. A broken transport is not an end of turn however many times it is asked. - Cover all four paths, including a real call() whose stream ends mid-drain.
…ceive paths Three gaps found in review, all the same shape as langwatch#756. `_drainSpokenTurn` is a second drain loop, on the user-simulator side, and it absorbed every rejection from its own `receiveAudio`. A socket close or a server error handed back the audio collected so far as a complete spoken user line. It now breaks only on a receive deadline. Two span tests were ending their turn by pushing a synthetic server `error` event, which is exactly the rejection that must now propagate; they push an empty audio delta instead, which is the real end-of-stream signal and does not go green if the propagation breaks. The ElevenLabs session-error handler resolved parked receives with the empty terminal chunk, so a broken session read as a clean end of turn. It now rejects them, and records the failure so every later receive keeps failing. A clean session end still resolves with the terminal chunk, and keeps doing so once the stream has ended, which is the ordinary shape of an agent hangup (langwatch#839). The contract table covered three adapters. It now covers all six, so no timeout producer can drift back to a plain Error unnoticed. Each entry verified red against its own adapter.
The narrowed catch in `_drainSpokenTurn` had no direct coverage: the two existing tests end their turn on the idle deadline, which passes either way. This one pushes an error after the first audio delta and requires it to reach the caller, so the simulator can never hand back half a sentence as the user's real line. Records the rule as AC7 of the spec: it is per drain loop, not per adapter, and langwatch#623 adds more of them as agent-initiated turns reach the other adapters.
a1f1494 to
19beb66
Compare
19beb66 to
331e22f
Compare
|
@rogeriochaves I rebased this PR onto the latest I also addressed the remaining review findings:
Local validation with Node 22.23.1:
The full-tree Could you please re-review the updated head ( |
Human Review BriefMode: Targeted Review. Closes #756, and the body names the cost precisely: this failure mode hid a P0 through CI, five rounds of automated review, and the original human reproduction. That is the argument for the change, and it is a good one. Decisions being ratified
Must Check
Ask AuthorDoes an adapter that throws a plain Production riskLow for production, high for trust: this changes which scenarios pass. Expect previously green suites to surface real failures, which is the point, and should be said out loud when it ships. |
langwatch-agent
left a comment
There was a problem hiding this comment.
No additional actionable correctness, security, or integrity finding in the external diff. The shared timeout discriminator is applied to both drain loops, while clean close and hard transport-error paths remain distinct in the adapters. I performed static review only; contributor code and dependencies were not executed.
LangWatch-Review: verdict=clean sha=331e22f8c007abd469ecd751bbbe3a96a531a2c9 p0=0 p1=0 p2=0 p3=0
langwatch-agent
left a comment
There was a problem hiding this comment.
External static review: no blocking concern found in the current diff. I did not execute branch code, install dependencies, or run contributor-provided scripts. Residual risk: runtime behavior remains covered by the repository CI.
LangWatch-Review: verdict=clean sha=331e22f8c007abd469ecd751bbbe3a96a531a2c9 p0=0 p1=0 p2=0 p3=0
Why
The TypeScript voice drain treated every error from its tail-silence
receiveAudiocall as a normal end of turn. Transport failures and adapter defects were therefore silently converted into truncated-but-successful agent turns, which is the worst failure mode for a test framework: the scenario keeps running and asserts against a turn that never happened. It is what hid the #697 P0 through CI, five rounds of automated review, and the original human reproduction.Closes #756
What changed
ReceiveTimeoutErroracross the six built-in voice adapters, while accepting any error namedTimeoutErrorfrom a custom adapter.first_chunk_timeoutonly on an actual deadline. A transport that dies before the first chunk is no longer traced as an agent that stayed quiet. This matches Python, and the comment above that catch already claimed the behaviour.DOMExceptionfromAbortSignal.timeout()qualifies on any runtime and a doubly loaded module cannot defeatinstanceof._drainSpokenTurn, the user-simulator's own drain loop, which had the identical defect.VoiceAgentAdapter.receiveAudio: aTimeoutError-named rejection, or an empty chunk for a terminal condition that is not a failure.specs/voice-drain-error-propagation.feature.How it works
The drain catches only errors recognized as receive deadlines. Built-in adapters create the same internal timeout type at their existing deadline sites; errors from sockets, transports, configuration, assertions, or other adapter code bypass that check and keep their original message and stack.
An adapter that ends a turn without failing returns an empty chunk instead. That is this codebase's existing end-of-stream signal (#648, #646, #695, #849), and it is what keeps a deliberate agent hangup (#839) a clean conclusion rather than a failed run.
What narrowing the catch exposed
Three live paths were only safe because the drain absorbed everything. Each is a genuine defect the catch was hiding, and each would have started failing real runs the moment this landed.
Pipecat rejected every receive once its socket closed. Verified against the production
call()wrapper: a bot that speaks and then hangs up failed withPipecatAgentAdapter: socket closed, no audio available, where it previously produced a normal turn. It was the last adapter still rejecting; ElevenLabs, OpenAI Realtime and Twilio had all converged on the terminal chunk already. It now resolves a parked receive with the terminal chunk on close, keeps returning it once the stream has ended, and wakes a parked receive ondisconnect()the same way. A socket ERROR is recorded on the inbox instead, so it keeps failing every later receive with the underlying message and cause.ElevenLabs resolved parked receives with the terminal chunk on a session ERROR, so a broken session read as a clean end of turn and the drain reported the partial audio as complete. Those now reject and the failure is recorded, so every later receive keeps failing. The clean path is unchanged and now also covers the case where the tail probe lands after the close rather than during it, which used to hit the
isConnected()guard and throw "not connected". Harmless while the drain swallowed everything, fatal once it does not, and it would have failed exactly the hangups #839 went out of its way to allow._drainSpokenTurnabsorbed every rejection from its ownreceiveAudio, so a socket close or server error handed back the audio collected so far as a complete spoken user line.Tests
specs/voice-drain-error-propagation.featurebinds three suites.drain-tail-error-propagation.test.tsasserts outcomes rather than "did not raise":voice.audio.receivespan is ERROR and is not labelledtail_silenceReceiveTimeoutError, a customTimeoutError-named error, andAbortSignal.timeout()'sDOMExceptionall end the turn cleanlyErrorvalue propagates instead of being read as a deadlineterminal_chunkend of turn withagentHungUpstill setreceive-timeout-contract.test.tscovers both halves of the adapter contract, faked at the network-client boundary with no adapter privates touched. It drives all six adapters to their real deadlines and asserts the rejection classifies as a receive timeout. This is the guard that matters most: an adapter that rejects its deadline with a plainErroris read as a hard failure, so a normal end of turn aborts the run, and no per-adapter test notices because each asserts its own message rather than the class. It then covers the other half, that a stream which ENDS is an end of turn: the terminal chunk on close, on a later probe after the close, and on a realcall()whose stream ends mid-drain, with a socket error still failing every receive.Every entry was verified red by reverting its own adapter.
voice-spans.test.tsgains the A4-negative, which was previously Python-only.openai-realtime-speak-user-turn.test.tsgains a server error mid-utterance that must reach the caller.Two span tests in
openai-realtime-spans.test.tswere ending their turn by pushing a synthetic servererrorevent, precisely because the loop swallowed everything. They push an empty audio delta instead, which is the real end-of-stream signal and does not go green if the propagation breaks.Test plan
pnpm exec vitest run— 96 files passed, 1 skipped; 1113 passed, 4 skipped.pnpm build:all,pnpm smoke:dist,pnpm lint:all,pnpm lint:lib,pnpm typecheck:allall clean.Anything surprising?
This PR had never actually run CI. It comes from a fork, so
javascript-ci,python-cianddocs-cisat ataction_requiredon every push since the branch was opened, while the check list showed green because the only checks that did run are the firefighting ones that skip themselves. Approved now.Separately,
eslint .reports pre-existing import-order errors across the test tree. CI does not gate on them; extending the lint gate to tests is tracked in #565.