fix(websocket): log on_disconnect errors, gate on accept, correct protocol docs + tests (v26.06.19)#45
Merged
Merged
Conversation
…tocol docs + tests + bump v26.06.19 Surfaced by an audit while validating implement-websocket (skill clean: messages flow, broadcast, disconnect cleanup all proven). The websocket module was untested (322 lines). - adapter: on_disconnect cleanup errors were swallowed by contextlib.suppress(Exception) with no logging -> now logged (warning + traceback), matching the handler-error path. - adapter: on_disconnect ran unconditionally in finally even when the handler raised before accept() -> now gated on the new WebSocketSession.accepted flag. - handler: WebSocketHandler docstrings implied on_connect/on_message are auto-dispatched (they are not; only on_disconnect is) -> corrected to state they are caller-invoked. Implementing on_message expecting framework dispatch was a silent no-op. - decorators: documented that the controller instance is a singleton shared across all connections (per-connection state belongs on the WebSocketSession, not self). Added WebSocketSession.accepted + tests/websocket/ (5 tests; module was untested). Regression guards confirmed to fail without the adapter fix. Gates: mypy --strict (607), ruff + format, full suite 3725 passed.
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
1 participant
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.
Summary
An audit while validating the
implement-websocketskill (which validated clean — messages flow, broadcast, and disconnect cleanup all proven over a real WS test client + live server) found real quality issues in the previously-untestedpyfly.websocketmodule (322 lines, no tests).Fixed
on_disconnectcleanup errors were silently swallowed. The adapter wrapped the hook incontextlib.suppress(Exception)with no logging, so a failing cleanup (leaked resource/lock) vanished. Now logged (warning+ traceback), matching the handler-error path.on_disconnectfired even when the connection was never accepted (unconditionalfinally).WebSocketSessionnow tracksaccepted, and the adapter gates the hook on it — no spurious disconnect for a handshake that never completed.WebSocketHandlerdocstrings were misleading. They impliedon_connect/on_messageare auto-dispatched; they are not (onlyon_disconnectis — the@websocket_mappingmethod owns accept + the receive loop). Implementingon_messageexpecting framework dispatch was a silent no-op. Docstrings now state they're caller-invoked. (The skill already documented this correctly.)Added
WebSocketSession.acceptedproperty.tests/websocket/suite (5 tests) — message flow, disconnect cleanup, accept-gating + error-logging, theacceptedflag, and the on_message-not-auto-dispatched contract. The gate + logging tests fail without the adapter fix (verified).Notes
WebSocketSession, notself.Gates
mypy --strict(607) ✓ ·ruff+ruff format✓ · full suite 3725 passed, 1 skipped.Bumps
v26.06.18 → v26.06.19, CHANGELOG,uv.lock.