Punish the host with a matchmaker violation if game setup times out

[BlackYps]

The host might be responsible for other players failing to connect to the game. The host will now receive a matchmaker violation every time the game setup times out. This might punish innocent hosts, but to prevent abuse it is important to acknowledge the possible contribution of the host as well.

Summary by CodeRabbit

• Bug Fixes
  • Improved match launch timeout handling by ensuring all players, including the host, are properly detected as disconnected when a match fails to launch within the timeout window. This enables more accurate failure handling and reporting.

[coderabbitai]

Walkthrough

The launch_match function in the ladder service now includes the host when raising a NotConnectedError for game launch timeouts. Previously, only guests were tracked as potentially disconnected; the host is now included in the set of players deemed not connected.

Changes

Cohort / File(s)	Change summary
Game launch timeout handling server/ladder_service/ladder_service.py	Modified launch_match timeout logic to include the host alongside previously-identified guests in the NotConnectedError exception when game launch fails to complete

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A host once hid from the game's cold stare,
But now in the timeout log, the host shall declare!
Connected or not, all must confess,
Guest and host alike—no more duress! 🎮

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately reflects the main change: when game setup times out, the host is now included in the disconnected players set, triggering a matchmaker violation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch host-violation

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

server/ladder_service/ladder_service.py (1)

686-689: Consider using explicit exception chaining.

For clarity, use raise ... from None to explicitly indicate this is a new exception, not chained from the caught TimeoutError.

         except asyncio.TimeoutError:
             connected_players = game.get_connected_players()
             # The host is included here, because he might be responsible for
             # a guest being unable to connect, e.g. because they didn't
             # receive his connect message.
             raise NotConnectedError([
                 player for player in guests
                 if player not in connected_players
-            ] + [host])
+            ] + [host]) from None

Based on static analysis hints.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6cca9f and 6c53a9a.

📒 Files selected for processing (1)

• server/ladder_service/ladder_service.py (1 hunks)

🧰 Additional context used

🪛 Ruff (0.14.3)

server/ladder_service/ladder_service.py

686-689: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: unit-test

🔇 Additional comments (1)

server/ladder_service/ladder_service.py (1)

683-689: The review comment is based on incomplete understanding of the code history.

The unconditional host penalty is not a new issue introduced by this change—it existed in the previous version. Before this commit, the code was raise NotConnectedError([host]), penalizing only the host on timeout. This commit actually refined the logic by conditionally penalizing guests who failed to connect while maintaining the intentional host penalty policy.

The unconditional host penalty represents an explicit design decision stated in the commit message: "Punish the host with a matchmaker violation if game setup times out." The code works as designed. While the review comment raises a valid fairness concern about false positives, it proposes changing an established policy rather than fixing a defect.

Likely an incorrect or invalid review comment.

[Gatsik]

Here's an idea: punish everyone, because everybody can be the cause. This might punish innocent players, but will 100% punish guilty ones.

[BlackYps]

Are you unhappy with the original proposal?

[Gatsik]

Maybe I'm somewhat unhappy, there are already some weird 'innocent' bans when players seemed to be connected, and this will just add more to that

Like at least checking the number of connected players before including host would somewhat reduce false positives -- if no one else were able to connect (or only few were able) then it is more likely that host is to blame

[BlackYps]

I coded this because there was suspicion that some players would maliciously send incorrect packets to other players if they are the host, preventing the game from starting. In this case, checking if the majority of players are connected would not help, because they could send invalid packets to just one player.
Luckily, it turned out that this wasn't actually the case. So it seems this functionality is not needed at the moment. We can add it at a later date, if we ever suspect again that players are abusing the host position.