fix(voice/#648): terminal drain on non-audio completion (EL + WebSocket)#693
fix(voice/#648): terminal drain on non-audio completion (EL + WebSocket)#693drewdrewthis wants to merge 4 commits into
Conversation
…bSocket py) The ElevenLabs and generic WebSocket adapters shared an audio-gated receive loop that returned ONLY on an audio frame, so a turn completing without audio drained to the response_timeout deadline and raised — a latent hang surfaced by /sweep during PR #647 (#646 fixed the same anti-pattern in OpenAI Realtime). Mirror the #646/PR647 reference pattern (and the Gemini Live / Pipecat idiom): return an empty AudioChunk on a non-audio terminal so the base _drain_agent_response loop exits cleanly instead of hanging to the deadline. - elevenlabs.py recv_audio: a socket close (ConnectionClosed) and a client_tool_call (tool-only turn, no client_tool_result path) each return an empty chunk. - websocket.py recv_audio: a clean server close (end of stream) returns empty. - elevenlabs.ts onMessage: client_tool_call resolves the active receiver with an empty chunk (socket close was already handled by onSocketClose). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lose, tool-only) Prove the audio-gated drain returns an empty AudioChunk (not a timeout/raise) on a non-audio completion, and that the normal audio path still drains (no regression). - python/tests/voice/test_audio_gated_drain.py (new): EL client_tool_call, EL socket close, and WebSocket socket close each return an empty chunk; normal audio for both adapters still returns the decoded payload. Terminal-case calls use a long recv budget under a short outer asyncio.wait_for ceiling, so an un-fixed adapter fails fast instead of stalling the suite. - elevenlabs.test.ts: add a client_tool_call terminal test; drop client_tool_call from the "swallows unknown events" case (it is now a terminal, not swallowed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughThe ElevenLabs and generic WebSocket voice adapters had a latent hang: their audio-gated drain loops only returned on audio events, so tool-only turns or clean socket closes would wait until timeout. This PR adds ChangesAudio-gated drain hang fix
Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/tests/voice/test_audio_gated_drain.py`:
- Around line 51-79: Add full type annotations to the `_scripted_ws` function
and the nested `fake_recv` function to comply with strict typing requirements.
Change the `frames` parameter type from `list` to `Sequence` from
`collections.abc` for better flexibility, add a return type annotation to
`_scripted_ws`, and add a return type annotation to the `fake_recv` async
function. Import `Sequence` from `collections.abc` at the top of the file if not
already present.
🪄 Autofix (Beta)
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
Run ID: f579411d-8edc-4b90-af52-3235bdb4cf1b
📒 Files selected for processing (5)
javascript/src/voice/adapters/__tests__/elevenlabs.test.tsjavascript/src/voice/adapters/elevenlabs.tspython/scenario/voice/adapters/elevenlabs.pypython/scenario/voice/adapters/websocket.pypython/tests/voice/test_audio_gated_drain.py
…rop rationale Address non-blocking review feedback on PR #693 (no production-logic change): - test_audio_gated_drain.py: parametrize the EL + WebSocket socket-close tests over both ConnectionClosedOK (clean) and ConnectionClosedError (abnormal), since production catches the base ConnectionClosed — both subclasses must terminate the drain cleanly. - elevenlabs.ts: expand the client_tool_call comment to explain why dropping the terminal when no receiver is in flight is safe (a terminal carries no payload; the drain always parks a waiter first) and why queuing an empty sentinel would be worse (a spurious empty turn on the next receiveAudio). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback (Metz/Beck, Fowler) on PR #693 — test-only: - Add test_elevenlabs_tool_only_turn_drain_exits_cleanly: drives _drain_agent_response end-to-end on a tool-only turn and asserts it returns an empty merged turn instead of raising FirstChunkTimeoutError. Guards the bug at the level it was reported (a drain-level hang), above the recv_audio unit tests. Red without the fix (drain hangs to the outer ceiling). - Rename the recv_audio unit tests *_terminates_drain -> *_returns_empty_chunk so the names match what they assert (the recv contract, not the drain loop). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/tests/voice/test_audio_gated_drain.py`:
- Line 93: Add explicit type annotations to the async test function signatures
that are missing them. For the async test functions
test_elevenlabs_client_tool_call_returns_empty_chunk, the test at line 127, the
test at line 168, and the test at line 224, add explicit return type annotations
(typically -> None for test functions). Additionally, for the parametrized test
functions that have a close_cls parameter (at lines 127 and 224), add explicit
type annotations to this parameter. Ensure all function signatures include
complete type information to satisfy pyright type checking in strict mode as
required by the project's coding guidelines.
🪄 Autofix (Beta)
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
Run ID: e6e8ae61-7212-4906-b4ba-0f1505fd0853
📒 Files selected for processing (1)
python/tests/voice/test_audio_gated_drain.py
Review verdict: READYReviewed at: 10-agent fan-out (principles, hygiene, security, test, proof-reviewer + 4 personas/design-soundness + drift) ran at No blocking concerns. No Addressed in this PR (review feedback)
Non-blocking (Decide / New Issue)
Lenses that passed clean
Verdict is prose, not a GitHub approval. |
|
Automated low-risk assessment This PR was evaluated against the repository's Low-Risk Pull Requests procedure and does not qualify as low risk.
This PR requires a manual review before merging. |
Why
The ElevenLabs (hosted ConvAI) and generic WebSocket adapters shared an audio-gated receive loop that returned ONLY on an audio frame, so a turn completing without audio drained to the
response_timeoutdeadline and raised — a latent hang surfaced by/sweepduring PR #647 (which fixed the same anti-pattern in the OpenAI Realtime adapter for #646).Closes #648
What changed
AudioChunkterminal pattern (proven for OpenAI Realtime; same idiom as Gemini Live / Pipecat) rather than inventing a new signal:recv_audioreturns an empty chunk on a non-audio terminal, so the base_drain_agent_responseloop exits through its existing empty-chunk break.elevenlabs.pyrecv_audio/elevenlabs.tsonMessage): treatclient_tool_callas a tool-only terminal — this adapter has noclient_tool_resultpath, so the hosted agent can never follow up with spoken audio. PY now also catchesConnectionClosed; the TS socket-close terminal was already handled byonSocketClose.websocket.pyrecv_audio): a clean server close (end of stream) returns empty instead of lettingConnectionClosedpropagate — the drain only catchesasyncio.TimeoutError, so an unhandled close crashed the turn.Test plan
python/tests/voice/test_audio_gated_drain.py(new) —cd python && PYTHONPATH=. uv run pytest tests/voice/test_audio_gated_drain.py→ 7 passed. Covers ELclient_tool_call→empty, EL socket-close→empty, WebSocket socket-close→empty (each socket-close case parametrized over bothConnectionClosedOKandConnectionClosedError, since production catches the baseConnectionClosed), plus normal-audio no-regression for both adapters. Terminal cases giverecv_audioa long budget under a short outerasyncio.wait_forceiling, so the un-fixed adapter fails fast (hang→timeout / propagatedConnectionClosed) instead of stalling the suite.javascript/src/voice/adapters/__tests__/elevenlabs.test.ts— added aclient_tool_call→empty terminal test (red without the fix:receiveAudiorejects with a timeout);client_tool_callremoved from the "swallows unknown events" case since it is now a terminal.cd javascript && pnpm exec vitest run src/voice/adapters/__tests__/→ 140 passed / 1 skipped.cd javascript && pnpm typecheck→ clean.Human verification
backend-only, no UI surface— this is a pure library/transport fix in the voice adapter drain logic (python/scenario/voice/adapters/{elevenlabs,websocket}.py,javascript/src/voice/adapters/elevenlabs.ts); there is no front-end, route, or visual surface to exercise, so there is no screenshot/recording to attach.To verify by hand:
cd python && PYTHONPATH=. uv run pytest tests/voice/test_audio_gated_drain.py→ 7 passed.cd javascript && pnpm exec vitest run src/voice/adapters/__tests__/→ 140 passed / 1 skipped;pnpm typecheck→ clean.3 failed, 2 passed; TS: the#648 client_tool_calltest times out after ~2s), confirming the tests gate the bug rather than passing vacuously. Restore withgit checkout HEAD -- <same paths>.How I can prove I was successful
No playable artifact — this is a pure library/transport fix. AC1's gating surface is the base drain loop (
_drain_agent_response/drainAgentResponse) exiting on an empty chunk, which the unit tests sit directly on (they assert the returnedAudioChunk(data=b""), not a code-path trace). The proof is falsifiability:origin/mainsource the terminal-case tests FAIL — Python:3 failed, 2 passed; TS: the#648 client_tool_calltest fails on a ~2sreceiveAudio timed out. With the fix, all pass. (Note: CI only ever runs HEAD, which is all-green, so the discriminating red-state is not visible from CI alone — it was verified locally by reverting just the source files and re-running.)client_tool_call(per the issue body) — so the transport-layer unit proof on the gating contract is the correct surface.Anything surprising?
#567(post-interrupt re-engage on EL hosted) is a separate follow-up that also touches the EL adapter; per the plan it should be driven AFTER this lands to avoid conflicts. Localpnpm lintreports a large pre-existing import-order/eslint-disable baseline across many untouched files (identical count with and without this diff) — not introduced here.