fix(ws_highway): swallow a mid-stream WebSocketDisconnect quietly - #845
Merged
Conversation
Pre-existing (byte-identical to origin/main), flagged by CodeRabbit on #844: the outer try's only guard was `except Exception as e: log.exception("highway_ws unhandled error")`. The inner `except WebSocketDisconnect` covers ONLY the post-`ready` keep-alive loop, and the drum_tab/notation sends have their own localized guards — but the ~13 other streamed sends (beats, sections, notes, chords, anchors, …) fall through to the blanket handler. Since WebSocketDisconnect is an Exception subclass, a routine tab-close mid-load was logged as an error at whichever send was in flight. Fix: a dedicated `except WebSocketDisconnect: return` before the blanket handler, matching the two localized guards and the lib/ coding guideline. tests/test_ws_highway_disconnect.py drives highway_ws with a fake websocket that raises WebSocketDisconnect on the first streamed send (`loading`), over a real minimal sloppak. Negative-checked: removing the guard makes it fail (the disconnect is logged as `highway_ws unhandled error`); the fix passes. Uses a raw handler on `feedBack.server` rather than caplog, since configure_logging() reroutes that logger through structlog where caplog doesn't observe it. Full suite green. Closes the review thread on #844. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe highway WebSocket handler now returns immediately on client disconnect during streaming. A regression test simulates the disconnect in a minimal runtime environment and verifies that no unhandled highway WebSocket error is logged. ChangesWebSocket disconnect handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_ws_highway_disconnect.py`:
- Around line 37-48: Update _DisconnectingWS.send_json to raise
WebSocketDisconnect only on its first invocation and allow subsequent calls to
complete, then change the related assertion to require exactly one send so
handlers that continue streaming after the disconnect fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8debcbe4-826d-4779-8947-328a6174b47f
📒 Files selected for processing (2)
lib/routers/ws_highway.pytests/test_ws_highway_disconnect.py
…e send) — CodeRabbit The stub now raises only on the first send and the test asserts ws.sends == 1, so a handler that caught the disconnect and kept streaming would fail. Still negative-checked: removing the guard fails the test.
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.
Follow-up to #844, fixing the disconnect-logging issue CodeRabbit flagged there. Pre-existing (byte-identical to
origin/main), so it correctly wasn't touched in the verbatim move.The issue
The outer
tryinhighway_wshad onlyexcept Exception as e: log.exception("highway_ws unhandled error"). The innerexcept WebSocketDisconnectcovers only the post-readykeep-alive loop, anddrum_tab/notationhave localized guards — but the ~13 other streamed sends (beats,sections,notes,chords,anchors, …) fall through to the blanket handler. SinceWebSocketDisconnectis anExceptionsubclass, a routine tab-close mid-load was logged as an error.Fix
A dedicated
except WebSocketDisconnect: returnbefore the blanket handler — matching the two localized guards and thelib/**coding guideline ("Handle WebSocket disconnects with try/except WebSocketDisconnect").Test
tests/test_ws_highway_disconnect.pydriveshighway_wswith a fake websocket that raisesWebSocketDisconnecton the first streamed send (loading), over a real minimal sloppak. Negative-checked: removing the guard fails the test (disconnect logged ashighway_ws unhandled error); the fix passes. It uses a raw handler onfeedBack.serverrather thancaplog, becauseconfigure_logging()reroutes that logger through structlog wherecaplogdoesn't observe it.Full suite 2401 passed; Codex 0 findings. Closes the review thread on #844.
🤖 Generated with Claude Code
Summary by CodeRabbit