Code Review: PR #1882 - Improve avatar voice_end timing and deepgram reliability

Summary

This PR improves the voice assistant system by replacing debounce timers with event-driven tts_audio_end handling and adds reliability improvements to Deepgram WebSocket ASR. Overall, this is a solid improvement that addresses real timing issues. Below are my findings across several key areas.

✅ Strengths

1. Event-Driven Architecture

• Excellent move from 500ms debounce timer to event-driven tts_audio_end handling
• This provides more precise timing and eliminates arbitrary delays
• The implementation correctly routes tts_audio_end events to avatar extensions

2. Code Cleanup

• Removing dead code from HeyGen (unused queues, is_speaking timeout logic) improves maintainability
• The simplified _loop_input_audio_sender in heygen_avatar_python/extension.py is much cleaner

3. Reliability Improvements

• Auto-reconnect with exponential backoff for Deepgram is a professional pattern
• Silence sender for EOT detection addresses a real-world problem (mic muting causing endless transcripts)
• Keep-alive mechanism prevents WebSocket timeouts

⚠️ Issues & Concerns

HIGH PRIORITY

1. Race Condition in send_voice_end() (anam.py:665, generic.py:570)

# Cancel any pending debounce timer
if self._speak_end_timer_task and not self._speak_end_timer_task.done():
    self._speak_end_timer_task.cancel()
    self._speak_end_timer_task = None

Issue: No await or try-except around the cancellation. If cancellation raises CancelledError, it could propagate.

Fix:

if self._speak_end_timer_task and not self._speak_end_timer_task.done():
    self._speak_end_timer_task.cancel()
    try:
        await self._speak_end_timer_task
    except asyncio.CancelledError:
        pass
    self._speak_end_timer_task = None

2. Missing Null Safety Check (extension.py:322 in anam, extension.py:276 in generic_video)

if self.recorder and self.recorder.ws_connected():
    await self.recorder.send_voice_end()

Issue: While self.recorder is checked, there's no validation that ws_connected() returning True means the connection is still valid when send_voice_end() executes. A disconnect could happen between the check and the call.

Recommendation: Add try-except around send_voice_end() calls or implement connection state locking.

3. Incorrect Reconnect Logic (deepgram_ws_asr_python/extension.py:310)

try:
    await self.start_connection()
    if self.is_connected():
        # Reset delay on success
    else:
        # Exponential backoff
        current_delay = min(current_delay * 2, self.max_reconnect_delay)

Issue: If start_connection() succeeds but is_connected() returns False, the code applies exponential backoff even though no exception was raised. This could indicate start_connection() has logic issues.

Recommendation: Review start_connection() to ensure it either succeeds (and sets connected=True) or raises an exception. The else branch suggests an inconsistent state.

MEDIUM PRIORITY

4. Busy-Wait Anti-Pattern (deepgram_ws_asr_python/extension.py:258)

while True:
    await asyncio.sleep(0.01)  # 10ms intervals

Issue: This is a busy-wait loop that runs continuously, checking conditions every 10ms even when nothing is happening. This wastes CPU cycles.

Better Approach: Use event-driven patterns:

# Instead of polling, use asyncio.Event or asyncio.Condition
silence_needed_event = asyncio.Event()

# Set the event when conditions are met
if should_send_silence:
    silence_needed_event.set()

# In the silence sender
await silence_needed_event.wait()

5. Magic Numbers Should Be Constants (deepgram_ws_asr_python/extension.py:63-64)

self.silence_gap_threshold: float = 0.3  # Start silence after 300ms gap
self.silence_max_duration: float = 2.0  # Send silence for max 2 seconds

Recommendation: Make these configurable via DeepgramWSASRConfig so users can tune behavior without code changes.

6. Resource Leak Risk (deepgram_ws_asr_python/extension.py:245)

if self.silence_sender_task:
    self.silence_sender_task.cancel()
    try:
        await self.silence_sender_task
    except asyncio.CancelledError:
        pass

