fix(anthropic_messages): log spend for interrupted /v1/messages streams - #35971
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
|
Greptile SummaryThis PR moves Anthropic messages streaming spend dispatch into generator teardown and removes Bedrock's extra async-generator layer so interrupted streams are billed promptly.
Confidence Score: 4/5The upstream-exception path needs to be fixed before merging because it dispatches both success and failure logging for the same failed stream. The teardown change correctly covers client disconnects, but its unconditional success dispatch also runs for ordinary upstream exceptions after partial output and conflicts with the proxy's subsequent failure handling. Files Needing Attention: litellm/llms/anthropic/experimental_pass_through/messages/streaming_iterator.py
|
| Filename | Overview |
|---|---|
| litellm/llms/anthropic/experimental_pass_through/messages/streaming_iterator.py | Moves logging into finally and onto the global worker, but regular upstream exceptions now trigger success logging before the proxy's failure logging. |
| litellm/llms/bedrock/messages/invoke_transformations/anthropic_claude3_transformation.py | Returns the inner async iterator directly so explicit closure reaches its teardown deterministically. |
| tests/test_litellm/llms/anthropic/experimental_pass_through/messages/test_streaming_iterator.py | Adds coverage for explicit closure after partial consumption and closure before the first chunk, but not an upstream exception after partial output. |
| tests/test_litellm/llms/bedrock/messages/invoke_transformations/test_anthropic_claude3_transformation.py | Verifies that closing the Bedrock iterator reaches logging setup, while leaving the exception-exit behavior uncovered. |
Reviews (1): Last reviewed commit: "fix(anthropic_messages): log spend for i..." | Re-trigger Greptile
| finally: | ||
| if collected_chunks: | ||
| await self._handle_streaming_logging(collected_chunks) |
There was a problem hiding this comment.
Exceptions trigger success logging
When a Bedrock stream yields chunks and then raises an upstream exception, this finally dispatches the partial response through the success handlers before the exception reaches the proxy failure handler, causing the same failed request to produce contradictory success and failure logging and a spend record marked as successful.
Knowledge Base Used: Cost Tracking and Budget Enforcement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This does not preserve with full spend logs, just tested it. I manually checked the recorded spend log in LiteLLM vs. the Bedrock invocation log. The input tokens matched but the Bedrock invocation logs had more output tokens. The solution in PR #36008 works with full spend log preservation. |
Review notes (AI generated)Main concern: this fix makes an interrupted stream produce a spend row again, but it bills only the chunks the client drained before disconnecting, not the full response Bedrock actually generated and billed. So it fixes the "zero rows" half of the regression while leaving the other half, the output tokens reading lower than the AWS Bedrock invocation logs, unfixed. That output undercount is the specific symptom being reported. Why it happens: on a client disconnect, Concrete repro on an early disconnect (client reads a 3 event prefix, real stream ends with The added test Secondary robustness note: Alternative approach for comparison: run the upstream read in a detached background task rooted so it is not garbage collected. The pump drains the provider stream to its terminal usage event and bills there, and the client facing generator only relays chunks off a queue, so a disconnect tears down the relay but not the drain. A |
TLDR
Problem this solves:
/v1/messageson bedrock invoke logged no spendHow it solves it:
finally, not only after full consumptionRelevant issues
Fixes #35958
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Proof was captured against a local proxy on
localhost:4000with a stub bedrockinvoke-with-response-streamendpoint (aws_bedrock_runtime_endpoint: http://127.0.0.1:8112) that speaks real AWS event-stream framing and emits real anthropic message events plusamazon-bedrock-invocationMetrics, because this sandbox has no outbound AWS access. Everything else in the path is the real proxy: real auth, real anthropic messages handler, real decoder, real cost calculation, realLiteLLM_SpendLogswrites. Someone with bedrock credentials should re-run the same two commands against a realbedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0deployment before mergeThe repro streams
/v1/messages, drops the connection after 5 SSE lines, then polls/spend/logs:Before, at
24dbd2b2db:After, at
fce256f584, three consecutive interrupted runs:A fully consumed stream at
fce256f584still writes exactly one row with the provider's own metrics, so nothing is double billed:Type
🐛 Bug Fix
Changes
BaseAnthropicMessagesStreamingIterator.async_sse_wrappercollected every SSE chunk it forwarded and then, once the upstream stream ended, dispatched spend logging for them. A client that hangs up mid-stream gets that generator closed instead, which raisesGeneratorExitat theyield, so the dispatch line was simply never reached and the request was billed nothing. The proxy-level disconnect billing added in #33736 cannot cover this path either, since_bill_partial_streamed_spend_on_disconnectreadsresponse.chunksoff aCustomStreamWrapperand the anthropic messages response is a bare async generator. Moving the dispatch into afinallymakes the chunks already streamed get logged on interruption, matching whatPassThroughStreamingHandler.chunk_processoralready does for the native anthropic passthrough (which is why the same interruption onanthropic/...does log):That alone was not enough on bedrock invoke.
bedrock_sse_wrapperwas itself an async generator re-yielding the wrapper's chunks, and closing the outer generator does not deterministically close the inner one, it only drops the reference and leaves thefinallyto whenever the event loop finalizes the abandoned asyncgen. In practice the spend log landed sometimes and was lost most of the time. It now returns the wrapper's iterator directly, so theaclose()the proxy already performs in_finalize_streaming_generator_cleanupreaches thefinallyright away:Logging dispatch also moves from a bare
asyncio.create_tasktoGLOBAL_LOGGING_WORKER, the same queuechunk_processoruses, since a task created while the request is being torn down has no owner keeping it aliveUsage on an interrupted stream is whatever the provider reported in the chunks the client did receive, so
message_startinput tokens plus the output tokens seen so far, not the tokens the model would have gone on to generate. Bedrock only sendsamazon-bedrock-invocationMetricson the final frame, and the proxy stops reading upstream on disconnect by design (_UpstreamClosingStreamingResponse), so billing the full generation would mean draining the abandoned stream to the end. This PR deliberately does not do that; it is the same partial-usage semantics #33736 chose for/chat/completionsQA runbook
Final Attestation