fix(tts): correct cosy audio end interval

[YiminW]

No description provided.

[github-actions]

Review: fix(tts): correct cosy audio end interval

The core fix is correct and aligns cosy_tts_python with how sibling extensions define this metric. azure_tts_python measures request_event_interval from first_chunk_ts, and xai_tts_python measures it between first and last audio chunk — so anchoring at the first audio chunk rather than the vendor request is the right semantics. Computing TTFB before send_tts_audio_start() is also a small accuracy win, since the await no longer inflates the measurement. Version bumps in manifest.json and pyproject.toml are consistent.

Two things I'd like to see addressed before merge.

1. Requests that produce no audio now report the old (wrong) interval

request_start_ts is only reset inside _handle_first_audio_chunk(), which runs under the if self.first_chunk: guard at extension.py:345. If a request ends without ever yielding an audio chunk — vendor error, or cancel_tts() firing during TTFB — _handle_first_audio_chunk() never runs, so _handle_tts_audio_end() (extension.py:576) still computes the interval from the vendor request timestamp.

There are 9 call sites for _handle_tts_audio_end(), several on error and interrupt paths, so this is reachable in practice. The result is that request_event_interval_ms means "time since first audio chunk" on the happy path and "time since vendor request" otherwise — a metric that silently changes definition is harder to debug than one that is consistently wrong. Downstream consumers do read this field directly (thymia_analyzer_python/extension.py:938).

The sibling extensions all handle this by reporting 0 when no first chunk was seen:

# azure_tts_python/extension.py:162
request_event_interval = 0
if self.first_chunk_ts > 0:
    request_event_interval = int((time.time() - self.first_chunk_ts) * 1000)

2. Prefer a separate first_chunk_ts field over overloading request_start_ts

Reusing request_start_ts for two different meanings across its lifetime has a few knock-on effects:

