[Bugfix] Return the client-disconnect error instead of discarding it in completions_v1

[ErenAta16]

Fixes #4781.

Motivation

generate() and completions_v1() both moved their streaming loop into a nested _inner_call, which signals client disconnect by returning create_error_response(HTTPStatus.BAD_REQUEST, 'Client disconnected') from inside an async with. Neither call site used that return value.

generate() — the disconnect branch returns from inside the async with, so the response = GenerateReqOutput(...) assignment below it is never reached, and await _inner_call() throws the error response away. The endpoint returns the still-None response, which FastAPI serialises as 200 with a null body. Running the pattern in isolation:

disconnect=False  before -> '200 GenerateReqOutput'  after -> '200 GenerateReqOutput'
disconnect=True   before -> None                     after -> '400 Client disconnected'

completions_v1() — same shape, reached through asyncio.gather, which collects the return values into a list that was discarded. This one degrades more quietly: after the early return, final_res stays None for the disconnected index, so assert final_res is not None never runs for it and the response is built from the choices that did finish.

disconnect_idx=None  before -> 200 choices=[0, 1, 2]  after -> 200 choices=[0, 1, 2]
disconnect_idx=1     before -> 200 choices=[0, 2]     after -> 400 (idx 1)

For contrast, /v1/chat/completions is correct and shows the intended shape: its async with and return create_error_response(...) sit directly in the endpoint body, so the return leaves the endpoint.

Modification

Take the return value at both call sites. generate() returns it when it isn't None; completions_v1() scans the gather results and returns the first non-None entry. The normal path is untouched in both — when no client disconnects every _inner_call returns None and the existing response is built and returned as before.

Checklist

• Pre-commit or other linting tools are used to fix the potential lint issues. ruff check reports one I001 on this file, and reports the same on an unmodified checkout, so it is pre-existing; ruff format --check likewise wants to reformat the file on both. Neither is touched here. The only line over the 120 limit (line 320) is also pre-existing.
• The modification is covered by complete unit tests. Not added: both paths need a live server plus a client that disconnects mid-stream, and there is no existing harness in tests/ for the disconnect branch of these endpoints. The numbers above come from running the control-flow pattern rather than the server, so I would rather say so than add a test I could not run. Happy to add one if you can point me at the right fixture.
• If this PR introduces a new feature, docs are updated. Not a feature.

[ErenAta16]

Closing this in favour of #4782, which fixes the same issue #4781, was opened seven days earlier, and ships a regression test that drives a real disconnect through a fake engine. I should have found it before opening this one; that is a preflight failure on my side, not a judgement call.

The one thing this PR had that #4782 does not is the completions_v1 site, where asyncio.gather collects the per-entry error responses and drops them. I have posted that hunk and the reasoning on #4782 so it can be folded in there, which is a better home for it than a competing PR on the same file.

[ErenAta16]

Reopened and narrowed to completions_v1 only, as suggested. The generate hunk is reverted here so this no longer overlaps #4782.

What is left is the batched path:

inner_results = await asyncio.gather(
    *[_inner_call(i, generators[i], sessions[i]) for i in range(len(generators))])
for inner_result in inner_results:
    if inner_result is not None:
        return inner_result

gather collects each coroutine's return value into a list that main discards, so a disconnect on one entry loses that entry's create_error_response(HTTPStatus.BAD_REQUEST, 'Client disconnected'). The visible outcome depends on where the disconnect lands: either the response is assembled from the surviving entries with a gap, or the assert final_res is not None above fires for the index that never completed.

The check has to scan the list rather than test a single value, which is the one structural difference from your fix in #4782.

Thanks for the offer to review. I have not added a test here, since the natural place for one is the harness you built in #4782 and duplicating the fake engine setup would be worse than extending it once yours lands. Happy to add a completions_v1 case on top of that afterwards, or to write a standalone one now if you would rather this PR be self-contained.

[lvhan028]

This PR conflicts with our ongoing api_server refactoring (#4797). Since that refactoring is currently a priority for the project and also already covers the same issue you addressed here, we've decided to close this PR for now to avoid duplication and merge conflicts.