Skip to content

[Fix] completions_v1: abort session and return 400 on client disconnect#4777

Open
AmirF194 wants to merge 1 commit into
InternLM:mainfrom
AmirF194:fix/completions-v1-disconnect-abort
Open

[Fix] completions_v1: abort session and return 400 on client disconnect#4777
AmirF194 wants to merge 1 commit into
InternLM:mainfrom
AmirF194:fix/completions-v1-disconnect-abort

Conversation

@AmirF194

Copy link
Copy Markdown

Fixes #4776

Root cause

completions_v1's non-streaming path (_inner_call, the disconnect branch) calls VariableInterface.async_engine.stop_session(request.session_id). AsyncEngine has never had a stop_session method (checked by enumerating every method on the class), so this raises AttributeError on every client disconnect instead of aborting the session. The three sibling disconnect handlers in this file (chat_completions_v1, /generate's non-streaming branch, /end_session) all correctly call session.async_abort(). git log -G shows this call site was added by #4655 (client-disconnect session-leak fix for the PyTorch MP engine), reusing a pre-refactor pattern from before #4253 introduced Session/SessionManager; not a revert of a prior attempt.

Fixing only that call is not sufficient: _inner_call's return create_error_response(...) is the return value of a coroutine handed to asyncio.gather(...), and asyncio.gather's result is discarded. A disconnected generator's choices[i] slot is left None, and building CompletionResponse from a choices list containing None raises a pydantic ValidationError, so the client still never gets the intended 400, just a different crash.

Fix

  • Route the disconnect abort through session.async_abort(), matching the sibling handlers.
  • Collect asyncio.gather's per-generator results and return the first non-None one (each _inner_call already returns the error response on disconnect and implicitly returns None otherwise), so the 400 actually reaches the caller.

Verification

Docker (python:3.11-slim, CPU torch/transformers/xgrammar/etc., no GPU needed), current HEAD 2aae7d77. Imported the real, unmodified completions_v1, VariableInterface, CompletionRequest, SessionManager, GenOut and drove the endpoint directly with a fake AsyncEngine.generate() yielding one item and raw_request.is_disconnected() returning True:

  • On unpatched main: AttributeError: 'FakeAsyncEngine' object has no attribute 'stop_session', raised from the exact line in the issue.
  • On this branch: returns a clean JSONResponse(400, {"message": "Client disconnected", ...}), and the active request handle's async_cancel is actually invoked.
  • The success (no-disconnect) path still returns the expected choices/usage payload, unaffected by the gather-result change.

Added two regression tests to tests/test_lmdeploy/serve/test_session_cleanup.py, mirroring that file's existing fake-engine style:

  • test_completions_v1_disconnect_aborts_active_handle: asserts the session's active handle is cancelled on disconnect (pins the stop_session -> async_abort fix).
  • test_completions_v1_disconnect_returns_client_disconnected_response: asserts the endpoint returns the 400 JSONResponse, not a crash (pins the gather-result-propagation fix).

Both fail on unpatched main (AttributeError) and pass on this branch. Ran the full tests/test_lmdeploy/serve/test_session_cleanup.py module: 11 passed, 0 failed, no regressions in the 9 pre-existing tests.

Not verified: no live end-to-end HTTP request through a running uvicorn server or a real inference backend; the fake AsyncEngine/raw_request drive the actual production completions_v1 function directly, not through FastAPI's request-handling layer.

ruff check clean on both changed files; pylint shows no new messages in the changed line ranges. No CHANGELOG entry found in this repo's convention for a fix of this size.

Fixes InternLM#4776

The disconnect branch in completions_v1's _inner_call called
VariableInterface.async_engine.stop_session(request.session_id), a method
AsyncEngine has never had, raising AttributeError on every disconnect
instead of aborting the session. Route it through session.async_abort(),
matching the three sibling disconnect handlers in this file.

Fixing that call alone was not enough: _inner_call's return value (the
400 error response) is discarded by asyncio.gather, so a disconnected
generator leaves its choices[] slot as None and CompletionResponse
construction raises a pydantic ValidationError instead of returning the
400. Collect gather's results and return the first non-None one.

Added two regression tests to test_session_cleanup.py: one asserting the
active request handle is actually cancelled on disconnect, one asserting
the endpoint returns the client-disconnected 400 rather than crashing.
Both fail on unpatched main and pass with this fix.
Copilot AI review requested due to automatic review settings July 23, 2026 08:19

Copilot AI 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.

Pull request overview

Fixes the legacy /v1/completions non-streaming disconnect path so it correctly aborts the active session and returns an HTTP 400 response instead of crashing.

Changes:

  • Replace the nonexistent AsyncEngine.stop_session(...) call with session.async_abort() in the disconnect handler.
  • Propagate disconnect error responses out of the per-generator asyncio.gather(...) by returning the first non-None inner result.
  • Add regression tests covering both session-handle cancellation and the returned 400 JSONResponse on disconnect.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lmdeploy/serve/openai/api_server.py Fix disconnect abort implementation and ensure the intended 400 response is actually returned (not lost inside gather).
tests/test_lmdeploy/serve/test_session_cleanup.py Add regression tests for /v1/completions disconnect behavior (abort + 400 response).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] legacy /v1/completions disconnect-abort calls nonexistent AsyncEngine.stop_session

2 participants