joshuafhiggins/connect4-moderator-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Releases 2 Activity

feat: add admin award winner command #23

Merged
remhelper merged 4 commits from feature/admin-award-winner into main 2026-03-06 05:49:50 +00:00
Conversation 2 Commits 4 Files Changed 2
+137
remhelper commented 2026-03-06 01:24:49 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Summary

  • add GAME:AWARD command for admins to award a winner
  • award winner updates match state, notifies players/observers, and advances tournaments

Testing

  • not run (not requested)

Resolves #22

## Summary - add GAME:AWARD command for admins to award a winner - award winner updates match state, notifies players/observers, and advances tournaments ## Testing - not run (not requested) Resolves #22
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2026-03-06 04:26:22 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy Link
Copy Source

Pull request overview

Adds an admin-only GAME:AWARD command path to forcibly award a winner for a given match, updating match state, notifying participants/viewers, and advancing tournaments when applicable.

Changes:

  • Add Server::handle_game_award to resolve a match by admin command and propagate win/loss notifications.
  • Wire GAME:AWARD <match_id> <winner_username> command parsing to call the new server handler.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/server.rs Implements match-award flow: validates admin, resolves winner, notifies players/viewers, updates tournament progression.
src/main.rs Adds command routing for GAME:AWARD to invoke the server handler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

## Pull request overview Adds an admin-only `GAME:AWARD` command path to forcibly award a winner for a given match, updating match state, notifying participants/viewers, and advancing tournaments when applicable. **Changes:** - Add `Server::handle_game_award` to resolve a match by admin command and propagate win/loss notifications. - Wire `GAME:AWARD <match_id> <winner_username>` command parsing to call the new server handler. ### Reviewed changes Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments. | File | Description | | ---- | ----------- | | `src/server.rs` | Implements match-award flow: validates admin, resolves winner, notifies players/viewers, updates tournament progression. | | `src/main.rs` | Adds command routing for `GAME:AWARD` to invoke the server handler. | --- 💡 <a href="/joshuafhiggins/connect4-moderator-server/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.<br><br>You can also share your feedback on Copilot code review. [Take the survey](https://www.surveymonkey.com/r/XP6L3XJ).
src/server.rs
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 04:26:21 +00:00
Copy Link
Copy Source

The match is removed from self.matches before player current_match fields are cleared. Since handle_play does matches_guard.get(...).unwrap(), a player sending a move during this window can cause a server panic. To avoid this race, clear both players' current_match (and abort threads) while the match is still present, or make handle_play tolerate a missing match instead of unwrapping.

The match is removed from `self.matches` before player `current_match` fields are cleared. Since `handle_play` does `matches_guard.get(...).unwrap()`, a player sending a move during this window can cause a server panic. To avoid this race, clear both players' `current_match` (and abort threads) while the match is still present, or make `handle_play` tolerate a missing match instead of unwrapping.
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 04:26:21 +00:00
Copy Link
Copy Source

