fix: always send asr error when error_handler

[diyuyi-agora]

No description provided.

[claude]

Code Review: PR #1996 - Always send ASR error when error_handler

Summary

This PR refactors error handling in the ByteDance ASR extension to ensure errors are always sent to clients, regardless of whether the error is reconnectable. Previously, only non-reconnectable errors were sent to clients, which could leave clients unaware of transient issues.

✅ Strengths

1. Improved Observability: The change ensures clients are always notified of errors, improving system observability and allowing upstream components to track transient failures.

2. Consistent Error Reporting: Both _handle_error() and _on_asr_error() methods now follow the same pattern - send error first, then attempt reconnection if applicable.

3. Clear Code Structure: The refactored code has better readability with explicit comments explaining the flow:

   • "Always send error regardless of whether it's reconnectable"
   • "If error is reconnectable and not stopped, attempt reconnection"

4. Version Bump: Appropriately incremented version from 0.3.14 to 0.3.15 in manifest.json.

🔍 Code Quality Observations

Good Practices:

• ✅ Maintains consistent error object construction in both methods
• ✅ Preserves all error context (module type, error codes, messages, vendor info)
• ✅ No changes to reconnection logic itself, reducing risk

Minor Considerations:

• The change moves from "reconnect OR notify" to "notify THEN reconnect", which is the correct behavior but increases the number of error notifications during reconnection attempts. This is actually desirable for monitoring but should be documented.

🐛 Potential Issues

1. Error Notification Frequency During Reconnections (extension.py:433, 901)

   • Issue: If reconnection fails repeatedly, clients may receive multiple error notifications for the same underlying issue.
   • Severity: Low - This is actually beneficial for monitoring, but may require rate limiting in high-retry scenarios.
   • Recommendation: Consider adding documentation about the expected error notification behavior during reconnection attempts.

2. No Test Coverage for Error Scenarios

   • Issue: The existing test file (tests/test_volcengine_asr.py) only covers the happy path with successful ASR results. There are no tests verifying:
     • Error notifications are sent for reconnectable errors
     • Error notifications are sent for non-reconnectable errors
     • The sequence of error notification → reconnection attempt
   • Severity: Medium
   • Recommendation: Add test cases covering:

     async def test_reconnectable_error_sends_notification():
         # Verify error notification sent before reconnection
     
     async def test_non_reconnectable_error_sends_notification():
         # Verify error notification sent without reconnection

🔒 Security Considerations

✅ No Security Concerns Identified

• Error messages are properly encapsulated in structured objects
• No sensitive information leakage in error handling
• Error codes are properly validated before use

⚡ Performance Considerations

✅ No Performance Impact

• The change is a control flow refactoring without additional computational overhead
• Error notification (previously conditional) is now unconditional, but this is a negligible cost
• Async operations remain properly awaited

🧪 Test Coverage

⚠️ Missing Test Coverage (see issue #2 above)

Current Coverage:

• ✅ Happy path: ASR result processing
• ✅ Basic integration with mocked WebSocket

Missing Coverage:

• ❌ Reconnectable error handling
• ❌ Non-reconnectable error handling
• ❌ Error notification verification
• ❌ Error notification timing relative to reconnection

📋 Recommendations

1. High Priority: Add test cases for error scenarios to prevent regression
2. Medium Priority: Add a comment in _handle_reconnect() documenting that error notifications are sent before each reconnection attempt
3. Low Priority: Consider adding metrics/logging to track error notification frequency for monitoring purposes

🎯 Verdict

Approval Status: ✅ APPROVED WITH SUGGESTIONS

This is a good refactoring that improves error visibility. The core logic change is sound and follows best practices for error handling in distributed systems. The main improvement needed is test coverage to ensure the new behavior is maintained over time.

Impact Assessment:

• Risk: Low - The change is straightforward and improves observability
• Breaking Changes: None - This is additive behavior that makes the system more informative
• Deployment: Safe to merge - Consider monitoring error notification rates initially

---

Review generated by Claude Code - automated PR analysis