Skip to content

Fix worker status for unusable terminal backend responses#615

Open
ushaket wants to merge 5 commits into
vllm-project:mainfrom
ushaket:fix/worker-unusable-terminal-response
Open

Fix worker status for unusable terminal backend responses#615
ushaket wants to merge 5 commits into
vllm-project:mainfrom
ushaket:fix/worker-unusable-terminal-response

Conversation

@ushaket

@ushaket ushaket commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes a scheduler correctness bug where requests could be marked as completed even when the backend resolved without a usable terminal response. It adds explicit terminal-response validation in the worker so malformed/empty terminal results are surfaced as errored with a clear diagnostic instead of being counted as successful requests.

Details

  • Added terminal response validation in WorkerProcess to guard final status transitions.
  • Updated request finalization logic so unusable terminal responses set:
    • status: errored
    • error message: [UNUSABLE_BACKEND_RESPONSE] backend resolved without a usable terminal response payload
  • Implemented GenerationResponse-aware usability criteria:
    • usable if non-empty text or output_metrics.total_tokens > 0
    • None terminal response is always unusable
    • non-GenerationResponse fallback remains bool(response) for generic/test compatibility
  • Added regression tests in tests/unit/scheduler/test_worker.py for:
    • no terminal response object -> errored
    • empty GenerationResponse -> errored
    • token-bearing GenerationResponse with empty text -> completed
  • Preserved existing cancellation/error flow behavior outside terminal-response validation.

Test Plan

  • Run targeted worker regression tests:
    • uv run pytest -q tests/unit/scheduler/test_worker.py -k "terminal_response or empty_generation_response or generation_response_with_tokens or invalid_initialization"
  • Verify expected outcomes:
    • missing terminal response is not marked completed
    • empty GenerationResponse is not marked completed
    • non-empty signal (output_tokens > 0) is accepted as completed
  • Confirm no lint issues on touched files.

