Repository navigation
fix(stt): notice a half-open websocket in the gladia, nabrah, sarvam and slng streams - #7914
Rehansanjay wants to merge 2 commits into
Conversation
…and slng streams Follow-up to livekit#7913, which closed this hole in the three plugins livekit#7900 names. These four are the rest of it. Without a heartbeat aiohttp never notices a socket that died without a FIN/RST: the receive side parks on `ws.receive()` forever, nothing reports a failure, and the stream neither reconnects nor lets a FallbackAdapter move on. The agent just stops hearing the user. gladia, nabrah and sarvam already raise on an unexpected ERROR frame, so the heartbeat is all they were missing. slng swallowed it too -- its `not TEXT` branch continued past it -- so it also raises now, keeping the cause from `ws.exception()`, matching the close branch above it. Two plugins have more than one websocket, and only one each needed this: - gladia's batch connect already passes `ClientWSTimeout(ws_receive=..., ws_close=10)`; the streaming connect in `_run` had no bound. - sarvam ships two streaming STTs and exports both. `stt_streaming.py`'s `RealtimeSpeechStream` already passes a heartbeat; `stt.py`'s `SpeechStream` did not. Not vulnerable, and untouched: baseten, fireworksai and gradium each bound the read with `asyncio.wait_for(ws.receive(), timeout=5)`; deepgram, elevenlabs, inworld, meta, microsoft-ai, openai, simplismart, smallestai, soniox, telnyx and xai already pass a heartbeat. assemblyai is covered by the open livekit#7547. Tests for nabrah and slng assert the socket is opened with the heartbeat, following tests/test_plugin_soniox_stt.py, and both fail without it. gladia's and sarvam's connects are inline in `_run`/`_run_connection` rather than `_connect_ws` methods, so they are not reachable by the test patterns in this repo; each is the same kwarg as a bounded sibling connect in the same file.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| session_url, | ||
| heartbeat=30.0, |
There was a problem hiding this comment.
🔴 Gladia heartbeat failure ends transcription silently
When Gladia's new heartbeat times out, _recv_messages_task logs the ERROR frame and exits normally after the socket closes. _run then completes without raising, so the fallback adapter cannot switch providers.
Learn more
A heartbeat timeout closes the aiohttp socket and delivers an ERROR frame to the waiting reader. The Gladia receive loop logs that frame without raising and then finishes when the closed socket ends iteration in _recv_messages_task. _run treats normal receive-task completion as the end of the stream. No connection error reaches the base stream or fallback adapter.
Example: The Gladia peer stops answering without sending FIN or RST. After an unanswered ping, the receive task logs the ERROR, finishes, and the stream closes rather than signaling provider failure.
Recommended fix: Handle WSMsgType.ERROR in _recv_messages_task by raising APIConnectionError, using self._ws.exception() as the cause when available. Add a test that triggers an ERROR frame and verifies _run propagates a connection failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
I claimed in the PR that gladia already raises on an unexpected ERROR frame. It
does not. Its only ERROR branch is in the batch path, which is also the connect
that was already bounded by ClientWSTimeout -- my scan matched that one and
reported the plugin as covered.
The streaming `_recv_messages_task` had no ERROR branch at all: the frame fell to
`logger.warning("Unexpected message type")`, the task ended normally, `_run` broke
out instead of reconnecting, and the base stream only reports a failure when
`_run` raises. So the heartbeat this PR adds would have detected the dead socket
and the stream would still have died quietly.
It now raises, keeping the cause from `ws.exception()`, matching nabrah and sarvam
-- both of which I re-checked, and whose streaming loops do raise as claimed.
Test in a new file rather than tests/test_plugin_gladia_stt.py, which is gated
behind `pytest.mark.plugin("gladia")` and would skip by default.
Raised by the automated review on the PR.
|
Right, and it caught a false claim in my PR description. Fixed. I wrote that gladia already raises on an unexpected ERROR frame. It does not. Its only ERROR branch is at It raises now, keeping the cause from I re-checked the other two the same way, by the enclosing function rather than by a text match:
So the claim held for those two and only gladia was wrong. Updating the PR description to match. |
This branch has not been deployed
1 participant
Follow-up to #7913. That PR closed this hole in the three plugins #7900 names; I then audited every streaming STT plugin for the same thing, and these four are the rest of it.
The bug, same as #7900
aiohttpdefaultsheartbeattoNone, so a socket that dies without a FIN/RST leaves the receive side parked onws.receive()forever. Nothing raises, the stream never reconnects, andstt.FallbackAdapternever switches. The agent stops hearing the user with no error anywhere.The audit
mainheartbeat=30.0heartbeat=30.0stt.py)heartbeat=30.0heartbeat=30.0+ raise on ERRORasyncio.wait_for(ws.receive(), timeout=5)Two plugins have more than one websocket, and only one each needed this:
ClientWSTimeout(ws_receive=..., ws_close=10). Only the streaming connect in_runhad no bound.stt_streaming.py'sRealtimeSpeechStreamalready passes a heartbeat;stt.py'sSpeechStream, reached throughSTT, did not.nabrah and sarvam already raise on an unexpected ERROR frame, so the heartbeat is all they were missing. gladia did not -- see the correction at the end. slng's
not TEXTbranch continued past it, which is the same hole #7913 found in the other three — so it raises now, keeping the cause fromws.exception()and matching the close branch above it.Tests
nabrahandslngget the test fromtests/test_plugin_soniox_stt.py: assert the socket is opened with the heartbeat. Both mutation-verified — removing the kwarg fails that plugin's test.gladia and sarvam are not tested, and I'd rather say so than imply otherwise. Their connects are inline in
_run/_run_connectionrather than in a_connect_wsmethod, so they aren't reachable by the patterns this repo uses. Each change is the same kwarg as a bounded sibling connect in the same file. If you want them covered, the way is to lift those connects into methods, which is a bigger diff than the fix — happy to do it if you'd prefer that.One thing worth flagging from the audit: baseten, fireworksai and gradium bound the read with a flat 5-second
wait_foronreceive(). That does catch a dead socket, but #5717 is about why a read timeout is the wrong instrument for STT, since a quiet user is the normal state. I haven't touched them, but they may be worth a look separately.Correction after review
I originally wrote that gladia already raises on an unexpected ERROR frame. It does not. Its only ERROR branch is in the batch path, which is also the connect that was already bounded by
ClientWSTimeout-- my audit matched that branch by text and reported the plugin as covered. The streaming_recv_messages_taskhad none, so the frame fell through to "Unexpected message type", the task ended normally, and_runbroke instead of reconnecting: the heartbeat would have found the dead socket and the stream would still have died quietly.Fixed in a follow-up commit with its own test, in a new file because
tests/test_plugin_gladia_stt.pyis gated behindpytest.mark.plugin("gladia"). I re-checked nabrah and sarvam by enclosing function rather than by text match, and both do raise in their streaming loops as described.