fix(streaming): stop stream_chunk_builder duplicating Anthropic thinking text - #35118
Conversation
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
_await_model_servable used poll_timeout (120s), the spend/log read-back budget. A stuck model reload therefore stalled every suite that creates a deployment for two minutes before failing Give create_model a fixed harness middle ground: model_servable_timeout=40s, polled every 2s, with each /v1/models call capped at 5s and clamped to the remaining deadline so one slow GET cannot overrun the wait. Happy path still returns on the first listing. Not derived from proxy general_settings or env Transport.get accepts an optional per-call timeout for that clamp. Unit tests cover the deadline arithmetic and clamp without a live proxy (cherry picked from commit c082a0e)
create_model returned after the first /v1/models hit that listed the model, so chat could still land on a cold gateway worker (numWorkers>1 / peer pod) and 400 Invalid model name. Require continuous listing for the product default add_deployment interval (30s) after first sight so every worker has synced from the DB; first listing still bounded at 40s (cherry picked from commit 7d1ee2f)
Keep the create_model DB-sync wait in the harness; the pure-function unit file is not needed for this PR (cherry picked from commit 8920465)
When less than one full poll interval remained in the first-listing budget, the pre-sleep check returned NotServable without another /v1/models call. Sleep only min(interval, time left) so a model that becomes listable in the last seconds of the timeout still gets a clamped final poll (cherry picked from commit 8439195)
A poll may start with remaining budget and still return after started+timeout if the transport overruns its clamp. Recheck the first-listing deadline after the response so a late listing does not open the continuous DB-sync phase (cherry picked from commit 7ff2bcb)
…l_servable_timeout test(e2e): bound the post-/model/new servable wait at 40s
* fix(mcp): resolve call_tool by registry without requiring tool map Multi-worker reloads put MCP servers in the registry from the DB but do not re-run tools/list on every process. Gating call_tool on tool_name_to_mcp_server_name_mapping made cold workers 500 with Tool not found after another worker had already listed the tool. Treat a registry match on server id/name/alias as enough; upstream rejects unknown tools * test(e2e): poll MCP register, tools/list, and tools/call across multi-worker lag Stage multi-worker gateways only load MCP servers and tool maps on the process that handled the request. Poll until the server is listed, the tool appears on tools/list, and tools/call is not a cold-worker 500 so key-access and Datadog MCP e2e stop racing the LB * Revert "fix(mcp): resolve call_tool by registry without requiring tool map" This reverts commit 8b56e51. * test(e2e): tighten MCP multi-worker lag classifier Only retry tools/call on gateway shapes Tool <name> not found and server_not_found, not any 500 that mentions tool/server not found, so upstream failures are not retried until the poll deadline * test(e2e): drop unit file for MCP lag classifier The live await_call_tool polls already cover multi-worker lag; a separate string-match unit module is not worth keeping (cherry picked from commit c274cf3)
…p_e2e_poll test(e2e): poll MCP tools across multi-worker lag (BerriAI#35047)
…ing text Anthropic streams a thinking block as N thinking_delta chunks and then one signature_delta. The Anthropic iterator builds that signature chunk by joining every prior thinking delta, so it carries the whole block's text again rather than an increment. get_combined_thinking_content appended that text on top of the parts it had already accumulated, so a rebuilt extended-thinking response came back with its reasoning emitted twice. reasoning_content on the same response was correct, which is what makes the two disagree. Skip the append when a block arrives with a signature and its text is exactly what has been accumulated so far. A provider that sends only a final increment alongside the signature is unaffected, since the texts differ.
| """ | ||
| if not isinstance(result, UnknownApiError) or result.status_code != 500: |
There was a problem hiding this comment.
Registry misses bypass polling
When a cold data-plane worker returns the MCP endpoint's HTTP 404 server_not_found response, this status gate returns False before inspecting the structured error, causing await_call_tool and await_call_tool_denied to fail immediately instead of waiting for registry propagation.
| if not listed: | ||
| first_seen_at = None | ||
| elif first_seen_at is None: | ||
| if t > started + timeout: | ||
| return NotServable(last_result=last_result) | ||
| first_seen_at = t | ||
| if db_sync_seconds <= 0: | ||
| return Servable() | ||
| elif t - first_seen_at >= db_sync_seconds: | ||
| return Servable() |
There was a problem hiding this comment.
Continuous listing misses cold workers
When the load balancer routes every /v1/models poll to the same synchronized worker, this continuous-listing window succeeds even though another worker has not loaded the model, causing the next model request routed to that cold worker to fail with Invalid model name.
Greptile SummaryThis PR fixes duplicate signed thinking text during streamed-response reconstruction and also revises unrelated E2E synchronization helpers.
Confidence Score: 3/5The streaming fix is sound, but both E2E synchronization helpers need correction before merging because they still release or fail callers during multi-worker propagation. The MCP retry classifier rejects the endpoint's actual 404 registry-miss response, while the model-servability poll can repeatedly observe one hot worker and incorrectly conclude that all workers have loaded the model. Files Needing Attention: tests/e2e/mcp/mcp_client.py, tests/e2e/proxy_client.py
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/streaming_chunk_builder_utils.py | Correctly avoids appending Anthropic's repeated signed thinking snapshot while retaining distinct signed increments. |
| tests/test_litellm/litellm_core_utils/test_streaming_chunk_builder_utils.py | Adds focused coverage for snapshot deduplication and preservation of genuine final increments. |
| tests/e2e/mcp/mcp_client.py | Adds registry-lag polling, but the status gate prevents retrying the endpoint's HTTP 404 server-not-found response. |
| tests/e2e/proxy_client.py | Adds model-list polling whose continuous visibility window can observe only a hot worker and still release callers before every worker has synchronized. |
| tests/e2e/transport.py | Propagates an optional request-specific timeout through both HTTP and split transports. |
Reviews (1): Last reviewed commit: "fix(streaming): stop stream_chunk_builde..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
stream_chunk_builder()returns Anthropic extended-thinking reasoning twice over, so the rebuiltthinking_blocks[0]["thinking"]is the whole reasoning trace concatenated with itselfreasoning_contenton that same rebuilt response is correct, so the two fields disagree about what the model actually reasonedHow it solves it:
Relevant issues
None open for this one. #33034 and its PR #33035 cover a different failure in the same function (thinking dropped when no signature ever arrives); this is orthogonal and the two touch different parts of the function, so they should not conflict beyond a trivial adjacency
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
I could not produce the live-proxy curl output the template asks for, because I do not have an Anthropic key I am able to spend on this, so I want to be upfront about that rather than pass off a test run as proof. What follows is what I could verify offline, and below it are the exact commands to reproduce against a real key
The mechanism, driving the real
ModelResponseIteratorfromlitellm/llms/anthropic/chat/handler.pyover the event sequence Anthropic sends for one thinking block, then handing the resulting chunks toChunkProcessor:Two separate signed thinking blocks in one turn stay separate and are each emitted once, before and after
To reproduce against a real key, with the proxy on
localhost:4000and a config exposing an Anthropic model with thinking enabled:Then open http://localhost:4000/ui/?page=logs, click into that request, and compare the assembled response's
thinking_blocks[0].thinkingagainstreasoning_content. Onlitellm_internal_stagingthe first is the second repeated twice; with this PR they matchType
Bug Fix
Changes
get_combined_thinking_contentaccumulates a block's text across chunks and flushes when a signature arrives. Anthropic streams a thinking block as Nthinking_deltachunks followed by onesignature_delta, andModelResponseIterator._handle_content_block_deltabuilds that final chunk by joining every prior thinking delta:So the signature chunk carries the block's whole text again rather than an increment, and appending it on top of the accumulated parts doubled the reasoning. The existing test for this function did not catch it because its fixture models the signature chunk as
{"type": "thinking", "thinking": None, "signature": "sig_block1"}, which is not what the iterator emitsThe guard is an exact-equality check, so it only suppresses a genuine repeat. A provider that sends the last increment alongside the signature still has that increment appended, which the second test pins
QA runbook
Beyond the curl above, worth exercising a turn that produces two separate thinking blocks and one that is cut short by
max_tokensmid-thinking, since both go through this function. The truncated case still returns no thinking blocks, unchanged by this PR and left to #33035Unit tests,
tests/test_litellm/litellm_core_utils/test_streaming_chunk_builder_utils.py: 19 passed. With the fix reverted,test_get_combined_thinking_content_does_not_duplicate_resent_thinkingfails and the other 18 pass. Mutating the guard so it always skips instead of skipping only exact repeats makestest_get_combined_thinking_content_keeps_a_genuine_final_incrementfail and the other 18 pass, so each new test is killed by its own mutant and neither is decorativeWhole directory
tests/test_litellm/litellm_core_utils/: 4 failed, 1400 passed. The same 4 fail on a clean tree with 1398 passed, so they are pre-existing (test_bedrock_converse_messages_pt_document_various_formats,test_bedrock_midstream_internal_server_error_wraps_for_fallback,test_blocks_ietf_protocol_assignments_old_oracle_metadata,test_logfire_logger_accepts_env_vars_for_base_url) and the delta is exactly the 2 tests added hereruff 0.15.3reports one unused-import error and one file needing reformat in these two files, both of which reproduce identically on a clean tree, so neither comes from this change. My longest added line is 99 charactersFinal Attestation