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 +76 -104
2 changed files with 76 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a4e5e25f0c - Show all commits

4
src/main.rs
View File

@@ -173,9 +173,9 @@ async fn handle_connection(
Ok(match_id) => {
let winner = parts[3].to_string();
if let Err(e) =
sd.handle_game_award(addr, match_id, winner).await
sd.handle_game_award_winner(addr, match_id, winner).await
{
error!("handle_game_award: {}", e);
error!("handle_game_award_winner: {}", e);
let _ = send(&tx, e.to_string().as_str());
}
}

176
src/server.rs
View File

@@ -1,3 +1,4 @@
use anyhow::anyhow;
use std::time::Instant;
use crate::{tournaments::*, types::*, *};
@@ -818,133 +819,104 @@ impl Server {
Ok(())
copilot-pull-request-reviewer[bot] commented 2026-03-06 05:28:32 +00:00 (Migrated from github.com)
Review
Copy Link

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); ```
}
pub async fn handle_game_award(
pub async fn handle_game_award_winner(
&self,
addr: SocketAddr,
match_id: u32,
winner_username: String,
) -> Result<(), anyhow::Error> {
if !self.auth_check(addr).await {
return Err(anyhow::anyhow!("ERROR:INVALID:AUTH"));
return Err(anyhow!("ERROR:INVALID:AUTH"));
}
let server_player_addr: SocketAddr = SERVER_PLAYER_ADDR.to_string().parse()?;
let matches_guard = self.matches.read().await;
let found_match =
matches_guard.get(&match_id).ok_or_else(|| anyhow!("ERROR:INVALID:AWARD"))?.clone();
drop(matches_guard);
let (player1_addr, player2_addr, viewers, demo_mode) = {
let mut matches_guard = self.matches.write().await;
let the_match = matches_guard
.get(&match_id)
.ok_or_else(|| anyhow::anyhow!("ERROR:INVALID:AWARD"))?
.clone();
let mut the_match = the_match.write().await;
let the_match = found_match.read().await;
if winner_username != the_match.player1.to_string()
&& winner_username != the_match.player2.to_string()
{
return Err(anyhow!("ERROR:INVALID:AWARD"));
}
if let Some(wait_thread) = &the_match.wait_thread {
wait_thread.abort();
}
self.matches.write().await.remove(&match_id);
if let Some(timeout_thread) = &the_match.timeout_thread {
timeout_thread.abort();
}
if let Some(wait_thread) = &the_match.wait_thread {
wait_thread.abort();
}
let player1_addr = the_match.player1;
let player2_addr = the_match.player2;
let viewers = the_match.viewers.clone();
let demo_mode = the_match.demo_mode;
if let Some(timeout_thread) = &the_match.timeout_thread {
timeout_thread.abort();
}
matches_guard.remove(&match_id);
(player1_addr, player2_addr, viewers, demo_mode)
};
self.broadcast_message(&the_match.viewers, &format!("GAME:WIN:{}", winner_username)).await;
let clients_guard = self.clients.read().await;
let player1_name = if player1_addr == server_player_addr {
SERVER_PLAYER_USERNAME.to_string()
if the_match.demo_mode {
let player_win = if winner_username != SERVER_PLAYER_USERNAME {
"WINS"
} else {
"LOSS"
};
let mut the_player = if the_match.player1 != SERVER_PLAYER_ADDR.parse()? {
copilot-pull-request-reviewer[bot] commented 2026-03-06 05:28:33 +00:00 (Migrated from github.com)
Review
Copy Link

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.
clients_guard.get(&the_match.player1).unwrap().write().await
} else {
clients_guard.get(&the_match.player2).unwrap().write().await
};
let _ = send(&the_player.connection, &format!("GAME:{}", player_win));
let _ = send(&the_player.connection, "TOURNAMENT:END");
the_player.color = Color::None;
the_player.current_match = None;
return Ok(());
}
let mut player1 = clients_guard.get(&the_match.player1).unwrap().write().await;
let mut player2 = clients_guard.get(&the_match.player2).unwrap().write().await;
player1.current_match = None;
player1.color = Color::None;
player2.current_match = None;
player2.color = Color::None;
let winner_tx = if player1.username == winner_username {
player1.connection.clone()
} else {
clients_guard
.get(&player1_addr)
.ok_or_else(|| anyhow::anyhow!("ERROR:INVALID:AWARD"))?
.read()
.await
.username
.clone()
player2.connection.clone()
};
let player2_name = if player2_addr == server_player_addr {
SERVER_PLAYER_USERNAME.to_string()
let loser_tx = if player1.username != winner_username {
player1.connection.clone()
} else {
clients_guard
.get(&player2_addr)
.ok_or_else(|| anyhow::anyhow!("ERROR:INVALID:AWARD"))?
.read()
.await
.username
.clone()
player2.connection.clone()
};
copilot-pull-request-reviewer[bot] commented 2026-03-06 05:28:33 +00:00 (Migrated from github.com)
Review
Copy Link

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.
let winner_addr = if player1.username == winner_username {
player1.addr.clone()
} else {
player2.addr.clone()
};
drop(player1);
drop(player2);
drop(clients_guard);
let winner_username = winner_username.trim().to_string();
let winner_is_player1 = winner_username == player1_name;
let winner_is_player2 = winner_username == player2_name;
let _ = send(&winner_tx, "GAME:WINS");
let _ = send(&loser_tx, "GAME:LOSS");
if !winner_is_player1 && !winner_is_player2 {
return Err(anyhow::anyhow!("ERROR:INVALID:AWARD"));
}
let winner_addr = if winner_is_player1 {
player1_addr
} else {
player2_addr
};
let loser_addr = if winner_is_player1 {
player2_addr
} else {
player1_addr
};
self.broadcast_message(&viewers, &format!("GAME:WIN:{}", winner_username))
.await;
let mut clients_guard = self.clients.write().await;
if winner_addr != server_player_addr {
let mut winner = clients_guard
.get_mut(&winner_addr)
.ok_or_else(|| anyhow::anyhow!("ERROR:INVALID:AWARD"))?
.write()
.await;
let _ = send(&winner.connection, "GAME:WINS");
winner.current_match = None;
winner.color = Color::None;
}
if loser_addr != server_player_addr {
let mut loser = clients_guard
.get_mut(&loser_addr)
.ok_or_else(|| anyhow::anyhow!("ERROR:INVALID:AWARD"))?
.write()
.await;
let _ = send(&loser.connection, "GAME:LOSS");
loser.current_match = None;
loser.color = Color::None;
}
drop(clients_guard);
if self.tournament.read().await.is_some() && self.matches.read().await.is_empty() {
if self.tournament.read().await.is_some() {
let mut tournament_guard = self.tournament.write().await;
let tourney = tournament_guard.as_mut().unwrap();
tourney.write().await.inform_winner(winner_addr, false);
tourney.write().await.next(&self).await;
if tourney.read().await.is_completed() {
*tournament_guard = None;
}
} else if !demo_mode && self.tournament.read().await.is_none() {
let clients_guard = self.clients.read().await;
if winner_addr != server_player_addr {
if let Some(winner) = clients_guard.get(&winner_addr) {
let _ = send(&winner.read().await.connection, "TOURNAMENT:END");
}
}
if loser_addr != server_player_addr {
if let Some(loser) = clients_guard.get(&loser_addr) {
let _ = send(&loser.read().await.connection, "TOURNAMENT:END");
if self.matches.read().await.is_empty() {
tourney.write().await.next(&self).await;
if tourney.read().await.is_completed() {
*tournament_guard = None;
}
}
}