Skip to content

[PENDING: #744, #712] fix: record a clean but short completion under ignore_eos as a failure - #746

Open
Bslabe123 wants to merge 3 commits into
kubernetes-sigs:mainfrom
Bslabe123:fix/truncated-response-655
Open

[PENDING: #744, #712] fix: record a clean but short completion under ignore_eos as a failure#746
Bslabe123 wants to merge 3 commits into
kubernetes-sigs:mainfrom
Bslabe123:fix/truncated-response-655

Conversation

@Bslabe123

Copy link
Copy Markdown
Contributor

Stacked on #744, which is stacked on #712: the first two commits are theirs, the last one is this change. Will rebase to a single commit once they merge.

Closes #655. Part of #606, Success classification.

What

A stream the server closes cleanly after a handful of tokens is a well-formed 200: no stream break (#530), no token-count mismatch (#565), no error. It landed in the success bucket and dragged the output-length distribution down silently.

  • ResponseMetrics.finish_reason: the server's reason verbatim (OpenAI finish_reason, Anthropic stop_reason), captured on every streaming and unary path (completion, chat, Anthropic, trace replay). ResponseMetrics.delivered_output_tokens(): server usage.completion_tokens when reported, else the client count.
  • RequestLifecycleMetric.max_tokens: what the request body asked for, also on each per-request report entry.
  • Under ignore_eos, a completion that delivered fewer tokens than max_tokens is recorded as a TruncatedResponseError failure (label truncatedresponseerror; the message names delivered-of-requested and the finish_reason). Its metrics and body stay on the record. The rule reads ignore_eos from the request body, not the client setting: Anthropic bodies never carry it, and the trace replay turns it off per request for tool calls.
  • Without ignore_eos, a short response stays a success and the summary reports successes.finish_reasons (reason to count) and successes.output_shortfalls.
  • The OTel finish_reason attribute read a never-populated extra_info key; it now reads the persisted field.

Two calls to check

  1. length with fewer tokens is still a truncation. A server that caps below max_tokens (a max_model_len cap) reports length and fewer tokens; under ignore_eos the requested budget was not delivered, so it fails, with the finish_reason in the message for diagnosis.
  2. The rule is on by default, since ignore_eos defaults to true. A server that ignores the ignore_eos field will now report every natural stop as truncated. That is the config misdescribing the run, and the fix is ignore_eos: false; the field description and docs/reports.md say so. Fallback if wanted: a report-config toggle.

Tests

  • tests/required/integration/test_truncated_response.py against the test: integration coverage for partial response body on a broken SSE stream #712 fake: short-with-stop under ignore_eos on (failure) and off (success plus shortfall), full budget, length-capped, server count over client count, unary chat, the report bucket, and a no-choices body that is not judged.
  • Parser (finish_reason on the last content frame, on a content-free frame, Anthropic message_delta), API layer, and reportgen fields.
  • e2e/tests/test_output_length_sim.py: sim v0.6.1 honours ignore_eos; on gives finish_reasons {"length": 10} and 0 shortfalls; off gives shortfalls equal to the per-request entries whose completion_tokens is below max_tokens. The fidelity test asserts the same on its ground truth.
  • The golden fixture serves fixed n_tokens regardless of max_tokens, so it now runs with ignore_eos: false (comment explains); the backend suite's 5-token canned replies likewise.

pdm run validate clean, 935 unit tests pass, sim e2e run locally.

@kubernetes-prow kubernetes-prow Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Aug 18, 2026
@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bslabe123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 18, 2026
@github-actions github-actions Bot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 18, 2026
@kubernetes-prow kubernetes-prow Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 18, 2026
@Bslabe123 Bslabe123 mentioned this pull request Aug 18, 2026
64 tasks
@github-actions github-actions Bot removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 18, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from f12e548 to 9986dd4 Compare August 19, 2026 20:16
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 19, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from 9986dd4 to 9e198cf Compare August 20, 2026 15:46
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 20, 2026
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 20, 2026
@Bslabe123 Bslabe123 added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Aug 20, 2026
@Bslabe123 Bslabe123 changed the title [WIP] fix: record a clean but short completion under ignore_eos as a failure [WIP] [PENDING: #744] fix: record a clean but short completion under ignore_eos as a failure Aug 21, 2026
@Bslabe123 Bslabe123 changed the title [WIP] [PENDING: #744] fix: record a clean but short completion under ignore_eos as a failure [WIP] [PENDING: #744, #712] fix: record a clean but short completion under ignore_eos as a failure Aug 21, 2026
@Bslabe123 Bslabe123 changed the title [WIP] [PENDING: #744, #712] fix: record a clean but short completion under ignore_eos as a failure [PENDING: #744, #712] fix: record a clean but short completion under ignore_eos as a failure Aug 21, 2026
@Bslabe123
Bslabe123 marked this pull request as ready for review August 21, 2026 05:24
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from 9e198cf to bdaed77 Compare August 26, 2026 15:50
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 26, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from bdaed77 to c4c8ffb Compare August 26, 2026 17:48
@github-actions github-actions Bot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 26, 2026
@github-actions github-actions Bot removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 26, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from c4c8ffb to 5c9ae3f Compare August 26, 2026 18:33
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 26, 2026
@kubernetes-prow kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2026
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from 5c9ae3f to 4e8d52a Compare August 27, 2026 20:14
@kubernetes-prow kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2026
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 27, 2026
Adds the kubernetes-sigs#606 Integration-tier regression test for kubernetes-sigs#531. The fix landed
in kubernetes-sigs#530 but nothing held it in place.
A 200 was counted as a success whatever its body held. Some servers and
proxies answer 200 and put the failure in the body: vLLM and SGLang emit
`data: {"error": {...}}` when generation fails mid-stream, and a proxy can
return the OpenAI error object whole. Both landed in the success bucket
with output_tokens 0 (streaming: extract_content returned None on the
error frame, so the parser skipped it and returned normally; unary: the
empty-choices early return). The run summary counted them toward
throughput and the per-request entry showed an empty response and no
error, so nothing pointed at the cause (kubernetes-sigs#713).

Two rules, both raised from process_response and handled by the client
the way a broken stream already is (StreamInterruptedError):

- InBandError: the body, or any SSE frame, carries a top-level `error`.
  error_msg is that object's JSON, the same shape as a non-200 body, so
  reportgen's parse_error_message pulls the server's message out of it.
  The stream is drained before raising, so the raw body is complete and
  the connection closes normally; if the stream breaks after the error
  frame the in-band error still wins over the transport symptom.
- EmptyResponseError: the body yields no completion content and no
  `usage`. A well-formed empty completion always carries usage (mandatory
  in a unary body, requested with stream_options.include_usage on every
  stream), so this only fires on bodies that are not completions.

The body is kept as response_data either way. Anthropic and trace-replay
streams get the InBandError rule through the shared parser; the empty
rule is applied in the completion and chat APIs only.

Tests: unit coverage on the parser and both APIs, and an integration
test (kubernetes-sigs#606 per-change lane) that drives the real client against the kubernetes-sigs#712
fake server serving a complete, well-formed 200 whose only frame is an
error payload, or nothing, in both paths, through to the report's
failure bucket. The fake gains missing_bytes=0 and a verbatim-body mode;
the client harness moves out of kubernetes-sigs#712's test into a shared module.

Addresses kubernetes-sigs#713
A stream the server closes cleanly after a few tokens is a well-formed 200
that no existing detector distinguishes from a legitimately short answer:
no stream break, no token-count mismatch, no error. It landed in the success
bucket and quietly depressed the output-length distribution, the kubernetes-sigs#564 shape
of a wrong number that reads like a normal one.

Capture the server's finish_reason (OpenAI finish_reason, Anthropic
stop_reason) on ResponseMetrics from every streaming and unary path, and
persist the request's max_tokens on RequestLifecycleMetric, so requested
versus delivered output length is computable at report time. Under
ignore_eos the server was told to generate the full max_tokens, so a
completion that delivered fewer (server usage.completion_tokens when
reported, else the client count) is recorded as a TruncatedResponseError
failure with its metrics and body kept; the rule reads ignore_eos from the
request body, so Anthropic bodies and the trace replay's per-request
ignore_eos=false are not held to it. Without ignore_eos the same shortfall
stays a success and is reported as an observation: successes.finish_reasons
and successes.output_shortfalls, plus max_tokens on each per-request entry.
The OTel finish_reason attribute, previously read from a never-populated
extra_info key, now reads the persisted field.

Tests: SSE parser and API-layer finish_reason capture, reportgen fields, an
Integration-tier suite against the kubernetes-sigs#712 fake server (short-with-stop under
ignore_eos on/off, full budget, length-capped, server count precedence,
unary chat, report bucket), and a sim-backed e2e that cross-checks
output_shortfalls against the per-request file. The golden sim fixture no
longer claims ignore_eos, since it serves fixed-length cases regardless of
max_tokens.

Addresses kubernetes-sigs#655
@Bslabe123
Bslabe123 force-pushed the fix/truncated-response-655 branch from 4e8d52a to 21ac133 Compare September 3, 2026 20:30
@github-actions github-actions Bot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Gracefully truncated responses are recorded as successes with wrong output length

1 participant