fix: tts duplicate error issue

[YiminW]

No description provided.

[github-actions]

Review of fix/tts_duplicate_error_issue

The core insight is correct and worth fixing: a vendor error frame is not necessarily terminal, and letting it drive tts_audio_end caused the terminal transition to fire early and then get attributed to whichever request was current when the real session boundary arrived. Centralizing that transition in _send_session_end() guarded by send_end_text, and having EVENT_TTS_END carry its own TTSAudioEndReason instead of b"", are both improvements.

Findings ordered by severity, verified against rime_tts/extension.py, rime_tts/rime_tts.py, and the two integration tests.

---

1. BLOCKING: the error path no longer releases stop_event, so request_tts can hang

The EVENT_TTS_ERROR branch in extension.py used to end with stop_event.set(). That is gone, and EVENT_TTS_END is now the only thing that sets it. But request_tts waits without a bound:

if self.sent_tts:
    self.stop_event = asyncio.Event()
    await self.stop_event.wait()   # no timeout

The new design assumes EVENT_TTS_END always eventually arrives. It does not, because _send_session_end() early-returns when send_end_text is False:

if not self.response_msgs or not self.send_end_text:
    return

send_end_text is only set True inside _send_text_internal, after ws.send(message_json) succeeds and it reaches the text_input_end branch. So for a request with text_input_end=True where the socket dies or _send_loop raises before the eos is written: request_tts is already in stop_event.wait(), send_end_text is still False, the ConnectionClosed handler calls _send_session_end() which no-ops, and the outer except Exception in _process_websocket only logs. No EVENT_TTS_END is ever queued. The request hangs, and since request_tts is driven serially, later requests block behind it until a flush or cancel_tts happens to rescue it.

A second variant has no rescue at all: a vendor sends a fatal error frame and then stops talking without closing the socket. Before this PR the error branch set stop_event and the request completed as ERROR. Now nothing terminates it.

Options, roughly in order of preference:

• Emit a terminal event unconditionally when the session is torn down with a request in flight, that is, also in the finally of _process_websocket and on the non-ConnectionClosed exception path. The send_end_text guard is the right idempotency mechanism against duplicate ends, but right now it also suppresses the only end.
• Add a bounded asyncio.wait_for(self.stop_event.wait(), timeout=...) in request_tts with an ERROR finalization on timeout, as a backstop.
• Optionally keep setting stop_event for errors classified fatal, reserving the new non-terminal behavior for non-fatal advisory errors.

---

2. Queue type annotations are now stale

_send_session_end puts a TTSAudioEndReason into the queue, but every annotation still says bytes | int: extension.py:49, and the response_msgs parameters and attributes on both RimeTTSynthesizer and RimeTTSClient, plus the reassignment in close(). If TTSAudioEndReason is an IntEnum this happens to type-check, which is exactly why it is easy to leave stale. Widening it keeps the payload contract readable, and that implicit contract is what caused the original bug.

---

3. Unexpected disconnect reports reason=ERROR with no ModuleError

_send_session_end(TTSAudioEndReason.ERROR) reaches _handle_tts_audio_end(reason=ERROR, error=None), so finish_request() gets error=None. The pre-PR error path passed a populated ModuleError with vendor_info. Consumers now see a request that ended in error with no diagnostic attached. Consider threading an optional ModuleError through the payload, or emitting a matching error message alongside it.

---

4. A request that hit a vendor error is now reported as a normal completion

More a design question, but it deserves an explicit answer in the description, because the new test pins it:

assert tester.error_request_ids == ["request-3"]
assert tester.request3_end_reasons == [TTSAudioEndReason.REQUEST_END]

For a genuinely advisory error that is right. For an error that truncated the audio, REQUEST_END tells consumers the turn finished cleanly, and the only signal otherwise is a separate error message they may not correlate. Can advisory and truncating errors be distinguished in the Rime protocol? If not, is REQUEST_END the safer default for downstream state machines?

Related, the fallback in _loop:

reason = data if isinstance(data, TTSAudioEndReason) else TTSAudioEndReason.REQUEST_END

Since _send_session_end is now the sole producer and always supplies a reason, this can only trigger on a bug, and it turns that bug into something that looks like a clean finish. A log_warn in the else branch would make it debuggable.

---

5. _check_dump_file_number is now dead code

Removing the call at test_append_interrupt.py:599 leaves the roughly 50-line method at line 628 with no callers. Either delete it or note why it stays. It also contains a blocking time.sleep(5) inside an async data callback, so deleting it is a side benefit.

The replacement is a comment with no assertion. _check_final_dump_file_number still runs at the end, so the terminal state is covered, but the mid-flush invariant is now unchecked. If the old assertion was racing the writer buffering, an event-driven check (wait for the flush, then assert) would preserve the coverage instead of dropping it.

---

6. Loosening the metadata assertion weakens an unrelated test

In test_append_input_without_text_input_end.py, strict equality became a subset check, so extra unexpected keys in received_metadata now pass silently. That may be intentional to let the extension add keys, but the error message still reads Expected: ... Received: ... as though it were equality, which will confuse whoever hits it. And the change is unrelated to the Rime fix, so it needs a line of justification in the description. If the intent is that expected keys must be present and match while extras are allowed, say that in the message and in a comment.

---

7. New test file: brittle timing and white-box coupling

Good that the two changed behaviors are pinned directly (the except ModuleVendorException: raise re-raise, and the reason-carrying END). Concerns:

• Sleep-based ordering. The main test depends on sleep(0.02) losing a race against request_tts recording text_input_end, and on sleep(0.2) sequencing request-4 audio after the request-3 close at sleep(0.1). Under CI load those margins will invert and it will flake, most likely as a hang until the harness timeout. Prefer asyncio.Event handshakes between the fake websocket and the tester.
• __new__ plus attribute injection, and monkeypatching _handle_server_message, means the big test re-implements the receive loop collaborators rather than exercising the real path. It would still pass if the real error classification in _handle_server_message regressed. The two small unit tests are the higher-value ones; consider whether the integration-style test earns its brittleness.
• ConnectionClosedOK(None, None) relies on the legacy websockets constructor signature and breaks on the modern API. Worth a comment pinning the intent.
• _TenEnvStub has no log_info, so adding an info log anywhere on this path fails the test with an unrelated AttributeError. A __getattr__ no-op or MagicMock is sturdier.
• Some files in this tests/ directory carry the Apache header (__init__.py, conftest.py, test_connection_lifecycle.py) and others do not; adding it here matches the newer files.

---

Minor

• Per AGENTS.md, commit and PR types are lowercase: fix: tts duplicate error issue, not Fix:.
• The PR body is empty. Since this changes terminal-transition semantics and loosens two existing assertions, a short note on the duplicate-error symptom, how to reproduce, and why the test changes are safe would help review and later bisects.
• Version bumps in manifest.json and pyproject.toml are consistent at 0.4.12.
• Pre-existing but adjacent: _handle_tts_audio_end wraps its whole body in if self.request_start_ts is not None, so a terminal event arriving when that is unset is silently dropped. Now that more paths funnel into this method, a log_warn on the else would help.

---

Overall the direction is right and the _send_session_end consolidation is a real improvement. Item 1 is the one I would want resolved before merge: the removed stop_event.set() was load-bearing for cases where no session boundary ever arrives.