Skip to content

fix(llm): raise OutputTooLongError when a non-streaming completion is truncated - #3827

Open
ebarkhordar wants to merge 3 commits into
vectorize-io:mainfrom
ebarkhordar:fix/3811-finish-reason-length-output-too-long
Open

fix(llm): raise OutputTooLongError when a non-streaming completion is truncated#3827
ebarkhordar wants to merge 3 commits into
vectorize-io:mainfrom
ebarkhordar:fix/3811-finish-reason-length-output-too-long

Conversation

@ebarkhordar

@ebarkhordar ebarkhordar commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Root cause

chat.completions.create() reports a token-limit truncation only through finish_reason. It never raises LengthFinishReasonError, which is what the except LengthFinishReasonError handler in call() converts into OutputTooLongError. Both structured-output and free-form calls go through create(), so that handler cannot fire for either of them, and _content_or_error returned the truncated body as though it were complete.

What that looked like downstream:

  • Structured output: the caller's json.loads failed on the cut, so a truncation surfaced as a JSON parse error. The retry ladder treated it as transient and re-sent a byte-identical request against the same token limit.
  • Free-form output: the truncated text was returned as the answer, with nothing to signal that it had been cut.

_extract_facts_with_auto_split recovers from truncation by catching OutputTooLongError and splitting the chunk, so on this path it could never fire.

Fix

Detect the truncation where the signal actually arrives and raise there. This matches the two sibling providers: litellm_llm raises on finish_reason == "length", and openai_responses_llm raises when the response status is incomplete with reason max_output_tokens.

The check sits ahead of the content read, so a response that is truncated before any visible token raises OutputTooLongError too. An earlier revision of this PR put it after the empty-content branch and described that as deliberate. That was wrong, and koriyoshi2041 caught it in review: the empty-content branch sets retryable = finish_reason not in {"content_filter"}, so length came back retryable and call() re-sent the same request against the same limit instead of letting the auto-split run.

Verification

  • New tests/test_openai_compatible_truncation.py, now 7 tests: 5 fail on main and all 7 pass on this branch. Two of the 7 are controls that pass on both, one for a normal finish_reason: "stop" response and one for an empty response with a non-truncation finish reason, which still raises the retryable ProviderResponseError.
  • The two tests for the empty truncation also fail at this PR's own earlier ordering, not only on main: the call() one logs 4 attempts against the same limit and then raises ProviderResponseError.
  • The two call() tests assert create.call_count == 1, pinning that a truncation is not retried against the same limit.
  • Ran the provider and fact-extraction suites (test_fact_extraction_retry, test_multi_llm_provider, test_openai_responses_provider, test_deepseek_tool_call_compat, test_consolidation_retry_budget, test_xai_oauth_llm and 8 more) on both main and this branch: the set of failures is identical, and all of them are pre-existing in test_xai_oauth_llm.py on main. ruff check, ruff format --check and ty check hindsight_api are clean, with a ty diagnostic profile identical to main.
  • Not verified: any behaviour against a live provider. The tests drive a mocked chat.completions.create, so what they pin is this repo's handling of a truncated response shape, not that a given provider emits that shape.
  • Also not run end to end: a truncated provider response travelling all the way into the auto-split. The two halves are covered separately, by the tests here and by the existing test_fact_extraction_retry.py, and the catch site at fact_extraction.py:1907 takes the same OutputTooLongError class this raises, but I did not exercise the whole path in one test.

Interaction with #3685

No textual conflict: #3685 edits the JSONDecodeError handler around line 1053 and this changes _content_or_error around line 343. They are complementary rather than competing, but the order matters, so it is worth stating. A truncated response no longer reaches that handler at all, because it raises before returning content. If it did reach it, parse_llm_json would repair the cut into valid but silently partial JSON, whereas raising lets the auto-split re-extract the whole chunk. Non-truncation malformed JSON is untouched by this change and still reaches #3685's repair path.

Fixes #3811

… truncated

chat.completions.create() reports a token-limit truncation only through
finish_reason. It never raises LengthFinishReasonError, so the handler in call()
that converts that exception into OutputTooLongError cannot fire for either of
the two call sites that use create(), and _content_or_error returned the
truncated body as though it were complete.

For structured output the caller's json.loads then failed on the cut, which the
retry ladder treated as a transient parse error and retried against the same
token limit. For free-form output the truncated text was returned with nothing
to signal the cut.

The fact-extraction auto-split recovers from truncation by catching
OutputTooLongError, so it could never fire on this path. Detect the truncation
where the signal arrives and raise there, matching the two sibling providers:
litellm_llm raises on finish_reason == "length" and openai_responses_llm raises
on an incomplete status of max_output_tokens.

