fix: enhance connection state checks

[diyuyi-agora]

No description provided.

[github-actions]

PR Review: fix: enhance connection state checks

Summary

This PR fixes a state synchronization bug in bytedance_llm_based_asr where extension.connected = True while the underlying WebSocket client has already disconnected. Two new unit test files are added. The core fix is sound, but there are a few issues worth addressing.

---

Core Fix — is_connected() ✅

Good fix. Adding self.client.connected to the check correctly handles the race window where the extension-level flag has not yet reflected the client's WebSocket disconnect.

This is the right approach given the callback-driven connection state pattern used in VolcengineASRClient.

---

Bug: Overly broad error suppression in send_audio

The new code silently swallows ALL exceptions when self.stopped=True:

except Exception as e:
    if self.stopped:
        return False          # swallows all exceptions
    self.ten_env.log(LogLevel.ERROR, f"Error sending audio: {e}")
    await self._handle_error(e)
    return False

A stray TypeError, AttributeError, or programming error during shutdown would be silently dropped, making post-mortem debugging impossible. Consider narrowing to known disconnect-related errors, or at minimum emit a debug log:

if self.stopped:
    self.ten_env.log_debug(f"Ignoring send_audio error during stop phase: {e}")
    return False

---

Bug: Misleading test name in test_stop_phase_error_suppression.py

test_send_audio_does_not_suppress_other_stop_phase_errors says "does not suppress" but the assertions prove the error is suppressed (log.assert_not_called(), send_asr_error.assert_not_awaited()). This is either:

1. A wrong test name — should be test_send_audio_suppresses_all_errors_during_stop_phase, or
2. A wrong implementation — only known disconnect errors should be suppressed, and this test should assert that ValueError is still logged

Option 2 is the safer choice and avoids hiding unexpected bugs during shutdown.

---

Question: frame.get_buf in test_connection_state_sync.py

frame = MagicMock()
frame.get_buf.return_value = b"\x00\x01"
await extension._handle_audio_frame(mock_ten_env, frame)

The extension's send_audio uses lock_buf()/unlock_buf(), not get_buf(). If the base class _handle_audio_frame in AsyncASRBaseExtension also uses lock_buf() for buffering, then frame.get_buf is never called and the mock setup has no effect — the test may be passing only because MagicMock returns a truthy default for any attribute access. Please confirm that _handle_audio_frame calls get_buf().

---

Nit: Duplicate test boilerplate

Both new test files contain the same ~20-line sys.path/sys.modules setup block. Minor, but a shared helper or conftest.py fixture would reduce duplication.

---

Minor: Version bump ✅

manifest.json bump 0.3.18 → 0.3.19 is consistent with patch-release convention for bug fixes.

[github-actions]

PR Review posted via Claude Code - see below for full review.

[github-actions]

Critical: Broken test in test_connection_state_sync.py

The test calls extension._handle_audio_frame() and asserts on extension.buffered_frames, but neither attribute exists on BytedanceASRLLMExtension (they appear in soniox_asr_python, not this extension). The assertion also expects log_debug with message 'send_frame: service not connected.' but the actual send_audio code calls log_warn('Not connected to ASR service, attempting to reconnect...'). This test will fail when run as-is.

Minor: Misleading test name

test_send_audio_does_not_suppress_other_stop_phase_errors implies non-disconnect errors are NOT suppressed during stop phase, but the assertions verify they ARE suppressed (log.assert_not_called(), send_asr_error.assert_not_awaited()). Name and contract are inverted. Suggested rename: test_send_audio_suppresses_all_errors_during_stop_phase.

Minor: Silent suppression of all exception types during stop

The new guard silently drops every exception type (including ValueError, AttributeError, etc.) not just expected disconnect errors. Consider either restricting to known disconnect types, or adding a log_debug call so there is still a trace: self.ten_env.log_debug(f'Suppressed error during stop: {e}').

Minor: Commented-out dead code in is_connected()

The finalize grace period block has been commented out for some time. Per AGENTS.md KISS: delete it rather than leave dead code.

Minor: Duplicated test boilerplate

Both new test files contain identical ~20-line module-injection setup. A shared conftest.py in tests/ would eliminate the duplication.

Nit: Version jump

manifest.json goes from 0.3.18 to 0.3.20, skipping 0.3.19. Worth confirming this is intentional.

What looks good

• The is_connected() fix with self.client.connected correctly addresses the race where wrapper state and underlying client state can diverge.
• _result_level_asr_info_fields is clean and the docstring accurately describes its behavior.
• result_level_fields threading through _build_metadata_with_asr_info, _extract_final_result_metadata, and _extract_non_final_result_metadata is consistent.
• test_stop_phase_error_suppression.py tests the intended suppression behavior correctly for the two main cases.