fix: add WebSocket endpoint registration and message type tests (#20)#356
fix: add WebSocket endpoint registration and message type tests (#20)#356tac0turtle merged 7 commits intomainfrom
Conversation
Automatically adds newly opened issues to the EVStack Evolve project board (orgs/evstack/projects/7). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tests (#20) Critical fix: RegisterRPCFuncs does not register a WebSocket endpoint. ev-abci was missing /websocket — clients could not connect via WebSocket at all. Fix adds an explicit WebsocketManager registration in startRPC. Documentation added to startRPC explaining RFC 6455 §11.8 message type handling as delegated to CometBFT's wsConnection (gorilla/websocket). New pkg/rpc/server_test.go with 7 integration tests: - TestWebSocket_TextValidJSON: health request over WS returns success - TestWebSocket_TextInvalidJSON: malformed JSON yields parse error (-32700) - TestWebSocket_BinaryFrame: binary frames yield parse error, not crash - TestWebSocket_CloseFrame: normal close (1000) closes connection cleanly - TestWebSocket_PingPong: server-initiated pings received within timeout - TestWebSocket_LargeMessage: exceeding read limit yields CloseMessageTooBig - TestRPCServer_WebSocketEndpointRegistered: end-to-end /websocket works Note: client-initiated ping test uses server-initiated pings instead to avoid a pre-existing data race in CometBFT's ws_handler.go (writeRoutine sets SetPingHandler concurrent with readRoutine's first advanceFrame call). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
IsUnexpectedCloseError(err, 1009) returns true when the code is NOT 1009, so the OR with IsCloseError made the assertion pass for any close frame. Replace with IsCloseError(err, CloseMessageTooBig) alone to enforce the specific code 1009. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug preventing WebSocket clients from connecting to the RPC server by correctly registering the /websocket endpoint. It significantly enhances the robustness and reliability of WebSocket communication by introducing a suite of integration tests that validate handling of various message types, connection lifecycle, and error conditions. The changes ensure that the RPC server's WebSocket functionality aligns with expected behavior and RFC standards. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical bug where the WebSocket endpoint was not being registered, preventing clients from connecting. The fix in pkg/rpc/server.go is correct and follows the established pattern from CometBFT. The addition of extensive integration tests in pkg/rpc/server_test.go is excellent, providing comprehensive coverage for various WebSocket message types and scenarios, which significantly improves the robustness of the RPC server. The code quality is high, and the new documentation is clear and helpful. I have one minor suggestion to improve maintainability in the new test file.
go mod tidy moves gorilla/websocket from indirect to direct now that the WebSocket test code imports it explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without OnDisconnect, disconnected WebSocket clients leave dangling subscriptions on the EventBus. Pass the EventBus to RPCServer so the WebSocket manager can call UnsubscribeAll on disconnect, matching CometBFT's node.go pattern. Also set ReadLimit from RPC config. Closes #18 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that when a WebSocket client subscribes to events and then disconnects, the OnDisconnect callback fires and removes all subscriptions for that client from the EventBus. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Partially addresses #20. Fixes a missing
/websocketendpoint registration bug and adds integration tests documenting current message type behavior.Issue #20 asks: Different WebSocket message types (text, binary, close, ping, pong) should be handled properly per RFC 6455 section 11.8.
Critical bug found:
pkg/rpc/server.goonly calledrpcserver.RegisterRPCFuncs()which registers HTTP endpoints and a JSON-RPC/handler but does NOT register a/websocketendpoint. WebSocket clients could not connect at all. Fixed by addingNewWebsocketManager+mux.HandleFunc("/websocket", wm.WebsocketHandler)matching CometBFT'snode.gopattern.What this PR adds:
/websocketendpoint registration (bug fix — WS was non-functional without this)startRPCexplaining WebSocket handler setup and per-frame-type behaviorWhat remains for full #20 closure:
"error unmarshaling request"parse error instead of a clear "unsupported message type" response. Fixing this requires wrapping CometBFT'sWebsocketManagerwith a custom handler that inspects the message type fromNextReader()before JSON decoding, or contributing the fix upstreamOnDisconnectcallback for subscription cleanup on connection close is not wired upBug details
startRPC()only calledrpcserver.RegisterRPCFuncs()which registers HTTP endpoints and a JSON-RPC/handler, but does NOT register a/websocketendpoint. CometBFT's ownnode.goseparately creates aWebsocketManagerand registers it at/websocket. This was missing from ev-abci.Current message type behavior (documented, not all fixed)
IsCloseErrorSetPongHandlerRelated: #18, #17
Test plan
make testpasses with race detectionmake lintpasses with zero warnings/websocketendpoint verified functional🤖 Generated with Claude Code