The check sits after the empty-content branch, so a response that is both empty
and truncated keeps its existing ProviderResponseError.

Fixes vectorize-io#3811
@strix-security

strix-security Bot commented Aug 27, 2026

Copy link
Copy Markdown

Strix Security Review

Warning

This pull request has 2 commits after the last Strix review (fd11c15). Strix has not reviewed these changes.
Automatic review on push is off for this repository. To review the latest changes, tag @strix-security in a comment, or turn on re-review on push.

No security issues found.

Updated for fd11c15.


Reviewed by Strix
Re-run review · Configure security review settings

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length check needs to happen before the empty-content branch. OpenAI-compatible providers can return content="" with finish_reason="length" when the limit is exhausted before any visible token; this branch currently converts that truncation into a retryable ProviderResponseError, so call() repeats the same request and fact extraction never reaches its OutputTooLongError auto-split path.

I reproduced this at fd11c15 by passing an empty-content/length response to _content_or_error: it raises ProviderResponseError, while the non-empty truncation tests pass. Moving the finish-reason check ahead of the content check and adding that case should keep the recovery contract consistent for both empty and partial truncations. The focused provider suites otherwise pass 12/12 with proxy variables cleared.

The finish_reason == "length" check sat after the empty-content branch, so a
budget exhausted before the first visible token raised the retryable
ProviderResponseError instead. call() retries that class, which re-sends the
identical request against the same limit, and _extract_facts_with_auto_split
never receives the OutputTooLongError it splits on.

Move the check ahead of the content read. Both sibling providers already do
this: litellm_llm coerces content to "" before its length check, and
openai_responses_llm calls _raise_if_truncated before reading output_text.

Two new tests cover the case, and both fail at the previous ordering. The
call() one pins create.call_count == 1, which is what the old ordering broke:
the run log at that ordering shows four attempts against the same limit before
it gave up with the wrong error class. A third test is a control that an empty
response with any other finish_reason still raises the retryable
ProviderResponseError, so the two paths stay distinct.

Reported by koriyoshi2041 in review on vectorize-io#3827.
@ebarkhordar

ebarkhordar commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

You are right, and the sentence in the description defending that ordering was wrong.

The cost is worse than a class change: the empty-content branch sets retryable = finish_reason not in {"content_filter"}, so length came back retryable, and call() retries a retryable ProviderResponseError. Running the call() case at the old ordering logs four attempts against the same limit before it gives up with the wrong class, so the auto-split never sees it.

Pushed in aeea639, with the length check ahead of the content read.

Correcting one line of this comment, edited in afterwards: I wrote that both siblings raise ahead of their content read. Only openai_responses_llm does, calling _raise_if_truncated before it reads output_text. litellm_llm reads content first, coercing it to "" at litellm_llm.py:330 and checking finish_reason at :343, and it carries no empty-content branch at all, which is why the bug cannot appear there. The precedent held for one sibling, not two. The comment in the source said the same thing and is trimmed to the accurate part in the follow-up commit.

Three tests: the _content_or_error unit for the empty case, a call() one pinning create.call_count == 1, and a control that an empty response with any other finish_reason still raises the retryable error. The first two fail at the previous ordering. Description corrected too.

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked at aeea639. The length guard now runs before the empty-content path, and the new call-level case confirms an empty truncation raises OutputTooLongError without retrying (create.call_count == 1); the non-truncated empty-response control still keeps the existing retryable provider error. The focused truncation suite passes 7/7, and git diff --check HEAD^ HEAD is clean.

…ment

The comment said both sibling providers raise ahead of their content read.
That is true of openai_responses_llm, which calls _raise_if_truncated before
reading output_text, and false of litellm_llm: it coerces content to "" at
litellm_llm.py:330 and only checks finish_reason at :343, so it reads content
first and has no empty-content branch at all.

Behaviour is unchanged; this only trims the comment to what the code does.
@ebarkhordar

Copy link
Copy Markdown
Contributor Author

Flagging that @koriyoshi2041 opened #3854 for the same issue about a day after this one. It makes the same call in the same place: raise OutputTooLongError from _content_or_error when finish_reason == "length", so .create() matches what .parse() already does.

One substantive difference between them. #3854 raises before the message is None check, so it also covers a truncated response that came back with no message at all; this PR raises after that check, so that case still surfaces as a ProviderResponseError. Its guard placement is the better of the two. What this PR has instead is provider, model and scope in the error text, and a wider regression file.

Happy to close this in favour of #3854 if the smaller diff is easier to take, or to move the guard above the message check here and carry the tests. No preference from me, whichever suits the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants