Skip to content

feat: support cartesia enable words and add metrics for some tts#2121

Merged
wangyoucao577 merged 5 commits into
mainfrom
dev/cartesia_tts_words
Apr 2, 2026
Merged

feat: support cartesia enable words and add metrics for some tts#2121
wangyoucao577 merged 5 commits into
mainfrom
dev/cartesia_tts_words

Conversation

@YiminW

@YiminW YiminW commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@YiminW
YiminW force-pushed the dev/cartesia_tts_words branch 4 times, most recently from 2466aaa to b067f53 Compare March 31, 2026 02:56
@YiminW

YiminW commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author
image

@github-actions

Copy link
Copy Markdown

test

@github-actions

Copy link
Copy Markdown

full review pending - ignore this placeholder

@github-actions

Copy link
Copy Markdown

Code Review - Part 1 of 2 (Critical and High severity)

CRITICAL

  1. aiohttp not added to requirements.txt

cartesia==2.0.15 was removed, but aiohttp (which the entire new implementation depends on) was not added. The extension will fail to import after this change.


HIGH

  1. Audio end signal can complete the wrong request

In _process_audio_data, the end-of-audio signal (audio_data is None) is acted on unconditionally without checking request_id against self.current_request_id. Audio data chunks for old requests are correctly skipped, but the None sentinel is not guarded. When cancel_tts() is called, old_client.stop() puts (None, old_request_id, 0) into pcm_queue. That sentinel can later drain through _run_with_restart into the new client's consumer loop and trigger a spurious early completion of the new request. A request_id guard is needed here.

  1. API key sent in every WebSocket message payload

_build_request_payload extracts known structural keys from params but does not strip api_key. The remaining params loop then forwards api_key in every request payload. The key is already used for authentication via the URL query string; sending it in message bodies is redundant and adds unnecessary exposure. Strip it from the payload per the AGENTS.md pattern.

  1. base_url misplaced inside params in property.json