• The declaration comment at extension.py:52 (# Timestamp when TTS request was sent to service) is now inaccurate for most of the field's life. At minimum it needs updating.
• request_start_ts doubles as the "is there an active request" guard at extension.py:155 (cancel_tts) and extension.py:570 (_handle_tts_audio_end). Those still work, but the field now carries two unrelated responsibilities, which is how the reset gets missed on the error paths above.
• It also forecloses re-deriving TTFB or a request-to-end total later, since the original timestamp is gone.

A separate field resolves both points and matches the established pattern:

# in __init__
# Timestamp of the first audio chunk for the current request
self.first_chunk_ts: datetime | None = None

# in request_tts, alongside the other per-request resets (~line 198)
self.first_chunk_ts = None

# in _handle_first_audio_chunk, replacing the request_start_ts reset
self.first_chunk_ts = datetime.now()

# in _handle_tts_audio_end
request_event_interval = 0
if self.first_chunk_ts is not None:
    request_event_interval = int(
        (datetime.now() - self.first_chunk_ts).total_seconds() * 1000
    )

This keeps request_start_ts as the single-meaning request anchor and leaves the existing guards untouched.

Test coverage

Asserting on request_event_interval_ms in tts_audio_end is a good addition, and interval < ttfb_value does catch the regression — pre-fix the interval would have been ~400ms against a ~250ms TTFB.

Two suggestions:

• The relative assertion is implicitly coupled to the mock's sleep values (250ms TTFB, then 3x50ms chunk gaps). An absolute bound documents the intent more clearly and fails more informatively:

  assert 100 <= tester.request_event_interval_ms < 250, (
      f"Expected interval near 150ms (3 chunks x 50ms), got "
      f"{tester.request_event_interval_ms}ms."
  )

  Including the actual value in the failure message matches what the neighbouring TTFB assertion already does.
• No test covers the zero-audio path from point 1. A case where the mock raises or completes without emitting PCM would pin down the intended value (0) and prevent it regressing back to a TTFB-inclusive number.

Minor

request_start_ts is never cleared when a request completes — _handle_tts_audio_end() sets current_request_id = None but leaves the timestamp set. Pre-existing rather than introduced here, and the current_request_id guard in cancel_tts() covers the main hazard, but clearing it alongside the other per-request state would make the lifecycle easier to reason about.

Nothing blocking on security or performance — the change is two timestamp arithmetic operations on an existing code path.

[github-actions]

Review: fix(tts): correct cosy audio end interval

Reviewed the diff against the pre-change source of config.py, cosy_tts.py, and extension.py. I did not run the test suite or task lint (no container available here), so the notes below come from reading the code.

The core fix is right: request_event_interval_ms measured from the first audio chunk instead of from request submission, so it no longer double-counts TTFB. Resetting request_start_ts = None in _handle_tts_audio_end is a nice side benefit — it makes the if self.request_start_ts: guard idempotent and prevents a second audio_end when the vendor's done=True arrives after an error path already ended the request. Version bumps are consistent across manifest.json and pyproject.toml.

A few things I think need attention before merge.

1. complete() raising can strand the incoming request (extension.py:190-195)

complete() now re-raises, and one of its two call sites is the "new request while previous unfinished" branch:

if not self.current_request_finished:
    self.client.complete()          # can now raise
    self.current_request_finished = True

self.current_request_id = t.request_id

If it raises here, current_request_id is still the previous request. The generic handler then evaluates if self.current_request_finished or t.text_input_end: and sends audio_end/finish_request for the old id. The new request never gets an id assignment, never gets an error, and never gets an audio_end — the caller waiting on that request has nothing to key off. Suggest wrapping this call site in its own try/except (log + cancel(), then continue into the new request), or moving the id assignment before the complete() call.

2. Vendor errors outside an active request are now silently dropped (extension.py:374-382)

The else: send_tts_error(...) branch is gone, replaced by an unconditional _handle_tts_audio_end(reason=ERROR, ...). But that method's entire body is behind if self.request_start_ts: — and request_start_ts is now set to None at the end of every request. So a MESSAGE_TYPE_CMD_ERROR arriving after the request ended (or before one started) produces no event at all, where previously it produced a tts_error. Worth keeping a fallback:

if self.request_start_ts:
    await self._handle_tts_audio_end(reason=TTSAudioEndReason.ERROR, error=error)
else:
    await self.send_tts_error(request_id=self.current_request_id or "", error=error)

Also self.client.cancel() on the next line is unguarded — self.client is set to None in on_stop, so if self.client: would be cheap insurance.

3. _clear_receive_queue() is best-effort only, and the queue is shared across sessions

AsyncIteratorCallback.on_data enqueues via asyncio.run_coroutine_threadsafe from the dashscope WS thread. cancel() runs on the event loop, so any put() coroutines already scheduled but not yet executed cannot run until cancel() returns — they land in the queue immediately after you clear it. The _cancelled flag only filters callbacks that enter after it's set.

Because start() reuses the same self._receive_queue for the new synthesizer, that late audio is consumed by the next request and attributed to it: extra bytes in total_audio_bytes, a wrong request_total_audio_duration_ms, and potentially an audible fragment of the cancelled utterance. Tagging queue items with a session/epoch counter and dropping mismatches in _process_audio_data would close this properly. (Swapping in a fresh asyncio.Queue() in start() won't work — the consumer is already parked on get() for the old one.)

Related: after client.cancel() clears the queue, a late PCM chunk that slips in hits send_tts_audio_data at line 353 while _handle_first_audio_chunk is skipped by the request_start_ts guard — audio data with no preceding audio_start.

4. Clearing the queue discards pending usage metrics

MESSAGE_TYPE_CMD_RESULT_GENERATED carries the vendor's character count, which feeds metrics_add_input_characters. In cancel_tts the order is client.cancel() (clears queue) → _handle_tts_audio_end → send_usage_metrics, so any char counts still queued are dropped before they're accounted. Those characters were already billed by the vendor. Consider draining non-PCM message types into their handlers rather than discarding everything, or at least filtering to PCM-only in _clear_receive_queue.

5. Sample-rate validation: duplicated truth, behavior change, and scope

Three concerns with the config.py addition:

• SUPPORTED_SAMPLE_RATES duplicates the key set of AUDIO_FORMAT_MAPPING in cosy_tts.py. Two places to update when a rate is added. The import direction (cosy_tts → config) blocks the obvious dedup, so either move the check into the client, or add a comment on both sides pointing at the other.
• It makes the fallback in _get_audio_format (log_warn + DEFAULT_AUDIO_FORMAT) unreachable dead code. Either drop it or keep the config check as a warning.
• It's a breaking change: a deployment with sample_rate: 32000 previously degraded to 16 kHz with a warning; it now fails validate_params() in on_init and surfaces a FATAL_ERROR. Per docs/ai/L1/04_conventions.md that severity is correct for invalid config, but it will hard-fail configs that used to run. Worth calling out in the PR description at minimum.

Also, none of this relates to the audio-end interval — it's unrelated scope in a fix(tts) commit and would be easier to review (and revert) as its own change.

6. on_stop blocks the event loop

self.client.cancel() calls synthesizer.streaming_cancel(), a synchronous SDK/network call, directly on the event loop inside an async def on_stop. During shutdown that stalls everything else. run_in_executor (or accepting it with a comment) would be clearer. Doing this after audio_processor_task.cancel() is the right order, though — the consumer is gone before the queue is cleared.

7. Test coverage

The three new unit tests in test_client.py are tight and well-scoped. Two gaps and one flake risk:

• The biggest behavior change is untested. Vendor error before text_input_end used to send a bare tts_error and let the request continue; it now terminates the session. There's no test for a MESSAGE_TYPE_CMD_ERROR arriving while current_request_finished is False.
• test_robustness.py reduces coverage of that same path. Adding text_input_end=True to tts_request_to_fail was needed to make the new audio_end assertion pass, but it converts the scenario away from the pre-end error case — exactly the one whose semantics changed. Consider keeping both variants.
• test_metrics.py timing assertion may flake. assert tester.request_event_interval_ms < tester.ttfb_value holds only while post-first-chunk work stays under ~500 ms; on a loaded CI runner that's not guaranteed. It also adds 350 ms to every run. An absolute upper bound on the interval (e.g. < 200) plus the existing TTFB floor would express the same intent with a wider margin.

Minor

• complete() logs the error and re-raises, so request_tts's handler logs the same failure with a traceback — duplicate log lines for one event.
• _clear_receive_queue emits log_debug("Cleared queued TTS responses") unconditionally, including the common case where nothing was queued.
• The while not empty() loop plus try/except QueueEmpty is belt-and-braces; harmless, but one or the other suffices in single-threaded loop context.
• Commit message and branch name both follow the repo's conventional-commit rules.

Nothing here is a blocker on the interval fix itself — items 1, 2, and 3 are the ones I'd want resolved, since each turns a recoverable condition into a silent one.

[github-actions]

Review: fix(tts): correct cosy audio end interval

Overall this is a solid, focused fix. Anchoring request_event_interval_ms to the first audio chunk instead of request submission is the right call — the old value silently folded TTFB into what is supposed to be the audio receive window. The incidental benefit that _handle_tts_audio_end now clears request_start_ts (making the whole method idempotent, since its body is guarded by if self.request_start_ts:) is a nice cleanup, and defaulting the interval to 0 when no audio arrived matches what elevenlabs_tts2_python, google_tts_python, azure_tts_python, and bytedance_tts_duplex already do.

A few things I think are worth addressing before merge.

1. complete() re-raising can strand a request id (correctness)

complete() now re-raises, and one of its two call sites is the new-request branch in request_tts():

if t.request_id != self.current_request_id:
    if not self.current_request_finished:
        self.client.complete()          # <-- can now raise
        self.current_request_finished = True

    self.current_request_id = t.request_id   # <-- never reached
    self.current_request_finished = False
    ...
    self.request_start_ts = datetime.now()

If complete() raises here, self.current_request_id is still the previous request. Control lands in except Exception, and with the newly added or t.text_input_end condition, _handle_tts_audio_end() fires — emitting tts_audio_end for the old request id and clearing state. The new t.request_id never gets tts_audio_start or tts_audio_end. If that message was the final chunk for the new request, its caller waits forever.

Suggestion: wrap the flush of the outgoing session so a vendor failure there doesn't abort adoption of the new request id, e.g.

if not self.current_request_finished:
    try:
        self.client.complete()
    except Exception as e:
        self.ten_env.log_warn(f"Error completing previous session: {e}")
        self.client.cancel()
    self.current_request_finished = True

That keeps the re-raise useful for the text_input_end call site (where the id is already correct) without letting it corrupt the transition.

2. Error classification for vendor complete() failures

Both complete() call sites are inside request_tts()'s try, so anything other than WebSocketConnectionClosedException is caught by the generic handler and reported as FATAL_ERROR. Per docs/ai/L1/04_conventions.md, a transient vendor disconnect or timeout should be NON_FATAL_ERROR — and a failure inside async_streaming_complete() is squarely in that category. Previously it was swallowed and logged; now it escalates to fatal, which is a larger swing than the fix intends. Consider translating it to NON_FATAL_ERROR explicitly.

Minor: since the generic handler also logs, the log_error + raise in complete() produces a duplicate log line for every such failure.

3. Interval end-point is now(), not the last chunk

docs/ai/L1/L2/extension_development.md defines the field as the wall clock "between the first audio chunk arrival and the last audio chunk arrival". The PR fixes the start point but the end point is still datetime.now() at _handle_tts_audio_end() time. For REQUEST_END the gap is small, but for INTERRUPTED (flush) the cancel can land well after the final chunk, inflating the value by the idle tail. xai_tts_python handles this with an explicit _last_audio_chunk_ts:

def _calculate_request_event_interval_ms(self) -> int:
    if self._first_audio_chunk_ts is None or self._last_audio_chunk_ts is None:
        return 0
    return int((self._last_audio_chunk_ts - self._first_audio_chunk_ts).total_seconds() * 1000)

Tracking last_chunk_ts alongside the new first_chunk_ts is a small change and would make this fully conformant. If the current approximation is deliberate, a comment saying so would help the next reader.

4. Sample-rate validation: breaking change + duplicated constant

Two points here.

SUPPORTED_SAMPLE_RATES in config.py restates the keys of AUDIO_FORMAT_MAPPING in cosy_tts.py. These will drift. Prefer a single source of truth — either derive the set from the mapping, or define it in config.py and have cosy_tts.py build its mapping keys from it. (Also note _get_audio_format()'s fallback-with-warning branch is now unreachable for any config that passed validate_params().)

More significantly, this converts "warn and silently fall back to 16 kHz" into a hard FATAL_ERROR at on_init. Any existing deployment with, say, sample_rate: 32000 in property.json currently limps along and will now fail to start. That's arguably correct — silent resampling mismatch produces wrong-pitch audio — but it is a behavior break shipped under a patch bump (0.4.2 → 0.4.3). Worth a minor bump and a line in the PR description, since the body is currently empty.

5. Queue drain is best-effort across threads

_clear_receive_queue() is a good addition, but AsyncIteratorCallback enqueues from the dashscope WS thread via asyncio.run_coroutine_threadsafe. A put scheduled just before cancel() runs can execute after the drain and survive it. The callback's _cancelled flag filters on_data at the source, which covers most of it, so this is a narrow window rather than a bug — but the docstring ("Remove responses already queued") slightly overstates the guarantee.

Two smaller notes on the same path:

• Since the queue is only mutated from the event loop, the while not empty() check already makes QueueEmpty unreachable; the try/except is harmless but dead.
• on_stop() now calls client.cancel(), which invokes the synchronous synthesizer.streaming_cancel() directly on the event loop thread. Pre-existing in cancel_tts(), so not introduced here, but it does mean shutdown can block the loop on a network call. Also, on_stop() leaves request_start_ts / current_request_id set.

6. Tests

The new coverage is well targeted. The test_robustness.py assertion that a request producing no audio reports interval 0 exercises the new first_chunk_ts is None branch directly, and adding text_input_end=True to the failing request is what makes the new or t.text_input_end path reachable — good pairing of fix and test.

Some suggestions:

• test_config_rejects_unsupported_sample_rate is a config test living in test_client.py; test_params.py is the natural home.
• In test_metrics.py, raising the simulated TTFB from 250 ms to 600 ms purely to make request_event_interval_ms < ttfb_value hold is an indirect assertion, and it adds 350 ms to the suite. Asserting the interval falls in an expected band derived from the simulated chunk spacing would test the actual semantics rather than an inequality, and would catch a regression that merely shrinks the interval incorrectly.
• Uncovered paths worth a test: the failure mode in point 1 (vendor error on complete() during a request-id switch), and on_stop() now cancelling the client.

Nothing here blocks the core interval fix, which is correct and worth landing — point 1 is the one I'd want resolved first, since it can leave a request permanently unterminated.

[github-actions]

Review: fix(tts): correct cosy audio end interval

The fix is correct and aligns cosy_tts_python with the documented semantics in docs/ai/L1/L2/extension_development.md:

request_event_interval_ms: wall-clock between the first audio chunk arrival and the last audio chunk arrival — the audio receive window. Not TTFB.

Previously cosy measured request_start_ts -> now, so the reported interval was inflated by TTFB. Version bumps in manifest.json and pyproject.toml are consistent (0.4.2 -> 0.4.3), and the interval is computed before the PCMWriter.flush() so flush latency is not folded into the metric. Nice touch that the no-audio case now reports 0 rather than a spurious duration.

Suggestions

1. Reset first_chunk_ts in _handle_tts_audio_end (defensive)

first_chunk_ts is only cleared in the new-request branch of request_tts. That is sufficient today because _handle_tts_audio_end sets current_request_id = None, forcing the next request down that branch. But _handle_tts_audio_end can be entered twice for one request — at extension.py:376 on MESSAGE_TYPE_CMD_ERROR when current_request_finished is true, and again at extension.py:397 if the client also sets done on that message. The guard is if self.request_start_ts:, which is never cleared, so the second call re-sends tts_audio_end with a stale first_chunk_ts. Clearing both timestamps where current_request_id is nulled makes the duplicate (pre-existing, not introduced here) report 0 instead of a misleading interval:

self.current_request_id = None
self.request_start_ts = None
self.first_chunk_ts = None
self.is_first_message_of_request = False

2. Extract the interval into a helper for parity with sibling extensions

xai_tts_python and deepgram_tts already expose _calculate_request_event_interval_ms() / _current_request_interval_ms(). Cosy already has _calculate_ttfb_ms(), so an inline if / int(...) in _handle_tts_audio_end is slightly out of step with both the local file and the cross-extension convention.

Also worth noting: those two extensions measure first chunk -> last chunk (they track _last_audio_chunk_ts), whereas this PR measures first chunk -> audio-end handling. For cosy the done signal follows the last chunk closely, so the delta is small, but it is a different definition than the doc wording. Fine as-is; flagging in case exact parity matters for downstream metrics comparison.

3. The same bug exists in ~8 other TTS extensions — worth a follow-up issue

These still compute the interval from request_start_ts and therefore include TTFB:

minimax_tts_websocket_python, tencent_tts_python, fish_audio_tts_python, elevenlabs_tts2_python, google_tts_python, azure_tts_python, nvidia_riva_tts_python, bytedance_tts_duplex

Out of scope for this PR, but the metric is not comparable across vendors until they are aligned.

Tests

Both additions are real regression guards, and the mechanism is sound — in the robustness test send_tts_audio_end fires before finish_request emits the error, so first_request_event_interval_ms is reliably populated before send_second_request is triggered. No ordering race.

Two minor points:

• test_metrics.py — asserting request_event_interval_ms < ttfb_value couples two independent metrics and is why the TTFB sleep had to grow 0.25s -> 0.6s. An absolute bound expresses the intent more directly and would let the original 250ms delay stand:

# ~150ms of chunk spacing; generous headroom for scheduling
assert tester.request_event_interval_ms < 400

The current form does catch the regression (a revert yields ~750ms > 600ms), so this is optional tightening rather than a correctness issue.

• test_robustness.py — assert tester.first_request_event_interval_ms == 0 conflates "field missing" with "wrong value". Splitting the is not None check out gives a clearer failure when the event stops carrying the field at all.

• get_property_to_json(None) in the new tts_audio_end branch of test_metrics.py is unguarded, unlike test_robustness.py which checks for an empty payload first. Safe for tts_audio_end specifically, but the inconsistency will confuse the next reader.

No security or performance concerns — two datetime.now() calls per request.

Overall: correct, well-scoped, documented semantics honored, tests cover both the happy path and the no-audio path. Suggestion 1 is the only one I would consider addressing before merge.

[YiminW]

1. on_error() will set done which aovids duplicate error msgs
2. not necessary
3. this pr only fix cosy tts issue

[github-actions]

Review: fix(tts): correct cosy audio end interval

The change is correct in direction and well scoped. request_event_interval_ms should describe the audio receive window, not TTFB plus the receive window, and docs/ai/L1/L2/extension_development.md already documents it that way. Version bumps in manifest.json and pyproject.toml are in sync (0.4.2 to 0.4.3), and the commit message follows the conventional-commit rules in AGENTS.md. A few things worth addressing.

1. Measured endpoint does not match the documented semantics

The docs define the field as the interval between first audio chunk arrival and last audio chunk arrival. The new code measures first chunk to datetime.now() inside _handle_tts_audio_end, so it also includes the gap between the last PCM chunk and the done/error signal, plus any await latency in the end-handling path before the subtraction. For the normal completion path that tail is usually small, but on the ERROR and INTERRUPTED paths it can be arbitrary.

xai_tts_python already implements the documented version with an explicit _last_audio_chunk_ts:

def _calculate_request_event_interval_ms(self) -> int:
    if self._first_audio_chunk_ts is None or self._last_audio_chunk_ts is None:
        return 0
    return int((self._last_audio_chunk_ts - self._first_audio_chunk_ts).total_seconds() * 1000)

Tracking last_chunk_ts alongside first_chunk_ts in _process_audio_data would make cosy match both the doc and the existing reference implementation, and would remove the sensitivity to whatever happens after the stream ends.

2. first_chunk_ts is never cleared on request completion

It is reset only in request_tts when a new request_id arrives. _handle_tts_audio_end leaves it set, which is the same pre-existing pattern as request_start_ts and also why that method can fire twice for one request and log current_request_id: None on the second pass. Clearing self.first_chunk_ts = None next to self.current_request_id = None at the end of _handle_tts_audio_end costs nothing and prevents a stale timestamp from ever being read on a path that does not go through the new-request branch. Not a bug I can demonstrate today, just hygiene.

3. Cross-extension inconsistency

request_event_interval_ms is emitted by roughly 20 TTS extensions and consumed by thymia_analyzer_python (line 938), which labels it generation_time. After this PR, cosy and xai use first-chunk-to-end while google (sent_ts), elevenlabs, bytedance, tencent and others still use request-sent-to-end, meaning they include TTFB. A downstream consumer comparing that metric across vendors gets values that are not the same quantity. This PR should not have to fix all of them, but a follow-up issue tracking the remaining extensions would help the docs and the implementations converge.

4. Test feedback

The new assertions are good regression coverage. In particular request_event_interval_ms == 0 for a request that produced no audio would have failed under the old code, which is exactly the right shape for this fix.

• test_metrics.py: assert tester.request_event_interval_ms < tester.ttfb_value is an indirect proxy. With the mock timings (600ms TTFB, roughly 150ms receive window) the margin is comfortable, but that assertion would also pass if the interval were computed from something else entirely. Asserting a bound on the interval itself, for example 0 < interval < 400, tests the property more directly and does not depend on the TTFB constant.
• test_metrics.py around line 66: json.loads(json_str) on the tts_audio_end payload has no empty-string guard, unlike test_robustness.py which does if not json_str: return. tts_audio_end should always carry a payload, so this is a robustness nit rather than a live failure.
• Raising the mock TTFB from 250ms to 600ms adds roughly 350ms per run and widens the timing margin, which is a reasonable trade. Worth confirming this suite is not run in a tight loop anywhere that would make it add up.
• test_robustness.py: the new branch relies on tts_audio_end for tts_request_to_fail being delivered before the test stops on the second request tts_audio_end. That holds given _handle_tts_audio_end sends audio_end before finish_request surfaces the error, but it is an ordering assumption. If it ever flakes, that is where to look.

Security and performance

Nothing of concern. Two datetime.now() calls per request, no new I/O, no new dependencies, no logging of credentials or payload text beyond what already existed.

Summary

Approve in spirit. Items 1 and 2 are worth folding in before merge (small, local changes), item 3 is a follow-up, item 4 is optional tightening.

[wangyimin-agora]

CosyVoice hardening update

The branch now contains the complete repository-owned CosyVoice hardening change, split into focused commits so the functional and test diffs are easy to review.

Runtime

• Keep the official DashScope SDK, with a preconnected SpeechSynthesizerObjectPool shared by extension instances in one worker.
• Return a synthesizer only after synchronous task completion; close and replace failed connections.
• Add bounded pool wait, first-audio, task, input-idle, and cancellation timeouts.
• Correct request lifecycle handling for completion, flush/cancel, stale callbacks, provider errors, and recovery on the next request.
• Preserve streaming multi-chunk text input. enable_ssml=true is rejected because the provider only accepts SSML as a single text chunk.
• Forward unknown params keys to DashScope so newly introduced provider parameters work without extension changes.
• Support workspace ID, custom headers, custom endpoint, and all supported PCM sample rates.
• Fix audio-end interval semantics, dump behavior, usage calculation, structured error reporting, connection status, connect-delay, and TTFB metrics.

Diagnostics and tests

• Add matched SDK and direct-WebSocket TTFB benchmark scripts for isolating SDK overhead.
• Add deterministic coverage for parameter forwarding, object-pool setup, dump/flush, metrics, provider errors, recovery, and request state transitions.
• Validation: Cosy standalone suite 11 passed; target Pylint 10.00/10; Black and git diff --check passed.

Current limits

• DashScope SDK remains the production transport; direct WebSocket is diagnostic only.
• Output remains mono 16-bit PCM.
• Word-level timestamps are not exposed yet.
• Streaming SSML is intentionally unsupported; normal text remains streaming.

[github-actions]

Reviewed the full diff (19 files, ~2.1k added / ~1.2k removed). This is effectively a rewrite of the extension rather than the interval fix the title suggests. The direction is good — the old current_request_finished flag juggling was genuinely hard to reason about, and replacing it with a single _ActiveTask + lease model is a real improvement. The _clear_if_active compare-and-clear guard is the right primitive, and it correctly prevents double-release between the watchdogs, complete(), and cancel().

Findings below, ordered by severity.

---

1. Blocking: empty final chunk ends the request while synthesis is still in flight

In extension.py request_tts:

if t.text_input_end:
    if not text and self.first_chunk:
        await self._finish_request_once(t.request_id, TTSAudioEndReason.REQUEST_END)
        return
    await self.client.complete(t.request_id)

self.first_chunk means "no audio chunk has arrived yet", not "this is the first message of this request". The old code used a dedicated is_first_message_of_request flag for exactly this distinction, and cleared it as soon as text was submitted.

Concrete failure:

1. Chunk 1 arrives with text "Hello there", text_input_end=False → synthesize_audio() starts the provider task.
2. Chunk 2 arrives with empty text and text_input_end=True, before the first audio callback lands (normal — TTFB is hundreds of ms).
3. not text is True and first_chunk is still True → the request is finished with REQUEST_END, zero audio bytes, and client.complete() is never called.

Result: the caller gets a successful tts_audio_end with no audio, the provider task is orphaned still holding its pool lease, and the lease is only reclaimed ~first_audio_timeout_ms later when _watch_first_audio fires — at which point the error is discarded as stale because current_request_id is already None. With pool_size: 1 that stalls the whole worker for 5s.

A trailing empty chunk carrying text_input_end is a normal shape in this flow; the old code special-cased it, which suggests it happens in practice. You already track the right signal — self.request_text_characters == 0 means no text was ever submitted for this request. Suggest gating on that instead of first_chunk.

2. Default pool_size: 1 serializes concurrent requests, then fails them

SharedPool is process-wide and borrow() gates on BoundedSemaphore(pool_size) with pool_wait_timeout_ms (default 1000). With the shipped property.json default of pool_size: 1, a second concurrent Cosy request in the same worker process waits 1s and then raises TimeoutError, surfaced as a NON_FATAL error and a failed utterance.

The README advises sizing to 1.5–2x peak concurrency, but the default that ships is 1. If a worker process can host more than one graph or channel — which the singleton-pool design explicitly anticipates — the default configuration breaks under any concurrency at all. Recommend either raising the default, or having pool exhaustion fall back to an unpooled synthesizer instead of failing the request.

3. first_audio_timeout_ms is armed at lease acquisition, not at end of input

_watch_first_audio starts when the task is created and fires 5s later by default. For streaming input where an upstream LLM feeds text chunks over several seconds, a perfectly healthy request can exceed 5s before first audio and be aborted. _watch_input_idle already covers the "upstream went quiet" case separately, so the first-audio deadline would be better measured from the last submitted chunk (or from text_input_end) rather than from lease acquisition.

Related: complete() passes self.config.task_timeout_ms to streaming_complete() while _watch_task sleeps on that same task_timeout_ms. Two deadlines at an identical value race by design. _clear_if_active makes the race safe, but the reported error code becomes nondeterministic between the two paths. Consider making the watchdog strictly longer, e.g. task_timeout_ms plus slack.

4. finish_request no longer receives the error

Previously finish_request(request_id=..., reason=..., error=error). Now _finish_request_once sends the error via send_tts_error() and calls finish_request(request_id, reason=reason) with no error. Also send_tts_audio_end is emitted before send_tts_error, which is the reverse order of most sibling extensions. Both may be deliberate, but please confirm the base state machine does not depend on error being threaded through finish_request.

Similarly, cancel_tts passes finish_base_request=False on the assumption that the base flush flow owns request-state cleanup. That is a strong assumption about AsyncTTS2BaseExtension, and siblings are inconsistent here (tencent calls finish_request in its cancel path, gradium does not). Worth a comment citing where in the base class that ownership lives — a wrong guess leaks request state silently.

---

Smaller items

• Version string duplicated three ways. User-Agent: "ten-cosy-tts/0.4.4" is hardcoded in cosy_tts.py, duplicated in manifest.json and pyproject.toml, and asserted literally in test_client.py. This will drift on the next bump. Derive it from one source, or drop the version from the UA.
• Character limits are magic numbers, measured inconsistently. 20000 and 200000 are inline literals; the per-chunk check uses len(text) (stripped) while the accumulator uses len(t.text) (unstripped). Promote to module constants and measure the same way in both.
• Breaking default changes are undocumented. model goes cosyvoice-v3 → cosyvoice-v3-flash and voice goes loongluna_v2 → longanyang. These change audio output for every deployment relying on defaults. The PR body is empty — please describe the scope, the rationale for the default swap, and a migration note. The title (fix: correct cosy audio end interval) also understates a rewrite that carries a feat: commit.
• dump_path default /tmp → ./. In a container, CWD is less predictable than /tmp. property.json already set ./, so this only affects callers relying on the model default, but it is a behavior change worth calling out.
• validate_params match statement. The match value: / case str() if ...: / case None: / case _: block replaces a one-line falsy check and is harder to read; case None is unreachable for these str-annotated fields. A plain if would be clearer.
• Watchdog exceptions are unobserved. The asyncio.create_task handles are stored on _ActiveTask but never awaited, so a failure inside a watchdog surfaces only as "Task exception was never retrieved". A done-callback that logs would make this debuggable.
• Benchmark scripts: env var naming is inconsistent. --workspace-id reads only DASHSCOPE_WORKSPACE_ID, while the extension and README use COSY_TTS_WORKSPACE_ID; --api-key accepts both prefixes. Also benchmark_sdk_ttfb.py builds the pool without workspace= while the extension passes it, so the two paths are not strictly comparable.
• Exact pin dashscope==1.26.4. Reasonable given the dependency on SpeechSynthesizerObjectPool, which looks like a low-level SDK surface — but the pin blocks patch updates. A one-line comment stating why it is pinned exactly would help whoever tries to bump it.

Security

Nothing alarming. to_str() correctly encrypts api_key and now redacts Authorization / authorization / x-api-key from both headers and params["headers"], operating on a deepcopy so it does not mutate live config. The benchmark scripts avoid printing keys and text, as documented.

Note that dashscope.api_key = config.api_key is a process-global mutation — pre-existing, and consistent with aliyun_asr_bigmodel_python, but it means the last extension to initialize wins globally. That is part of why the pool signature check exists; the check makes the conflict loud, which is the right call.

Test coverage

The MockClientStream consolidation is a clear win — it removes five bespoke stateful streamer classes and the tests read much better for it. Adding request_event_interval_ms assertions (including the zero-audio case in test_robustness) directly covers the original bug.

Gaps, all on newly added surface area:

• No test for the empty-final-chunk path in finding 1. A test sending text, then an empty text_input_end chunk, would fail today.
• No coverage of any of the three watchdogs (first_audio, task, input_idle) — the bulk of the new risk.
• No coverage of pool-wait exhaustion / TimeoutError, nor of the signature-mismatch ValueError when two configs disagree.
• No coverage of semaphore accounting across return_lease / discard_lease, including the return_synthesizer() is False branch that the code comment flags as a real SDK edge case.
• No coverage of validate_params rejections: bad sample_rate, pool_size out of range, input_idle_timeout_ms >= 23000, enable_ssml=true.
• test_client.py pokes SharedPool._pool / _semaphore / _signature directly behind a pylint: disable=protected-access, with a manual _reset_shared_pool() at the start and end of each test. A SharedPool.reset_for_testing() classmethod or a pytest fixture would be less brittle and would guarantee reset on failure — right now the trailing reset is skipped if an assertion raises, leaking singleton state into the next test.

Conventions

Please confirm task format && task check && task lint was run in the container per AGENTS.md. task lint fails on any pylint warning, and this diff moves a lot of imports around.

---

Nice work on the lifecycle model overall — the lease/active-task split is a genuine improvement over what was there. Findings 1 and 2 are the ones I would want resolved before merge.

[github-actions]

Review: fix(tts): correct cosy audio end interval

Solid rework of the Cosy TTS lifecycle. Pool-based preconnect, explicit watchdogs, and structured ProviderError/ProviderCompletion types are a real improvement over the old boolean-flag state machine. Notes below, roughly by severity.

Correctness

1. task_timeout_ms is used for two racing mechanisms. complete() passes it to streaming_complete(timeout), while _watch_task independently sleeps the same duration starting at lease acquisition. The watchdog starts earlier, so it will normally win: it calls _abort_active -> discard_lease -> synthesizer.close() while the streaming_complete worker thread is still blocked on that same object. complete() then finds _clear_if_active False and returns silently. Consider deriving the watchdog deadline as task_timeout_ms plus a margin, or dropping one of the two.

2. Error is no longer passed to finish_request. Previously finish_request(request_id, reason, error=error). Now _finish_request_once emits send_tts_audio_end, then a separate send_tts_error, and calls finish_request(request_id, reason=reason) with no error. Downstream consumers that correlate the error with request completion will see a behavior change. Was that intentional?

3. cancel_tts relies on the base class calling finish_request. The finish_base_request=False path is explained by the comment, but if that assumption is wrong the request state wedges permanently. I could not verify it — ten_ai_base is not vendored in the repo. Sibling extensions differ here (tencent_tts_python does call it via _handle_tts_audio_end, gradium_tts_python does not), so please confirm against the base implementation.

4. provider_params() is now order-dependent. Dropping _EXTENSION_PARAM_NAMES in favor of update_params() popping from self.params means provider_params() is only correct after update_params() has run. validate_params() already calls it for the enable_ssml check, and test_client.py had to add an explicit config.update_params() — that added line is the smell. The filter version was self-contained and safe in any order; consider keeping it, or at least documenting the required call sequence on both methods.

5. pool_size: 1 plus pool_wait_timeout_ms: 1000 serializes the process. The BoundedSemaphore(pool_size) is shared by every Cosy instance in the worker, so with the shipped defaults a second concurrent request fails after 1s with TimeoutError. Fine for one-session-per-worker, but a graph with two Cosy nodes will fail under the default property.json. Worth calling out in the README beyond the current production sizing note.

Quality

6. Version string duplicated. SharedPool._headers hardcodes ten-cosy-tts/0.4.4, which must now be updated in lockstep with manifest.json and pyproject.toml — and it is asserted in test_pool_uses_custom_url_workspace_and_headers, so every version bump breaks a test. Read it from the manifest or a single module constant.

7. Magic numbers. 20000 / 200000 (extension.py) and 23000 (config.py) should be named constants with a comment pointing at the provider limit they encode.

8. match in validate_params is heavier than what it replaced. All three required fields are str, so case None is unreachable, and the new form also drops the original falsy check. The prior if not value or (isinstance(value, str) and value.strip() == "") was shorter and covered more.

9. Unobserved watchdog exceptions. The three asyncio.create_task watchdogs have no done-callback; if _abort_active raises inside one, it surfaces only as Task exception was never retrieved.

10. dump_path default moved from /tmp to ./. Dumps now land in the app working directory. Given *.pcm must never be committed, /tmp was the safer default even though property.json already used ./.

Security

Good: to_str now encrypts Authorization/authorization/x-api-key in both headers and params["headers"]; the pool signature-mismatch ValueError does not echo the key; benchmark scripts document that keys and text are never printed. Nothing to flag.

Dependencies

dashscope==1.26.4 exact pin matches the conventions doc, but aliyun_asr_bigmodel_python declares dashscope>=1.26.0 in the same container. Compatible today; any future bump has to move both together. Since SpeechSynthesizerObjectPool is a newer SDK surface, a comment on why the pin is exact would help.

Test coverage

mock_client.py and the two SharedPool tests are good additions, and the mock-based restructuring of the existing suites reads cleaner. The gap is that the riskiest new code is untested:

• first_audio_timeout_ms, task_timeout_ms, input_idle_timeout_ms watchdog firing
• pool_wait_timeout_ms exhaustion -> TimeoutError
• pool signature-mismatch ValueError (two instances, different pool_size)
• discard_lease vs return_lease selection on cancel and on error

These are the paths most likely to regress silently, and they are all unit-testable with the existing MagicMock pool. Also: MockClientStream.get_audio_data blocks on a bare await asyncio.Future() to simulate cancellation — works, but an asyncio.Event that is never set would express the intent more clearly.

Before merge

Per docs/ai/L1/04_conventions.md, confirm task format && task check && task lint passes in the container — the pylint not-an-iterable / no-member pragmas in config.py suggest pylint is unhappy with model_fields iteration, so please verify those suppressions are still the minimal set after the final refactor commit.

[github-actions]

Review of the cosy_tts_python changes. The core interval fix looks right, and several parts of the rewrite are genuine improvements: moving blocking DashScope calls off the event loop with asyncio.to_thread, replacing the tuple[bool, int, str | bytes | None] queue protocol with typed QueueItem/ProviderError/ProviderCompletion dataclasses, consolidating five duplicated audio-end/error paths into a single _finish_request_once, and deleting the per-test bespoke streamer classes in favour of a shared MockClientStream. Pinning dashscope==1.26.4 exactly is the right call given SpeechSynthesizerObjectPool is a recent API.

One process note first: the title says "correct cosy audio end interval" but the diff is a ~1200-line rewrite of the client lifecycle, connection pooling, config extraction, and error classification. Most of my comments concern the parts beyond the stated fix. Landing the interval fix separately from the pooling rewrite would be easier to review and much safer to revert if the pool misbehaves in production.

I could not run the tests or import ten_ai_base in this environment, so items 1 and 2 are questions rather than assertions.

1. extra_metadata is passed to three base-class methods no other extension passes it to.
I grepped all ~90 extensions. send_tts_ttfb_metrics(..., extra_metadata=...) appears in 20 places, so that one is clearly supported. But zero others pass it to send_usage_metrics, send_tts_error, or metrics_connect_delay — every sibling call site is await self.send_usage_metrics(self.current_request_id) and send_tts_error(request_id=..., error=...). If any of those signatures does not accept the kwarg this raises TypeError on the error path and the connect-delay path, i.e. exactly the paths least likely to be covered by a happy-path test. Worth confirming against ten_ai_base.tts2 and dropping the ones that are unsupported.

2. cancel_tts no longer calls finish_request.
On main the chain is cancel_tts -> _handle_tts_audio_end -> finish_request. finish_base_request=False changes that. bytedance_tts_duplex, the closest analogue and also duplex-streaming, does emit tts_audio_end from its own cancel_tts. If the base class calls finish_request itself after cancel_tts returns then this is correct and the flag is a good guard — but it is load-bearing, documented only in a code comment, and untested. test_flush_logic exercises the flush but asserts nothing about finish_request firing exactly once.

3. Race between complete() and cancel() can double-use a pooled synthesizer.
complete() reads self._active under _active_lock, then releases the lock before await asyncio.to_thread(active.lease.synthesizer.streaming_complete). A concurrent cancel() — a flush arriving while text_input_end=True is in flight — clears _active and calls SharedPool.return_lease(active.lease), returning the synthesizer to the pool while complete() is still blocked inside streaming_complete on that same object, and another request may already have borrowed it. The later _clear_if_active check prevents a duplicate completion event but not concurrent use of the connection. In production this would surface as cross-talk between requests, audio from A appearing under B, under interruption load. Consider holding the lock across the provider call, or a per-lease done guard so only one of complete/cancel can act on a given lease. return_lease and discard_lease have a milder version of the same pattern: they read cls._pool under the lock then act outside it, so a concurrent release_client() means returning into a pool that has been shut down.

[github-actions]

Review: fix(tts): correct cosy audio end interval

Reviewed the full diff (~1977/-1229 across 19 files). The headline fix is right, but the PR bundles a large rewrite alongside it, and a few things are worth confirming before merge.

The core fix is correct

request_event_interval_ms now measures first-audio-chunk to audio-end instead of request-start to audio-end. That matches the documented contract in docs/ai/L1/L2/extension_development.md:462:

request_event_interval_ms: wall-clock between the first audio chunk arrival and the last audio chunk arrival for this request_id — the audio receive window. Not TTFB.

The old code used request_start_ts, which folded TTFB into the interval. Good that this is locked down by assertions rather than left implicit: test_metrics.py now asserts request_event_interval_ms < ttfb_value, and test_robustness.py asserts a no-audio request reports 0. That is the right shape of test for this class of bug.

---

1. Blocking question: does the base class accept extra_metadata on these three methods?

This is my main concern. The PR passes extra_metadata= to send_tts_audio_end, send_usage_metrics, and send_tts_error. I grepped the whole repo: zero other call sites pass extra_metadata to any of those three, while send_tts_ttfb_metrics(extra_metadata=...) has 20 call sites across sibling extensions.

ten_ai_base is not vendored in the tree, so I could not confirm the signatures statically. If any of those three does not accept the kwarg, this is a TypeError on every request completion — the hot path, not an edge case.

Worth noting the unit tests would not catch this: they patch CosyTTSClient, so the mocked boundary is the vendor client, while send_tts_audio_end and friends are real base-class calls. Could you confirm against the ten_ai_base version pinned for this extension, ideally by pointing at the signature? If only send_tts_ttfb_metrics supports it, the other three calls need the kwarg dropped.

2. SharedPool turns a per-instance resource into process-global singleton state

SharedPool keys a process-wide pool on (api_key, url, workspace_id, headers) and raises when a second instance presents a different signature:

elif cls._signature != signature:
    raise ValueError("Cosy TTS pool is already initialized with different credentials, ...")

The README documents this, and the constraint is genuinely imposed by DashScope authenticating at handshake time, so the design is defensible. But it is a real behavioral regression in configuration flexibility: previously each extension instance built its own SpeechSynthesizer, so two graphs in one worker with different Cosy keys or endpoints worked. Now the second one hard-fails in on_init. Since worker processes can host multiple graphs, please confirm that is acceptable for the deployment model — and if so, the error message would be more actionable if it named the extension instance that already owns the pool.

Two smaller things in the same class:

• return_lease and discard_lease read cls._pool under the lock, then call return_synthesizer outside it. A concurrent release_client() can shut the pool down in that window, returning an object into a dead pool. Narrow, but the fix is just to hold the lock across the call.
• borrow() calls _ensure_pool_locked(), which can create a pool without incrementing _clients. If start() fails between pool creation and register(), the pool is never shut down.

3. update_params() silently changes blacklist semantics

value = self.params.pop(param_name)
if not self.is_black_list_params(param_name):
    setattr(self, param_name, value)

The pop happens unconditionally, the setattr does not. So a blacklisted param is now dropped entirely — previously it stayed in params and was forwarded to the vendor. That may well be the intent (blacklist = drop), but it is an unannounced semantic change to a shared config helper, and nothing in the tests pins it. Worth an explicit test either way.

Relatedly, to_str() still encrypts config.params.get("headers", {}), but headers is now a model field that update_params() pops out of params. Harmless, just dead code.

4. .env.example is missing the new variable

Per docs/ai/L1/08_security.md, .env.example is the complete variable catalog. It currently has only COSY_TTS_KEY= (line 142) — which already does not match the COSY_TTS_API_KEY that property.json reads. This PR adds a third variable, COSY_TTS_WORKSPACE_ID, also absent. Since you are touching this extension anyway, worth fixing all three so the catalog is accurate.

5. Global-state leakage between tests in test_client.py

_reset_shared_pool() is called manually at the top and bottom of each test. If an assertion fails mid-test, the trailing reset never runs and SharedPool._pool leaks into the next test as a stale MagicMock — turning one real failure into a confusing cascade. An autouse fixture with try/finally would make these tests order-independent:

@pytest.fixture(autouse=True)
def _clean_shared_pool():
    _reset_shared_pool()
    yield
    _reset_shared_pool()

6. Version string duplicated into a header and asserted on

SharedPool._headers() hard-codes "User-Agent": "ten-cosy-tts/0.4.4", and test_client.py asserts on that exact string. That makes three places to bump per release (manifest.json, pyproject.toml, this literal), with a test that fails only if someone bumps two of them. Either read it from the manifest or drop the version from the UA.

Smaller notes

• dump_path default changed /tmp to ./. Dumps now land in the worker CWD (the app dir) rather than /tmp. Most siblings use /tmp. If intentional, fine — flagging because it can quietly litter the working directory.
• Magic numbers. 20000 and 200000 character limits deserve named constants. Also the per-chunk check uses stripped text while request_text_characters += len(t.text) accumulates unstripped length — inconsistent, though only by whitespace.
• validate_params match block. The four-arm match computing a missing bool is more machinery than the if not value or (isinstance(value, str) and value.strip() == "") it replaced. Per the KISS guidance in 04_conventions.md, the one-liner read better.
• Benchmark scripts. ~700 lines of dev tooling now ship inside the extension package, with _percentile, _metric_summary, _first_env, and _milliseconds duplicated verbatim between the two files. R codes are disabled in tools/pylint/.pylintrc so CI will not complain, but note scripts/ is not in pylint ignore= (only tests is), so these will be linted — please confirm task lint is clean. Also, adding scripts/__init__.py makes them an importable subpackage of the extension; a plain directory would keep them clearly out-of-band.
• dashscope==1.26.4 exact pin matches the convention and is justified by the new SpeechSynthesizerObjectPool API. Compatible with aliyun_asr_bigmodel_python at >=1.26.0. A one-line comment on why it is pinned exactly would help the next person.

Test coverage

Consolidating five hand-rolled streamer classes into mock_client.py is a clear improvement — the old per-test Session/Streamer pairs were ~100 lines each of near-identical state machines. Two gaps:

• No test covers the SharedPool signature-mismatch rejection, which is the new failure mode most likely to bite in production (item 2).
• No test covers discard_lease on a broken connection, which is the recovery path the PR description highlights.

The message_type=-1 in-band delay sentinel in MockClientStream is a bit opaque; a comment naming it as a test-only sleep marker would help.

---

Summary: the interval fix is correct and well-tested. Item 1 is a genuine blocker until the extra_metadata signatures are confirmed, since it would fail on every request. Item 2 is a design tradeoff that needs an explicit sign-off rather than a code change. The rest are cleanups.

[github-actions]

Reviewed the full diff (19 files, +2014/-1245). The headline fix is right and matches repo convention: request_event_interval_ms now measures from the first audio chunk rather than request submission, same as azure_tts_python and xai_tts_python. Folding request state into _begin_request/_finish_request_once behind _finish_lock removes the current_request_finished flag juggling, and mock_client.py is a real improvement over five hand-rolled Streamer classes.

One blocking bug and a few things worth tightening.

1. Blocking: empty final chunk aborts a request that has real text

In request_tts:

if t.text_input_end:
    if not text and self.first_chunk:
        await self._finish_request_once(t.request_id, TTSAudioEndReason.REQUEST_END)
        return
    await self.client.complete(t.request_id)

self.first_chunk stays True until the first audio chunk arrives in _handle_audio_chunk. It is not a proxy for "no text submitted yet". For the common streaming pattern where an LLM sends text then a separate terminating empty chunk:

1. request_tts(text="Hello there.", text_input_end=False) submits the text.
2. request_tts(text="", text_input_end=True) arrives before the first audio callback (normal, audio is hundreds of ms away).
3. text is empty and first_chunk is still True, so the request finishes as REQUEST_END and returns.

client.complete() is never called, so the DashScope task is never finalized and the lease never returns to the pool. With COSY_TTS_POOL_SIZE = 2, two such requests exhaust it. tts_audio_end goes out with request_total_audio_duration_ms=0, and since current_request_id is now None every subsequent audio chunk is dropped by the stale-item filter in _process_audio_data. The user hears nothing.

The old code used is_first_message_of_request for this. You already track what is needed:

if not text and self.request_text_characters == 0:

Worth a regression test: two request_tts calls where the second is text="", text_input_end=True, asserting complete() is called and audio still flows.

2. Error classification

Per docs/ai/L1/04_conventions.md, invalid user input for a single request should be NON_FATAL_ERROR. The bare except Exception in request_tts maps everything to FATAL_ERROR, including three recoverable per-request conditions: the 20000-char chunk limit, the 200000-char task limit, and RuntimeError("Cosy TTS request overlap"). Suggest a dedicated exception type for these mapped to NON_FATAL_ERROR. Also 20000/200000 are DashScope protocol limits and read better as named constants.

3. bind_task() may capture a stale task id

lease = await asyncio.to_thread(SharedPool.borrow, self.config, callback)
callback.bind_task(lease.synthesizer.get_last_request_id())

This reads the task id before streaming_call() submits the task. On a fresh object that is presumably empty (harmless, on_event guards with if self.task_id and ...). On a reused pooled object it may return the previous task id, in which case on_event discards every event via the "Discarded Cosy TTS event for a stale task" branch, silently losing billed_characters and request_uuid behind a warning. Can you confirm what dashscope 1.26.4 returns immediately after borrow_synthesizer()? If it can be stale, bind after the first streaming_call. Also, a None return makes str(header.get("task_id", self.task_id)) produce the literal string "None".

4. SharedPool lock released before the SDK call it guards

return_lease and discard_lease both do:

with cls._lock:
    current_pool = cls._pool
if lease.pool is current_pool:
    lease.pool.return_synthesizer(lease.synthesizer)

A concurrent release_client() can drop _clients to zero and call shutdown() in the window between reading current_pool and return_synthesizer. Same class of issue in borrow. Holding the lock across the SDK call would serialize borrows, so if that is deliberate it deserves a comment; otherwise a per-pool refcount so shutdown() cannot run with leases outstanding. Minor and related: borrow() calls _ensure_pool_locked(), so a borrow without a prior register creates a pool with _clients == 0 that release_client() then never shuts down. Not reachable today, but it makes the invariant implicit.

5. Version string duplicated a third time

SharedPool._headers hardcodes "User-Agent": "ten-cosy-tts/0.4.4" and test_client.py asserts that exact string. The version now lives in manifest.json, pyproject.toml, and this literal, so the next bump either drifts silently or breaks a test for an unrelated reason. Suggest dropping the version from the UA or deriving it from the manifest.

6. Test hygiene

test_client.py mutates SharedPool class state and resets it with a bare _reset_shared_pool() at the end of each test. Any failing assertion above that line skips the reset and leaks a MagicMock pool into later tests, turning one real failure into a cascade. An autouse fixture resetting in teardown would make these order-independent.

Coverage is otherwise solid: test_pool_uses_custom_url_workspace_and_headers, test_borrow_forwards_provider_params_and_pcm_sample_rate, and the new interval assertions all pin down what this PR changes. Two gaps: the case in section 1, and nothing covers a mid-stream cancel() actually returning vs discarding the lease, which is the trickiest logic here and only exercised on the happy path.

7. Smaller notes

• dump_path default changed /tmp to ./, so dumps land in the worker CWD. Matches property.json so probably intentional, but easy to miss in the diff.
• SUPPORTED_PCM_SAMPLE_RATES (config.py) and AUDIO_FORMAT_MAPPING (cosy_tts.py) must stay in sync. With DEFAULT_AUDIO_FORMAT gone, borrow() does an unguarded AUDIO_FORMAT_MAPPING[config.sample_rate], a KeyError if they diverge. frozenset(AUDIO_FORMAT_MAPPING) removes the coupling.
• validate_params uses a 5-line match where the original was one line. Given these are all typed str/int in Pydantic, the original if not value or (isinstance(value, str) and not value.strip()) was clearer and more in line with the repo KISS principle.
• utils.redact_json has no other caller in the repo (every other extension uses explicit utils.encrypt on known keys). Worth confirming it exists in ten_ai_base 0.7.58 and redacts nested provider dicts the way test_to_str_redacts_nested_provider_secrets expects. The manifest dep was tightened from "0.7" to "0.7.58" here, so this is the one thing I could not verify from the repo alone. Same question for send_usage_metrics(extra_metadata=) and metrics_connect_delay(), which only cartesia_tts uses.
• finish_request no longer receives error=; send_tts_error is now a separate call after send_tts_audio_end. If the base class used that argument for state transitions, this is a silent behavior change worth a sanity check.
• dashscope==1.26.4 exact pin is right given the SpeechSynthesizerObjectPool dependency, but aliyun_asr_bigmodel_python pins dashscope>=1.26.0 in the same container. They resolve today; ~=1.26.4 keeps the API guarantee while allowing patch fixes.
• 703 lines of benchmark scripts ship inside the extension package. Well written and documented, but dev-only and they will be linted and packaged with the extension. Consider a repo-level tools directory.
• Version jumps 0.4.2 to 0.4.4, skipping 0.4.3. Intentional?

Before pushing

Per docs/ai/L1/07_gotchas.md, task lint is fatal on a single W0611, and this diff removes a lot of imports (asyncio, AsyncMock, MagicMock, pytest, RequestState, WebSocketConnectionClosedException, Optional in places):

sudo docker exec ten_agent_dev bash -c "cd /app && task format && task check && task lint"

Overall a good, well-structured refactor. The lifecycle is genuinely easier to reason about than what it replaces and the interval fix is correct. Section 1 is the one I would want fixed before merge; 2 through 4 are correctness issues I would want at least answered.

[github-actions]

Reviewed the full diff (19 files, +2018/-1244). This is a solid rewrite of the Cosy TTS lifecycle — the pooled SDK path, single-writer _finish_request_once, and typed QueueItem/ProviderError dataclasses are a real improvement over the old tuple-based queue and scattered current_request_finished flag. Notes below, most-important first.

1. first_chunk guard for empty-text requests is wrong after audio has streamed (extension.py)

In request_tts, the text_input_end branch is:

if not text and self.first_chunk:
    await self._finish_request_once(t.request_id, TTSAudioEndReason.REQUEST_END)
    return
await self.client.complete(t.request_id)

self.first_chunk is only flipped to False in _handle_audio_chunk, i.e. when the first audio chunk actually arrives from the provider. For a normal streaming request ("hello" → "" + text_input_end=True), if the final empty chunk arrives before the provider returns its first audio frame — which is the common case for a short synthesis — first_chunk is still True, so the extension finishes the request early and never calls client.complete(). The DashScope task is left open (lease never returned to the pool) and the audio that arrives afterward is dropped as a stale callback.

The old code used a dedicated is_first_message_of_request flag set at request start and cleared once text was submitted, which is the correct signal. Suggest tracking whether any non-empty text was submitted for the request, e.g. self.request_text_characters == 0, instead of first_chunk.

2. send_tts_audio_end before send_tts_error ordering (extension.py _finish_request_once)

_finish_request_once emits audio_end → usage_metrics → tts_error. Other extensions in the repo (elevenlabs, google, minimax) send the error first, then audio_end/finish_request. Downstream consumers that treat audio_end as the request terminator may drop the trailing error. Worth confirming the intended contract with ten_ai_base 0.7.58, or reordering for consistency.

3. manifest.json pins ten_ai_base to 0.7.58

Every other extension in the repo declares "version": "0.7" (73 occurrences); only spatius_avatar_python pins a patch (0.7.40). Pinning a patch means this extension stops resolving when the system package moves to 0.7.59. If redact_json, metrics_connect_delay, and extra_metadata= on send_tts_audio_end/send_usage_metrics are all new in 0.7.58, a floor is needed — but please confirm the resolver treats 0.7.58 as a floor rather than an exact match, otherwise this will break on the next base bump.

4. SharedPool process-wide singleton is a real constraint, documented but sharp

The first extension instance in a worker wins the pool config; a second instance with a different api_key/url/workspace_id/headers raises ValueError from _ensure_pool_locked and fails on_init. The README calls this out, which is good. Two follow-ups:

• The failure surfaces as a generic FATAL_ERROR with request_id="", which is hard to diagnose in a multi-graph deployment. A KEYPOINT log naming both signatures would help.
• _clients refcounting: if SharedPool.register succeeds but a later step in on_init raises, self._registered stays False and release_client is never called, so the pool leaks for the process lifetime. Setting _registered = True immediately after register returns (it already is) is fine, but the try in on_init swallows the exception without calling client.stop() — so an on_init failure after start() leaks the refcount. Consider calling await self.client.stop() in the except branch.

5. dashscope==1.26.4 exact pin

pyproject.toml and requirements.txt both move from >=1.26.4 to ==1.26.4. Understandable given the dependency on SpeechSynthesizerObjectPool internals (the return_synthesizer() is False behavior in SharedPool.return_lease is undocumented SDK behavior). But aliyun_asr_bigmodel_python declares dashscope>=1.26.0 in the same tree — an exact pin here will conflict at install time if that extension ever resolves to a newer version in the same environment. Worth a comment in pyproject.toml explaining why the pin is exact so it is not "cleaned up" later.

6. Smaller items

• config.py validate_params: the match statement replacing if not value or ... changes behavior — sample_rate=0 and model="" were previously caught by the falsy check; now only ""/None are. sample_rate is separately validated against SUPPORTED_PCM_SAMPLE_RATES, so this is covered, but the match is more code than if value is None or (isinstance(value, str) and not value.strip()) for the same result. KISS per 04_conventions.
• config.py dump_path default changes /tmp → ./. That matches property.json, but it means dumps land in the worker CWD. Intentional?
• SharedPool._headers hardcodes "ten-cosy-tts/0.4.4", and test_client.py asserts on that exact string. Every version bump now requires editing two files plus manifest.json/pyproject.toml. Reading the version from the manifest, or dropping the version from the UA, would avoid the drift.
• cosy_tts.py synthesize_audio/complete/cancel use bare except Exception around the asyncio.to_thread calls. Fine for provider-boundary robustness, but task lint (pylint, strict per 04_conventions) may flag broad-exception-caught — please confirm task check && task lint passes in the container.
• scripts/benchmark_*.py: nice that API keys and text payloads are never printed. Both scripts default --workspace-id from DASHSCOPE_WORKSPACE_ID while the extension and README use COSY_TTS_WORKSPACE_ID. Aligning them (or adding COSY_TTS_WORKSPACE_ID to _first_env) would avoid confusion.
• tests/mock_client.py: encoding the delay as str(delay).encode() in a QueueItem with message_type=-1 is clever but opaque. A dedicated delay field or a separate sentinel type would read better.

Test coverage

Good: the MockClientStream consolidation removes a lot of duplicated stateful mock classes, and the new test_metrics.py assertion (request_event_interval_ms < ttfb_value) directly pins the bug this PR is named for. test_robustness.py asserting a zero interval for an audio-less request is the right complement.

Gaps worth adding:

• A test for the case in item 1 above: non-empty text, then text_input_end=True with empty text, where completion arrives after the end-marker. That is the regression path.
• No test for SharedPool signature mismatch (two instances, different api_key) or for discard_lease on a failed connection — both are new load-bearing paths.
• test_state_machine.py drops the RequestState import and the pytest import; confirm nothing else in that file still needs them (unused imports fail task lint).

Commit messages and branch name follow the conventional-commit rules in AGENTS.md. Nothing security-sensitive stood out — secrets stay in params, to_str() routes through utils.redact_json, and the benchmark scripts avoid logging keys.

[github-actions]

Reviewed the full diff (19 files, +2042/-1247). This is a large rewrite of cosy_tts_python -- object-pool connection reuse, typed error/completion payloads, and test-harness consolidation -- carrying the titled interval fix. Direction is good: MockClientStream replaces five bespoke streamer classes, and _finish_request_once centralizes teardown that was duplicated across five call sites. Notes below, most significant first.

The titled fix is correct and matches convention

request_event_interval_ms now anchors at first_chunk_ts instead of request_start_ts, so TTFB is no longer double-counted into the interval. This matches siblings: google_tts_python anchors on sent_ts (set at first chunk), cartesia_tts on speaking_start_ms, xai_tts_python explicitly on _first_audio_chunk_ts. The zero-audio case returning 0 also matches google/cartesia. Good that test_metrics.py asserts request_event_interval_ms < ttfb_value rather than a bare threshold -- that pins the semantic, not the timing.

1. on_complete is a no-op, so a stalled provider finish can starve the pool

The reasoning in the comment is sound for the normal path, but it leaves complete(), cancel(), and _abort_active() as the only release paths. If DashScope emits task-finished and the socket then goes quiet without streaming_complete() returning, the asyncio.to_thread(...streaming_complete) call blocks indefinitely -- there is no timeout on it, and commit 275b764 removed local timeouts in favor of provider ones.

One wedged task holds one of only COSY_TTS_POOL_SIZE = 2 leases. Two wedged tasks starve the extension permanently with no error surfaced, since _get_or_create_active then blocks on borrow_synthesizer forever. Worth confirming the SDK streaming_complete has an internal deadline; if not, an asyncio.wait_for wrapper is cheap insurance against a hang that needs a worker restart to clear.

2. Dump writes can be silently dropped

_handle_audio_chunk uses recorder_map.get(request_id) and skips when absent, but _flush_pcm_writer uses .pop(). A late callback for an already-finished request_id (which the stale-callback guard permits while current_request_id has not yet been reassigned) reaches send_tts_audio_data but never the dump file. The old code checked config.dump explicitly and logged. Suggest logging once when a chunk arrives with dump enabled but no recorder, so a truncated dump is diagnosable.

3. _clients count can desync from _registered

If the task is cancelled between register() completing in the thread (count incremented) and self._registered = True, the increment is never matched by release_client and the pool never shuts down. Unlikely in on_init, but the fix is free: set the flag before the await, or use try/finally. The ordering inside register itself is correct -- the count increments only after _ensure_pool_locked succeeds.

4. Pool signature-mismatch diagnostic is thin

_ensure_pool_locked raises ValueError when a second instance registers a different (api_key, url, headers) signature. The README documents this honestly, but the failure lands in on_init as FATAL_ERROR without telling the operator which instance holds the pool or what signature it holds. Two Cosy instances with different keys in one graph is plausible. Including both signatures non-secret parts (URL, header keys, not the key) would make this self-service.

5. format field is stored but never read

CosyTTSConfig.format is added, forced to "pcm", asserted in a test, and never read -- the real format comes from AUDIO_FORMAT_MAPPING[sample_rate]. It is also in _IGNORED_PARAM_KEYS, so the field exists only to be overwritten. Per the YAGNI/KISS guidance in docs/ai/L1/04_conventions.md, dropping the field and keeping the ignored-key entry is simpler. If retained for log visibility, add a comment.

6. Magic numbers in request_tts

20000 (per-chunk chars) and 200000 (per-task chars) are inline literals raised as bare ValueError. These look like provider limits -- hoist to module constants beside COSY_TTS_POOL_SIZE with a comment citing the DashScope limit. Also request_text_characters += len(t.text) uses the unstripped length while the 20000 check uses stripped text; minor inconsistency in what is measured.

7. Sample-rate validation is now strict -- confirm intended

Previously an unsupported rate warned and fell back to DEFAULT_AUDIO_FORMAT (now deleted). Now validate_params raises and SharedPool.borrow does an unguarded AUDIO_FORMAT_MAPPING[...] lookup. Fail-fast is better and matches the FATAL_ERROR guidance for invalid required config, but it turns a previously-working misconfiguration into a startup failure. Worth a line in the PR description in case example graphs carry an out-of-range rate.

8. Security: pinning and redaction both improved

dashscope==1.26.4 is the right call now that the code depends on SpeechSynthesizerObjectPool, an internal-ish surface a minor bump could move. to_str() moving to utils.redact_json(self.model_dump()) now covers nested secrets, and test_to_str_redacts_nested_provider_secrets proves it; the config: prefix with LOG_CATEGORY_KEY_POINT is preserved so QA matching still works. Benchmark scripts correctly avoid printing keys or text.

Nit: User-Agent: ten-cosy-tts/0.4.4 hardcodes the version, duplicating manifest.json and pyproject.toml -- three places to bump. A test asserts the literal, so a miss fails loudly rather than shipping wrong, but one source would be better.

9. Test coverage: much improved, three gaps

test_client.py grew 51 to 210 lines with real coverage of param extraction, URL precedence, redaction, and pool wiring. _reset_shared_pool() is necessary given class-level state, and the protected-access disable is correctly file-scoped. Gaps:

• No test for the signature-mismatch ValueError -- the documented multi-instance constraint and least obvious behavior here.
• No test for return_lease when return_synthesizer returns False. That comment describes subtle SDK behavior; a test would lock in exactly what an SDK bump silently changes.
• mock_client.py encodes delays as sentinel message_type=-1 with the float stringified into payload bytes. It works but is opaque for a file five modules now depend on; a small Sleep dataclass would read better.

10. Conventions and CI

Commits are conventional and correctly scoped, though fix(tts): undersells ten commits of refactor and feat work -- a reader hunting the interval fix has to find it among the others. Not blocking.

Please confirm task format && task check && task lint ran in-container before merge. The refactor removed several import users, and docs/ai/L1/07_gotchas.md flags that one W0611 fails CI. Three to verify where the prior users were deleted in this diff: test_basic.py still imports Optional and MESSAGE_TYPE_CMD_COMPLETE, and test_robustness.py still imports Optional.

Net: the interval fix is right, the pooling architecture is a genuine improvement, and the test consolidation pays for itself. Items 1, 2, and 3 are the ones I would want addressed or explicitly waved off before merge -- item 1 has a production-starvation shape.