fix(ws): tolerate a client that disconnects mid-broadcast - #3181
Merged
Conversation
AssetConsumer.asset_update did an unguarded await self.send(). A browser
can disconnect in the window between the group_send dispatch and this
send, so the ASGI server has already emitted 'websocket.close' and
channels raises RuntimeError("Unexpected ASGI message 'websocket.send',
after sending 'websocket.close'") — a disconnect-vs-broadcast race
surfacing as an unhandled error in Sentry (ANTHIAS-1K).
The nudge is best-effort (the client's 5s poll backs it up) and
group_discard runs in disconnect(), so the stale channel is already on
its way out. Swallow the RuntimeError and log at debug rather than let it
reach Sentry.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR hardens the WebSocket broadcast path by tolerating a client disconnect that occurs between a group_send dispatch and the consumer’s subsequent send(), preventing a benign race from surfacing as an unhandled runtime error.
Changes:
- Wrap
AssetConsumer.asset_update()’ssend()in atry/except RuntimeErrorand drop the best-effort nudge on send-after-close. - Add unit tests covering the happy path and the send-after-close RuntimeError swallow behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/anthias_server/app/consumers.py |
Swallow RuntimeError from send() during mid-broadcast disconnects and log at debug. |
tests/test_consumers.py |
Adds tests for forwarding asset_id and for suppressing send-after-close errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review: logging the swallowed RuntimeError without detail made the expected send-after-close race indistinguishable from an unexpected RuntimeError out of send(). Log the asset_id and exc_info at debug — same swallow behavior, but diagnostics are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Address Copilot re-review: the broad `except RuntimeError` swallowed every RuntimeError from send(), which contradicts the intent (tolerate only the disconnect race) and could hide genuine failures. Match the channels "Unexpected ASGI message 'websocket.send'" message and re-raise anything else. Add a regression test asserting an unrelated RuntimeError still propagates. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Address Copilot re-review: the substring check matched any RuntimeError
mentioning "Unexpected ASGI message 'websocket.send'", which could hide
an unrelated ASGI state bug. Require the close/completed clause too
('websocket.close' or 'response already completed') so only the genuine
send-after-close race is swallowed. Add a test that a websocket.send
RuntimeError without the close clause still propagates.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The direct `consumer.send = AsyncMock()` assignments tripped mypy's method-assign check (run-mypy CI failure). Patch the instance attribute with mock.patch.object instead — same behaviour, restores cleanly, and no assignment to a method-typed attribute. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
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.
What
AssetConsumer.asset_updatedid an unguardedawait self.send(). A browser can disconnect in the window between thegroup_senddispatch and this send, so the ASGI server has already emittedwebsocket.closeand channels raisesRuntimeError("Unexpected ASGI message 'websocket.send', after sending 'websocket.close'").Why
A benign disconnect-vs-broadcast race that surfaces as an unhandled error in Sentry (ANTHIAS-1K, seen on pi5). The WS frame is only a best-effort "something changed" nudge — the client's 5s poll keeps the table consistent — and
group_discardruns indisconnect(), so the stale channel is already on its way out.How
Wrap the send in
try/except RuntimeError, log atdebug, and drop the nudge. Scoped toRuntimeErrorso a genuine failure elsewhere still propagates.Tests
New
tests/test_consumers.py: the happy path forwards theasset_idframe; a send-after-closeRuntimeErroris swallowed without propagating.🤖 Generated with Claude Code