base_url was added under params in property.json, but it is a top-level CartesiaTTSConfig field, not a Cartesia API parameter. As a result, self.config.base_url stays empty string (falling back to the hardcoded default correctly), but base_url also ends up in self.config.params and gets forwarded to the Cartesia API in every request payload (see issue 3). Either remove it from property.json (the fallback default is already wss://api.cartesia.ai) or promote it to the top level of the JSON object.

@github-actions

Copy link
Copy Markdown

Code Review - Part 2 of 2 (Medium, Low/Nits, and Test feedback)

MEDIUM

  1. API key embedded in WebSocket URL query string

URL query parameters appear in proxy/load-balancer access logs, distributed tracing, and error reporting. The Cartesia WebSocket API also accepts an X-Api-Key header. Consider passing the key via aiohttp's headers parameter instead of the URL query string to reduce log exposure.

  1. Using TypeError as flow control in stop()

Putting bare None into typed queues so consumers raise TypeError on unpack is fragile. The exception will appear in logs as a confusing error, and any future refactoring of the consumer unpack will silently break shutdown. A clean sentinel tuple like (None, empty_string, 0) would let consumers detect shutdown without relying on an exception.

  1. request_seq_id_map grows without bound

A new entry is written on every request but entries are never removed. Pop old entries when finishing a request.

  1. Dead code: audio_end_sent flag

audio_end_sent is initialised and reset to False in _reset_tts_request_info() but is never set to True anywhere in this diff. It appears to be a leftover from a refactor or a missing write path.


LOW / NITS

  1. Commented-out production code in _handle_ws_message

The block handling done in chunk messages is commented out. Add a TODO explaining why if it is still under investigation, otherwise remove it before merging.

  1. _validate_first_timestamp semantics mismatch

The docstring says First text start_ms == first audio frame timestamp but the implementation only enforces >= (returns False only when text starts before audio). Update the docstring to match the actual rule, or add a tolerance check in both directions if equality is truly intended.

  1. Version jump skips 0.8.0

The version was bumped from 0.7.0 to 0.8.1, skipping 0.8.0. Minor, but can be confusing in changelogs. Consider 0.8.0 or 1.0.0 given the scope of this rewrite.

  1. _last_word_end_ms not cleaned up on cancel

cancel() clears TTFB tracking but not _last_word_end_ms[request_id]. The entry is only removed on a done message, which will not arrive for a cancelled context. A re-used request ID could acquire a spurious leading space word.


TEST FEEDBACK

  • The new parametrised test_handle_timestamps_spacing_cases is a solid addition with good CJK, punctuation, and boundary coverage.
  • test_flush_logic removed the assertion that calculated audio duration matches request_total_audio_duration_ms from the event. Was this intentional (non-deterministic in mock) or should it be preserved with a looser tolerance?
  • The _make_mock_client helper is defined but only used in test_dump_functionality. The other tests (test_flush_logic, test_ssml_presets, test_ssml_metadata_overrides) duplicate the same setup boilerplate. Consolidating around _make_mock_client would reduce maintenance burden.

Overall the architecture change is an improvement and the test suite has been thoughtfully updated. The requirements.txt omission and the audio-end race condition are the most pressing items to address before merge.

@github-actions

Copy link
Copy Markdown

test

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown

deleting test

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown

PR Review: feat: support cartesia enable_words and add metrics for some TTS

Overview

This PR delivers two main features:

  1. Cartesia TTS rewrite - drops the Cartesia SDK in favor of raw aiohttp WebSockets, adding full-duplex producer/consumer queues, auto-reconnect, and word-level timestamp support (enable_words)
  2. Metrics for google_tts, qwen3_tts, rime_tts - consistent addition of metrics_add_recv_audio_chunks, metrics_add_output_characters, and send_usage_metrics

The overall direction is good. Below are the concerns found.


Bugs

1. _base_start_ms and _all_samples are single-context - breaks under concurrent contexts

cartesia_tts.py in _handle_ws_message has these fields shared across all context IDs. Cartesia WebSocket supports multiplexed contexts (different context_id values). If a second request arrives while the first is still streaming, _base_start_ms will be overwritten, corrupting timestamps. Fix: key these per context_id like _last_word_end_ms already does.

2. asyncio.create_task fire-and-forget without stored references

In on_init and cancel_tts, asyncio.create_task calls for client.start(), _run_with_restart(_process_audio_data), and _run_with_restart(_process_transcription) are not stored. Python 3.10 docs warn that strong references are required to avoid GC. There is no way to cancel() them in on_stop. Task handles should be stored and cancelled in on_stop.

3. Incomplete request cleanup in error_callback

error_callback calls send_tts_audio_end + finish_request only when has_received_text_input_end is True. When False, only send_tts_error is called with no finish_request. If the error fires mid-stream before text_input_end, the request is left hung. The base class likely expects finish_request for every started request.


Code Quality

4. Misleading variable name _all_samples

The formula stores byte_count / 2 * 1000, not an actual sample count. The downstream division by sample_rate gives milliseconds. A clearer name or comment would help.

5. Inconsistent character-metrics method

In _process_transcription (cartesia), the call is metrics_add_input_characters. In google_tts, qwen3_tts, and rime_tts the call is metrics_add_output_characters. If these measure the same metric, the call sites should be consistent.

6. Callbacks as closures inside on_init

error_callback, fatal_error_callback, and ttfb_metrics_callback are async closures inside on_init. Making them proper methods would be simpler and more testable.


Test Issues

7. _validate_first_timestamp docstring/logic mismatch

In test_subtitle_alignment.py, the docstring says equality (==) but the check only requires first_text_start >= first_audio_ts. The docstring and success message should be updated to say >=.

8. first_audio_frame_ts can be None at comparison

self.first_audio_frame_ts is typed as int | None. _run_validations does not guarantee it is non-None after the audio_frames length guard. A simple assert after the guard would clarify the invariant and satisfy a type checker.


Minor / Nits

  • _run_with_restart restarts after clean exit: The wrapper sleeps before checking _is_stopped after STOP_SENTINEL. Checking _is_stopped before the sleep would avoid one extra sleep cycle on graceful shutdown.

  • Dead CartesiaTTSApiException: Defined in cartesia_tts.py but no longer raised after SDK removal. Per the YAGNI principle in AGENTS.md, delete it.

  • property_subtitle_alignment.json path: Confirm path resolution works when tts_guarder integration test runner sets config_dir (file lives in tests/configs/ but is referenced from integration_tests/tts_guarder/).


Summary

The metrics additions to google_tts, qwen3_tts, and rime_tts are clean and straightforward. The Cartesia rewrite is architecturally sound but has correctness issues worth addressing before merge: shared per-context timestamp state (1), dangling task references (2), and incomplete error cleanup (3).

@YiminW

YiminW commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author
image

@github-actions

Copy link
Copy Markdown

Code Review — PR #2121: Cartesia TTS full-duplex rewrite + enable_words

This is a substantial and well-structured refactor. Replacing the Cartesia Python SDK with a raw aiohttp WebSocket client gives much more control over the connection lifecycle and enables the full-duplex queue architecture needed for word timestamps. The new integration test for subtitle alignment is a great addition. A few issues worth addressing below.


🐛 Bugs

1. asyncio.wait(FIRST_EXCEPTION) prevents clean shutdown of channel tasks

cartesia_tts.py_connection_loop:

done, pending = await asyncio.wait(
    self._channel_tasks,
    return_when=asyncio.FIRST_EXCEPTION,
)

FIRST_EXCEPTION returns when a task raises or when all tasks complete. If _send_text_loop returns normally after receiving the None shutdown sentinel, asyncio.wait will not return until _receive_loop also finishes or raises. This prevents a clean reconnect cycle when the send side exits first. asyncio.FIRST_COMPLETED is the safer choice here to ensure both tasks are torn down promptly when either one exits for any reason.


2. Silent request drop when client is None

extension.pyrequest_tts:

if self.client is None:
    # Will be handled by _run_with_restart; skip this request
    return

The comment is misleading — _run_with_restart wraps the consumer tasks, not request submission. Text inputs queued here are completely lost. The window where client is None (between cancel_tts tearing down the old client and the new one being ready) could span several hundred milliseconds. Consider queuing the text input to a local buffer and flushing it once the client is ready, or at minimum log a warning.


3. Empty transcript sent to Cartesia for context-close

cartesia_tts.py_send_one_text:

# Empty text with text_input_end — close the context.
# Cartesia requires a transcript field, so send a single space.
close_payload = self._build_request_payload("", context_id)

The comment says "send a single space" but the code passes "". The Cartesia API requires a non-empty transcript. This should be " " (a space) as the comment says, otherwise the API may return an error.


4. _all_samples stores num_samples × 1000, not samples

cartesia_tts.py_handle_ws_message:

self._all_samples += float(len(audio_bytes)) / 2 * 1000

The comment says "Accumulate samples" but the variable accumulates samples * 1000. The later division by sample_rate cancels the 1000 factor and produces correct milliseconds, so the math is right — but the naming is very confusing. Consider renaming to _accumulated_audio_ms and making the intent explicit.


5. Shared _base_start_ms / _all_samples are per-instance but reset per done message

If two contexts are in-flight simultaneously (a valid scenario in the Cartesia multiplexed WebSocket protocol), receiving a done for context A resets _base_start_ms = 0.0 and _all_samples = 0.0, corrupting timestamps for context B that may still be receiving audio. These should be keyed by context_id like _last_word_end_ms already is.


6. assert in production code path

cartesia_tts.py_send_text_loop and _receive_loop:

assert self._ws and not self._ws.closed

assert statements are silently removed when Python runs with -O (optimise flag). Replace with an explicit guard + exception or early return.


⚠️ Potential Issues

7. pending_config_update read without lock

extension.pyrequest_tts:

if self.pending_config_update:
    await self._apply_config_update(self.pending_config_update)

pending_config_update is written under config_update_lock in update_configs but read here without acquiring the lock. Under Python's GIL this is unlikely to cause data corruption, but it is a correctness issue if update_configs is called concurrently.


8. _pending_inputs can cause duplicate messages on reconnect

cartesia_tts.py_send_one_text:

except Exception:
    self._pending_inputs.append(t)
    raise

If the send raises after partially writing (e.g. an OSError mid-frame), or if the same input is appended in multiple failure paths, it will be replayed on the next connection causing duplicate TTS synthesis. Consider clearing an input from _pending_inputs only after confirmed delivery, or tracking which inputs have been confirmed.


9. last_text_end_ms resets on task restart

extension.py_process_transcription:

last_text_end_ms = 0
while True: ...

This is a local variable. If _run_with_restart triggers a restart of _process_transcription mid-stream (e.g. after a transient error), last_text_end_ms resets to 0, potentially causing the next space-word to be emitted at timestamp 0 instead of the correct continuation point.


🧪 Test Issues

10. asyncio.sleep(0.5) in subtitle alignment test is fragile

test_subtitle_alignment.py:

await asyncio.sleep(0.5)
ten_env.log_info("Running validations")
self._run_validations(ten_env)

A 500ms wait after tts_audio_end is not reliable — under load or on slow CI runners the last tts_text_result may not have arrived. Consider waiting for a sentinel (e.g. a tts_text_result with turn_status == 1) rather than sleeping.

11. _validate_first_timestamp checks < not ==

test_subtitle_alignment.py:

if first_text_start < first_audio_ts:
    return (False, ...)

The class docstring says rule 1 is start_ms == first audio frame timestamp, but the check only fails when text comes before audio, not when it comes much later. If this is intentional (text can arrive after audio) update the docstring; otherwise tighten the check with a tolerance window.


🔍 Minor / Style

  • _requires_reconnection missing enable_words: if enable_words is toggled, the add_timestamps field in the payload changes, but _requires_reconnection doesn't check it. Depending on whether Cartesia respects per-message add_timestamps changes, this may not require reconnection, but it's worth a comment.

  • Version bump 0.7.0 → 0.8.1 (skipping 0.8.0): not a problem but looks like an oversight.

  • _run_with_restart sleeps min_delay after a normal return: this adds a 100ms gap every time a consumer task exits cleanly (stop signal). On shutdown, on_stop may wait up to 100ms unnecessarily. Trivial but worth noting.


Overall the architecture is significantly cleaner than before and the new queue-based model is the right approach for full-duplex TTS. The issues above (especially #1#6) should be addressed before merging.

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown

Removing placeholder - see full review below

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown

PR Review: feat: support cartesia enable words and add metrics for some tts

This is a significant architectural improvement replacing the synchronous, SDK-based Cartesia client with a full-duplex aiohttp WebSocket implementation, adding word-timestamp support, and refactoring the extension to use producer/consumer queues. The overall design is solid. Below are findings organized by severity.


BUGS

1. _all_samples unit mismatch (cartesia_tts.py)

The accumulation in _handle_ws_message stores samples x 1000, not raw sample count. The math works out numerically, but the variable name _all_samples is misleading — it actually holds sample_count * 1000. This will cause silent calculation errors if anyone reads or modifies this code later. Rename to _accumulated_sample_millis or restructure as:

self._sample_count += float(len(audio_bytes)) / 2
cur_pcm_start_ms = int(self._base_start_ms + self._sample_count * 1000 / sample_rate)

2. _validate_first_timestamp can raise TypeError (test_subtitle_alignment.py)

first_audio_frame_ts starts as None and is only set if audio frames arrive. The _run_validations guard checks len(self.audio_frames) == 0 but that is a different check from first_audio_frame_ts is None. Add an explicit guard:

if first_audio_ts is None:
    return False, "No audio frames received"

POTENTIAL ISSUES

3. _wait_for_client_available has no timeout (extension.py)

async def _wait_for_client_available(self) -> bool:
    while not self._is_stopped:
        if self.client is not None:
            return True
        await asyncio.sleep(0.01)
    return False

If the client is never recreated (e.g., due to a bug in cancel_tts), this loops indefinitely. Adding a max-wait (e.g., 2-3 s) with a log warning would prevent silent hangs.

4. id(t) for deduplication is fragile (cartesia_tts.py — _buffer_pending_input)

CPython may reuse memory addresses once an object is garbage-collected. If a TTSTextInput is freed while still referenced by _pending_inputs, a new object could get the same id and be silently dropped. Use a monotonically-incrementing counter instead.

5. last_text_end_ms resets to 0 on task restart (extension.py — _process_transcription)

last_text_end_ms is a local variable inside _process_transcription. When _run_with_restart restarts the coroutine on failure, it resets to 0, potentially producing incorrect start_ms for the next batch mid-session.

6. Shared _base_start_ms / _all_samples across contexts (cartesia_tts.py)

These fields are reset in the done handler but initialized lazily on first chunk or timestamps. If Cartesia interleaves responses from two contexts, one context's done could reset the base used by the other's in-flight chunks, corrupting timestamps. Consider keying these by context_id like _last_word_end_ms already does.

7. base_url double-presence in property.json

property.json sets "base_url": "wss://api.cartesia.ai" inside the params dict. _build_request_payload strips it via params.pop("base_url", None), but config.py::update_params does not strip it, so the value persists in self.config.params. Either remove base_url from property.json's params section or explicitly strip it in update_params.


TEST CONCERNS

8. asyncio.sleep(0.5) is fragile (test_subtitle_alignment.py)

await asyncio.sleep(0.5)
ten_env.log_info("Running validations")

This hardcoded wait is used to allow the last tts_text_result to arrive after tts_audio_end. Under heavy CI load this could race. Consider waiting for an explicit final signal (e.g., a tts_text_result with turn_status == 1) rather than a blind sleep.

9. DURATION_MISMATCH_THRESHOLD_MS = 1000 may be too loose

A 1-second mismatch between text and audio duration would still pass the alignment test. For subtitle alignment a tighter threshold (e.g., 200-300 ms) would make the test more meaningful.

10. Unused ten_env parameter in validator methods (test_subtitle_alignment.py)

All _validate_* methods accept ten_env: AsyncTenEnvTester but none of them use it — only _run_validations needs it. Drop the parameter from the individual validators and simplify their signatures to () -> tuple[bool, str].


MINOR / STYLE

11. requirements.txtaiohttp unpinned

cartesia==2.0.15 was pinned; the replacement aiohttp is not. A future breaking release (e.g., 4.x) could silently break this. Pin to at least a compatible range: aiohttp>=3.9,<4.

12. Version jump from 0.7.0 to 0.8.1 skips 0.8.0

Minor, but worth aligning if there is a versioning convention.

13. Docstrings removed without replacement

Several methods lost their docstrings (_clamp_float, _format_ratio, _normalize_time_string, _compose_ssml_text, _apply_ssml_tags_safe) with no inline comments replacing non-obvious logic.


STRENGTHS

  • Full-duplex queue architecture is a clean improvement over the blocking iterator approach.
  • Auto-reconnect with exponential backoff and MAX_RETRY_TIMES guard is well-structured.
  • Config update buffering during active requests (pending_config_update) handles the concurrent update edge case nicely.
  • Space-insertion logic (_should_insert_space, _is_cjk) for multi-language subtitle joining is thoughtful.
  • Removing the cartesia SDK dependency in favor of raw aiohttp gives more control and removes a transitive dependency.
  • New test_subtitle_alignment.py adds meaningful end-to-end validation for the word-timestamps feature.

Summary: The core architecture change is sound. Issues 1 (unit mismatch naming), 2 (None comparison crash in test), 6 (shared timestamp state across concurrent contexts), and 8 (fragile test sleep) are the most important to address before merging.

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown

PR Review: feat: support cartesia enable words and add metrics for some tts

This is a substantial architectural refactor replacing the cartesia SDK with a direct aiohttp WebSocket implementation, moving to a full-duplex queue-based model. The overall design is solid and test coverage is good. Below are specific issues worth addressing.


Bugs

1. _all_samples stores scaled units, not actual sample count

In cartesia_tts.py:

self._all_samples += float(len(audio_bytes)) / 2 * 1000
cur_pcm_start_ms = int(self._base_start_ms + self._all_samples / sample_rate)

This is mathematically correct (the x1000 cancels with /sample_rate), but the variable named _all_samples actually holds num_samples x 1000, not the sample count. This will confuse future readers. Consider renaming or restructuring to store the actual sample count and multiply by 1000 only at the use site.

2. base_url in params in property.json does not set config.base_url

The new property.json adds base_url inside params. However config.base_url is a top-level field on CartesiaTTSConfig, not part of params. This entry is silently popped in _build_request_payload and never reaches _get_ws_url. As written it looks functional but does nothing. Move it to the top level or remove it.

3. _validate_first_timestamp docstring vs. implementation mismatch

The docstring says "Rule 1: First text start_ms == first audio frame timestamp." But the check only fails when first_text_start < first_audio_ts. If first_text_start is significantly after first_audio_ts (misalignment in the other direction), the test still passes. Either fix the docstring or use an absolute tolerance check.


Security

4. Raw config dict (potentially containing API key) logged in update_configs

self.ten_env.log_info(f"tts_update_configs: {configs}", ...)

If a caller passes a dict with api_key inside params, the key is logged in plaintext. A safe log already happens later via new_config.to_str(sensitive_handling=True). Remove the early raw log or redact sensitive fields before logging.


Design Concerns

5. _wait_for_client_available has no timeout

async def _wait_for_client_available(self) -> bool:
    while not self._is_stopped:
        if self.client is not None:
            return True
        await asyncio.sleep(0.01)
    return False

If client recreation fails silently (an error path that does not set _is_stopped), this spins indefinitely blocking the incoming request. Add a deadline (e.g., 5 seconds) and return False on timeout.

6. Potential double tts_audio_end for the same request

request_tts can send INTERRUPTED for an unfinished previous request via _handle_completed_request, while _process_audio_data may concurrently send REQUEST_END for that same request when the audio-end sentinel from the stopped queue arrives late. The last_audio_end_request_id guard helps, but it is read/written from two concurrent coroutines without synchronization. A comment (or a lock) documenting why this is safe would help.

7. _base_start_ms / _all_samples are single-context instance state

These are reset on "done" but shared across all contexts. Since only one request is active at a time this works in practice, but a per-context map (like the existing _last_word_end_ms) would make the isolation explicit and more robust.


Nits

  • CARTESIA_API_VERSION is hardcoded. Consider exposing it as a config field so users can opt in to newer API versions without a code change.
  • _buffer_pending_input uses id(t) for deduplication. Safe in practice since _pending_inputs holds a reference, but a short comment explaining the invariant would help.
  • requirements.txt has an unpinned aiohttp. Consider adding a floor version (e.g., aiohttp>=3.9) to prevent silent breakage.

Positives

  • Eliminating the cartesia SDK in favour of aiohttp gives full control over the protocol and removes a transitive dependency.
  • The reconnect loop with exponential backoff and MAX_RETRY_TIMES guard is well-structured.
  • The new test_subtitle_alignment.py integration test is comprehensive and covers the key alignment invariants.
  • Replacing scattered event-status integer constants with typed queues and clean callback interfaces is a significant readability improvement.
  • _should_insert_space / _is_cjk handling for CJK and punctuation edge cases is a nice addition that was previously hidden inside the SDK.

@YiminW
YiminW force-pushed the dev/cartesia_tts_words branch from f191e7e to 117f423 Compare April 1, 2026 09:42
@github-actions

github-actions Bot commented Apr 1, 2026

Copy link
Copy Markdown

Code Review

This is a substantial and well-structured PR. The move from a request-response model to a full-duplex queue-based architecture is a clear improvement, and the new word-timestamp/subtitle alignment support is a nice addition. Below are my findings, ordered by severity.


Bugs / Correctness

1. Misleading variable name causes silent precision bug — _all_samples in cartesia_tts.py

The variable is named _all_samples but it stores num_samples × 1000 (an intermediate product). The math is correct ((samples × 1000) / rate == ms), but the log line f"all_samples={self._all_samples:.0f}" will print a value 1000× larger than the actual sample count, which will be confusing during debugging. Rename to _accumulated_samples_ms or accumulate true sample counts and multiply by 1000 only at the use site.


2. Race between _process_audio_data and cancel_tts on pending_audio_end

cancel_tts unconditionally calls send_tts_audio_end(reason=INTERRUPTED) and sets last_audio_end_request_id. Concurrently, _process_audio_data may already be about to enter _handle_completed_request (it checks if not self.pending_audio_end then sets it and awaits). Because both coroutines can be scheduled concurrently, the guard can be bypassed and two audio_end events can be emitted for the same request ID. A asyncio.Lock around audio_end emission, or checking last_audio_end_request_id at the top of the end-signal handler in _process_audio_data, would close this window.


3. cancel_tts sends audio_end(INTERRUPTED) even when speaking never started

If cancel_tts is called before any audio chunk arrives (is_speaking == False, request_total_audio_duration == 0), it still emits send_tts_audio_end. The old code guarded this with if self.sent_ts. Whether emitting a zero-duration audio_end(INTERRUPTED) is intentional should be noted with a comment.


4. Subtitle alignment test: rule 1 check is too lenient

def _validate_first_timestamp(...):
    """Rule 1: First text start_ms == first audio frame timestamp."""
    if first_text_start < first_audio_ts:
        return False, ...
    return True, "First text start_ms matches first audio frame timestamp"

The docstring says == but the implementation accepts any value >= first_audio_ts, so a subtitle starting one second after audio would pass. Consider abs(first_text_start - first_audio_ts) > SOME_TOLERANCE_MS to actually validate alignment.

Similarly, the docstring for _validate_audio_frames_ascending says tolerance 1ms but the check uses > 10. Pick one value and document it.


Test Coverage

5. Removed audio-duration assertion in test_flush_logic

The old test verified that the reported request_total_audio_duration_ms matched bytes actually received (tolerance 10 ms). This assertion was removed without explanation. Duration accuracy matters for downstream metrics. Please either restore it (adapted for the new architecture) or add a comment explaining why it is no longer verifiable under the mock.


Code Quality / Minor

6. Polling loop instead of asyncio.Event in _wait_for_client_available

while not self._is_stopped:
    if self.client is not None:
        return True
    if time.monotonic() >= deadline:
        return False
    await asyncio.sleep(0.01)

A 10 ms polling interval for a 5-second window creates up to 500 unnecessary wake-ups. An asyncio.Event (set when a new client is created) would be more efficient.


7. _run_with_restart loops an extra cycle after clean shutdown

When stop() is called, sentinels unblock the consumer coroutines, which return normally. _run_with_restart then sleeps min_delay (0.1 s) and restarts. Since self.client is None, the inner loop re-sleeps, and _is_stopped is eventually seen. This is correct but wasteful. Consider checking _is_stopped immediately after a normal (non-exception) return from the consumer.


8. Version skip: 0.7.0 → 0.8.1

The manifest jumps from 0.7.0 to 0.8.1, skipping 0.8.0. Intentional?


9. property.json puts base_url inside params

base_url is a top-level field on CartesiaTTSConfig, but property.json places it inside params. update_params() correctly pops it out, but this will confuse operators who try to configure it at the top level. Consider documenting which form is canonical, or removing the params["base_url"] path entirely.


Summary

Area Severity
_all_samples naming / log confusion Low-medium
Double audio_end race condition Medium
Subtitle alignment test rule 1 too lenient Medium
Duration assertion removed from flush test Medium
_wait_for_client_available polling Low
Extra restart cycle on shutdown Low
Version skip in manifest Low
base_url placement in property.json Low

Overall this is clean, well-thought-out work. The full-duplex split, auto-reconnect loop with exponential backoff, config hot-reload, and the new subtitle alignment test infrastructure are all solid. Addressing points 2, 4, and 5 before merge would reduce the risk of subtle correctness issues in production.

@wangyoucao577
wangyoucao577 merged commit e15d7cd into main Apr 2, 2026
34 checks passed
@wangyoucao577
wangyoucao577 deleted the dev/cartesia_tts_words branch April 2, 2026 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants