fix: recover streamed responses completed output - #30933
Conversation
b11638f to
af142c1
Compare
Greptile SummaryThis PR fixes a bug where the terminal
Confidence Score: 4/5The streaming fix itself is safe to merge; all findings are non-blocking style concerns. The core streaming recovery logic is correct and well-tested with mock-only unit tests. The backfill only activates when the provider sends an empty output, leaving existing behaviour unchanged for well-behaved providers. The README additions are out-of-scope for this PR but do not affect runtime behaviour. The inline _MAX_CONTENT_INDEX and the returned-by-reference items from _recovered_streamed_output_items are minor quality concerns with no realistic failure path given the sequential, single-use nature of the iterator. README.md bundles unrelated Terraform deployment documentation; consider splitting into a separate PR. litellm/responses/sse_output_recovery.py defines _MAX_CONTENT_INDEX inline rather than in constants.py.
|
| Filename | Overview |
|---|---|
| litellm/responses/streaming_iterator.py | Adds output-item tracking state and backfill logic to recover empty response.completed output from previously streamed output_item.done / output_text.done events; logic is correct and sequential. |
| litellm/responses/sse_output_recovery.py | New shared module implementing record_output_item_chunk and record_output_text_chunk helpers; _MAX_CONTENT_INDEX guard prevents large allocations, but the constant is defined inline rather than in constants.py. |
| tests/test_litellm/llms/chatgpt/responses/test_chatgpt_responses_transformation.py | Adds two focused mock-only tests covering the recovery and authoritative-output-preservation paths; no real network calls. |
| README.md | Adds a large "Deploy on AWS or GCP with Terraform" documentation section and two deployment badge images, unrelated to the streaming bug fix; team rule requires docs to live in the litellm-docs repo. |
Comments Outside Diff (1)
-
README.md, line 6-145 (link)Unrelated documentation added to README
This PR's stated scope is a streaming iterator bug fix, but it bundles a large "Deploy on AWS or GCP with Terraform" section (140+ lines) plus two new deployment badge images. The team rule requires documentation additions to live in the
litellm-docsrepo rather than this repository. These changes should be split into a separate PR targeting the docs repo.Rule Used: Prevent documentation from being added - needs to ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix: recover streamed responses complete..." | Re-trigger Greptile
| def _recovered_streamed_output_items(self) -> List[Dict[str, Any]]: | ||
| output_items: Dict[int, Dict[str, Any]] = { | ||
| **self._streamed_text_only_output_items | ||
| } | ||
| output_items.update(self._streamed_output_items) | ||
| return [item for _, item in sorted(output_items.items())] |
There was a problem hiding this comment.
Recovered items are returned by reference, not deep-copied
_recovered_streamed_output_items returns the same dict objects stored in _streamed_output_items / _streamed_text_only_output_items. If transform_streaming_response mutates any item in completed_chunk["response"]["output"] in-place, those mutations will silently persist in the iterator's state dicts. A shallow copy per item (e.g. [dict(item) for ...]) would prevent accidental aliasing without meaningful overhead.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
af142c1 to
03a12b3
Compare
03a12b3 to
0306916
Compare
|
Closing in favor of #30934. The replacement PR was opened from a fresh branch to avoid stale automated feedback from the earlier branch history |
Relevant issues
Related to #25429 and #26179
Pre-Submission checklist
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Verified against a live local proxy started with:
Reproduction command:
Before this change, the final
response.completedevent hadresponse.output: []even thoughresponse.output_item.donecontained the completed assistant messageAfter this change, the final
response.completedevent includes the recovered output item withOK my lordLocal checks run:
Type
Bug Fix
Test
Changes
This updates the per-request Responses streaming iterator to remember completed output items seen earlier in the same SSE stream. When a terminal
response.completedevent has an emptyresponse.output, the iterator backfills it from the previously streamedresponse.output_item.doneorresponse.output_text.doneevents. If the provider already sends a non-empty completed output, that output remains authoritativeThe regression tests cover the reported ChatGPT streaming shape, verify that existing completed output is preserved, and confirm recovered output items are copied before being attached to the completed response