feat: add admin award winner command #23

2026-03-06T01:24:49Z

remhelper commented

2026-03-06 01:24:49 +00:00

(Migrated from github.com)

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

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

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

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

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

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

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

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

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

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.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: joshuafhiggins/connect4-moderator-server#23