Issue: If stop_connection() is called multiple times concurrently, tasks could be cancelled twice, or new tasks could be created after cleanup starts.

Fix: Add a lock or atomic flag to prevent concurrent stop_connection() calls.

7. Inconsistent Error Handling

• deepgram_ws_asr_python/extension.py:297 logs errors but continues reconnection attempts indefinitely
• No circuit breaker pattern to stop after N consecutive failures
• Could lead to infinite reconnection loops if credentials are invalid

Recommendation: Add a max retry count or circuit breaker pattern.

LOW PRIORITY

8. TypeScript Type Safety (authInitializer/index.tsx:64)

const graph = graphList.find((g) => g.name === graphParam);
if (graph) {
  const graphId = graph.graph_id || graph.name;

Issue: Using || fallback suggests uncertain data model. What if graph_id is an empty string? Consider using nullish coalescing (??) instead.

Fix:

const graphId = graph.graph_id ?? graph.name;

9. Memory Accumulation (deepgram_ws_asr_python/extension.py:294)

await asyncio.sleep(2.0)  # Check every 2 seconds

Minor: The reconnect monitor runs indefinitely. If the extension is stopped/started multiple times without proper cleanup, multiple monitor tasks could accumulate.

Fix: Already handled by _should_reconnect flag, but ensure on_deinit is always called.

🔒 Security Assessment

✅ No Critical Security Issues Found

• API keys and credentials are properly handled through config
• No SQL injection, XSS, or command injection vectors
• WebSocket connections properly validate state before sending data

🔹 Minor Considerations

1. Reconnection Backoff Cap: The max_reconnect_delay of 30s is reasonable, but for invalid credentials, this means trying every 30s forever. Consider adding credential validation before reconnection.

2. Resource Exhaustion: If input_audio_queue grows unbounded during connection failures, it could lead to memory exhaustion. Consider adding queue size limits with maxsize parameter.

🧪 Test Coverage

❌ Major Gap: No Tests for New Features

The PR adds significant new functionality but includes no new tests:

1. Missing Tests:

   • send_voice_end() method in anam.py and generic.py
   • tts_audio_end event handling in all avatar extensions
   • Silence sender logic in Deepgram
   • Auto-reconnect logic in Deepgram
   • URL parameter parsing in playground

2. Existing Tests:

   • Found basic tests for heygen_avatar_python
   • No tests found for anam_avatar_python or generic_video_python
   • No tests for deepgram_ws_asr_python (different from deepgram_asr_python)

📋 Recommended Test Cases:

# For avatar extensions
async def test_send_voice_end_cancels_pending_timer():
    # Test that send_voice_end() properly cancels debounce timer
    
async def test_tts_audio_end_reason_1_triggers_voice_end():
    # Test that reason=1 in tts_audio_end calls send_voice_end()
    
async def test_tts_audio_end_reason_0_does_not_trigger():
    # Test that other reason codes don't trigger voice_end

# For Deepgram
async def test_silence_sender_activates_after_gap():
    # Test silence sender starts after 300ms gap
    
async def test_reconnect_with_exponential_backoff():
    # Test reconnection delays increase exponentially
    
async def test_reconnect_stops_when_should_reconnect_false():
    # Test reconnection respects shutdown flag

📊 Performance Considerations

✅ Improvements:

• Event-driven voice_end reduces latency compared to 500ms timer
• Removing dead code from HeyGen reduces memory footprint

⚠️ Concerns:

1. Busy-wait loop in silence sender (10ms polling) - see issue Updating texts and adding gif #4 above
2. No back-pressure on input_audio_queue - could grow unbounded
3. Multiple timer tasks - ensure old tasks are properly cancelled

📝 Code Style & Best Practices

✅ Good:

• Consistent logging with contextual prefixes ([DEEPGRAM-SILENCE], [ANAM_TTS_END])
• Descriptive variable names
• Helpful inline comments explaining reason=1 meaning

🔹 Suggestions:

1. Extract magic numbers to config or constants
2. Add type hints to all new methods (e.g., send_voice_end() -> bool)
3. Consider docstrings for complex logic like silence sender
4. Consistent error handling - some places log and continue, others raise

🎯 Recommendations

Before Merge:

1. ✅ Fix race condition in send_voice_end() (HIGH)
2. ✅ Add error handling around avatar WebSocket calls (HIGH)
3. ✅ Review reconnect logic in Deepgram (HIGH)
4. ✅ Replace busy-wait loop with event-driven approach (MEDIUM)
5. ✅ Add basic test coverage for new functionality (MEDIUM)

Follow-up:

1. Make silence thresholds configurable
2. Add circuit breaker pattern to reconnection logic
3. Add queue size limits to prevent memory exhaustion
4. Add comprehensive integration tests

✨ Overall Assessment

Rating: 7/10 - Good improvements with some issues to address

This PR solves real problems (timing precision, reliability) with sound architectural choices (event-driven design, auto-reconnect). The code is generally well-structured and readable. However, there are some synchronization concerns, missing tests, and performance anti-patterns that should be addressed before merge.

The changes align well with improving the voice assistant experience, and once the issues above are resolved, this will be a solid improvement to the codebase.

Reviewed by: Claude Sonnet 4.5
Review Date: 2025-12-16

Pull Request Review

Summary

This PR makes meaningful improvements to avatar timing and ASR reliability. The changes are well-structured and address real issues with debounce-based timing and WebSocket stability. Overall, this is a solid enhancement with a few areas for consideration.

Code Quality & Best Practices

Strengths

✅ Event-driven design: Replacing the 500ms debounce timer with tts_audio_end event handling is a cleaner, more deterministic approach (anam.py:665-688, generic.py:573-595)

✅ Dead code removal: Removing unused queues and state tracking from heygen extension improves maintainability (extension.py removed audio_queue, video_queue, is_speaking, speaking_lock)

✅ Consistent patterns: The send_voice_end() implementation is identical across anam and generic extensions, showing good consistency

✅ Clear documentation: Inline comments explain the rationale for changes (e.g., "NOTE: voice_end is now triggered by tts_audio_end event")

Areas for Improvement

1. Exponential Backoff Implementation (deepgram_ws_asr_python/extension.py:295-332)

# Current implementation in _reconnect_monitor
current_delay = self.reconnect_delay  # Reset on success
# Exponential backoff
current_delay = min(current_delay * 2, self.max_reconnect_delay)

⚠️ Issue: The delay reset logic only happens on successful connection, but current_delay is a local variable that gets reset on every iteration of the while loop. This means exponential backoff won't work as intended.

Recommendation: Store current_delay as an instance variable:

# In __init__
self.current_reconnect_delay = self.reconnect_delay

# In _reconnect_monitor
if self.is_connected():
    self.current_reconnect_delay = self.reconnect_delay  # Reset
else:
    await asyncio.sleep(self.current_reconnect_delay)
    self.current_reconnect_delay = min(
        self.current_reconnect_delay * 2, 
        self.max_reconnect_delay
    )

2. Silent Continue After Max Silence Duration (extension.py:287-293)

if silence_elapsed > self.silence_max_duration:
    self.ten_env.log_info(...)
    silence_start_time = 0.0
    continue  # ⚠️ Goes to top of while loop, sleeps, checks again

The continue here causes the loop to restart immediately. After hitting max duration, the code should wait for real audio to resume before attempting to send silence again. This could lead to tight loop spinning if conditions persist.

Recommendation: Add a state flag or ensure the gap threshold check prevents re-entry.

3. Error Handling in send_voice_end() (anam.py:686)

else:
    self.ten_env.log_error("Failed to send voice_end message")
return success

No exception handling around the _send_message() call. If it raises an exception, the function will crash instead of gracefully handling it.

Recommendation: Add try-catch block:

try:
    success = await self._send_message(end_message)
except Exception as e:
    self.ten_env.log_error(f"Exception sending voice_end: {e}")
    return False

Potential Bugs

Critical

🔴 Race condition in silence sender (extension.py:242-309)

The _silence_sender() task runs in a tight 10ms loop checking self.last_audio_frame_time, which is updated from send_audio() (line 847). Both methods access this without synchronization.

While Python's GIL provides some protection for simple assignments, the compound condition checks could lead to race conditions:

# In _silence_sender (line 269-274)
if (self.last_audio_frame_time > 0 
    and (now - self.last_audio_frame_time) > self.silence_gap_threshold
    and (self.last_interim_text or self.accumulated_segments)):

Impact: Medium - Unlikely to cause crashes but could cause timing inconsistencies

Recommendation: Use asyncio.Lock() or atomic operations for state shared between tasks

Medium

🟡 Missing cleanup check in stop_connection (extension.py:755-774)

The task cancellation uses try/except asyncio.CancelledError and passes, but doesn't check if tasks were actually cancelled before setting to None. If a task is stuck, this could leave dangling tasks.

Recommendation: Add timeout to task cancellation:

if self.silence_sender_task:
    self.silence_sender_task.cancel()
    try:
        await asyncio.wait_for(self.silence_sender_task, timeout=2.0)
    except (asyncio.CancelledError, asyncio.TimeoutError):
        pass

Performance Considerations

Positive Changes

✅ Eliminates unnecessary timeouts: Removing the 500ms debounce delay reduces latency in avatar responses

✅ Removes dead queues: The heygen cleanup removes unused audio_queue and video_queue that were never consumed

Concerns

⚠️ Tight polling loop (extension.py:260)

while True:
    await asyncio.sleep(0.01)  # 10ms intervals

The silence sender runs at 100Hz continuously. While 10ms sleeps are lightweight, this adds constant background activity.

Impact: Low - Modern async runtimes handle this well, but consider increasing to 50ms (20Hz) if 10ms granularity isn't strictly required for audio gap detection.

⚠️ URLSearchParams queried on every render (authInitializer/index.tsx:56-70)

The useEffect that checks URL params doesn't have proper dependencies and runs on every graphList update. The urlParamApplied.current guard helps, but this could be optimized.

Recommendation: Add [] dependency array to useEffect to run only on mount, or move the check to after graphList.length > 0 is confirmed once.

Security Concerns

Low Risk

🟢 URL parameter injection (authInitializer/index.tsx:60)

const graphParam = urlParams.get("graph");
if (graphParam) {
    const graph = graphList.find((g) => g.name === graphParam);

The code safely validates that graphParam exists in graphList before using it, preventing injection of arbitrary graph IDs. This is correct.

✅ No direct security issues identified. The changes don't introduce authentication bypass, XSS, or injection vulnerabilities.

Test Coverage

Missing Test Scenarios

The PR description doesn't mention test coverage. These areas would benefit from tests:

1. Avatar timing tests: Verify that tts_audio_end with reason=1 triggers send_voice_end() within expected timeframe
2. Reconnection tests: Verify exponential backoff behavior and maximum retry limits
3. Silence sender edge cases:
   • What happens when mic mutes mid-speech?
   • Does keep-alive prevent timeouts during 5+ minute silence?
4. URL param tests: Verify ?graph=name correctly selects graph on page load
5. Race condition tests: Concurrent audio frames during silence detection

Recommendation: Add integration tests to agents/integration_tests/asr_guarder/ for the Deepgram reconnection scenarios.

Specific File Comments

property.json & rebuild_property.py

✅ Correct routing fix: Adding avatar to tts_audio_end destination is essential for the new event-driven approach. The comments clearly explain this is for timing coordination, not just analytics.

heygen_avatar_python/extension.py

✅ Excellent cleanup: Removing 74 lines of unused code (idle_reset_task, speaking_lock, is_speaking state machine) significantly improves maintainability. The simpler audio sender loop is easier to reason about.

deepgram_ws_asr_python/extension.py

⚠️ Complex state machine: The silence sender now manages multiple state variables (silence_start_time, last_keepalive_time, silence_gap_threshold). Consider extracting this into a separate SilenceSenderManager class for better testability.

playground authInitializer

✅ Minor feature: URL param support is a nice UX improvement for deep linking. Implementation is straightforward.

Recommendations Priority

High Priority

1. Fix exponential backoff implementation (store delay as instance variable)
2. Add exception handling to send_voice_end()
3. Add integration tests for reconnection behavior

Medium Priority

4. Review silence sender state management for race conditions
5. Consider extracting silence sender logic to separate class
6. Add timeout to task cancellation in stop_connection()

Low Priority

7. Optimize tight polling loop interval (10ms → 50ms if acceptable)
8. Add dependency array to URL param useEffect

Overall Assessment

Rating: Approve with Minor Changes

This PR delivers meaningful improvements:

• More deterministic avatar timing
• Better ASR reliability with reconnection
• Cleaner codebase (removed dead code)
• Useful UX enhancement (URL params)

The issues identified are mostly minor and can be addressed in follow-up PRs, though fixing the exponential backoff would be ideal before merge.

Estimated Risk: Low - Changes are well-isolated to specific extensions and don't affect core framework.

Pull Request Review

This PR improves avatar voice timing synchronization and Deepgram WebSocket reliability. Overall, the changes are well-structured and address real-world production issues. Below are my findings:

✅ Positive Aspects

1. Improved Avatar Timing Architecture

• Event-driven approach: Replacing the 500ms debounce timer with tts_audio_end event handling is a significant improvement. This provides deterministic timing based on actual TTS completion rather than arbitrary delays.
• Consistent pattern: The implementation is consistent across all three avatar extensions (anam, generic, heygen), making the codebase more maintainable.

2. Deepgram Reliability Improvements

• Silence sender for EOT: The solution to handle muted mic scenarios (line 242-328 in deepgram_ws_asr_python/extension.py) is well-designed with proper threshold management.
• Auto-reconnect with exponential backoff: The reconnection logic (line 330-371) follows best practices with proper bounds checking.
• Keep-alive mechanism: Prevents WebSocket timeouts during idle periods.

3. Code Quality

• Comprehensive logging with appropriate log levels
• Clear comments explaining the rationale
• Proper task cancellation in cleanup methods

🔍 Potential Issues

1. Race Condition in Avatar Extensions (Medium Priority)

Location: anam_avatar_python/extension.py:321-326, generic_video_python/extension.py:275-280

if self.recorder and self.recorder.ws_connected():
    await self.recorder.send_voice_end()
else:
    ten_env.log_warn(
        "[ANAM_TTS_END] Recorder not ready, cannot send voice_end"
    )

Issue: There's a TOCTOU (time-of-check-time-of-use) race condition. The recorder could disconnect between the check and the send.

Recommendation: Wrap the send in a try-except block:

if self.recorder:
    try:
        await self.recorder.send_voice_end()
    except Exception as e:
        ten_env.log_warn(f"Failed to send voice_end: {e}")

2. Memory Leak Risk in Deepgram (Low Priority)

Location: deepgram_ws_asr_python/extension.py:242

Issue: The _silence_sender() task runs in an infinite while True loop without checking a stop condition. While tasks are cancelled in stop_connection(), if cancellation fails, this could continue running.

Recommendation: Use a control flag:

self._silence_sender_running = True

async def _silence_sender(self):
    while self._silence_sender_running:
        # ... existing code

3. Potential Deadlock in HeyGen (Low Priority)

Location: heygen_avatar_python/extension.py:76-77

Issue: The removed speaking_lock was protecting state, but now there's no synchronization mechanism. While the removal simplifies the code, concurrent calls to _loop_input_audio_sender() and shutdown could cause issues.

Recommendation: Verify that the queue-based approach provides sufficient thread safety, or add a comment explaining why synchronization is unnecessary.

4. Hardcoded Silence Frame Size (Low Priority)

Location: deepgram_ws_asr_python/extension.py:252-254

# 10ms of silence at 16kHz mono (16-bit = 2 bytes per sample)
# 16000 samples/sec * 0.01 sec * 2 bytes = 320 bytes
silence_frame = bytes(320)

Issue: This is hardcoded for 16kHz but self.config.sample_rate could be different.

Recommendation: Calculate dynamically:

silence_duration_ms = 10
bytes_per_sample = 2
samples = (self.config.sample_rate * silence_duration_ms) // 1000
silence_frame = bytes(samples * bytes_per_sample)

🔒 Security Concerns

No major security issues identified. The changes don't introduce new attack surfaces. However:

1. WebSocket reconnection: The auto-reconnect feature is good, but ensure the api_key isn't logged in error messages during reconnection attempts (appears to be handled correctly).

2. Resource exhaustion: The silence sender runs continuously - ensure it cannot be exploited to cause excessive WebSocket traffic (current implementation has appropriate throttling with keepalive_interval).

⚡ Performance Considerations

1. Silence Sender Overhead (Minor)

Location: deepgram_ws_asr_python/extension.py:260

The silence sender wakes up every 10ms. While this is acceptable, consider:

• Using longer intervals when no speech is pending
• Leveraging event-driven wakeup instead of polling

2. Queue Growth in Avatar Extensions

Location: anam_avatar_python/extension.py:347

The input_audio_queue has no size limit. During network issues, this could grow unbounded.

Recommendation: Set a maxsize:

self.input_audio_queue = asyncio.Queue(maxsize=100)

3. Async Task Creation

Location: Multiple files

Tasks are created with asyncio.create_task() but not always tracked. This is acceptable for fire-and-forget tasks, but consider storing references for better lifecycle management.

🧪 Test Coverage

Concern: This PR doesn't include tests for the new functionality:

1. Missing test cases:

   • tts_audio_end event handling with reason=1
   • Silence sender activation when mic is muted
   • Auto-reconnect with exponential backoff
   • Graph routing changes (tts_audio_end to avatar)

2. Recommendation: Add integration tests to agents/integration_tests/ following the guarder pattern, or add unit tests for the new methods.

📋 Code Style & Best Practices

Follows TEN Framework Conventions ✅

• Proper use of async/await patterns
• Appropriate logging categories (LOG_CATEGORY_VENDOR)
• Consistent error handling

Minor Suggestions:

1. Magic numbers: The values 0.3, 2.0, 5.0 in silence sender could be config parameters
2. Import placement (line 303 in anam_avatar_python/extension.py): import json is inside a method - consider moving to top-level
3. Type hints: Good use of type annotations, but Optional could be replaced with | None for consistency with newer Python style (already used elsewhere)

🎯 Graph Configuration Changes

Location: property.json and rebuild_property.py

The routing changes to send tts_audio_end to avatar extensions are correct and well-documented. The comments clearly explain why this is needed.

🎨 Playground URL Parameter Feature

Location: playground/src/components/authInitializer/index.tsx

Review:

• Clean implementation using useRef to prevent double application ✅
• Handles edge case where graph name doesn't exist ✅
• Good comment explaining the UUID vs name discrepancy ✅

Minor suggestion: Consider URL decoding:

const graphParam = decodeURIComponent(urlParams.get("graph") || "");

📊 Summary

✅ Recommendation

APPROVE with suggestions

The changes solve real production issues and improve system reliability. The identified issues are minor and can be addressed in follow-up PRs or before merge. The architecture improvements (event-driven voice_end, auto-reconnect) are solid enhancements.

Before Merge:

1. Consider the race condition fix in avatar extensions
2. Add basic integration tests for the new features
3. Review the silence sender stop condition

Post-Merge:

1. Monitor WebSocket reconnection behavior in production
2. Track silence sender performance metrics
3. Consider making timing thresholds configurable

Great work on improving the reliability of the voice assistant! 🎉