In the non-tournament path, this only sends TOURNAMENT:END when !demo_mode. However, normal match completion in handle_play sends TOURNAMENT:END even for demo games (to the human player). Awarding a demo match currently won’t notify the client that the match/tournament flow ended, which can leave the client state inconsistent. Align this behavior with the existing match-end logic (send TOURNAMENT:END to the human player even in demo mode, and only skip the server-opponent send).

        } else if self.tournament.read().await.is_none() {
In the non-tournament path, this only sends `TOURNAMENT:END` when `!demo_mode`. However, normal match completion in `handle_play` sends `TOURNAMENT:END` even for demo games (to the human player). Awarding a demo match currently won’t notify the client that the match/tournament flow ended, which can leave the client state inconsistent. Align this behavior with the existing match-end logic (send `TOURNAMENT:END` to the human player even in demo mode, and only skip the server-opponent send). ```suggestion } else if self.tournament.read().await.is_none() { ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 04:26:21 +00:00
Copy Link
Copy Source

matches_guard.remove(&match_id) happens before validating that winner_username matches one of the players (and before any later fallible lookups). If validation fails (or a client lookup fails), this will abort threads and drop the match even though the command returns an error. Consider deferring match removal/state teardown until after winner/participants are validated (or use a two-phase approach where you only remove once you know the award will succeed).
`matches_guard.remove(&match_id)` happens before validating that `winner_username` matches one of the players (and before any later fallible lookups). If validation fails (or a client lookup fails), this will abort threads and drop the match even though the command returns an error. Consider deferring match removal/state teardown until after winner/participants are validated (or use a two-phase approach where you only remove once you know the award will succeed). ```suggestion ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2026-03-06 05:28:33 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy Link
Copy Source

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

## Pull request overview Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments. --- 💡 <a href="/joshuafhiggins/connect4-moderator-server/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.<br><br>You can also share your feedback on Copilot code review. [Take the survey](https://www.surveymonkey.com/r/XP6L3XJ).
src/server.rs
@@ -818,6 +819,127 @@ impl Server {
Ok(())
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 05:28:32 +00:00
Copy Link
Copy Source

Winner validation is comparing winner_username (a username from the command) to the_match.player1/2.to_string() (socket addresses). This will reject valid usernames and/or accept address strings. Resolve winner identity via the usernames map or by reading the Client.username for player1/player2, then verify the winner belongs to this match.


        // Validate that the declared winner is actually one of the players in this match
        let clients_guard = self.clients.read().await;
        let player1_client = clients_guard.get(&the_match.player1);
        let player2_client = clients_guard.get(&the_match.player2);

        // If we cannot resolve both players, or the winner username doesn't match either, reject
        if let (Some(p1_arc), Some(p2_arc)) = (player1_client, player2_client) {
            let p1 = p1_arc.read().await;
            let p2 = p2_arc.read().await;

            if winner_username != p1.username && winner_username != p2.username {
                return Err(anyhow!("ERROR:INVALID:AWARD"));
            }
        } else {
            return Err(anyhow!("ERROR:INVALID:AWARD"));
        }
        drop(clients_guard);
Winner validation is comparing `winner_username` (a username from the command) to `the_match.player1/2.to_string()` (socket addresses). This will reject valid usernames and/or accept address strings. Resolve winner identity via the `usernames` map or by reading the `Client.username` for `player1/player2`, then verify the winner belongs to this match. ```suggestion // Validate that the declared winner is actually one of the players in this match let clients_guard = self.clients.read().await; let player1_client = clients_guard.get(&the_match.player1); let player2_client = clients_guard.get(&the_match.player2); // If we cannot resolve both players, or the winner username doesn't match either, reject if let (Some(p1_arc), Some(p2_arc)) = (player1_client, player2_client) { let p1 = p1_arc.read().await; let p2 = p2_arc.read().await; if winner_username != p1.username && winner_username != p2.username { return Err(anyhow!("ERROR:INVALID:AWARD")); } } else { return Err(anyhow!("ERROR:INVALID:AWARD")); } drop(clients_guard); ```
src/server.rs
@@ -821,0 +860,4 @@
wait_thread.abort();
}
if let Some(timeout_thread) = &the_match.timeout_thread {
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 05:28:33 +00:00
Copy Link
Copy Source

Potential deadlock: this function holds a read-lock on the match (found_match.read().await) and then acquires a write-lock on self.matches to remove the match. Elsewhere (e.g., handle_play) the lock order is matches.write() -> match.write(), which can deadlock with match.read() -> matches.write(). Reorder to avoid holding the match lock while taking self.matches.write(), e.g. remove/take the match entry from the map first (under matches.write()), drop the map lock, then operate on the match.

Potential deadlock: this function holds a read-lock on the match (`found_match.read().await`) and then acquires a write-lock on `self.matches` to remove the match. Elsewhere (e.g., `handle_play`) the lock order is `matches.write()` -> `match.write()`, which can deadlock with `match.read()` -> `matches.write()`. Reorder to avoid holding the match lock while taking `self.matches.write()`, e.g. remove/take the match entry from the map first (under `matches.write()`), drop the map lock, then operate on the match.
src/server.rs
@@ -821,0 +895,4 @@
player1.color = Color::None;
player2.current_match = None;
player2.color = Color::None;
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 05:28:33 +00:00
Copy Link
Copy Source

This uses multiple unwrap()s when fetching players from clients_guard (and similarly in the demo-mode branch). If a player disconnects before an admin awards the match, the client entry may already be removed (see handle_disconnect), causing a panic and leaving tournament/match cleanup incomplete. Prefer returning an ERROR:INVALID:AWARD (or terminating the match) when a client is missing, while still ensuring the match is removed and the tournament is advanced based on the stored SocketAddrs.

This uses multiple `unwrap()`s when fetching players from `clients_guard` (and similarly in the demo-mode branch). If a player disconnects before an admin awards the match, the client entry may already be removed (see `handle_disconnect`), causing a panic and leaving tournament/match cleanup incomplete. Prefer returning an `ERROR:INVALID:AWARD` (or terminating the match) when a client is missing, while still ensuring the match is removed and the tournament is advanced based on the stored `SocketAddr`s.
src/server.rs
@@ -821,1 +940,4 @@
Ok(())
}
pub async fn handle_tournament_start(
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-03-06 05:28:33 +00:00
Copy Link
Copy Source

For non-tournament matches, handle_play sends TOURNAMENT:END to both players when the match finishes, but handle_game_award_winner only does this in the demo-mode branch. Without this, clients may not transition back to the post-match state after an admin-awarded result. Consider mirroring handle_play behavior by sending TOURNAMENT:END to both players when self.tournament is None.

For non-tournament matches, `handle_play` sends `TOURNAMENT:END` to both players when the match finishes, but `handle_game_award_winner` only does this in the demo-mode branch. Without this, clients may not transition back to the post-match state after an admin-awarded result. Consider mirroring `handle_play` behavior by sending `TOURNAMENT:END` to both players when `self.tournament` is `None`.
Sign in to join this conversation.
Reviewers
No Reviewers
copilot-pull-request-reviewer[bot]
Labels
Clear labels
bug
Something isn't working
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 rust
Pull requests that update rust code
 wontfix
This will not be worked on
No labels
Milestone
No items
No Milestone
Assignees
Clear assignees
joshuafhiggins (Joshua Higgins)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: joshuafhiggins/connect4-moderator-server#23