From e2fb677f3ccbbc45c4616bc9b44ad2860959a26b Mon Sep 17 00:00:00 2001 From: mubashir1osmani Date: Tue, 14 Jul 2026 17:47:47 -0700 Subject: [PATCH] test(e2e): scope the negative-spend guard honestly, drop the #25846 claim The guard was described as covering the bedrock cache-token regression (#25846). It does not. That bug needs prompt_tokens to exclude cache tokens so prompt_tokens - cache_hit falls below zero; this test drives uncached traffic, so cache_hit is 0 and the subtraction cannot go negative. Reverting the clamp leaves the test green, confirmed by mutation: the only way to turn it red was an injected fault unrelated to the bug. State the scope and the limitation in the docstring, mark the cell fail_before_fix=unproven explicitly, and record the live bedrock cache-token path as a still-open gap in the matrix. The test keeps its real value as a cross-provider non-negative invariant sweep. --- .../coverage_registry/quota_management.yaml | 2 +- .../SPEND_TRACKING_COVERAGE_MATRIX.md | 5 +-- .../spend_tracking/test_spend_tracking_e2e.py | 32 +++++++++++-------- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/tests/e2e/coverage_registry/quota_management.yaml b/tests/e2e/coverage_registry/quota_management.yaml index 8dc65a39bba..d20839c3073 100644 --- a/tests/e2e/coverage_registry/quota_management.yaml +++ b/tests/e2e/coverage_registry/quota_management.yaml @@ -33,4 +33,4 @@ - {id: quota_management.spend_tracking.failure.writes_failure_row, module: quota_management, tier: P1, behavior: spend_tracking, variant: failure, assertions: [writes_failure_row], exercised_on: [chat_completions], source: "proxy/spend_tracking/spend_log_error_logger.py", rationale: "A failed call writes a failure-status spend row"} - {id: quota_management.spend_tracking.spend_calculate.returns_cost, module: quota_management, tier: P2, behavior: spend_tracking, variant: spend_calculate, assertions: [returns_cost], exercised_on: [spend_calculate], source: "proxy/spend_tracking/spend_management_endpoints.py", rationale: "/spend/calculate prices a hypothetical request at nonzero cost"} - {id: quota_management.spend_tracking.pagination.keeps_total, module: quota_management, tier: P2, behavior: spend_tracking, variant: pagination, assertions: [keeps_total], exercised_on: [chat_completions], source: "proxy/spend_tracking/spend_management_endpoints.py", rationale: "Spend-logs v2 pagination caps page size without losing the total"} -- {id: quota_management.spend_tracking.non_negative.never_negative, module: quota_management, tier: P1, behavior: spend_tracking, variant: non_negative, assertions: [never_negative], exercised_on: [chat_completions, embeddings], source: "litellm_core_utils/llm_cost_calc/utils.py", rationale: "No provider (gemini/anthropic/openai/bedrock) writes a negative spend row across streaming/non-streaming chat and embeddings; derived cache-token counts must clamp at zero (BerriAI/litellm#25846)"} +- {id: quota_management.spend_tracking.non_negative.never_negative, module: quota_management, tier: P1, behavior: spend_tracking, variant: non_negative, assertions: [never_negative], exercised_on: [chat_completions, embeddings], source: "litellm_core_utils/llm_cost_calc/utils.py", fail_before_fix: unproven, rationale: "No provider (gemini/anthropic/openai/bedrock) writes a negative spend row across streaming/non-streaming chat and embeddings; negative spend credits a budget back, letting a key outspend its cap. Broad invariant sweep on uncached traffic: it does NOT cover the bedrock cache-token path (#25846), which needs cache_hit > prompt_tokens to fire, so it stays unproven until a test drives that path"} diff --git a/tests/e2e/quota_management/spend_tracking/SPEND_TRACKING_COVERAGE_MATRIX.md b/tests/e2e/quota_management/spend_tracking/SPEND_TRACKING_COVERAGE_MATRIX.md index 80f2f096d8f..b02680b11e4 100644 --- a/tests/e2e/quota_management/spend_tracking/SPEND_TRACKING_COVERAGE_MATRIX.md +++ b/tests/e2e/quota_management/spend_tracking/SPEND_TRACKING_COVERAGE_MATRIX.md @@ -29,7 +29,8 @@ proxy + SpendLogs rows. Status: `covered` / `partial` / `gap`. | Modality | Existing | Status | Live e2e | |----------|----------|--------|----------| -| Non-negative spend (all providers) | `test_anthropic_claude3_transformation.py`, `llm_cost_calc/utils.py` (#25846) | partial | yes (`test_no_provider_logs_negative_spend`) | +| Non-negative spend (all providers, uncached traffic) | `test_anthropic_claude3_transformation.py`, `llm_cost_calc/utils.py` | partial | yes (`test_no_provider_logs_negative_spend`) | +| Negative cost from bedrock cache-token split (#25846) | `test_anthropic_claude3_transformation.py` (mock) | gap (live) | no - needs bedrock streaming + cached prefix; the sweep above does not reach it | | Chat (non-stream) | `test_cost_calculator.py`, `local_testing/test_completion_cost.py` | covered | yes (`test_chat_completion_writes_nonzero_spend_row`) | | Chat (streaming) | `test_streaming_interrupt_spend_tracking.py` | partial | yes (`test_streaming_chat_completion_tracks_spend`) | | Embedding | `test_cost_calculator.py` (#29956) | partial | yes (`test_embedding_writes_nonzero_spend_row`) | @@ -69,7 +70,7 @@ proxy + SpendLogs rows. Status: `covered` / `partial` / `gap`. | `test_tag_spend_matches_sum_of_tagged_logs` | `/spend/tags` SUM/COUNT == tagged rows | | `test_end_user_spend_attributed_on_row` | `end_user` attributed + costed | | `test_each_model_on_a_shared_key_gets_its_own_row` | per-model/provider rows, correct model + cost, distinct request_ids matching response id | -| `test_no_provider_logs_negative_spend` | no provider (gemini/anthropic/openai/bedrock) writes spend < 0 across streaming/non-streaming chat + embeddings; each provider still logs a positive row (non-vacuous) | +| `test_no_provider_logs_negative_spend` | no provider (gemini/anthropic/openai/bedrock) writes spend < 0 across streaming/non-streaming chat + embeddings; each provider still logs a positive row (non-vacuous). Uncached traffic only, so it does not reach the #25846 cache-token path; `fail_before_fix=unproven` | | `test_failure_call_writes_failure_status_row` | failed call -> `status=failure`, `spend=0` | | `test_spend_calculate_returns_nonzero_cost` | cost-map smoke (no batch wait) | | `test_spend_logs_endpoint_returns_spend` | `/spend/logs` returns 200 + the key's spend, never a 5xx (intermittent-500 regression) | diff --git a/tests/e2e/quota_management/spend_tracking/test_spend_tracking_e2e.py b/tests/e2e/quota_management/spend_tracking/test_spend_tracking_e2e.py index b4f7af62016..fd9b179a944 100644 --- a/tests/e2e/quota_management/spend_tracking/test_spend_tracking_e2e.py +++ b/tests/e2e/quota_management/spend_tracking/test_spend_tracking_e2e.py @@ -444,10 +444,9 @@ class _SweepModel: # Chat providers this guard sweeps when the proxy exposes them, each non-streaming and -# streaming: gemini, anthropic, openai, and bedrock - the surface #25846's negative -# streaming-cost bug lived on. gemini/anthropic/bedrock are registered by the suite's -# driver_models; gpt-5.5 is only wired on the stage/docker gateways. Adding a provider -# is one line. +# streaming: gemini, anthropic, openai, and bedrock. gemini/anthropic/bedrock are +# registered by the suite's driver_models; gpt-5.5 is only wired on the stage/docker +# gateways. Adding a provider is one line. _CHAT_SWEEP: tuple[_SweepModel, ...] = ( _SweepModel("gemini-2.5-flash", "gemini-2.5-flash"), _SweepModel("claude-haiku-4-5", "anthropic/claude-haiku-4-5"), @@ -461,15 +460,22 @@ _EMBEDDING_SWEEP = _SweepModel("openai-text-embedding-3-small", "text-embedding- def test_no_provider_logs_negative_spend( client: SpendClient, scoped_key: str ) -> None: - """No provider ever writes a negative spend row. One key sweeps every chat - provider the proxy exposes, both non-streaming and streaming, plus an embedding; - then every logged row is asserted non-negative. A negative cost is a real billing - bug: cache-token accounting that lets a derived token count fall below zero - (BerriAI/litellm#25846, a bedrock-anthropic streaming regression) surfaces here as - spend < 0, and both bedrock and the streaming leg are exercised on purpose because - that is the path that regression rode in on. The per-provider positive-row check - keeps the guard non-vacuous - a pipeline that silently logs 0 (or drops the row) - fails instead of sliding past a bare `>= 0`.""" + """No provider ever writes a negative spend row. One key sweeps every chat provider + the proxy exposes, both non-streaming and streaming, plus an embedding; then every + logged row is asserted non-negative. Negative spend is a billing bug in any form: it + credits the customer's budget back, so a key can outspend its cap without ever + tripping it. The per-provider positive-row check keeps the guard non-vacuous - a + pipeline that silently logs 0 (or drops the row) fails instead of sliding past a + bare `>= 0`. + + Scope, deliberately stated: this is a broad invariant sweep, NOT a regression test + for the bedrock cache-token bug (#25846). That bug needs prompt_tokens to exclude + cache tokens so `prompt_tokens - cache_hit` goes below zero; the traffic here is + uncached, so `cache_hit == 0` and that subtraction can't go negative. Reverting + #25846's clamp would leave this test green. Verified by mutation on 2026-07-14: the + only way to turn it red was an injected fault unrelated to that bug, so the cell + stays fail_before_fix=unproven. Proving it needs a test that drives the cache-token + path itself (bedrock streaming with a cached prefix).""" present = frozenset(entry.model_name for entry in client.gateway.model_info()) chat = tuple(m for m in _CHAT_SWEEP if m.call in present) assert len(chat) >= 2, (