Related Issues


  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes AI-assisted code completion
  • Includes code generated by an AI application
  • Includes AI-generated tests (NOTE: AI written tests should have a docstring that includes ## WRITTEN BY AI ##)

git log

commit 368abbd
Author: Uri Shaket [email protected]
Date: Mon Mar 2 23:55:49 2026 +0200

mvoe validation logic to RequestHandlers

Signed-off-by: Uri Shaket <[email protected]>

commit 4388149
Author: Uri Shaket [email protected]
Date: Mon Mar 2 23:57:34 2026 +0200

revert worker changes

Signed-off-by: Uri Shaket <[email protected]>

commit 9db04b6
Author: Uri Shaket [email protected]
Date: Wed Jul 15 11:25:12 2026 +0300

fix tests

Signed-off-by: Uri Shaket <[email protected]>

commit c55e6a4
Author: Uri Shaket [email protected]
Date: Wed Jul 15 13:23:05 2026 +0300

self CR

Signed-off-by: Uri Shaket <[email protected]>

commit 509ce95
Author: Uri Shaket [email protected]
Date: Thu Jul 16 17:28:58 2026 +0300

Clarify unusable backend response error message

Signed-off-by: Uri Shaket <[email protected]>
Co-authored-by: Cursor <[email protected]>

Co-authored-by: Cursor [email protected]
Signed-off-by: Uri Shaket [email protected]

@ushaket
ushaket marked this pull request as draft February 26, 2026 09:38
@ushaket

ushaket commented Feb 26, 2026

Copy link
Copy Markdown
Contributor Author

This introduce worker dependency on GenerationResponse, not sure if that's the right way to go,
The reason I added it is that the we need to somehow know what we're looking for, in the future we might have ResponseT that doesn't return text and total_tokens, so if we want to keep this check in the worker, we'll need to be aware of the different ResponseT possible

@sjmonson

sjmonson commented Mar 2, 2026

Copy link
Copy Markdown
Collaborator

Hmm, yeah I am not a fan of depending on GenerationResponse in the worker. Can you maybe just check inside the OpenAIRequestHandler.compile_streaming and OpenAIRequestHandler.compile_non_streaming methods? It does not generalize as nicely but what qualifies as an unusable response may differ between backends/endpoints. If you throw an exception in those methods it should propagate up to the worker.

@ushaket
ushaket marked this pull request as ready for review March 2, 2026 21:59
@ushaket

ushaket commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

Done

@sjmonson sjmonson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you rebase and get rid of the merge commits?

@dbutenhof dbutenhof added this to the v0.7.0 milestone Mar 30, 2026
@mergify

mergify Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

@ushaket, this project requires a linear history on feature branches.
Your PR contains merge commits. Please rebase your branch against main
and remove them.

You can do this by running:
git pull --rebase upstream main

@mergify mergify Bot added the needs-rebase label Mar 30, 2026
@ushaket
ushaket force-pushed the fix/worker-unusable-terminal-response branch from 60e1abc to d35b823 Compare March 30, 2026 13:12
@mergify mergify Bot removed the needs-rebase label Mar 30, 2026
@ushaket
ushaket force-pushed the fix/worker-unusable-terminal-response branch from d35b823 to d82eb51 Compare March 30, 2026 13:18
@sjmonson sjmonson modified the milestones: v0.7.0, v0.7.2 Jul 1, 2026
@ushaket
ushaket force-pushed the fix/worker-unusable-terminal-response branch from d82eb51 to 4388149 Compare July 15, 2026 07:54
ushaket added 2 commits July 15, 2026 11:25
Signed-off-by: Uri Shaket <[email protected]>
Signed-off-by: Uri Shaket <[email protected]>
@dbutenhof dbutenhof added community contribution An opportunity for contribution from the GuideLLM community already invested in this area. bug Represents a user-visible defect labels Jul 15, 2026

@dbutenhof dbutenhof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have really mixed feelings about this. I don't like the idea of hacking this into each request handler separately -- and you've only handled a subset of the http request handlers in this PR. Maybe that's OK, but it feels weird.

I like your original idea of triggering this through the worker, perhaps by delegating to a backend validate_response method (which could, if necessary, delegate to individual request handlers where more specialized knowledge is required). Your check, for example, is pretty much generic and might even be a default implementation on the base Backend class that everyone would get by default.

(The risk, of course, is that such a default might break some weird special handlers that don't have "normal" text/token outputs; which means a generalization would require extensive testing through all the validations.)

Comment on lines +578 to +586
def _validate_compiled_response(self, response: GenerationResponse) -> None:
"""Raise when endpoint produced a terminal payload with no usable output."""
has_text = bool(response.text and response.text.strip())
output_tokens = response.output_metrics.total_tokens or 0
if not has_text and output_tokens <= 0:
raise ValueError(
"[UNUSABLE_BACKEND_RESPONSE] backend resolved without a usable "
"terminal response payload"
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of "without a usable terminal response payload", we should be more specific. Something like "empty response payload" would help.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done 🙏

{"choices": [], "usage": {}},
],
)
def test_non_streaming_raises_for_unusable_terminal_payload(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These tests (and the ones starting at 671) feel like duplicates of each other. But that's an issue for a different PR.

Tests are working for me, and I don't really have any additional complaints to add onto what the others have said.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@ushaket

ushaket commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@dbutenhof Thanks for the feedback, I agree the per-handler approach is a bit awkward, and it currently only covers text/chat.

I prototyped extending this to Responses (same text / tool_calls / tokens check) and Embeddings (raw data[].embedding check, since those responses never have text). Happy to include either or both in this PR if that’s useful.

How would you like to proceed?

Keep this PR scoped to text/chat and take broader coverage in a follow-up
Add Responses here
Add Responses + Embeddings here
Pull @sjmonson in and decide together (especially vs. a Backend.validate_response hook)

I’m fine with any of these, just want to align before I push more surface area into this PR.

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

Labels

bug Represents a user-visible defect community contribution An opportunity for contribution from the GuideLLM community already invested in this area.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Worker marks completed/successful when final response has no usable data

5 participants