Skip to content

fix: prevent sendStartGameMsg from crashing server on client disconnect#3939

Merged
evanpelle merged 1 commit into
openfrontio:mainfrom
berkelmali:fix/sendstart-rethrow-crash
May 16, 2026
Merged

fix: prevent sendStartGameMsg from crashing server on client disconnect#3939
evanpelle merged 1 commit into
openfrontio:mainfrom
berkelmali:fix/sendstart-rethrow-crash

Conversation

@berkelmali
Copy link
Copy Markdown
Contributor

@berkelmali berkelmali commented May 16, 2026

The catch block in sendStartGameMsg() re-throws the error, which means a single client's WebSocket failure (e.g. disconnected during game start) propagates up and can crash the entire game server. The start() method calls sendStartGameMsg() in a forEach loop over all clients, so one bad client kills the game for everyone.

Changes:

  • Added readyState check before sending
  • Replaced re-throw with structured error logging
  • A single client failure now logs the error and continues gracefully

If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)

Description:

Describe the PR.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

barfires

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4f5885bb-9900-441b-ba00-78c25ddebbea

📥 Commits

Reviewing files that changed from the base of the PR and between c70d258 and aa6f844.

📒 Files selected for processing (1)
  • src/server/GameServer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/server/GameServer.ts

Walkthrough

The sendStartGameMsg method in GameServer.ts now verifies the WebSocket is OPEN before sending and returns early with a warning if not. Exceptions from ws.send are caught and logged with game and client identifiers instead of being thrown.

Changes

Game Messaging Robustness

Layer / File(s) Summary
WebSocket state validation and error handling
src/server/GameServer.ts
sendStartGameMsg checks ws.readyState and skips sending with a warning if not OPEN. The send catch block logs an error with game and client context and a normalized error string instead of re-throwing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🌐 A socket checks before it speaks,
If not OPEN, a warning peeks,
Errors caught, the server logs,
No thrown storms, just calmer bogs,
Game starts safe when connection's sleek.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main fix: preventing a server crash caused by client disconnections during the game start message sending process.
Description check ✅ Passed The description clearly explains the problem (re-throw causing server crash), identifies the specific scenario (forEach loop over clients), and lists the concrete changes made to fix it.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

The catch block in sendStartGameMsg() re-throws the error, which means
a single client's WebSocket failure (e.g. disconnected during game start)
propagates up and can crash the entire game server. The start() method
calls sendStartGameMsg() in a forEach loop over all clients, so one
bad client kills the game for everyone.

Changes:
- Added readyState check before sending
- Replaced re-throw with structured error logging
- A single client failure now logs the error and continues gracefully
@berkelmali berkelmali force-pushed the fix/sendstart-rethrow-crash branch from c70d258 to aa6f844 Compare May 16, 2026 14:17
@github-project-automation github-project-automation Bot moved this from Triage to Final Review in OpenFront Release Management May 16, 2026
@evanpelle evanpelle added this to the v32 milestone May 16, 2026
@evanpelle evanpelle merged commit 749f496 into openfrontio:main May 16, 2026
9 of 12 checks passed
@github-project-automation github-project-automation Bot moved this from Final Review to Complete in OpenFront Release Management May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

2 participants