Fix ezai asr finalize problem

[Vackwin]

Summary

Brief description of changes

Type of Change

• Bug fix
• New feature
• Documentation update
• Performance improvement
• Code refactoring

Testing

• Tests added/updated
• All tests pass
• Manual testing completed

Documentation

• Documentation updated
• Examples provided if needed

Try to fix finalize problem, remove redundant "<END>" transcript.

[plutoless]

@claude review the pr

[github-actions]

Code Review for PR #2051: Fix ezai asr finalize problem

Thank you for working on fixing the finalize issue in the EZAI ASR extension. I've reviewed the changes and have several observations regarding code quality, potential issues, and best practices.

---

✅ Positive Changes

1. Improved Error Handling: Addition of ModuleErrorVendorInfo provides better error tracking and debugging capability.
2. Better Finalize Approach: Replacing the synthetic <END> message with a silence audio package is a more robust solution that works with the actual ASR protocol.
3. Error Recovery: Adding reconnection trigger on verify errors (line 325) improves resilience.
4. Exception Handling in send_audio: The try-catch block (lines 570-602) prevents crashes from audio send failures.

---

🔴 Critical Issues

1. Removed Reconnection Retry Logic (extension.py:509-522)

Severity: HIGH

The PR removes the can_retry() check before attempting reconnection:

# REMOVED CODE:
if not self.reconnect_manager.can_retry():
    self.ten_env.log_warn("No more reconnection attempts allowed")
    await self.send_asr_error(...)
    return

Problem: Without this guard, the system may attempt infinite reconnections, leading to:

• Resource exhaustion
• Excessive error logging
• Degraded system performance
• Potential memory leaks

Recommendation: Restore the retry limit check. The ReconnectManager exists specifically to prevent infinite reconnection loops.

2. Audio Dumper Lifecycle Issue (extension.py:173, 197-198)

Severity: MEDIUM

The PR moves audio_dumper.start() from start_connection() to on_init():

# NEW location - on_init()
if self.config.dump:
    self.audio_dumper = Dumper(dump_file_path)
    await self.audio_dumper.start()  # Line 176

# REMOVED from start_connection()

Problems:

1. The dumper is started only once during initialization, but it should restart on reconnection
2. After a reconnection, the dumper may be writing to a stale/closed file handle
3. No corresponding stop() call before reconnection attempts

Recommendation: Keep dumper lifecycle management in start_connection() and stop_connection() to ensure proper file handling across reconnections.

3. Dead Code Removal (extension.py:108-114)

Issue: The PR correctly removes CustomWebSocketClient.finalize() but the method is still present in the original code (lines 108-113). This creates confusion about which version is canonical.

Impact: The diff shows removal, but reviewing the actual file shows the method still exists. This inconsistency suggests a merge conflict or incomplete change.

---

⚠️ Design & Logic Concerns

4. Magic Numbers Without Configuration (extension.py:461-463)

# Hardcoded values
mute_duration_ms = 700

Issue: The silence duration is hardcoded at 700ms, which may not be optimal for all use cases or ASR configurations.

Recommendation: Make this configurable, potentially using the existing (but unused) config.mute_pkg_duration_ms field.

5. Incomplete Error Information Extraction (extension.py:304-312)

error_msg = str(error)
error_code = "error"
if isinstance(error, dict):
    error_msg = error.get("message", str(error))
    error_code = str(error.get("status", "error"))

Issue: The error extraction logic only handles dict types, but websocket errors can be Exception objects, strings, or dicts.

Recommendation: Add more robust error handling for different error types.

6. Redundant Method Call (extension.py:403)

The PR changes the finalize method but doesn't update the method being called. Verify all callers are updated.

---

🟡 Code Quality Issues

7. Inconsistent Logging Levels (extension.py:573, 466, 601)

# Line 573: Info level for routine operation
self.ten_env.log_info(f"Dumping {len(buf)} bytes")

# Line 601: Error level in catch block (correct)
self.ten_env.log_error(f"Error sending audio to Deepgram Flux: {e}")

Issues:

• Line 573 logs every audio frame at INFO level, which will flood logs in production
• The error message mentions "Deepgram Flux" but this is an EZAI extension (copy-paste error)

Recommendation:

• Use DEBUG for high-frequency operations
• Fix vendor name in error message

8. Insufficient Connection State Validation (extension.py:464-466)

if not self.is_connected():
    self.ten_env.log_debug("finalize mute pkg: client is not connected")
    return

Issue: Silent failure without error reporting. Callers don't know the finalize failed.

Recommendation: Send an error notification when finalize fails due to disconnection.

---

🔵 Testing & Documentation Concerns

10. Test Coverage

The PR indicates:

• ✅ Manual testing completed
• ❌ Tests added/updated - NOT checked
• ❌ Documentation updated - NOT checked

Recommendations:

1. Add tests for the new finalize behavior
2. Test reconnection scenarios, especially the error-triggered reconnection
3. Verify audio dumper behavior across reconnections
4. Document the change from synthetic <END> to silence package approach

11. Breaking Changes

The removal of CustomWebSocketClient.finalize() and _handle_finalize_api() may affect:

• Any code that directly calls these methods
• Subclasses that override these methods
• Integration tests that rely on the old behavior

Recommendation: Document these as breaking changes and verify no other code depends on them.

---

🟢 Performance Considerations

12. Silence Buffer Allocation (extension.py:479-481)

empty_audio_bytes_len = int(
    mute_duration_ms * self.config.sample_rate / 1000 * bytes_per_sample
)
frame_buf = bytearray(empty_audio_bytes_len)

Analysis:

• For 16kHz audio, 700ms = ~22,400 bytes
• Allocation happens on every finalize call
• Memory allocation is acceptable for this size

Observation: This is fine for the use case, but consider pre-allocating if finalize is called frequently.

---

🔒 Security Considerations

13. Error Message Information Disclosure (extension.py:307-321)

Observation: Error messages from the vendor are forwarded to clients. Verify that EZAI errors don't contain:

• API tokens
• Internal system paths
• Sensitive debug information

---

📋 Summary

Must Fix Before Merge:

1. ⛔ Restore reconnection retry limit check
2. ⛔ Fix audio dumper lifecycle management
3. ⛔ Fix logging levels (remove INFO level per-frame logging)
4. ⛔ Fix copy-paste error message ("Deepgram Flux" → "EZAI ASR")

Strongly Recommended:

5. Make mute duration configurable
6. Add test coverage for new behavior
7. Validate connection state properly in finalize
8. Improve error type handling

Nice to Have:

9. Document breaking changes
10. Pre-allocate silence buffer if performance critical
11. Review error message information disclosure

---

The core approach of using silence packages instead of synthetic <END> messages is sound and aligns with proper protocol design. However, the removal of the reconnection retry logic and the audio dumper lifecycle issues need to be addressed before merging.

Let me know if you'd like me to elaborate on any of these points or if you need code examples for the fixes!

[plutoless]

it's recommended to fix below 2 issues

⛔ Fix logging levels (remove INFO level per-frame logging)
⛔ Fix copy-paste error message ("Deepgram Flux" → "EZAI ASR")