fix: guard all ws.send() calls with readyState check to prevent server crashes#3936
Conversation
|
|
WalkthroughGameServer now checks a client's WebSocket ChangesWebSocket Readystate Guards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/server/GameServer.ts`:
- Around line 1004-1012: The review calls out remaining unguarded WebSocket
sends in joinClient and sendStartGameMsg (and the shown error-send/close block)
that can throw if the socket disconnects; update each place (joinClient,
sendStartGameMsg, and the error-send in GameServer where you check
client.ws.readyState) to 1) verify client.ws.readyState === WebSocket.OPEN
before calling ws.send, and 2) wrap ws.send in a try/catch to swallow/log send
errors so a transient disconnect doesn’t crash the flow, keeping the subsequent
ws.close call safe and preserving existing error logging semantics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa49ede5-50d4-4590-b2f7-70ae23acece7
📒 Files selected for processing (1)
src/server/GameServer.ts
| if (client.ws.readyState === WebSocket.OPEN) { | ||
| client.ws.send( | ||
| JSON.stringify({ | ||
| type: "error", | ||
| error: reasonKey, | ||
| } satisfies ServerErrorMessage), | ||
| ); | ||
| client.ws.close(1000, reasonKey); | ||
| } |
There was a problem hiding this comment.
PR objective is not fully met: two ws.send() calls are still unguarded
Nice fix here, but there are still unguarded sends at Line 211 (joinClient) and Line 786 (sendStartGameMsg). Those paths can still throw if the socket disconnects at the wrong time.
Suggested patch
@@
- client.ws.send(
- JSON.stringify({
- type: "error",
- error: "full-lobby",
- } satisfies ServerErrorMessage),
- );
+ if (client.ws.readyState === WebSocket.OPEN) {
+ client.ws.send(
+ JSON.stringify({
+ type: "error",
+ error: "full-lobby",
+ } satisfies ServerErrorMessage),
+ );
+ }
@@
- ws.send(
- JSON.stringify({
- type: "start",
- turns: this.turns.slice(lastTurn),
- gameStartInfo: this.gameStartInfo,
- lobbyCreatedAt: this.createdAt,
- myClientID: client.clientID,
- } satisfies ServerStartGameMessage),
- );
+ if (ws.readyState === WebSocket.OPEN) {
+ ws.send(
+ JSON.stringify({
+ type: "start",
+ turns: this.turns.slice(lastTurn),
+ gameStartInfo: this.gameStartInfo,
+ lobbyCreatedAt: this.createdAt,
+ myClientID: client.clientID,
+ } satisfies ServerStartGameMessage),
+ );
+ }🤖 Prompt for 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.
In `@src/server/GameServer.ts` around lines 1004 - 1012, The review calls out
remaining unguarded WebSocket sends in joinClient and sendStartGameMsg (and the
shown error-send/close block) that can throw if the socket disconnects; update
each place (joinClient, sendStartGameMsg, and the error-send in GameServer where
you check client.ws.readyState) to 1) verify client.ws.readyState ===
WebSocket.OPEN before calling ws.send, and 2) wrap ws.send in a try/catch to
swallow/log send errors so a transient disconnect doesn’t crash the flow,
keeping the subsequent ws.close call safe and preserving existing error logging
semantics.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
…r crashes Several ws.send() calls in GameServer were missing WebSocket.OPEN readyState guards. If a client disconnects between being iterated and the send() call, the unguarded send throws an exception that can crash the entire game server process, affecting all connected players. The broadcastLobbyInfo() method already had this guard correctly, confirming this was an oversight in the other methods. Affected methods: - endTurn(): called every ~100ms, highest crash risk - prestart(): called when transitioning lobby to game - kickClient(): crash during kick operation - handleSynchronization(): called every 10 turns for desync detection
berkelmali
force-pushed
the
fix/guard-ws-send-readystate
branch
from
201059f to
6619c5a
Compare
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
4 participants
Description:
Several ws.send() calls in GameServer.ts were missing WebSocket.OPEN readyState guards. This can lead to server crashes if a client disconnects precisely between a check and the send. Added guards to prestart, kickClient, and handleSynchronization.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires