feat: meter audio and video generation - #1425
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive media-usage tracking system for non-LLM modalities (such as image, video, TTS, and ASR) to meter usage alongside LLM tokens. It implements thread-safe usage recording, aggregation, and validation across various providers and tools. Feedback was provided to ensure safe dictionary access when retrieving video durations to prevent potential attribute errors.
ba22ad1 to
11537a2
Compare
OliverBryant
changed the title
11537a2 to
f2e4567
Compare
f2e4567 to
b549d60
Compare
46bba4e to
3b14971
Compare
…ble to fail Fourth review round. Both blocking findings were consequences of how I fixed NEW-A last round. N1 — `_turn_delta` no longer deep-copies the whole details list. `usage.snapshot()` copied the entire cumulative list under the lock and then discarded everything before `_initial_details_len`. The list grows monotonically across turns (seeds restore the persisted list), and the path is polled once per agent step *and* once per streamed LLM chunk (`runtime.py` `_raise_if_interrupted` -> `interrupt_reason_for_quota`), while holding the lock that serialises every LLM adapter's token write. Measured: 10.6us at 100 rows, 103us at 1000, 487us at 5000 — versus 0.3us for the tail. New `TokenUsage.detail_tail(start)` returns the tail plus `tool_calls` in one lock acquisition, preserving exactly the atomicity the snapshot was added for, at O(delta) instead of O(total). The now-redundant `_copy_details(delta_details)` at both quota call sites is gone, since the rows come back detached. N2 — the regression test for that fix could not fail. `read_delta` called `_turn_delta()` with no argument, so it went through `get_token_usage()` inside a bare `threading.Thread`, which does not inherit contextvars: it lazily built a fresh empty `TokenUsage` disconnected from the object the writer mutated, and observed `([], 0)` either way. Against the pre-fix code `snapshot()` was never called at all, so the patch never fired, `entered` was never set, and the writer's `wait()` timed out silently with its return value unchecked. The single assertion degenerated to `0 == 0` on both sides. `usage` is now passed explicitly, and the test asserts `entered.is_set()` and `released.is_set()` so a silent timeout fails. Mutation-verified this time against the committed test rather than a side script: 50/50 failures with two unlocked reads, 0/50 with `detail_tail`. That distinction is the gap that let the empty test through last round. Also addressed from this round: the `REQUESTS` error message now reports the caller's raw value rather than the coerced one (N7); `_lock` is an `RLock` so a future nested acquisition cannot self-deadlock (N5); `__post_init__` normalises `details`, covering the constructor and `from_dict` together (N3); `copy_detail_rows` replaces the same copy expression reimplemented four times, removing the filter asymmetry where `to_dict` would raise on a non-dict row that the others skipped; `estimate_tokens` is renamed `estimate_media_tokens` and exported, since three unrelated `estimate_tokens` already exist with different algorithms (N9); its CJK ranges gain punctuation, fullwidth forms and Ext-A (N10); the module docstring, the `add_media_usage` "Raises" section, the aggregators' detached-list contract, and the details-are-dicts invariant are documented (N13, N8, N14, N12); and three false or contradictory comments are corrected (N11, N16, plus the stale seed rationale). Test gaps closed: `_coerce_int`'s hardening exercised through the LLM `add_token_usage` path (N6), the `model_id` branch of `aggregate_media_usage_by_model` (N17), and `test_concurrent_merge_loses_no_counts` rewritten with a barrier and 8x100 merges — one merge per thread of a one-row source would have passed against an unlocked `merge` (N15). Self-review caught that adding normalisation to `__post_init__` made `snapshot()` copy twice, doubling it from 487us to 1066us at 5000 rows; `snapshot` now assigns `details` after construction, back to 526us. mypy also rejected `threading.RLock` as an annotation (it is a factory, not a class), so the property is typed through a `TYPE_CHECKING` alias. NEW-F stays in xorbitsai#1495, C2 in xorbitsai#1466, and P4/P5/P8/P9/P10 in xorbitsai#1460/xorbitsai#1461. N4 (string fields reaching the JSON column uncoerced) and N19 (`media_calls` has no consumer yet) are noted as accepted for this layer: the producers that supply those strings land in xorbitsai#1424/xorbitsai#1425/xorbitsai#1457, and xorbitsai#997 names the `media_calls` consumer.
3b14971 to
90bdac5
Compare
…ble to fail Fourth review round. Both blocking findings were consequences of how I fixed NEW-A last round. N1 — `_turn_delta` no longer deep-copies the whole details list. `usage.snapshot()` copied the entire cumulative list under the lock and then discarded everything before `_initial_details_len`. The list grows monotonically across turns (seeds restore the persisted list), and the path is polled once per agent step *and* once per streamed LLM chunk (`runtime.py` `_raise_if_interrupted` -> `interrupt_reason_for_quota`), while holding the lock that serialises every LLM adapter's token write. Measured: 10.6us at 100 rows, 103us at 1000, 487us at 5000 — versus 0.3us for the tail. New `TokenUsage.detail_tail(start)` returns the tail plus `tool_calls` in one lock acquisition, preserving exactly the atomicity the snapshot was added for, at O(delta) instead of O(total). The now-redundant `_copy_details(delta_details)` at both quota call sites is gone, since the rows come back detached. N2 — the regression test for that fix could not fail. `read_delta` called `_turn_delta()` with no argument, so it went through `get_token_usage()` inside a bare `threading.Thread`, which does not inherit contextvars: it lazily built a fresh empty `TokenUsage` disconnected from the object the writer mutated, and observed `([], 0)` either way. Against the pre-fix code `snapshot()` was never called at all, so the patch never fired, `entered` was never set, and the writer's `wait()` timed out silently with its return value unchecked. The single assertion degenerated to `0 == 0` on both sides. `usage` is now passed explicitly, and the test asserts `entered.is_set()` and `released.is_set()` so a silent timeout fails. Mutation-verified this time against the committed test rather than a side script: 50/50 failures with two unlocked reads, 0/50 with `detail_tail`. That distinction is the gap that let the empty test through last round. Also addressed from this round: the `REQUESTS` error message now reports the caller's raw value rather than the coerced one (N7); `_lock` is an `RLock` so a future nested acquisition cannot self-deadlock (N5); `__post_init__` normalises `details`, covering the constructor and `from_dict` together (N3); `copy_detail_rows` replaces the same copy expression reimplemented four times, removing the filter asymmetry where `to_dict` would raise on a non-dict row that the others skipped; `estimate_tokens` is renamed `estimate_media_tokens` and exported, since three unrelated `estimate_tokens` already exist with different algorithms (N9); its CJK ranges gain punctuation, fullwidth forms and Ext-A (N10); the module docstring, the `add_media_usage` "Raises" section, the aggregators' detached-list contract, and the details-are-dicts invariant are documented (N13, N8, N14, N12); and three false or contradictory comments are corrected (N11, N16, plus the stale seed rationale). Test gaps closed: `_coerce_int`'s hardening exercised through the LLM `add_token_usage` path (N6), the `model_id` branch of `aggregate_media_usage_by_model` (N17), and `test_concurrent_merge_loses_no_counts` rewritten with a barrier and 8x100 merges — one merge per thread of a one-row source would have passed against an unlocked `merge` (N15). Self-review caught that adding normalisation to `__post_init__` made `snapshot()` copy twice, doubling it from 487us to 1066us at 5000 rows; `snapshot` now assigns `details` after construction, back to 526us. mypy also rejected `threading.RLock` as an annotation (it is a factory, not a class), so the property is typed through a `TYPE_CHECKING` alias. NEW-F stays in xorbitsai#1495, C2 in xorbitsai#1466, and P4/P5/P8/P9/P10 in xorbitsai#1460/xorbitsai#1461. N4 (string fields reaching the JSON column uncoerced) and N19 (`media_calls` has no consumer yet) are noted as accepted for this layer: the producers that supply those strings land in xorbitsai#1424/xorbitsai#1425/xorbitsai#1457, and xorbitsai#997 names the `media_calls` consumer.
90bdac5 to
fc0ebbc
Compare
…ble to fail Fourth review round. Both blocking findings were consequences of how I fixed NEW-A last round. N1 — `_turn_delta` no longer deep-copies the whole details list. `usage.snapshot()` copied the entire cumulative list under the lock and then discarded everything before `_initial_details_len`. The list grows monotonically across turns (seeds restore the persisted list), and the path is polled once per agent step *and* once per streamed LLM chunk (`runtime.py` `_raise_if_interrupted` -> `interrupt_reason_for_quota`), while holding the lock that serialises every LLM adapter's token write. Measured: 10.6us at 100 rows, 103us at 1000, 487us at 5000 — versus 0.3us for the tail. New `TokenUsage.detail_tail(start)` returns the tail plus `tool_calls` in one lock acquisition, preserving exactly the atomicity the snapshot was added for, at O(delta) instead of O(total). The now-redundant `_copy_details(delta_details)` at both quota call sites is gone, since the rows come back detached. N2 — the regression test for that fix could not fail. `read_delta` called `_turn_delta()` with no argument, so it went through `get_token_usage()` inside a bare `threading.Thread`, which does not inherit contextvars: it lazily built a fresh empty `TokenUsage` disconnected from the object the writer mutated, and observed `([], 0)` either way. Against the pre-fix code `snapshot()` was never called at all, so the patch never fired, `entered` was never set, and the writer's `wait()` timed out silently with its return value unchecked. The single assertion degenerated to `0 == 0` on both sides. `usage` is now passed explicitly, and the test asserts `entered.is_set()` and `released.is_set()` so a silent timeout fails. Mutation-verified this time against the committed test rather than a side script: 50/50 failures with two unlocked reads, 0/50 with `detail_tail`. That distinction is the gap that let the empty test through last round. Also addressed from this round: the `REQUESTS` error message now reports the caller's raw value rather than the coerced one (N7); `_lock` is an `RLock` so a future nested acquisition cannot self-deadlock (N5); `__post_init__` normalises `details`, covering the constructor and `from_dict` together (N3); `copy_detail_rows` replaces the same copy expression reimplemented four times, removing the filter asymmetry where `to_dict` would raise on a non-dict row that the others skipped; `estimate_tokens` is renamed `estimate_media_tokens` and exported, since three unrelated `estimate_tokens` already exist with different algorithms (N9); its CJK ranges gain punctuation, fullwidth forms and Ext-A (N10); the module docstring, the `add_media_usage` "Raises" section, the aggregators' detached-list contract, and the details-are-dicts invariant are documented (N13, N8, N14, N12); and three false or contradictory comments are corrected (N11, N16, plus the stale seed rationale). Test gaps closed: `_coerce_int`'s hardening exercised through the LLM `add_token_usage` path (N6), the `model_id` branch of `aggregate_media_usage_by_model` (N17), and `test_concurrent_merge_loses_no_counts` rewritten with a barrier and 8x100 merges — one merge per thread of a one-row source would have passed against an unlocked `merge` (N15). Self-review caught that adding normalisation to `__post_init__` made `snapshot()` copy twice, doubling it from 487us to 1066us at 5000 rows; `snapshot` now assigns `details` after construction, back to 526us. mypy also rejected `threading.RLock` as an annotation (it is a factory, not a class), so the property is typed through a `TYPE_CHECKING` alias. NEW-F stays in xorbitsai#1495, C2 in xorbitsai#1466, and P4/P5/P8/P9/P10 in xorbitsai#1460/xorbitsai#1461. N4 (string fields reaching the JSON column uncoerced) and N19 (`media_calls` has no consumer yet) are noted as accepted for this layer: the producers that supply those strings land in xorbitsai#1424/xorbitsai#1425/xorbitsai#1457, and xorbitsai#997 names the `media_calls` consumer.
fc0ebbc to
1066ea9
Compare
… real Fifth review round. Three of the five blocking findings were about claims I made rather than code I wrote, so each is verified by mutation against the committed test this time, not a side script. Locking tests now require the lock (finding 2). Replacing `_lock` with `nullcontext()` left all six concurrency tests passing. Investigating why showed my earlier "~85% count loss" figure was wrong: it came from a mutation that inserted `sleep(0)` between the read and the write, not from the real implementation. On CPython 3.12 even a bare `self.n += 1` loses nothing at 8x200000 threads-iterations, so no increment-counting test can justify the lock. What the lock actually protects is the *pairing* of the counter with its detail rows: `record_media_call` bumps and appends as one operation, and an unlocked reader lands between them. The test now asserts that invariant under `sys.setswitchinterval(1e-9)` (required — at the default interval the window is unhittable). Mutation-verified: 6/6 runs tear without the lock, 0/6 with it. `detail_tail`'s atomicity test now exercises the interleaving (finding 3, the second round this test was shown not to test its claim). Both earlier versions wrapped `detail_tail` itself and released the writer *before* delegating to the real accessor, so the write always completed before the locked read began. The interleaving is now forced from inside the lock, by patching `copy_detail_rows` — called after the lock is taken and `details` read, but before `tool_calls`. Mutation-verified against the reviewer's own torn implementation (two acquisitions with a sleep between): it fails with media=0, tool_calls=1. The unit-per-modality invariant is enforced, not just documented (finding 5). `MEDIA_UNIT_BY_CALL_TYPE` rejects a unit that does not match the call type at the write boundary. The invariant was stated in three docstrings and violated by this PR's own tests; the reviewer found six sites, and the new check found four more (`seconds` paired with `tts`) that a grep for `images`+`video` missed. All ten fixed. Verified every downstream producer in xorbitsai#1424/xorbitsai#1425/xorbitsai#1457 pairs legally, so the new constraint breaks none of them. `_coerce_float` catches `OverflowError` (finding 1). `float(10**400)` raised it uncaught, so the error-swallowing wrapper dropped the entire billing row — the opposite of the documented reject-to-0.0 contract. `_coerce_int` was already fixed for this in an earlier round; the sibling was missed. The token-vs-unit pricing precedence claim is removed rather than implemented (finding 4). It existed only as a docstring sentence: nothing expressed or enforced "price by tokens instead of by unit", the row schema carries no discriminator, and `provider_tokens` has zero consumers outside this file. Implementing it would add a billing dimension to a PR that wires no producers and has no pricing consumer, so the aspirational sentence is gone and the design question stays tracked in xorbitsai#1461. Minor: `add_media_usage` reports the caller's raw quantity in the REQUESTS error (the prior fix covered only the direct path); the wrapper's warning log now includes model/model_id/unit/quantity, not just call_type; the two stale `bind_usage_to_thread` references are dropped (the symbol does not exist); `_turn_delta`'s return type is parameterised. Simplifications taken: `TypeGuard` from stdlib `typing` rather than `typing_extensions` (the project requires >=3.11), and `task_tracker`'s `_copy_details` is deleted in favour of the `copy_detail_rows` helper this PR added for exactly that consolidation. Not changed, with reasons: `resolve_billing_model` and `record_media_seconds` are flagged as having no production caller, but both are consumed by xorbitsai#1425/xorbitsai#1457 in this same series — removing them here would only move the diff. Findings 7-9 and 11-16 are noted as accepted or tracked in the reply.
10883c8 to
d137799
Compare
Wire the audio and video modalities into the media usage primitives: ASR billed by transcribed seconds, TTS by input characters, and music, sound effects and video by duration. Video shares the duration-billed path because it shares the same unit invariants, not to pad the change. Also lands `web/tracking/standalone_usage.py`, which `/speech/transcribe` needs here: it binds a TokenUsage for work that is not a tracked agent task and reports it to the quota hook on exit. Without it that endpoint records into a throwaway object nothing reads — the provider bills and the usage evaporates. `bind_usage_to_thread` runs each call inside `copy_context()` + `ctx.run` so a binding cannot outlive the job on a pooled executor thread, and `_report` checks `has_usage_record_hook()` before checking out a DB session, since with no hook installed the record call is a guaranteed no-op. Fix ASR recording 0 seconds on every call outside audio_tool. `/speech/transcribe` and the Telegram voice handler both called `transcribe()` without `verbose=True`, so the provider returned a bare string with no timings and every record was an unbillable 0-second entry. Both now pass it; each already read the text via `getattr(result, "text", result)`, so the richer result needs no other handling. The Telegram test fakes accepted no `verbose` argument at all, unlike every real provider, so they were widened to match the real signature — a test double narrower than the interface it stands in for is what let this ship. Unify ASR billing identity on the model name. The three entry points previously wrote three different identities for one physical model: audio_tool wrote a name into `model_id` (its registries are keyed by `model_name`, so the membership test was a name test), `/speech/ transcribe` wrote the real DB id, and Telegram read a bare `.model` attribute with no placeholder filter. The aggregator groups on `model_id or model`, so one model produced up to three rows that could never be reconciled once written. Only the name is available on all three paths, so `model_id` is now left unset everywhere and Telegram goes through the shared resolver. The same name-into-`model_id` mistake is fixed on both TTS paths. Bill provider calls that already happened. Music and sound effects raised on an empty or malformed result *before* recording, so a call that succeeded at the HTTP level but returned nothing went unmetered even though the provider charged for it. Recording now precedes validation in both, matching TTS, and the policy is stated in a comment at each site — the four otherwise-symmetric tools previously had three different implicit rules. Delete the duplicate billing helpers. `record_asr_seconds` is now a thin wrapper over the shared `record_media_seconds`, and audio_tool's private `_resolve_billing_model` copy delegates to the shared `resolve_billing_model` instead of reimplementing it without the placeholder filter. That copy billed the Xinference default ASR/TTS model — which exposes no `model_name` and whose default-getter builds a separate instance, so identity matching never hit — under the literal string "default". It now falls back to the provider class name, which attributes cost to something real.
`FakeASR.transcribe` in test_model_api took only (audio, language, format), so the endpoint's new `verbose=True` raised TypeError and the request returned 500. The real `BaseASR.transcribe` takes `verbose` and `**kwargs`; the double was narrower than the interface it stood in for, which is the same reason the missing `verbose=True` went unnoticed in the first place. The fake now mirrors the real signature and records the flag, and the call assertion checks `verbose=True` is actually passed — so a revert of the endpoint change fails here instead of silently recording 0-second, unbillable ASR usage.
standalone_usage.py sits on the quota path and shipped with no tests: the original split lost this file, which exists in xorbitsai#997 but landed in neither this branch nor main. Recovered and migrated to the merged media API (xorbitsai#1527), where the unit is derived from call_type rather than passed in. Two changes beyond the mechanical migration: - Stub has_usage_record_hook. _report gained a short-circuit that returns before checking out a session when no hook is installed, which the xorbitsai#997 tests predate — dropped in verbatim they would pass vacuously, asserting nothing. - Scope the entry-point wiring guard to transcribe_speech_input. The KB ingest and Telegram entry points the original asserted on bind their scopes in a later PR in this series. Added a case for the short-circuit itself, so the no-hook fast path cannot regress into a pool checkout per transcription.
d137799 to
2e24aa0
Compare
|
Rebased onto main now that the media billing primitives (#1527) have merged. What was droppedThis branch carried its own full copy of the media primitives — What was migrated, and why it would have failed silently#1527 removed Two TTS call sites in The rebase itself was conflict-free, and Verified end-to-end rather than by signature: constructing a real Recovered test file
Two changes beyond the mechanical migration were necessary:
Added a case for the short-circuit itself, so the no-hook fast path cannot regress into a pool checkout per transcription. The recovered suite was mutation-tested three ways against the real module, each caught by a different test: dropping Note that |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
This PR adds media-usage events for ASR, TTS, music, sound effects, and video on top of the shared media primitives, and introduces a standalone usage sink for work that does not run under TaskTracker.
It records provider-call quantities before post-provider validation and adds ContextVar/thread-bridge coverage, but the current producer wiring still leaves Telegram ASR without a reporting lifecycle, Ark asynchronous video without duration reconciliation, and model identity fields inconsistent.
The scope is therefore materially broader than a local helper change: it changes quota-visible behavior across web, Telegram, media tools, and tracking tests.
Blocking: yes — recommended event: REQUEST_CHANGES
Update summary
Commit 78931f4 (feat: meter audio and video generation) adds the audio/video producers and standalone reporting path; commit 910b8f1 (fix: widen the /speech/transcribe test fake to the real ASR signature) aligns the transcription fake with the real ASR call contract; and commit 2e24aa0 (test: cover the standalone usage sink) adds coverage for the standalone sink.
The branch is rebased onto the merged media primitives from #1527, so the carried primitive/image implementation was dropped and this review covers the resulting audio/video metering diff.
Approach verdict
acceptable-with-reservations. Reusing TokenUsage.details, deriving units from MediaCallType, centralizing ASR duration resolution, and adding a reusable usage_scope sink is a leveraged approach for synchronous task-scoped and standalone producers. The reservations are architectural: Telegram records before any reporting lifecycle exists, Ark asynchronous video crosses the task-completion boundary without a pending/reconciliation mechanism, and the producer paths discard or overload the canonical model identity instead of carrying a structured name/ID pair. The approach is not ready to merge until the three major billing/identity gaps below are resolved.
Findings
Major findings
Root A — Telegram ASR has no reporting lifecycle (major)
src/xagent/web/channels/telegram/bot.py:1406 records ASR usage while Telegram is preparing the message, before _process_user_messages_batch calls AgentManager.execute_task and before TaskTracker binds the task usage. record_asr_usage only appends to the current ContextVar; on an unbound path that is a throwaway accumulator, while a reused long-lived queue task can expose a stale binding. When the later tracker starts, it installs a fresh task accumulator, so these provider-billed ASR seconds are not persisted or sent to quota_hooks.record_usage, and stale-context cases can misattribute usage. Wrap the transcription in usage_scope(owner_user_id) using the authorized owner resolved around bot.py:1652, or move the tracker boundary so the whole operation has one explicit lifecycle; add a behavioral hook test for owner attribution and exactly-once reporting. This is the same underlying concern raised in PR #997 reviews 4786040090 and 4796045078 and inline comment 3664560548; current conversation comment 5365144438 only says Telegram is deferred to a later PR and gives no concrete tracking number, so it is not a waiver.
Root B — Async video duration is finalized as zero (major)
At src/xagent/core/tools/core/video_tool.py:734-740, Ark calls with wait_for_result=False return created-task metadata without a duration (src/xagent/core/model/video/ark.py:354-362). coerce_duration therefore produces None, and the record_media_seconds call at video_tool.py:736 records a permanent quantity=0 row before the provider task completes. TaskTracker can then persist and report that zero row with no provider-task key, pending event, callback, or later reconciliation path, so every such asynchronous generation is under-billed. Either require metered video calls to wait for terminal duration, use a positive requested duration only where the provider contract guarantees that is the billed quantity, or persist a pending event keyed by provider task ID and reconcile it exactly once at completion. The Xinference n>1 multiplication in the current head is a separate fix and is not part of this finding. PR #997 issue comment 5090613906 and inline/review history including 3662326661 and 4796045078 already identified the missing async true-up; no concrete current-PR fix or waiver exists.
Root C1 — ASR drops unique model identity and can merge configurations (major)
src/xagent/web/api/model.py:1023-1025 has the resolved db_model and its unique model_id in scope, but deliberately records only model_name. model_name is not unique, so two configured rows such as different IDs sharing a provider name are persisted identically; aggregate_media_usage_by_model then keys both rows by the same name and cannot price or attribute them separately. Carry a structured (model_name, model_id) through the ASR registry/adapter and record both fields, then add coverage with duplicate names and different IDs; update the name-keyed registry rather than discarding the DB identity at this endpoint. The earlier PR #1422 identity discussion (inline 3801868215 and reply 3802262493) deferred a structured resolver, and issue #1460 does not make the ID unavailable here: this endpoint has it and the current PR discards it.
Minor findings
Root C2 — Video media details have the wrong identity field shape (minor)
src/xagent/core/tools/core/video_tool.py:740 writes the configured actual_model_id into model and leaves model_id empty, losing the provider model_name. The aggregation fallback can still preserve same-modality totals by grouping on that ID, so the immediate defect is malformed external/UI metadata and loss of the canonical field rather than the ASR collision above. Pass the provider name as model and the configured ID as model_id, and test with deliberately distinct values. This is the display/field-shape issue previously described in PR #997 review 4819750857 and inline comment 3664560558.
Root C3 — Music media details duplicate the configured ID (minor)
src/xagent/core/tools/core/music_tool.py:167-168 calls resolve_billing_model(configured_model_id, model), which returns the configured ID before consulting the provider name, and then writes that same ID to model_id. Music totals can still group by the populated ID, but consumers expecting model=name and model_id=id receive malformed metadata and the provider name is lost. Propagate the provider name separately and keep configured_model_id only in model_id; add a test where the provider name and configured ID differ.
Root C4 — Sound-effect media details duplicate the configured ID (minor)
src/xagent/core/tools/core/sound_effect_tool.py:178-179 has the same scalar-resolver problem as music: normal rows carry the configured ID in both fields, so the provider model_name is unavailable to display or external consumers. Keep the configured ID in model_id, resolve the provider name for model, and add a distinct-name/distinct-ID regression test. These C2-C4 occurrences share one structured identity fix contract but remain separate comments because their impacts and call sites differ.
Root D — The thread bridge shares a non-thread-safe accumulator (minor)
src/xagent/web/tracking/standalone_usage.py:130 captures one mutable TokenUsage and binds that same object on every invocation of a reusable wrapper. copy_context() isolates the binding, not the object; TokenUsage explicitly documents itself as not thread-safe, and concurrent LLM/tool read-modify-write operations or report snapshots can lose counters or observe partial details. No production caller currently uses this new bridge, which limits present impact but leaves a reusable API trap. Synchronize mutation, detached snapshots, merge, and report, or create per-invocation child usage values and merge them under a lock; add repeated concurrent submissions and report-ordering tests.
Root E — The entry-point wiring test checks source text, not billing behavior (minor)
tests/web/tracking/test_standalone_usage.py:199 only asserts that the literal usage_scope( appears in inspect.getsource(model.transcribe_speech_input). It would pass with the wrong user, wrong scope boundary, no record_asr_usage, or a non-reporting hook, and the changed Telegram tests likewise do not observe a quota report. Replace it with a behavioral endpoint test that captures the hook and asserts authenticated owner, ASR quantity/identity, and exactly-once reporting; add the analogous Telegram assertion with Root A. This repeats the source-inspection gap from PR #997 review 4819750857, but it is kept separate from Root A's production defect.
Root F — ASR duration-precedence branches lack behavior tests (minor)
tests/core/tools/core/test_media_tool_billing_policy.py:138-149 covers a bare string and one ordered segment, but not provider totals in duration, audio_duration, or duration_seconds, invalid/non-finite fallback, the maximum of unsorted segment ends, or the intentional no-timing row. A regression in resolve_asr_seconds could therefore leave the suite green while undercounting or changing precedence. Add focused resolver tests for each provider-total field, invalid/non-finite fallback to the maximum segment end, and no usable timing; retain the existing zero-second test. The remaining gap is the same contract area noted in PR #997 inline comment 3656328337.
Root G — Quota documentation claims an undefined pricing precedence (minor)
src/xagent/web/services/quota_hooks.py:43-45 says provider-reported image tokens can take precedence over resolution pricing, but the authoritative token_context.py:874-884 contract says no token-vs-unit pricing rule or discriminator is defined, and aggregation carries no pricing basis. An external hook receiving both provider_tokens and resolution can therefore choose a different price basis than another consumer. Remove the precedence promise until pricing is explicitly modeled, or add a pricing-basis discriminator and align the raw/aggregate contracts and tests. This effectively reintroduces the concern addressed in PR #1422 inline comment 3812115500 and issue comment 5341049838; issue #1461 concerns a different mixed-details union and does not define this precedence.
Root H — The rollback lifecycle branch is unreachable in tests (minor)
tests/web/tracking/test_standalone_usage.py:27-28 hard-codes _FakeSession.in_transaction() to False, while test_compatibility_session_is_disposed at :95-103 checks only that the session closes. Removing the explicit rollback in src/xagent/web/tracking/standalone_usage.py:52-62 would leave these tests green, including the hook-error case. Make transaction state configurable, exercise successful and raising hooks with an active transaction, assert rollback occurs before close, and retain an inactive-transaction case; this is a test-quality gap rather than evidence of a current production leak.
Prior-history disposition
The prior video result-dictionary guard is DROPPED and is not re-reported: review body 4950267724 and inline comment 3795265363 asked for a non-empty-dictionary guard before result.get, but the reviewed BaseVideoModel, Ark, and Xinference contracts return dictionary-shaped results and the base path already performs later result.get accesses. That concern is therefore pre-existing/out-of-contract rather than caused by this PR, and it is distinct from Root B's new asynchronous-duration lifecycle defect. The current Xinference n>1 duration multiplication is fixed and intentionally omitted. No qinxuyi waiver applies.
Testing and review limitations
No tests, linters, formatters, builds, or dependency installs were run; this is a static review against base b2ddb5adba1a3f405301ba3f16277e679ec7b276 and head 2e24aa0fe7e90d287864479e821fe8e8ffcd3105, with the complete current and linked history exports reconciled from their raw IDs. The history-extraction helper failed with usage_limit_reached, so prior status was reconstructed from the raw exports and verifier evidence. The Simplification Lens was also unavailable because of usage_limit_reached; no simplification findings are reported. No qinxuyi waiver applies.
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues:
- Root A —
src/xagent/web/channels/telegram/bot.py:1406, major — Telegram ASR is recorded before any reportable lifecycle and is dropped or misattributed. [new] - Root B —
src/xagent/core/tools/core/video_tool.py:736, major — Ark asynchronous video records a permanent zero-second event without completion reconciliation. [new] - Root C1 —
src/xagent/web/api/model.py:1023, major — ASR omits the unique model ID and can merge duplicate model names. [new]
Minor findings C2, C3, C4, D, E, F, G, and H do not independently block.
| # the shared resolver so a provider exposing no model_name is | ||
| # never billed under a placeholder, and all three ASR entry | ||
| # points agree on the name the aggregator groups by. | ||
| record_asr_usage( |
There was a problem hiding this comment.
record_asr_usage runs while Telegram is preparing the message, before AgentManager.execute_task creates TaskTracker, and no usage_scope or equivalent reporting sink surrounds it. A voice message therefore writes to a throwaway or stale ContextVar; its provider-billed seconds never reach the quota hook and can be misattributed across reused queue work. Please wrap the transcription in usage_scope(owner_user_id) using the authorized owner, or move the tracker boundary, and add an exactly-once hook assertion. The later-PR note in conversation comment 5365144438 has no concrete tracking number and does not cover this path.
There was a problem hiding this comment.
Confirmed and agreed — the recording happens before any reportable lifecycle exists, so those ASR seconds are billed by the provider and metered as nothing, with stale-binding cases able to misattribute.
Deferring this one rather than fixing it here: the fix is a new reporting lifecycle (owner resolution + scope boundary + exactly-once), not a change to the metering call, and it would materially widen a PR whose scope is metering audio/video against the merged primitives.
Tracked with a concrete number as you asked: #1582. It records the mechanism, the owner-resolution point near bot.py:1652, and the behavioural test (hook capture, owner attribution, exactly-once) you specified.
| # first video's duration was recorded. | ||
| per_video_seconds = coerce_duration(result.get("duration")) | ||
| billable_count = max(1, int(n or 1)) | ||
| record_media_seconds( |
There was a problem hiding this comment.
For Ark with wait_for_result=False, the provider returns created-task metadata without a duration, so None reaches this call and becomes a permanent zero-second usage row before the provider task completes. Task completion then reports the undercount with no provider-task key or callback to reconcile the final duration. Please require metered calls to wait for terminal duration, use a provider-guaranteed requested-duration billing rule, or persist and reconcile a pending event keyed by the provider task ID; the fixed Xinference n>1 multiplication is separate.
There was a problem hiding this comment.
Confirmed. Verified against ark.py:354-362: the wait_for_result=False branch returns created-task metadata with no duration key, so coerce_duration yields None and the row is written as a permanent 0-second event with nothing keyed to the provider task to reconcile against.
Deferring rather than fixing here: all three of your options (force a terminal wait, bill the requested duration where the contract guarantees it, or persist a pending event and true it up) add a settlement mechanism, which is a different concern from metering the call. Tracked as #1583, which records the three options and why option 3 is the most faithful but needs a reconciliation store and exactly-once semantics.
The identity half of this call site (C2) is fixed in caebad9 — see the separate thread.
| # on this path alone would split one physical model into two | ||
| # billing rows that can never be reconciled once written. Name is | ||
| # the one identity all three share. | ||
| record_asr_usage( |
There was a problem hiding this comment.
db_model.model_id is available here but is deliberately discarded. Two DB rows can share model_name, so recording only that non-unique name merges ASR usage from distinct configurations and prevents separate pricing/attribution. Please carry a structured (model_name, model_id) through the ASR registry/adapter and record both fields, then add a duplicate-name/different-ID regression test. The earlier #1422 identity discussion (inline 3801868215/reply 3802262493) does not make this endpoint's ID unavailable.
There was a problem hiding this comment.
Not changing this one, and I want to lay out the reasoning rather than just decline.
Leaving model_id unset here is deliberate, and the rationale is in the eight-line comment directly above the call at model.py:1015-1022. The aggregator groups on model_id or model. The other two ASR entry points — audio_tool and the Telegram channel — only ever see name-keyed registries (model_service keys them by model_name), so no DB id is available on those paths. Setting the id on this path alone therefore does not improve attribution: it splits one physical model into two billing rows, one keyed by id and one by name, that can never be reconciled once written. That is a worse failure than the name collision, because it is silent and unrepairable, whereas the collision is visible as an aggregate.
You are right that this endpoint has the id and that duplicate names collide — that part of the finding is accurate. The disagreement is only about whether fixing it at one of three call sites is an improvement. It is not; it needs the structured (name, id) pair carried through the ASR registry and adapter so all three entry points agree, which is exactly the resolver deferred in #1422 (inline 3801868215, reply 3802262493).
Happy to open a tracking issue for the structured identity work, or to do it in a follow-up PR if you would rather see it soon. If you still want the id recorded here despite the split, say so and I will make the change — but I did not want to introduce an unreconcilable billing split silently.
| """ | ||
| from ...core.model.chat.token_context import get_token_usage | ||
|
|
||
| caller_usage = get_token_usage() |
There was a problem hiding this comment.
copy_context() creates a fresh ContextVar binding, not a fresh TokenUsage; every reuse of this wrapper mutates the same caller_usage. TokenUsage explicitly is not thread-safe, so concurrent LLM/tool/media updates or report snapshots can lose counters or observe partial details. Please synchronize mutation, snapshots, merge, and report, or create per-invocation child usage and merge it under a lock; add repeated concurrent-submission and report-ordering tests.
There was a problem hiding this comment.
Accurate on the mechanics — copy_context() isolates the binding, not the object, and TokenUsage documents itself as not thread-safe (locking it is tracked in #1526).
Not changing it in this PR. As you note, there is no production caller: the only users today are this file's own tests, so there is no current concurrency to lose counters under. Adding synchronisation or a per-invocation merge here would mean designing a merge contract for a class whose thread-safety story is already tracked separately, on an API with no consumers yet — the right time to settle that is when the first real caller lands (the KB ingest path in the embedding/rerank PR is the likely one), so the locking scope can be chosen against actual usage rather than guessed.
The docstring already states the wrap-time capture requirement. If you would rather the unused bridge not ship at all until it has a caller, I am happy to drop bind_usage_to_thread from this PR and reintroduce it with its consumer — that would remove the API trap entirely and is arguably cleaner. Let me know which you prefer.
|
Thanks for the thorough pass — several of these were real, and two of them (E and H) were defects I introduced when recovering the lost test file, which is exactly the kind of thing a source-text assertion hides. Pushed Fixed in this PR
Every fix was mutation-checked — reverting the code makes the new test fail. Worth stating because for C3/C4/H the old tests passed against the broken behaviour, so "green" was not evidence of anything. Deferred, with tracking numbers
You said a deferral without a concrete number is not a waiver, which is fair, so both issues carry the mechanism, the specific call sites and the tests you asked for. Both need a new reporting/settlement mechanism rather than a change to the metering call, which is why they are not in a PR scoped to metering audio/video against the merged primitives (#1527). Disagreed / awaiting your call
I have resolved the eight threads that are done and left A, B, C1 and D open since they need your call. |
caebad9 to
f8be80a
Compare
Carries the provider name and the configured id in the fields they belong in, drops a pricing promise the authoritative contract does not make, and closes the test gaps that let those defects stay green. Identity fields (`model` = provider name, `model_id` = configured id): - music/sound_effect passed the configured id as resolve_billing_model's first argument, which returns it unchanged, so the same id was written into both fields and the provider name was lost. Pass None instead so the resolver falls through to the provider's model_name, with the class name as the last resort rather than the placeholder "default". - video wrote the configured id into `model` and left `model_id` empty. Populate both; the aggregator groups on `model_id or model`, so row identity is unchanged while the name is no longer discarded. Pricing contract: quota_hooks claimed provider-reported image tokens can take precedence over resolution pricing, but add_media_usage explicitly declines to define that rule -- there is no discriminator in the row schema to express it, and the aggregate groups purely by (model, unit, call_type, resolution). Removed the promise and pointed at the authoritative contract and xorbitsai#1461. Tests: - The transcribe wiring guard only asserted that the literal "usage_scope(" appeared in the endpoint source, which would pass with the wrong user, the wrong scope boundary, or no recording at all. Replaced with a behavioural test that drives the endpoint and asserts what reaches the quota hook: owner, unit, quantity, identity, exactly once. - _FakeSession.in_transaction() was hard-coded False, leaving _report's rollback branch unreachable -- deleting the rollback kept the suite green. Made the transaction state configurable and added cases for the success path, the raising-hook path and the inactive case, asserting rollback happens before close. - resolve_asr_seconds had two covered branches out of several. Added tests for each provider total field, the fallback to segment ends, unsorted segments, unusable totals (non-finite, negative, boolean, non-numeric) and the no-timing case. - Added distinct-name/distinct-id regression tests for the music and sound-effect identity fix. Telegram ASR lifecycle (xorbitsai#1582) and async video reconciliation (xorbitsai#1583) are tracked separately; both need a new reporting/settlement mechanism rather than a change to the metering call.
f8be80a to
62c4ca3
Compare
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
PR Summary
This is the third of five PRs implementing issue #997's non-LLM media usage metering. It wires ASR, TTS, music, sound-effect, and video generation into the shared metering primitives introduced in PR #1422 (billing by transcribed seconds, input characters, or duration as appropriate), and adds web/tracking/standalone_usage.py to bind and report a TokenUsage for work done outside a tracked agent task (used by /speech/transcribe). It also claims to fix three issues raised in earlier #997 rounds: missing verbose=True causing 0-second ASR billing, fragmented model-identity fields breaking usage aggregation, and validation-before-recording ordering that let successfully-billed provider calls go unmetered.
Blocking: yes — recommended event: REQUEST_CHANGES
Approach Verdict
Acceptable-with-reservations. PR #1422's underlying metering primitives are sound, and this PR is mostly straightforward call-site wiring rather than new architecture. However, two design-level concerns run across the whole diff rather than any single line:
- Inconsistent billing-order policy within one PR. The PR's own stated invariant — and the new test module's docstring — is "usage is recorded as soon as the provider call returns, before the response is validated." TTS follows this (records before unpacking the response). ASR does not: it records only after
_aggregate_segmentssucceeds, so a provider call that succeeded and was billed can still throw during post-processing and end up unrecorded (see M1 below). A PR whose stated purpose is fixing this exact bug class should apply its own policy uniformly across all the call sites it touches. - Several of the bug classes this PR claims to fix are only partially fixed. The "validation-before-recording" bug is fixed for music/sound-effect/video's top-level success path but reappears inside ASR's post-processing (M1). The "fragmented model identity" fix is applied at 4 of 5 new call sites but omitted at the Telegram one (M2), and even in a shipped call site (video) the placeholder filter isn't applied to a last-resort fallback (N1). This pattern is worth calling out explicitly rather than treating each occurrence as an isolated minor bug.
Findings
Major
M1 — ASR usage not recorded when post-processing throws, despite the provider call already having succeeded and been billed
src/xagent/core/tools/core/audio_tool.py:787,798,881
Usage is recorded (line 798) only after _aggregate_segments (line 787) returns successfully. That function can raise ValueError ("Segment start or end time is missing or null") or TypeError/ValueError from unguarded float()/indexing on malformed segment data. The broad except Exception at line 881 swallows any of these and returns {"success": False} without ever recording usage — even though the provider call already succeeded and was billed by the provider. This is exactly the "provider charged, we didn't meter" bug class this PR claims to fix for music/sound-effect, still present in ASR's own post-processing step. It also contradicts the code's own comment at this site (claiming the opposite policy), the new test module's docstring ("usage is recorded as soon as the provider call returns, before the response is validated"), and TTS's actual behavior in the same file (records before unpacking, ~line 983).
Suggested fix: compute audio_seconds from the raw provider result and call record_asr_seconds immediately after that, before _aggregate_segments runs.
M2 — Telegram ASR billing call omits fallback=, can bill under the forbidden placeholder identity
src/xagent/web/channels/telegram/bot.py:1408
resolve_billing_model(None, asr_model) has no fallback= argument, so it falls back to the shared helper's default fallback="default" — the exact placeholder string the module's own documented invariant forbids billing under. All four other new call sites added in this PR (audio_tool.py, music_tool.py, sound_effect_tool.py, video_tool.py) pass an explicit non-forbidden fallback; Telegram is the sole omission. The adjacent comment even claims "a provider exposing no model_name is never billed under a placeholder" — false for this call site specifically. Triggers when the configured ASR model's own DB name is itself a placeholder-like string.
Suggested fix: pass an explicit fallback= matching the pattern used at the other 4 call sites.
M3 — Zero test coverage for video_tool.py's billing logic
tests/core/tools/core/test_media_tool_billing_policy.py
No test in this file exercises video_tool.py's billing logic: not the duration * n batch multiplication, not the wait_for_result=False zero-duration path, and not video's model-identity handling — including the identity fix that was actually shipped for video in this same PR. tests/core/tools/core/test_video_tool_core.py exists but has no assertions on record_media_seconds, coerce_duration, or resolve_billing_model. Given the PR's stated purpose is metering correctness, the complete absence of video billing tests is a real coverage gap.
Suggested fix: add a video-specific test class here mirroring the existing music/sound-effect coverage.
Minor
N1 — video_tool's last-resort "default" model_id fallback bypasses the placeholder filter
src/xagent/core/tools/core/video_tool.py:262
_model_id_for_model can return the literal "default" when a video model's DB model_id is empty (a data-integrity edge case), and this value is passed raw as model_id= without going through resolve_billing_model's placeholder filter — unlike music_tool.py/sound_effect_tool.py, which pass model_id=configured_model_id or "" so an empty string falls through correctly. Low likelihood, but inconsistent with the PR's own invariant.
N2 — Music/sound-effect "reported duration" is actually an echo of the request, not the API response
src/xagent/core/model/music/elevenlabs.py:206, src/xagent/core/model/sound_effect/elevenlabs.py:218
raw_response["music_length_seconds"] / ["duration_seconds"] echo back the request parameter, not the actual API response (both endpoints are consumed as byte-stream iterators with no structured duration field available). For the common "auto-length" case, music_tool.py's coerce_duration(reported) or coerce_duration(requested) fallback yields a 0-second billed row despite a real, billed generation happening — same bug class as the already-tracked async-video issue #1583, for a different modality.
N3 — bind_usage_to_thread has zero production callers
src/xagent/web/tracking/standalone_usage.py:106
Every thread-hop in this PR's diff uses asyncio.to_thread, which per its own docstring doesn't need this wrapper. Already raised in review (comment r3828425649); the author offered to drop it and reintroduce with its first consumer, and that thread has no reply yet.
N4 — _report's session-lifecycle handling duplicates task_tracker.py's existing helper
src/xagent/web/tracking/standalone_usage.py:36
Near-verbatim duplicate of _record_usage_on_event_loop in web/services/task_tracker.py:223-244, including the existing _new_short_session helper (task_tracker.py:114-117) that could be reused directly. The new has_usage_record_hook() short-circuit isn't applied to the older sibling path.
N5 — Stale comment about model_id comparison
src/xagent/core/tools/core/audio_tool.py:804
Says the value here "would still not match the real DB model_id that /speech/transcribe records" — but /speech/transcribe also deliberately leaves model_id unset, so the comparison doesn't hold. Underlying design is fine; comment wording needs a fix.
N6 — standalone_usage._report adds a fresh synchronous DB checkout on the event loop
Mirrors the same pattern already used by task_tracker.py's _record_usage_on_event_loop (not a regression), but is a fresh, higher-frequency, latency-visible instance of a previously-flagged tradeoff. Worth a follow-up to confirm the app-layer hook's latency budget under live HTTP request load — not blocking.
N7 — Dead test scaffolding
tests/core/tools/core/test_media_tool_billing_policy.py:100
_RecordingASR's verbose_seen field is never instantiated anywhere in this test file — looks like leftover scaffolding from an intended-but-never-written assertion that verbose=True reaches the ASR call site.
Previously Raised, Deferred with Tracking Issues (not blocking this review)
- Telegram voice transcription (
bot.py~line 1406) records usage into an unboundTokenUsagethat nothing reports. Raised in review, confirmed by the author, tracked as issue #1582, deliberately deferred. - Async video generation (
wait_for_result=False,video_tool.py~lines 725-753) always bills 0 seconds with no completion-time reconciliation, and no fallback to requested duration. Raised in review, confirmed by the author, tracked as issue #1583, deliberately deferred.
Both remain open in the current diff — noted for completeness, not as new findings.
Simplification Opportunities
src/xagent/core/tools/core/music_tool.py:153-179/src/xagent/core/tools/core/sound_effect_tool.py:166-187:shrink:Near-identical ~35-line blocks (extract raw_response duration key ->coerce_duration(reported) or coerce_duration(requested)->record_media_seconds(..., resolve_billing_model(...))), including duplicated comments. A shared helper would collapse both safely.src/xagent/web/tracking/standalone_usage.py:106:yagni:bind_usage_to_threadhas zero callers anywhere in this PR or the repo. Drop it and its dedicated test now; reintroduce with the first real consumer (see N3).
net: -50 to -60 lines from collapsing the music/sound_effect duplication, plus ~140 lines if bind_usage_to_thread and its test are removed now.
Blocking Status & Recommended Decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues:
src/xagent/core/tools/core/audio_tool.py:787-798— Major — ASR usage unrecorded when_aggregate_segmentsthrows after a billed provider call[new]src/xagent/web/channels/telegram/bot.py:1408— Major — ASR billing call missingfallback=, can bill under forbidden placeholder identity[new]tests/core/tools/core/test_media_tool_billing_policy.py— Major — zero billing-logic test coverage for video_tool.py[new]
For context, not blocking:
- Telegram unbound
TokenUsagefor voice transcription[prior, tracked #1582, not blocking] - Async video generation always bills 0 seconds, no reconciliation
[prior, tracked #1583, not blocking]
| # Recorded only after every step that can still fail, so a call that | ||
| # errors out is not billed. Routed through the shared ASR recorder | ||
| # so every entry point meters identically. | ||
| record_asr_seconds( |
There was a problem hiding this comment.
Major (M1): record_asr_seconds fires only after _aggregate_segments (line 787) returns successfully. _aggregate_segments can raise ValueError/TypeError on malformed segment data, and the broad except Exception around this block (line 881) swallows it and returns {"success": False} without ever recording usage — even though the provider call already succeeded and was billed. This contradicts the comment here claiming the opposite policy, the new test module's docstring ("usage is recorded as soon as the provider call returns, before the response is validated"), and TTS's own behavior in this file (records before unpacking). Please move record_asr_seconds to fire right after audio_seconds is computed from the raw provider result, before _aggregate_segments runs.
There was a problem hiding this comment.
Fixed in 27a91e1. Confirmed, and I reproduced it end to end before changing anything:
provider returns duration=42.0 (billed)
-> _aggregate_segments raises ValueError (segment missing end)
-> broad handler returns success:False
-> media rows recorded: 0
Provider charged for 42 seconds, we metered nothing. Exactly the bug class this metering exists to remove, reappearing inside the PR that removes it — and the comment at the site asserted the opposite policy while TTS a few hundred lines down already recorded before unpacking.
record_asr_seconds now runs immediately after resolve_asr_seconds, before _aggregate_segments and the rest of the post-processing. Same scenario after the fix records one 42-second row while the tool still reports success: False, which is the intended decoupling: metering follows what the provider did, not whether our post-processing liked the result.
Added test_asr_bills_when_post_processing_raises, mutation-checked — moving the record back past _aggregate_segments turns it red. This also corrects the stale model_id comment you flagged as N5.
| # points agree on the name the aggregator groups by. | ||
| record_asr_usage( | ||
| result, | ||
| model_name=resolve_billing_model(None, asr_model), |
There was a problem hiding this comment.
Major (M2): resolve_billing_model(None, asr_model) has no fallback=, so it defaults to fallback="default" — the placeholder string the module's own invariant forbids billing under (see media_usage.py, "Never bill a placeholder identity"). All 4 other new call sites in this PR pass an explicit fallback; this is the sole omission. Please pass an explicit fallback= matching the pattern used elsewhere, e.g. resolve_billing_model(None, asr_model, fallback=configured_model_id or type(asr_model).__name__).
There was a problem hiding this comment.
Fixed in 27a91e1 — and your finding had a third instance I found while auditing for it.
Telegram now passes fallback=type(asr_model).__name__, matching the other call sites, and the comment no longer claims a guarantee the code did not provide.
The instance you did not flag: /speech/transcribe at api/model.py was doing str(db_model.model_name) with no resolver at all. model_name is String(100), nullable=False — not null, but nothing constrains it to be meaningful, so an empty or literally "default" name went straight into the billing row. That is the same defect one layer over, so it is now routed through resolve_billing_model too, with the configured id as fallback.
Verified end to end through the real endpoint:
db_model.model_name |
billed identity |
|---|---|
whisper-large |
whisper-large |
default |
configured-asr-id |
'' |
configured-asr-id |
All five resolve_billing_model call sites in the repo now pass an explicit fallback; I checked rather than assumed.
|
|
||
| assert len(entries) == 1 | ||
| assert entries[0]["model"] == "configured-sfx-id" | ||
| assert entries[0]["model"] != "_UnnamedSoundEffectModel" |
There was a problem hiding this comment.
Major (M3): This file has no coverage for video_tool.py's billing logic — no test exercises the duration * n batch multiplication, the wait_for_result=False zero-duration path, or video's model-identity handling (including the identity fix shipped for video in this PR). test_video_tool_core.py also has no assertions on record_media_seconds/coerce_duration/resolve_billing_model. Given this PR's stated purpose is metering correctness, please add a video-specific billing test class here.
There was a problem hiding this comment.
Fixed in 27a91e1. You were right that video had no billing coverage at all — including the identity fix I shipped for it earlier in this same PR, which was landing untested.
Added three tests mirroring the music/sound-effect structure:
test_video_bills_duration_times_count— theduration * nmultiplication (provider reports one duration, generates and bills for n)test_video_records_provider_name_and_configured_id_separately— the name/id splittest_video_without_duration_is_recorded_as_unmeasured_seconds— the async path records seconds with quantity 0 rather than switching units; reconciling that row is Async video generation records a permanent zero-second usage row with no reconciliation #1583
| model=resolve_billing_model( | ||
| None, video_model, fallback=str(actual_model_id) | ||
| ), | ||
| model_id=str(actual_model_id), |
There was a problem hiding this comment.
Minor (N1): _model_id_for_model (defined earlier in this file, unchanged by this PR) can return the literal "default" as a last-resort fallback when a video model's DB model_id is empty, and this value is passed raw as model_id= to the billing call without going through resolve_billing_model's placeholder filter — unlike music_tool.py/sound_effect_tool.py, which pass model_id=configured_model_id or "" so it falls through to the filtered path. Consider defaulting to "" like the sibling tools.
There was a problem hiding this comment.
Fixed in 27a91e1. Confirmed: _model_id_for_model bottoms out at the literal "default" when a model exposes neither an id nor a name, and that string went straight into model_id without the placeholder filter every other site applies.
Now dropped to "" instead, matching music_tool/sound_effect_tool's configured_id or "". Since the aggregator groups on model_id or model, an empty id falls through to the resolved provider name rather than inventing a phantom model that every unidentifiable provider would share.
| _report(user_id, usage) | ||
|
|
||
|
|
||
| def bind_usage_to_thread(fn: Callable[..., T]) -> Callable[..., T]: |
There was a problem hiding this comment.
Minor (N3): bind_usage_to_thread has zero production callers in this PR or the wider repo — every thread-hop in this diff uses asyncio.to_thread, which per its own docstring doesn't need this wrapper. This was already raised (comment r3828425649) and you offered to drop it and reintroduce with the first consumer; taking you up on that — please drop it and its dedicated test for now.
| T = TypeVar("T") | ||
|
|
||
|
|
||
| def _report(user_id: Optional[int], usage: TokenUsage) -> None: |
There was a problem hiding this comment.
Minor (N4): This session-lifecycle handling (short-session creation, rollback-if-in-transaction, close) is near-verbatim duplicate of _record_usage_on_event_loop in web/services/task_tracker.py:223-244, including the existing _new_short_session helper (task_tracker.py:114-117) that could be reused directly. Also, the has_usage_record_hook() short-circuit here isn't applied to the older sibling path, so the two implementations will drift over time. Consider extracting a shared helper.
| # name-keyed registries (model_service keys _asr_models by | ||
| # model_name), so the only identity in scope here is a name — | ||
| # writing it into model_id would persist a name under an id | ||
| # field and still not match the real DB model_id that |
There was a problem hiding this comment.
Minor (N5): This comment says the value here "would still not match the real DB model_id that /speech/transcribe records" — but /speech/transcribe also deliberately leaves model_id unset (see web/api/model.py), so the comparison doesn't hold. The design itself (leaving model_id unset for name-keyed ASR/TTS registries) is fine; just the reasoning in the comment needs a wording fix.
| assert entries[0]["call_type"] == "sound_effect" | ||
|
|
||
|
|
||
| class _RecordingASR: |
There was a problem hiding this comment.
Minor (N7): _RecordingASR's verbose_seen field is never instantiated anywhere in this test file or the suite — looks like dead scaffolding from an intended-but-never-written assertion that verbose=True reaches the ASR call site. Either wire up that assertion or drop the unused field.
There was a problem hiding this comment.
Fixed in 27a91e1, though not by deleting it.
You are right that _RecordingASR was never instantiated. But the assertion it was scaffolding for is worth having: without verbose=True the provider returns a bare string with no timings, so every ASR call meters as an unbillable 0 seconds. The flag reaching the provider is what makes ASR billable at all — that is the defect the verbose=True additions in this PR exist to fix, and nothing was checking it stayed fixed.
So I wrote the missing assertion instead: test_telegram_passes_verbose_and_bills_a_real_duration now asserts verbose_seen is True and that the resulting row carries a real duration under a non-placeholder identity. Happy to delete the class instead if you would rather keep the test module narrower.
The stated invariant -- record as soon as the provider call returns, before the response is validated -- held for TTS, music, sound effect and video but not for ASR, and the identity rules were applied at four of five call sites. Both gaps are the bug class this metering exists to remove, reappearing inside the PR that removes it. ASR recording order (M1): transcribe_audio resolved the duration and then recorded it only after _aggregate_segments had run. That function raises ValueError on a segment missing start/end, and the tool's broad handler turns any raise into success:False -- so a provider call that succeeded and was billed went entirely unmetered. Reproduced end to end: provider returns 42s, post-processing raises, zero rows recorded. The record now happens immediately after the duration is resolved, matching TTS in the same file. This also corrects the comment at the site, which asserted the opposite policy, and the stale model_id note flagged separately (N5). Telegram identity fallback (M2): resolve_billing_model(None, asr_model) fell through to the helper's own default of "default" -- the exact placeholder the module invariants forbid as a billing identity, while the adjacent comment claimed the opposite. Now passes an explicit fallback like the other four call sites. Video billing coverage (M3): no test exercised video's metering at all. Added the duration * n multiplication, the provider-name/configured-id split shipped earlier in this PR, and the async no-duration path that records seconds with quantity 0. Video placeholder id (N1): _model_id_for_model bottoms out at the literal "default" when a model exposes neither id nor name, and that was written straight into model_id. Dropped to "" instead, matching music/sound_effect, so the aggregator falls through to the resolved name rather than inventing a phantom model shared by every unidentifiable provider. Dead scaffolding (N7): _RecordingASR captured a verbose flag no test ever asserted on. Completed the intended assertion rather than deleting it -- verbose=True reaching the provider is what makes ASR billable at all, since without it the provider returns a bare string and every call meters as an unbillable 0 seconds. bind_usage_to_thread (N3): removed along with its tests. It had no callers anywhere in the repo, every thread hop in this diff uses asyncio.to_thread which copies the context already, and shipping an unused concurrency helper built on a documented non-thread-safe object is a trap. It can return with its first real consumer, where the locking contract can be settled against actual usage. Every fix is mutation-checked: reverting the ASR record past _aggregate_segments, or the video/music fallbacks, turns the new tests red.
a986445 to
27a91e1
Compare
3 participants
Third of five changes splitting #997.
Wires the audio and video modalities into the primitives from #1422: ASR
billed by transcribed seconds, TTS by input characters, and music, sound
effects and video by duration. Video shares the duration-billed path
because it shares the same unit invariants, not to pad the change.
Also lands
web/tracking/standalone_usage.py, because/speech/transcribeneeds it here. It binds aTokenUsagefor work thatis not a tracked agent task and reports it to the quota hook on exit;
without it that endpoint records into a throwaway object nothing reads —
the provider bills and the usage evaporates.
bind_usage_to_threadrunseach call inside
copy_context()+ctx.runso a binding cannot outlivethe job on a pooled executor thread, and
_reportcheckshas_usage_record_hook()before checking out a DB session, since with nohook installed (the stock configuration) the record call is a guaranteed
no-op and the pool checkout is pure overhead.
Review feedback addressed from #997
/speech/transcribeand the Telegram voice handler both calledtranscribe()withoutverbose=True, so the provider returned a barestring with no timings and every usage record was an unbillable
0-second entry. Both now pass it; each already read the text via
getattr(result, "text", result), so nothing else changes.points wrote three different identities for one physical model:
audio_tool wrote a name into
model_id(its registries are keyed bymodel_name, sox in self._asr_modelswas a name test),/speech/transcribewrote the real DB id, and Telegram read a bare.modelattribute with no placeholder filter. The aggregator groups onmodel_id or model, so one model produced up to three rows that couldnever be reconciled once written. Only the name is available on all
three paths — audio_tool never sees a DB id — so
model_idis now leftunset everywhere and Telegram goes through the shared resolver. The
same name-into-
model_idmistake is fixed on both TTS paths.sound effects raised on an empty or malformed result before
recording, so a call that succeeded at the HTTP level but returned
nothing went unmetered even though the provider charged for it.
Recording now precedes validation in both, matching TTS. The four
otherwise-symmetric tools had three different implicit rules; the
policy is now stated in a comment at each site, as requested.
record_asr_secondsis nowa thin wrapper over the shared
record_media_seconds, and audio_tool'sprivate
_resolve_billing_modelcopy delegates to the sharedresolve_billing_modelrather than reimplementing it without theplaceholder filter. That copy billed the Xinference default ASR/TTS
model — no
model_name, and its default-getter builds a separateinstance so identity matching never hit — under the literal string
"default". It now falls back to the provider class name, and a testcovers that previously-uncovered default-model path.
The Telegram test fakes accepted no
verboseargument at all, unlikeevery real ASR provider, so they were widened to match the real signature
and now assert
verbose=Trueis passed. A test double narrower than theinterface it stands in for is what let this ship unnoticed.
Testing
CI. Local runs are not usable in my environment — importing
xagent.core.model.chathangs on this machine, onmainas well as onthese branches — so I am relying on CI rather than claiming a local pass
I did not get.