-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(stt): guard connect_to_deepgram against False start() (#6302) #6303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ce11b4e
8118ff7
96d1e73
e4527e5
1eade51
67dbf90
0bfe768
67bacb8
604a5e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| """Tests for connect_to_deepgram start() guard (#6302). | ||
|
|
||
| Verifies that connect_to_deepgram returns None when dg_connection.start() | ||
| returns False, preventing dead connections from being passed to callers. | ||
| """ | ||
|
|
||
| import os | ||
| import sys | ||
| from types import ModuleType | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Minimal stubs — only what streaming.py actually needs at import time | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| # Stub database, heavy deps, and deepgram before importing. | ||
| # deepgram stubs must match test_streaming_deepgram_backoff.py pattern to avoid | ||
| # import-order pollution when pytest collects both files in the same process. | ||
| for _mod_name in [ | ||
| 'database', | ||
| 'database._client', | ||
| 'database.redis_db', | ||
| 'database.conversations', | ||
| 'database.memories', | ||
| 'database.users', | ||
| 'firebase_admin', | ||
| 'firebase_admin.auth', | ||
| 'firebase_admin.messaging', | ||
| 'models', | ||
| 'models.other', | ||
| 'models.transcript_segment', | ||
| 'models.chat', | ||
| 'models.conversation', | ||
| 'models.notification_message', | ||
| 'utils.log_sanitizer', | ||
| 'deepgram', | ||
| 'deepgram.clients', | ||
| 'deepgram.clients.live', | ||
| 'deepgram.clients.live.v1', | ||
| 'websockets', | ||
| 'websockets.exceptions', | ||
| ]: | ||
| sys.modules.setdefault(_mod_name, MagicMock()) | ||
|
|
||
| os.environ.setdefault('DEEPGRAM_API_KEY', 'fake-for-test') | ||
| # NOTE: Do NOT set sys.modules['deepgram'].LiveTranscriptionEvents here. | ||
| # MagicMock auto-generates attributes on access, and overwriting would pollute | ||
| # shared pytest state for test_streaming_deepgram_backoff.py's close/error handler tests. | ||
|
|
||
| # Now import the real streaming module | ||
| from utils.stt.streaming import connect_to_deepgram | ||
|
|
||
|
|
||
| class TestConnectToDeepgramStartGuard: | ||
| """Verify connect_to_deepgram returns None when start() returns False.""" | ||
|
|
||
| @patch('utils.stt.streaming.deepgram') | ||
| def test_returns_none_when_start_fails(self, mock_dg): | ||
| """If dg_connection.start() returns False, must return None (#6302).""" | ||
| mock_dg_conn = MagicMock() | ||
| mock_dg_conn.start.return_value = False | ||
| mock_dg.listen.websocket.v.return_value = mock_dg_conn | ||
|
|
||
| result = connect_to_deepgram( | ||
| on_message=MagicMock(), | ||
| on_error=MagicMock(), | ||
| language='en', | ||
| sample_rate=16000, | ||
| channels=1, | ||
| model='nova-3', | ||
| ) | ||
| assert result is None | ||
|
|
||
| @patch('utils.stt.streaming.deepgram') | ||
| def test_returns_connection_when_start_succeeds(self, mock_dg): | ||
| """If dg_connection.start() returns True, returns the connection.""" | ||
| mock_dg_conn = MagicMock() | ||
| mock_dg_conn.start.return_value = True | ||
| mock_dg.listen.websocket.v.return_value = mock_dg_conn | ||
|
|
||
| result = connect_to_deepgram( | ||
| on_message=MagicMock(), | ||
| on_error=MagicMock(), | ||
| language='en', | ||
| sample_rate=16000, | ||
| channels=1, | ||
| model='nova-3', | ||
| ) | ||
| assert result is mock_dg_conn | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,9 +285,16 @@ async def connect_to_deepgram_with_backoff( | |
| logger.warning("Session ended, aborting Deepgram retry") | ||
| return None | ||
| try: | ||
| return await asyncio.to_thread( | ||
| result = await asyncio.to_thread( | ||
| connect_to_deepgram, on_message, on_error, language, sample_rate, channels, model, keywords | ||
| ) | ||
| if result is not None: | ||
| return result | ||
| # start() returned False — retry unless this is the last attempt | ||
| if attempt == retries - 1: | ||
| logger.error('Deepgram start() returned False on all %d attempts — giving up', retries) | ||
| return None | ||
| logger.warning('Deepgram start() returned False (attempt %d/%d), retrying...', attempt + 1, retries) | ||
| except Exception as error: | ||
| logger.error(f'An error occurred: {error}') | ||
| if attempt == retries - 1: # Last attempt | ||
|
|
@@ -361,6 +368,9 @@ def on_unhandled(self, unhandled, **kwargs): | |
|
|
||
| result = dg_connection.start(options) | ||
| logger.info(f'Deepgram connection started: {result}') | ||
| if not result: | ||
| logger.error('Deepgram connection start() returned False — connection not established') | ||
| return None | ||
|
Comment on lines
369
to
+373
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For the permanent case described in the PR (self-hosted DG doesn't support WebSocket streaming) this is fine. However, Consider raising a dedicated exception instead of returning result = dg_connection.start(options)
logger.info(f'Deepgram connection started: {result}')
if not result:
raise Exception('Deepgram connection start() returned False — connection not established')
return dg_connection…and catch/re-raise as |
||
| return dg_connection | ||
| except websockets.exceptions.WebSocketException as e: | ||
| raise Exception(f'Could not open socket: WebSocketException {e}') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NonereturnThe two tests verify that
connect_to_deepgramitself returnsNone/ the connection correctly, but there is no test for the higher-levelconnect_to_deepgram_with_backoff(orprocess_audio_dg) to confirm that aNonereturn fromconnect_to_deepgramis propagated without triggering retries or raising an unhandled exception.Given that
connect_to_deepgram_with_backoffhas its own retry / raise logic, a test that patchesconnect_to_deepgramto returnNoneand asserts the backoff function also returnsNone(with zero retries) would make the contract explicit and catch regressions if the backoff loop is later refactored.