feat: meter image generation and editing - #1424
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 thread-safe media-usage tracking framework for non-LLM models (such as image generation, video, TTS, ASR, embedding, and reranking) to meter their usage alongside LLM tokens. The review feedback highlights several issues, including missing image_count parameters in DashScope and Gemini models that would lead to under-billing multi-image requests, a key mismatch in Gemini's token usage extraction, a potential ValueError when validating None call types, and a file descriptor leak in the OpenAI image editor.
|
Follow-up on the The four Gemini — reverted to a fixed count of 1. The Gemini API has no DashScope — kept OpenAI already had One suggestion I did not apply: the comment on Both behaviours are now pinned by tests: the Gemini case asserts |
c5e325d to
6e83f0d
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.
6e83f0d to
dff76f2
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.
dff76f2 to
65ffd6c
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.
65ffd6c to
e423ba7
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.
e423ba7 to
1b03632
Compare
Wire the image providers into the media usage primitives so image generation and editing are billed alongside LLM tokens. `record_image_usage` normalises each provider's usage payload into a single `add_media_usage` call, billed in `MediaUnit.IMAGES` with the request's `n` as the quantity so multi-image requests are not under-billed as one. The resolution tier is recorded so billing can price by (model, resolution); providers that also report real tokens (Gemini, OpenAI gpt-image) pass them through so a token-based price can take precedence. Gemini and DashScope now record usage immediately after the usage payload is parsed, before the response body is validated. Both providers raise structural RuntimeErrors on a 200 the provider has already billed — a safety-blocked finish reason or a missing image field — and recording after those checks dropped usage for exactly those responses. Gemini's retry_on matches only 429/5xx, so they are never retried either. Applies to both generate_image and edit_image on both providers. The Xinference inpainting branch now forwards n and size. It previously dropped both while usage was still recorded as n, over-billing whenever n>1. Unreachable today because **kwargs is stripped from the exposed tool schema, but it would have gone live silently the moment that changed.
The previous commit applied `image_count=kwargs.get("n", 1)` uniformly
to Gemini and DashScope, but the two clients treat `n` differently, so
one of them was over-billing.
Gemini reverts to a fixed count of 1. The Gemini API has no multi-image
parameter, and this client forwards only `temperature` out of `**kwargs`
into `generationConfig` — a caller-supplied `n` is silently dropped. The
response parser also stops at the first `inlineData` part. Billing `n`
there charged for images the provider never generated.
DashScope keeps `n`. It spreads `**kwargs` into the request
`parameters`, so `n` does reach the provider, which generates and bills
for that many images; the recorded count has to match the invoice.
Images 2..n are then discarded by the parser, which reads only
`content[0]` — real, but a separate defect from metering, and noted in a
comment rather than silently under-billed around.
Both behaviours are now pinned by tests: the Gemini case asserts `n`
never reaches the request body and that the billed quantity stays 1, and
the DashScope case asserts `n` does reach `parameters` and that the
billed quantity follows it.
Two tests asserted that Gemini bills the caller's `n`. It cannot: both `generate_image` and `edit_image` build `generationConfig` without any multi-image parameter — `generate_image` forwards only `temperature` out of `**kwargs` — so `n` is dropped before the request goes out and the parser returns a single image. Billing it charges for images the provider was never asked to produce. `test_gemini_edit_forwards_image_count` is replaced by `test_gemini_edit_does_not_bill_unsupported_n`, which asserts `n` is absent from the outgoing request body and that `image_count` is therefore not passed at all. The safety-blocked case now expects `quantity == 1` rather than 3, for the same reason. The DashScope tests are unchanged and still expect the caller's `n`: that client spreads `**kwargs` into the request `parameters`, so `n` does reach the provider and does get billed by it.
1b03632 to
13226e0
Compare
|
Rebased onto This PR previously carried the media billing primitives itself — One migration was needed, and it would have failed silently. Verified rather than assumed:
The API migration is squashed into the commit that introduced the call rather than left as a fixup on top, so each commit stands on its own. #1425, #1457 and #1463 still carry the same superseded primitives and need the same treatment; they are stacked, so each one's rebase should follow the one before it. |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
The current six-file diff adds record_image_usage and wires both generate/edit paths in Gemini, DashScope, OpenAI, and Xinference into the existing TokenUsage.details media-accounting flow, with focused helper/provider tests. It preserves provider token metadata, effective image counts, and resolution, and deliberately records Gemini/DashScope billed HTTP-200 attempts before result validation while retaining the existing OpenAI/Xinference cleanup and success boundaries. The direction is appropriate, but review-only verification found four new major billing-correctness issues and two minor contract/coverage gaps that should be addressed before merge.
Blocking: yes — recommended event: REQUEST_CHANGES
Independent design verdict
Verdict: acceptable-with-reservations. The macro approach is sound: provider-level boundaries are the only places that simultaneously know what was requested, what the provider billed, the effective resolution/count, and provider-reported tokens. Routing those attempts into the already-persisted TokenUsage.details list reuses TaskTracker persistence, current-turn quota deltas, and aggregation without creating a second sink, schema, or transaction path. The before/after validation placement is also conceptually coherent for the different provider contracts.
The reservations are at the billing-contract boundaries rather than at the overall direction: configured model identity is stamped on an outer retry wrapper but is not available to the inner provider that records the row, the default retry policy can repeat billed invalid responses, and the image helper documents a pricing-precedence rule that the shared primitive does not implement. Those reservations are captured in the line-level findings below.
Prior review roots
All prior source occurrences were checked against the current head and the exact base. The following are the only prior roots; none is re-reported as a new finding.
| Root | Prior source IDs | Status and verification |
|---|---|---|
P1 — DashScope image_count omitted |
3795222306, 3795222322, review body 4950216871; author reply 5324965634 | FIXED. Generate and edit now pass kwargs.get("n", 1), the unchanged kwargs reach DashScope parameters, and the current tests assert the request and recorded quantity. The reply is technically correct. |
P2 — Gemini image_count allegedly omitted |
3795222332 (image-count subissue), 3795222344, review body 4950216871; author reply 5324965634 | DROPPED. Gemini does not forward n into its request and parses at most one output, so quantity 1 is the current provider/client contract; the base had the same behavior. The reply is technically correct. |
| P3 — Gemini token-key mismatch | 3795222332 (completion-token subissue), review body 4950216871; author reply 5324965634 | DROPPED. Gemini maps candidatesTokenCount to completion_tokens before the helper; the current provider-token assertion is 16, and the mapping was already present in base. The reply's technical explanation is correct (its older numeric example is stale, but that does not change the result). |
P4 — None call-type validation |
3795222348, review body 4950216871; rebase reply 5364782405 | DROPPED / REFACTORED IN BASE / OUT OF SCOPE. The reported None -> "None" behavior belonged to the pre-rebase snapshot. Base d74660c2 contains the replacement #1527 contract, which requires a valid call_type, and token_context.py is unchanged by this PR. The rebase explanation is corroborated by the current six-file diff. |
| P5 — OpenAI edit file-descriptor leak | 3795222355, review body 4950216871; rebase reply 5364782405 | FIXED. The incremental append loop keeps already-open handles visible to finally, and the current regression test covers a later local-open failure. |
Current confirmed findings
Critical
No Critical findings.
Major
N1 — src/xagent/core/model/image/usage.py:31 — pre-coercion can drop or corrupt media rows
_read eagerly applies int() before the shared media write boundary and catches only TypeError/ValueError. A provider boolean becomes 1 instead of the shared boundary's deliberate sanitized 0; int(float("inf")) raises OverflowError, which reaches the helper's outer best-effort handler before add_media_usage, so the entire billable media row disappears. The same pre-coercion pattern for image_count can turn malformed values into a billable default or drop the row, bypassing the boundary's zero-valued-row invariant.
Suggestion: Select the raw alias value without int()/max() coercion and pass raw token/count values to add_media_usage; use 1 only when n is genuinely absent. Let the shared boundary handle booleans, non-finite values, invalid values, and overflow while retaining the media row, and add True, inf, and overflowing-count regression cases.
N2 — src/xagent/core/model/image/gemini.py:374 — billed invalid-200 responses are retried and metered repeatedly
Gemini records usage before structural validation, then raises RuntimeError for safety-blocked or otherwise unusable responses. The production create_image_model path does not supply a selective retry predicate, while create_retry_wrapper defaults to retrying every exception; provider failures are also broadly surfaced as RuntimeError. Consequently, an invalid but already-billed 200 can trigger multiple provider requests and append multiple media rows, up to the configured retry-attempt count. The direct-provider tests do not exercise this production wrapper path.
Suggestion: Preserve a distinct classification for transient transport/status failures and make invalid-response errors non-retryable in the production wrapper (rather than retrying every RuntimeError); retain the per-attempt accounting semantics for genuinely retried transport failures. Add a create_image_model integration test with a safety-blocked/invalid 200 that asserts one transport attempt and one media row.
N3 — src/xagent/core/model/image/openai.py:158 — configured model IDs never reach provider-level usage records
All eight provider-level recording calls pass model_name but omit model_id. model_service attaches the actual configured database ID to the outer GenericRetryWrapper, whereas the inner provider executes record_image_usage; the aggregator therefore falls back to the non-unique provider-facing name. Two separately configured models that share a name (for example, different endpoints or pricing configurations) collapse into one billing group.
Suggestion: Carry the actual configured database/model ID onto the inner provider for raw, wrapped-default, and create_image_model construction paths, then pass it on every recording call. Add an integration test with two same-name configured models and assert that aggregate_media_usage_by_model produces distinct model-ID groups.
N5 — src/xagent/core/model/image/dashscope.py:219 — authoritative DashScope usage count/dimensions are ignored
The new early-meter path records request n and requested size, but never reads provider-reported successful-image count or dimensions. Documented DashScope partial/task responses can report usage.image_count, width, and height (and supported variants use output_* aliases); for example, a request for two images can return one successful image. The current code records two before the parser raises on the partial output, so local quantity and resolution grouping diverge from the provider's usage data. This is distinct from fixed P1: forwarding request n is correct for fully successful requests, but it is not a substitute for authoritative response usage.
Suggestion: Prefer validated provider-reported successful count and dimensions when present, with request n/size only as a documented fallback when those fields are absent. Apply the same logic to generate and edit, covering mismatched, partial, invalid, and output_* alias responses.
Minor
N7 — tests/core/model/image/test_image_usage_recording.py:207 — provider and wrapper accounting seams remain untested
The current edit-side tests monkeypatch record_image_usage and return valid responses, so they remain green if accounting is removed or moved after validation. There are no provider-level OpenAI/Xinference accounting tests, no real create_image_model retry-attempt test, and no persisted-row test for invalid Gemini/DashScope edit responses. In addition, test_record_image_usage_never_raises_on_garbage passes {} rather than a non-dict, so it does not exercise the claimed non-dict branch.
Suggestion: Add a compact provider matrix through a real TokenContextManager for OpenAI/Xinference generate/edit, add invalid-200 edit cases for Gemini/DashScope, and exercise the production retry wrapper with attempt/row assertions. Change the garbage fixture to a list or other non-dict input and assert the best-effort contract.
N8 — src/xagent/core/model/image/usage.py:53 — pricing-precedence documentation is not an implemented contract
The new helper docstring says that real image tokens take precedence over resolution pricing, and the test comment repeats the claim. The shared media primitive only stores raw provider token fields and resolution; aggregation has no price-basis discriminator or precedence branch, and the in-tree quota path forwards the rows without pricing them. This wording can cause downstream callers to rely on behavior that does not exist; precedence is tracked as separate pricing work.
Suggestion: Rewrite the helper and test wording to say that provider tokens are forwarded as raw metadata and resolution is retained for downstream grouping, with no token-vs-resolution pricing choice enforced here. If precedence is required, add an explicit pricing-layer contract/discriminator and consumer tests rather than an unenforced promise in the image helper.
Simplification Lens
The mechanical Simplification Lens was unavailable: the review-spark run failed with usage_limit_reached. No simplification suggestions are invented or promoted.
Verification note
This is review-only verification from the current source, the exact base commit, the complete PR/reply history exports, and the linked discussion history. Tests, linters, formatters, dependency installation, and runtime reproductions were not run in this review.
Blocking status & recommended decision
The following confirmed major roots block merge:
src/xagent/core/model/image/usage.py:31— major, malformed provider token/count coercion can drop or corrupt billable media rows. [new]src/xagent/core/model/image/gemini.py:374— major, the production retry wrapper can repeat billed invalid-200 requests and media rows. [new]src/xagent/core/model/image/openai.py:158— major, missing configured model IDs collapse distinct billing identities. [new]src/xagent/core/model/image/dashscope.py:219— major, request-derived count/resolution can disagree with authoritative DashScope usage. [new]
Blocking: yes
Recommended event: REQUEST_CHANGES
e5497a3 to
ae04127
Compare
|
All six findings addressed in Every guard is mutation-tested. Reverting each fix turns committed tests red — N1 pre-coercion: 10 red; N2 retry predicate: 14 red; N3 model_id: 8 red (2 more for the composite id); N5 request-only values: 9 red. Deleting the edit-side accounting call in all four providers turns 11 red, and relocating Gemini's generate metering to after validation turns 2 red, which is the seam N7 identified — those previously turned nothing red. N2 was not fully closed by converting the explicit raises. The structural checks validate container shapes but never element types, so a 200 whose Three defects introduced by my own first pass, caught before pushing:
Two scope notes. I did not wire up the existing Local verification: ruff check/format, isort and mypy clean on the touched files; codespell clean repo-wide. |
ae04127 to
a68e1d6
Compare
a68e1d6 to
5989b15
Compare
|
Update: head is now 1. Payload walking ran in the wrong frame. 2. A billed 200 with a malformed usage block was charged, unmetered and unretried at once. The metering call sits after the first reads of the parsed body, so a 200 whose 3. Unbounded provider values reached billing fields. A reported Corrections to figures in my earlier comment: the Now 109 tests in the file, all guards mutation-tested — the four new ones turn 6, 3, 7 and 1 tests red respectively when reverted. CI was green on the previous head ( Worth saying plainly: the two rounds of self-review found more real billing defects than the original review did, and every one of them was in code I had just written to fix the previous round. The tests are the only reason I can claim any of it is closed. |
343b969 to
304f26a
Compare
|
Ready for re-review. Head What was fixed
126 tests, every guard mutation-tested. #1584 was opened for an adjacent gap and then fixed here rather than deferred, so it is closed. Self-review found more than the review didTen further defects surfaced from adversarial passes over my own diff — nine of them introduced by my own fixes. The ones that mattered:
An independent sweep of ~85k calls across the reachable input space then came back clean: no raise, no missing row, no gate/boundary disagreement. Two code-level imprecisions it judged unreachable from all 8 call sites were fixed anyway, since "unreachable today" stops holding after a refactor. Two process notes, because they bear on how much the numbers above are worthOne mutation run reported 0 tests red and I first read it as "this guard is untested". It was my mutation script failing to apply and producing unreachable code; done properly it turns 2 red. A broken mutation script manufactures false confidence in both directions, so I now confirm the mutated source reads as intended before trusting a count. And every defect above was found by executing the code, never by reading it. The static checks were clean on the version that failed a successful image call and recorded nothing. ScopeTwo things deliberately left alone rather than folded in: the existing |
304f26a to
476b401
Compare
|
Correction to my previous comment: I resolved the threads too early. What broke. Closing an unreachable imprecision, I rewrote the alias gate to compare the raw value:
Real under-billing of a real provider count. Every previous iteration of this gate erred by being too loose; this one erred by being too strict, and I was only testing for the first kind. Why the tests missed it, and why that is the more important part. The property test I had added — and pointed to in this thread as the thing that makes enumeration unnecessary — asserted only that everything the gate accepts, the boundary bills. A gate that wrongly rejects a billable value satisfies that trivially. The invariant needed both halves. It is bidirectional now, and the reverse half is exactly what caught this. Fixing it surfaced a second regression from the same edit: tightening the zero case to On the process. The ~85k-call sweep I cited as evidence of convergence explicitly classified this direction as "safe" and did not test it. So "CI green + 126 tests + a large clean sweep" was not sufficient, and I presented it as if it were. Both regressions are now mutation-tested, and I am not resolving the thread again until CI is green on the new head. 135 tests. |
f8264c9 to
3a78b13
Compare
3a78b13 to
e3393df
Compare
Addresses the four blocking review findings on the image metering path.
Raw provider values now reach the shared media write boundary uncoerced.
Pre-coercing in the image helper defeated guards deliberately built into
that boundary: int(True) is 1, so a provider JSON boolean billed a token
the boundary reads as 0, and int(float("inf")) raised OverflowError inside
the helper's own best-effort handler, dropping the entire billable row to
salvage one bad field. The boundary sanitises bool/negative/non-finite and
overflowing values while keeping the row, and a zero-quantity row is its
convention for a billed-but-unmeasured call. Alias order is preserved past
an unusable value, so a payload reporting prompt_tokens "bad" alongside a
valid input_tokens still bills the valid one.
Billed invalid 200s are no longer retried. Providers meter before
structural validation because the charge is real, but create_image_model
passed no retry predicate and create_retry_wrapper defaults to retrying
every exception, so one safety-blocked response produced up to max_retries
provider calls and max_retries billing rows. Those validation failures now
raise InvalidImageResponseError, a RuntimeError subclass so every existing
caller and documented contract is unchanged, and the installed predicate
excludes only that type. Transport failures stay retryable, where
per-attempt accounting is correct.
Converting the explicit raises was not sufficient on its own: the
structural checks validate container shapes but never element types, so a
200 whose content is [null] or whose candidates are strings failed
implicitly with TypeError or AttributeError, reached the blanket handler as
a plain RuntimeError, and was retried and re-billed. Rather than enumerate
every malformed shape a provider might send, a failure while walking an
already-metered body is now classified positionally as an invalid response,
preserving the original error as the cause.
Xinference meters before walking the response body, like the other two
providers. Its client returns response.json() verbatim -- raw server JSON
despite the annotation -- so a malformed but billed 200 raised while being
parsed, before the metering call, inside the try whose handler rewraps into a
plain RuntimeError. The row was lost and the error stayed retryable, so a single
billed call became max_retries charges with nothing recorded: measured 5 provider
calls and 0 rows on both generate and edit. Body-walk failures are now
classified with invalid_response_from, as in gemini and dashscope.
Reading the usage payload there also went through getattr(), which never finds
a dict key, so a dict-shaped response always reported zero provider tokens.
Those tokens are now recorded -- a behaviour change, in the direction of billing
what the provider reported.
The configured model id now reaches the row. model_service stamped it on
the outer retry wrapper while the inner provider does the recording, and
aggregation groups on model_id or model, so two same-name configured
models collapsed into one billing group. Every provider takes a model_id,
both construction paths supply it, and all eight recording calls pass it.
get_image_model_instance uses the row's own model_id instead of a
name-plus-provider composite of two non-unique halves.
DashScope prefers its own reported usage over the request. A partially
successful 200 reports usage.image_count and output dimensions, so a
two-image request returning one image was billed as two and grouped under
the requested resolution. Request n and size remain the documented
fallback, and both branches normalise resolution to WxH so one physical
resolution cannot split into two aggregate keys. A non-string size is
dropped rather than stringified, since edit_image reads it straight from
caller kwargs and an int would become a half-resolution key. Integer counts
skip the float round-trip, which would silently round anything above 2^53.
Also reworded the helper and test comments that claimed token pricing takes
precedence over resolution: tokens are forwarded as raw metadata and
resolution is retained for grouping, with no pricing choice enforced here.
Precedence is tracked in xorbitsai#1461.
Tracking the first unusable alias uses a sentinel rather than comparing the
held value to 0. `fallback == 0` invokes a raw provider value's own __eq__
once one is held, so a payload with two unusable aliases whose first has a
raising __eq__ escaped the helper and dropped the entire row -- the same
failure this change set out to fix, reintroduced by the alias-order fix.
The provider usage payload is walked inside record_image_usage rather than in
the caller's argument list. An argument expression runs in the caller's frame,
outside the helper's swallow, so a payload whose `get` raises took down the
whole image call and recorded nothing -- failing a successful 200 that had
returned a real image. Reported counts and dimensions are also bounded: the
write boundary folds a value too large for a float to quantity 0.0, so
forwarding one billed nothing for a real image, and an unbounded dimension
became a several-hundred-character aggregate key joining no price table.
The alias gate mirrors every reduction the write boundary applies to a token
field, not just the ones that raise. The boundary floors tokens with
max(0, ...), so a negative first alias billed 0 while a usable later alias went
unread -- `{"prompt_tokens": -5, "input_tokens": 9}` billed 0 instead of 9.
Token values are bounded for the same reason a reported count is: a 401-digit
provider integer both shadowed the real alias and landed in the persisted row.
The gate tests int(value) alone, evaluated entirely inside its own guard, and
must be neither looser nor stricter than the boundary. Deriving the decision
from `value` was wrong in both directions: comparing `value` cannot order a
numeric string against an int, and a truncation test rejects 7.9 and
Decimal("7.5") -- the boundary bills all of those, so rejecting them made a
real provider count lose to a later alias. A zero result is usable only when
the provider reported an integral zero, which is a measurement that must stop
the scan, unlike -0.5 and 1e-9 which merely truncate to zero. The gate's
agreement with the boundary is pinned as a bidirectional property; the
single-direction version of it passed while the string case was broken. The gate's agreement with the
boundary is now pinned as a property over a spread of value shapes, since every
disagreement found so far was a different shape.
Each provider-controlled read inside the helper is guarded individually rather
than only by the outer handler, since reaching that handler means no row at all.
A row carrying the request's own values is strictly better than losing the
evidence that a billable call happened.
Both providers now read their usage block defensively, because those reads
happen before the metering call and inside the same try as the structural
checks. A 200 whose usageMetadata is a list, or whose whole body is a JSON
array, previously raised there and was classified as an already-billed invalid
response -- an error that asserts the row exists and instructs the retry policy
not to try again, while no row had been written. Such a call was charged,
unmetered and unretried at once; it now records a zero-token row.
Alias selection is verified by differential test against a reference
implementation written from the contract rather than from the code, over every
pair of 25 provider value shapes. Assertion-shaped tests kept missing real
defects here: a one-directional property passed while numeric strings were being
wrongly rejected, and making it bidirectional only worked because that happened
to be the missing direction. A reference implementation does not depend on
guessing which direction to assert, and it independently fails on both of those
regressions.
The unwrapped construction path in model_service is covered too. Every other
test drives a provider through the retry wrapper, so "no wrapper at all" was an
untested combination: there the InvalidImageResponseError must reach the caller
directly while the billed row is still written, and a provider that only behaved
correctly under the wrapper would have looked fine everywhere else.
All four providers now have that coverage, generate and edit, over well-formed
and malformed response bodies. The gap that let the xinference defect through
was not the technique but its reach: two providers had differential coverage and
two did not, and the untested one was the only one missing both halves of the
fix.
DashScope's reported-vs-request count and resolution are covered the same way,
over the cross-product of reported and request shapes. Both differential tests
were checked by injecting known defects -- removing the fractional-count
rejection, removing the resolution normalisation -- rather than assumed
effective; each injection fails them.
The two count validators deliberately disagree on fractions: a token field
accepts 7.9 because the boundary bills 7, while a reported image count rejects
2.5 because counts are discrete and a malformed one should fall back to the
request's n. Pinned so a later pass cannot quietly unify them.
The wrapped construction path is covered per provider and per method, since
the recording call is duplicated at eight sites and a missed one is invisible
from any single test.
Every guard added here is mutation-tested: reverting it turns at least one
committed test red, including the deletion or post-validation relocation of
any accounting call, which the previous monkeypatched edit tests could not
detect.
e3393df to
7c43638
Compare
3 participants
Second of five changes splitting #997. Replaces #1423, which was built
on #997's July base and silently reverted the
default_image_abilitiesrefactor that has since landed on main. This branch is rebuilt on
current main and keeps that refactor intact.
Wires the image providers into the primitives from #1422 so image
generation and editing are billed alongside LLM tokens.
record_image_usagenormalises each provider's usage payload into oneadd_media_usagecall, billed inMediaUnit.IMAGESwith the request'snas the quantity so multi-image requests are not under-billed as one.Resolution is recorded so billing can price by (model, resolution);
providers that also report real tokens (Gemini, OpenAI gpt-image) pass
them through so a token-based price can take precedence.
Review feedback addressed from #997
DashScope both raise structural
RuntimeErrors on a 200 the providerhas already billed — a safety-blocked finish reason, a missing image
field, an unparseable
output— and recording sat after thosechecks, so exactly those responses went unmetered. Gemini's
retry_onmatches only 429/5xx, so they are never retried either. Recording now
happens immediately after the usage payload is parsed and before any
content validation, in both
generate_imageandedit_imageon bothproviders. OpenAI and Xinference never had this bug — recording there
is already reached after a successful call.
nand
sizebefore calling the provider while usage was still recordedas
n. Both are now forwarded, so the billed count matches what wasrequested. Unreachable today because
**kwargsis stripped from theexposed tool schema, but it would have gone live silently the moment
that changed.
Testing
tests/core/model/image/— 59 cases pass, including main'stest_default_abilities_wiring.py, which guards the refactor thisrebuild preserves. Two new cases in
test_image_usage_recording.pydrive the providers themselves with astubbed transport rather than testing the helper in isolation, since the
ordering bug only exists at the call site. Verified by mutation:
removing the early
record_image_usagefrom Gemini fails thesafety-blocked case with
media_calls == 0.