fix(dashboard): stream the cascade relay on servers that advertise ASGI 2.3 - #118
Merged
Conversation
…GI 2.3 `POST /api/plugins/hermes-talk/cascade-tts` returned HTTP 200 and then zero bytes of PCM, followed by a ClientDisconnect in the log once the browser gave up. The dashboard's cloned voice degraded to text-only with no error on any surface. Starlette's StreamingResponse only trusts send() for disconnect signalling when the server advertises ASGI spec_version >= 2.4. Below that it races the body generator against a disconnect listener, and that listener calls receive() FIRST — taking the browser's http.request messages off the channel. The relay's own request.stream() then only ever saw http.disconnect, so it waited forever for text that had already arrived. Uvicorn advertises "2.3" for HTTP, hardcoded in both h11_impl.py and httptools_impl.py, so this was the branch every dashboard took. Only its websocket protocols say 2.4, which is why the websocket lanes never showed it. Verified in the installed venv: starlette 1.3.1, uvicorn 0.41.0. RelayResponse owns the request body channel and takes Starlette's own >= 2.4 path verbatim — the OSError mapping and the background callback included — so a server that really does advertise 2.4 behaves identically. A browser that vanishes mid-upload now raises ClientDisconnect into the feeder, handled as an abort on the same path as a stream that ends without its `done` line, rather than escaping as an error. The existing tests could not have caught this: they drive the route with a fake request and never touch an ASGI server, and Starlette's TestClient omits spec_version from the scope, so a test through it takes the same broken branch and HANGS instead of failing. The new coverage drives the real relay body over a real single-consumer ASGI channel — the property that matters, since the bug is two coroutines competing for one message — and asserts on the scope VALUE, never a version string. `starlette` becomes a dev/test dependency: it is where StreamingResponse and ClientDisconnect actually live, and this behavior is untestable in CI without the real class, which is how the deadlock shipped. The module still falls back to its own stub when starlette is absent, so the plugin installs with no web dependency of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 4, 2026
Merged
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.
The dashboard cascade relay deadlocks on the ASGI server Hermes actually ships with.
POST /api/plugins/hermes-talk/cascade-ttsreturns HTTP 200, zero bytes of PCM, then aClientDisconnectin the log when the browser gives up — so the dashboard's cloned voice degrades to text-only with no error on any surface.Root cause
Starlette's
StreamingResponse.__call__only trustssend()for disconnect signalling when the server advertises ASGIspec_version >= 2.4. Below that it runs a task group racingstream_responseagainstlisten_for_disconnect(receive)— and that listener callsreceive()first, taking the browser'shttp.requestmessages off the channel. The relay's ownrequest.stream()then only ever seeshttp.disconnect, so it waits forever for text that already arrived.Uvicorn advertises
"2.3"for HTTP. That is the branch every Hermes dashboard takes — not an edge case.The fix returns a
RelayResponsethat owns the request body channel, and handles a mid-uploadClientDisconnectin the feeder as an abort (same path as a stream that ends without itsdoneline) rather than letting it escape as an error.What I verified independently
Everything below I checked against the installed source rather than taking the writeup's word for it:
AppData\Local\hermes\hermes-agent\venv): starlette 1.3.1, uvicorn 0.41.0, fastapi 0.133.1, Python 3.11.13. CI resolves starlette 1.6.0 — I readStreamingResponse.__call__in both and they are byte-identical in the branch that matters, so the fix is not pinned to one starlette.starlette/responses.py:spec_version = tuple(...scope.get("asgi", {}).get("spec_version", "2.0")...),if spec_version >= (2, 4): ... else: <task group racing stream_response against listen_for_disconnect(receive)>. Andlisten_for_disconnectis a barewhile True: message = await receive().grep'd the installed package:"spec_version": "2.3"in bothprotocols/http/h11_impl.py:205andprotocols/http/httptools_impl.py:227. The contrast holds too:websockets_impl.py:186andwsproto_impl.py:174say"2.4", which is exactly why the websocket lanes never showed this."2.0"— which is what makes the TestClient warning real, and why I did not test through it.git diff --stat==git diff --ignore-all-space --stat.One place the patch note was incomplete, and it mattered. Its
RelayResponsewas:That silently drops two things starlette's own
>= 2.4path does: theOSError→ClientDisconnectmapping, and thebackgroundcallback. Neither is used by this route today, but a subclass that quietly diverges from its base is a trap for whoever adds a background task later. This PR takes starlette's>= 2.4branch verbatim instead, so a server that genuinely advertises 2.4 and this class behave identically — and keepssuper().__call__for the non-http (websocket denial) case.The test, and the mistake I made writing it
The instruction not to use
TestClientis right — it omitsspec_version, takes the same broken branch, and a test through it hangs rather than fails. But my first attempt at a fail-without-fix test was also wrong, and it is worth recording why:I drove the stock
StreamingResponsewith a trivial body generator and expected a timeout. It passed cleanly. A body generator that does not read the request body never deadlocks —stream_responsefinishes first and the task group cancels the listener. The race only bites when two coroutines compete for the same messages.So the test now drives the real
_cascade_pcm_stream, over a realstarlette.requests.Request, on a single-consumer ASGI channel where each message is delivered exactly once — which is the actual mechanism. The two halves are a differential on the same scope, the same channel, and the same relay body:test_the_stock_streaming_response_loses_the_upload_on_the_same_scope— stock response: body isb""and the cascade never dials, because no text reached it. The reported symptom exactly.test_the_relay_response_delivers_the_pcm_on_the_same_scope—RelayResponse: the PCM comes back and the socket saw[" ", "Hello there.", ""].Plus:
test_relay_streams_on_a_server_advertising_asgi_below_2_4— assertsreceive_calls == 0and a well-formedhttp.response.start→ body → terminal empty-body sequencetest_relay_streams_identically_when_the_server_advertises_2_4— the 2.4 scope behaves the sametest_the_route_returns_a_relay_response_not_a_bare_streaming_responsetest_a_client_that_vanishes_mid_upload_is_an_abort_not_an_error— no PCM, and no "malformed"/"unrecognized" receipt in the logEvery assertion is pinned to the scope VALUE, never a version string, so a future uvicorn advertising 2.4 leaves these meaningful rather than vacuous.
I confirmed the tests bite. Neutering
RelayResponse.__call__to fall through to the racing base fails exactly two —AssertionError: the relay must own the request body channelandassert b'' == b'\x07\x08...'— and passes the other 21.One judgement call:
starletteas a dev dependencyThe bug is starlette behavior, so testing it needs starlette. An
importorskipwould have skipped in CI — which is precisely how this shipped in the first place, so I did not do that.Instead I split the import shim into two tiers.
StreamingResponseandClientDisconnectare starlette's — fastapi only re-exports them — so importing them from their real home lets the dev extra installstarlettealone, without pulling a web framework into a plugin that has no web dependency.APIRouter/HTTPException/Requeststill come from fastapi or fall back to the existing stubs, so the rest oftest_dashboard_api.pyandtest_dashboard_cascade.pybehave exactly as before (verified: 52/52 still pass against the real starlette base). Runtime dependencies are unchanged — the module still falls back to its own stub when starlette is absent, and the plugin installs without it.Gates
uv run --extra dev pytest -q→ 1657 passed, 40 skipped, 5 xfailed, 0 failed.ruff check .clean on 0.16.5. As on #117, the 12 known-baseline failures (#93) did not reproduce in a freshuvvenv.— SmokeDev