[Fix] completions_v1: abort session and return 400 on client disconnect#4777
Open
AmirF194 wants to merge 1 commit into
Open
[Fix] completions_v1: abort session and return 400 on client disconnect#4777AmirF194 wants to merge 1 commit into
AmirF194 wants to merge 1 commit into
Conversation
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.
Contributor
There was a problem hiding this comment.
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 withsession.async_abort()in the disconnect handler. - Propagate disconnect error responses out of the per-generator
asyncio.gather(...)by returning the first non-Noneinner result. - Add regression tests covering both session-handle cancellation and the returned
400 JSONResponseon 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4776
Root cause
completions_v1's non-streaming path (_inner_call, the disconnect branch) callsVariableInterface.async_engine.stop_session(request.session_id).AsyncEnginehas never had astop_sessionmethod (checked by enumerating every method on the class), so this raisesAttributeErroron 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 callsession.async_abort().git log -Gshows 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 introducedSession/SessionManager; not a revert of a prior attempt.Fixing only that call is not sufficient:
_inner_call'sreturn create_error_response(...)is the return value of a coroutine handed toasyncio.gather(...), andasyncio.gather's result is discarded. A disconnected generator'schoices[i]slot is leftNone, and buildingCompletionResponsefrom achoiceslist containingNoneraises a pydanticValidationError, so the client still never gets the intended400, just a different crash.Fix
session.async_abort(), matching the sibling handlers.asyncio.gather's per-generator results and return the first non-Noneone (each_inner_callalready returns the error response on disconnect and implicitly returnsNoneotherwise), so the400actually reaches the caller.Verification
Docker (
python:3.11-slim, CPUtorch/transformers/xgrammar/etc., no GPU needed), current HEAD2aae7d77. Imported the real, unmodifiedcompletions_v1,VariableInterface,CompletionRequest,SessionManager,GenOutand drove the endpoint directly with a fakeAsyncEngine.generate()yielding one item andraw_request.is_disconnected()returningTrue:AttributeError: 'FakeAsyncEngine' object has no attribute 'stop_session', raised from the exact line in the issue.JSONResponse(400, {"message": "Client disconnected", ...}), and the active request handle'sasync_cancelis actually invoked.choices/usagepayload, unaffected by thegather-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 thestop_session->async_abortfix).test_completions_v1_disconnect_returns_client_disconnected_response: asserts the endpoint returns the400JSONResponse, not a crash (pins thegather-result-propagation fix).Both fail on unpatched main (
AttributeError) and pass on this branch. Ran the fulltests/test_lmdeploy/serve/test_session_cleanup.pymodule: 11 passed, 0 failed, no regressions in the 9 pre-existing tests.Not verified: no live end-to-end HTTP request through a running
uvicornserver or a real inference backend; the fakeAsyncEngine/raw_requestdrive the actual productioncompletions_v1function directly, not through FastAPI's request-handling layer.ruff checkclean on both changed files;pylintshows no new messages in the changed line ranges. No CHANGELOG entry found in this repo's convention for a fix of this size.