Repository navigation
A reply cut off by a client disconnect is stored empty #2039
Description
The bug
Found while writing the test for #1946. When the client hangs up mid-reply (closing the tab, Stop, a dropped connection), the assistant turn is left with empty text and no completed_at, although the answer had already streamed. The comment in stream() says the opposite: "finally still runs on a client disconnect ... so a partial reply is never lost".
Why
Under uvicorn (ASGI spec 2.3), Starlette's StreamingResponse watches for http.disconnect and cancels the response's anyio task group. That cancellation is level-triggered: every await inside the cancelled scope raises again. stream()'s finally awaits transact(...), which runs through run_in_threadpool, so it is cancelled before _complete (or _discard_turn, or the deferred _rename) runs.
_release_model_after() does release the model today, but only because release_use takes an uncontended asyncio.Lock, which does not yield. A contended lock at that moment would leave the model marked in use.
How to reproduce
The test in the #1946 PR runs the app behind a real uvicorn socket, lets the stub llama-server send the answer and then hold the stream open, and closes the client. test_a_reply_the_client_hangs_up_on_keeps_its_text is marked xfail(strict=True) pointing here; it passes once this is fixed, which will fail the run until the marker is removed.
Stored afterwards:
{'role': 'assistant', 'content': {'text': '', 'citations': []}, 'completed_at': None}
Likely fix
Shield the cleanup from the cancellation, e.g. wrap the body of stream()'s finally (and the release in _release_model_after()) in anyio.CancelScope(shield=True). Checked locally: with the finally shielded, the partial reply is stored with its citations and the xfail test passes.
Activity
test(local): cover a chat reply the client hangs up on
MODSetter commentedon Sep 29, 2026
This now has a test waiting for it on
dev. #2041 is merged, and it characterises this bug rather than fixing it:tests/integration/chat/test_chat.py::test_a_reply_the_client_hangs_up_on_keeps_its_textis markedxfail(strict=False)with this issue as its reason. It asserts what the code's own comment promises — that a partial reply survives a hang-up — and currently fails, storingtext: ''with nocompleted_at. Whoever fixes this deletes thexfailmarker and the test goes green; no new test is needed.The
live_urlfixture intests/integration/chat/conftest.pyis what made this reachable. The in-process transport buffers a response to its end and never reports a disconnect, so before #2041 there was no way to exercise the path at all.stall_after_answer()holds the stub's stream open so a client can hang up mid-reply.The mechanism, from #2041's Known-gaps entry in
docs/architecture/chat.md:That last sentence is the part worth keeping in mind: the in-use mark survives by luck, not design, because releasing it needs no await on a contended lock.
test_a_reply_the_client_hangs_up_on_frees_the_modelpasses unconditionally and pins that, so a fix here must not regress it.The
xfailis deliberately not strict: the failure is a race between the cancellation and the save, andaddoptscarries-x, so an XPASS on a slow runner would stop the whole suite rather than just reporting. If you fix this, drop the marker rather than flipping it to strict.Still open and claimable.
fukalous commentedon Sep 30, 2026
I am taking this. The local fix shields both turn settlement and model-use release from Starlette disconnect cancellation, removes the existing xfail, and updates the chat architecture note. The focused chat suite and full integration suite pass locally. The implementation was AI-assisted and the diff was reviewed and approved by a human before submission.
MODSetter commentedon Sep 30, 2026
Fixed by #2062, merged into
dev, by @fukalous.The
finallyblock instream()andrelease_use()in_release_model_afterboth run insideanyio.CancelScope(shield=True)now, so Starlette cancelling the response task onhttp.disconnectno longer kills the save. Both cleanup paths were fixed, not just the one this issue named: the in-use release previously survived only because its lock happened to be uncontended. Thexfailadded in #2041 is gone and the test passes outright.The Known gaps line this issue quoted is deleted in the same PR. Closing by hand: GitHub only auto-closes on a merge to the default branch, and we merge through
dev.