fix(streaming): recover partial usage on sync mid-stream failure (#14457) - #35349
Conversation
Greptile SummaryThis PR aligns synchronous stream-failure accounting with the asynchronous path.
Confidence Score: 4/5The partial-usage recovery failure path must preserve raw-chunk usage before this PR is safe to merge. An exception from partial stream assembly is suppressed without storing usage, after which failure logging resets the response cost to zero and loses billable partial spend. Files Needing Attention: litellm/litellm_core_utils/streaming_handler.py
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/streaming_handler.py | Adds synchronous partial-usage recovery, but an assembly exception still leaves failure logging without recovered usage and zeros the partial spend. |
| tests/test_litellm/litellm_core_utils/test_streaming_handler.py | Adds focused regression tests for missing provider usage, synchronous failures, cost preservation, and idempotent recovery. |
Reviews (1): Last reviewed commit: "fix(streaming): recover partial usage on..." | Re-trigger Greptile
| partial_response = litellm.stream_chunk_builder( | ||
| chunks=self.chunks, | ||
| messages=self.messages, | ||
| logging_obj=self.logging_obj, | ||
| ) |
There was a problem hiding this comment.
Recovery failure still zeros spend
When stream_chunk_builder raises while recovering an interrupted sync stream, the surrounding catch suppresses the error without stashing combined_usage_object; failure_handler then resets response_cost to zero, so the partial spend this path is intended to preserve is still lost. The completed-stream path already falls back to calculate_total_usage(chunks=self.chunks) for this condition.
Knowledge Base Used: Cost Tracking and Budget Enforcement
There was a problem hiding this comment.
Good catch — addressed in the follow-up commit. _record_partial_usage_for_failure now mirrors the completed-stream path: if stream_chunk_builder raises, we fall back to calculate_total_usage(chunks=self.chunks) via model_response_creator before stashing combined_usage_object / response_cost. Added a regression that mocks the builder raise and asserts usage is still recovered.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Does this fix it for /v1/messages as well? I posted at #33736 (comment) that a similar solution fixed it for /chat/completions for me, but not /v1/messages. I don't see any test cases for /v1/messages Anthropic endpoints in that PR nor this PR. |
|
@nuernber Good question — short answer: this PR does not fix What this PR covers
Why
|
6847bbe to
3be2712
Compare
…riAI#14457) Sync __next__ mirrors async by stashing estimated usage/cost before failure_handler. Recovery passes messages into stream_chunk_builder and falls back to calculate_total_usage if the builder raises.
3be2712 to
ec5006a
Compare
Relevant issues
Fixes #14457
Design note
Root cause
Sync
CustomStreamWrapper.__next__mid-stream failures calledfailure_handlerwithout first recovering usage from chunks already delivered. The async path already did this via_log_stream_failure_and_raise→_record_partial_usage_for_failure. Separately, recovery calledstream_chunk_builder(chunks=...)withoutmessages/logging_obj, so when a provider only emits usage on the final chunk, mid-stream disconnect left prompt tokens unestimated.Current data flow
self.chunks.stream_chunk_builder(chunks, messages, logging_obj)→ usage → success logging / spend.combined_usage_object+response_cost, then failure handlers.failure_handlerzerosresponse_costbecausecombined_usage_objectwas never set._bill_partial_streamed_spend_on_disconnect) is a separate path already covered by fix(proxy): bill partial streamed spend when the client disconnects mid-stream #33736 — out of scope here.Where usage is lost
Why existing accounting fails
_failure_handler_helper_fnonly preserves spend whencombined_usage_objectis present; otherwise it setsresponse_cost = 0. Sync never populated that stash.Idempotency risks
Recovery overwrites (does not accumulate)
combined_usage_object/response_cost. A later router-fallback success log on the same request id overwrites again. Covered by a regression that double-calls recovery.Proposed minimal fix
_record_partial_usage_for_failure()in sync__next__beforefailure_handler(mirror async).messages=self.messagesandlogging_obj=self.logging_objintostream_chunk_builderinside recovery (same contract as end-of-stream assembly).Pre-Submission checklist
@greptileaito re-request a review after pushing changes)Type
🐛 Bug Fix
✅ Test
Changes
failure_handler.Local verification
ruff check litellm/litellm_core_utils/streaming_handler.py— passpytest tests/test_litellm/litellm_core_utils/test_streaming_handler.py— 103 passedstreaming_handler_with_usage) — 8 passedFinal Attestation
Test plan
test_streaming_handler_with_usage)failure_handlerfailure_handlerpreserves stashedresponse_cost