feat(lobby): Swiss tournament organizer for lobby broker (#4612)#4615
feat(lobby): Swiss tournament organizer for lobby broker (#4612)#4615claytonlin1110 wants to merge 4 commits into
Conversation
…#4612) Implements lobby-only Swiss events with pairing, standings, and P2P self-reported results across lobby-broker, both shells, and the client. Bumps wire protocol to v12. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Swiss-pairing tournament organizer system, including a backend registry, protocol updates (bumping to version 12), and a React frontend for creating, joining, and managing tournaments. The review feedback highlights several critical issues that must be addressed: a backtracking bug in the pairing algorithm for odd-sized brackets, missing winner validation on unequal game wins, premature tournament expiration due to static creation timestamps, and multiple WebSocket connection leaks on component unmounts and RPC errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if players.len() == 1 { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
[HIGH] Backtracking bug for odd-sized brackets
When the pool of players has an odd length, backtrack_pair will eventually recurse down to a single remaining player. Because it returns None when players.len() == 1, the entire backtracking search fails and falls back to the greedy pairing strategy. Returning Some(acc.clone()) instead allows the remaining player to be floated correctly.
| if players.len() == 1 { | |
| return None; | |
| } | |
| if players.len() == 1 { | |
| return Some(acc.clone()); | |
| } |
| if result.player_a_wins == result.player_b_wins && result.winner_player_key.is_some() { | ||
| return Err("Draw results must not specify a winner".to_string()); | ||
| } |
There was a problem hiding this comment.
[HIGH] Missing winner validation on unequal game wins
validate_match_result currently only ensures that draw results do not specify a winner. It does not verify that the reported winner_player_key actually matches the player with more game wins when the game wins are unequal. This allows clients to self-report invalid match results (e.g., reporting 2-0 but marking the other player as the winner).
if result.player_a_wins == result.player_b_wins {
if result.winner_player_key.is_some() {
return Err("Draw results must not specify a winner".to_string());
}
} else {
let expected_winner = if result.player_a_wins > result.player_b_wins {
&pairing.player_a
} else {
player_b
};
if result.winner_player_key.as_ref() != Some(expected_winner) {
return Err("Winner must match the player with more game wins".to_string());
}
}| pub fn check_expired(&mut self, timeout_secs: u64, env: &impl BrokerEnv) -> Vec<String> { | ||
| let now = env.now_ms(); | ||
| let cutoff = now.saturating_sub(timeout_secs.saturating_mul(1000)); | ||
| let expired: Vec<String> = self | ||
| .tournaments | ||
| .iter() | ||
| .filter(|(_, t)| t.created_at < cutoff) | ||
| .map(|(code, _)| code.clone()) | ||
| .collect(); | ||
| for code in &expired { | ||
| self.tournaments.remove(code); | ||
| } | ||
| expired | ||
| } |
There was a problem hiding this comment.
[HIGH] Premature tournament expiration
Tournaments are reaped purely based on created_at < cutoff. Since created_at is never updated after registration, any active tournament that takes longer than timeout_secs to complete will be abruptly deleted mid-game. To fix this, update created_at (or introduce a last_activity_at field) on any write/mutation operation (like join_tournament, start_round, report_result) and use that for expiration checks.
| const client = await connect(); | ||
| if (!client || cancelled) return; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on unmount
If the connect() promise resolves after the component has unmounted (or after dependencies change), cancelled will be true. However, the newly opened client is never closed, leading to a leaked WebSocket connection. Ensure the client is closed if the effect has been cancelled.
const client = await connect();
if (!client) return;
if (cancelled) {
client.close();
return;
}
| const handleCreate = async () => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const client = await connect(); | ||
| if (!client) return; | ||
| const created = await client.createTournament({ | ||
| name: name.trim(), | ||
| displayName: displayName.trim(), | ||
| totalRounds: rounds, | ||
| }); | ||
| client.close(); | ||
| navigate(`/tournament/${created.tournamentCode}`, { | ||
| state: { playerKey: created.playerKey, organizer: true }, | ||
| }); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.createTournament throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed creation attempt. Declare client outside the try block and close it in a finally block.
const handleCreate = async () => {
setLoading(true);
setError(null);
let client;
try {
client = await connect();
if (!client) return;
const created = await client.createTournament({
name: name.trim(),
displayName: displayName.trim(),
totalRounds: rounds,
});
navigate(`/tournament/${created.tournamentCode}`, {
state: { playerKey: created.playerKey, organizer: true },
});
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
client?.close();
setLoading(false);
}
};
| const handleJoin = async () => { | ||
| if (!code) return; | ||
| setJoining(true); | ||
| setError(null); | ||
| try { | ||
| const client = await connect(); | ||
| const view = await client.joinTournament(code, displayName.trim()); | ||
| setTournament(view); | ||
| const self = view.standings.find( | ||
| (s) => s.displayName.toLowerCase() === displayName.trim().toLowerCase(), | ||
| ); | ||
| if (self) setPlayerKey(self.playerKey); | ||
| client.close(); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } finally { | ||
| setJoining(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.joinTournament throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed join attempt. Declare client outside the try block and close it in a finally block.
| const handleJoin = async () => { | |
| if (!code) return; | |
| setJoining(true); | |
| setError(null); | |
| try { | |
| const client = await connect(); | |
| const view = await client.joinTournament(code, displayName.trim()); | |
| setTournament(view); | |
| const self = view.standings.find( | |
| (s) => s.displayName.toLowerCase() === displayName.trim().toLowerCase(), | |
| ); | |
| if (self) setPlayerKey(self.playerKey); | |
| client.close(); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| setJoining(false); | |
| } | |
| }; | |
| const handleJoin = async () => { | |
| if (!code) return; | |
| setJoining(true); | |
| setError(null); | |
| let client; | |
| try { | |
| client = await connect(); | |
| const view = await client.joinTournament(code, displayName.trim()); | |
| setTournament(view); | |
| const self = view.standings.find( | |
| (s) => s.displayName.toLowerCase() === displayName.trim().toLowerCase(), | |
| ); | |
| if (self) setPlayerKey(self.playerKey); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| client?.close(); | |
| setJoining(false); | |
| } | |
| }; |
| const handleStartRound = async () => { | ||
| if (!code) return; | ||
| try { | ||
| const client = await connect(); | ||
| await client.startRound(code); | ||
| client.close(); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.startRound throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed start round attempt. Declare client outside the try block and close it in a finally block.
const handleStartRound = async () => {
if (!code) return;
let client;
try {
client = await connect();
await client.startRound(code);
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
client?.close();
}
};
| const handleReport = async ( | ||
| matchId: string, | ||
| winnerKey: string | null, | ||
| aWins: number, | ||
| bWins: number, | ||
| ) => { | ||
| if (!code) return; | ||
| try { | ||
| const client = await connect(); | ||
| await client.reportMatchResult(code, matchId, winnerKey, aWins, bWins); | ||
| client.close(); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.reportMatchResult throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed report attempt. Declare client outside the try block and close it in a finally block.
const handleReport = async (
matchId: string,
winnerKey: string | null,
aWins: number,
bWins: number,
) => {
if (!code) return;
let client;
try {
client = await connect();
await client.reportMatchResult(code, matchId, winnerKey, aWins, bWins);
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
client?.close();
}
};
| const handleDrop = async () => { | ||
| if (!code) return; | ||
| try { | ||
| const client = await connect(); | ||
| await client.dropFromTournament(code); | ||
| client.close(); | ||
| navigate("/tournament"); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.dropFromTournament throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed drop attempt. Declare client outside the try block and close it in a finally block.
const handleDrop = async () => {
if (!code) return;
let client;
try {
client = await connect();
await client.dropFromTournament(code);
navigate("/tournament");
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
client?.close();
}
};
| const handleEnd = async () => { | ||
| if (!code) return; | ||
| try { | ||
| const client = await connect(); | ||
| await client.endTournament(code); | ||
| client.close(); | ||
| navigate("/tournament"); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[HIGH] WebSocket connection leak on RPC error
If client.endTournament throws an error, the execution jumps straight to the catch block, bypassing client.close(). This leaks the WebSocket connection on every failed end attempt. Declare client outside the try block and close it in a finally block.
| const handleEnd = async () => { | |
| if (!code) return; | |
| try { | |
| const client = await connect(); | |
| await client.endTournament(code); | |
| client.close(); | |
| navigate("/tournament"); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } | |
| }; | |
| const handleEnd = async () => { | |
| if (!code) return; | |
| let client; | |
| try { | |
| client = await connect(); | |
| await client.endTournament(code); | |
| navigate("/tournament"); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| client?.close(); | |
| } | |
| }; |
Fixes ESLint no-unused-vars failure in check-frontend CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Use MenuShell description instead of subtitle and re-add the lazy DraftSpectatorPage import dropped during tournament route wiring. Co-authored-by: Cursor <cursoragent@cursor.com>
Lobby brokers accept protocol 11 during rollout; v10 is below the floor. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
I found several substantive blockers in the current tournament implementation:
[HIGH] Tournament authority is stored on the socket, but the UI closes that socket immediately. Evidence: client/src/pages/TournamentLandingPage.tsx:66; crates/lobby-broker/src/broker.rs:353. Why it matters: creating a tournament stores organizer authority in ConnState, then closing the socket triggers on_disconnect and unregisters the tournament; joined players also lose joined_tournaments, so later report/drop calls from fresh sockets are unauthorized. Suggested fix: keep one tournament WebSocket alive for the page lifetime, or switch to explicit organizer/player tokens and stop deleting tournaments on ordinary socket close.
[HIGH] Match result validation accepts a winner that contradicts the game-win score. Evidence: crates/lobby-broker/src/tournament.rs:326. Why it matters: a client can report 2-0 while naming the losing player as winner_player_key, corrupting match points and standings. Suggested fix: when wins differ, require winner_player_key to equal the paired player with the higher game-win count.
[HIGH] Active tournaments expire solely by creation time. Evidence: crates/lobby-broker/src/tournament.rs:275; crates/phase-server/src/main.rs:1004. Why it matters: the native server reaper runs with a 300s timeout, so an in-progress Swiss event lasting more than five minutes can be deleted mid-round even if players are actively joining/reporting. Suggested fix: track last_activity_at and update it on tournament mutations, or restrict stale reaping to abandoned registration state.
[MED] Worker reaping stops for tournament-only broker state. Evidence: lobby-worker/broker-wasm/src/lib.rs:175. Why it matters: is_empty() only checks lobby games, so after the first non-expiring alarm a Durable Object with only tournaments stops rescheduling cleanup and can retain abandoned tournaments until another mutation. Suggested fix: make the WASM emptiness predicate include both lobby games and tournaments.
[MED] Odd-sized Swiss brackets always miss the backtracking success path. Evidence: crates/lobby-broker/src/tournament.rs:698. Why it matters: every odd bracket recurses to one remaining player, returns None, and falls back to greedy pairing that can create avoidable rematches despite a valid float-down solution. Suggested fix: treat one remaining player as a successful partial pairing and let pair_bracket_with_backtracking identify that player as the float.
[MED] Tournament subscription cleanup unsubscribes but never closes the WebSocket. Evidence: client/src/services/tournamentClient.ts:296. Why it matters: navigating away removes the listener and sends UnsubscribeLobby, but the socket stays open and continues counting as a live broker connection. Suggested fix: make the page cleanup close the tournament client, including the cancelled-connect path.
[LOW] New tournament UI chrome bypasses the i18n boundary. Evidence: client/src/pages/TournamentLandingPage.tsx:87. Why it matters: frontend-authored labels/buttons/descriptions will remain English-only despite client/src/i18n/README.md requiring t() for chrome strings. Suggested fix: add tournament keys to the appropriate i18n namespace and route new authored UI text through useTranslation.
|
Thanks for putting time into this. I’m going to close this PR in its current form. This is a large cross-stack feature touching the lobby broker, shared protocol, native server, Cloudflare worker, and React client. A change at this scale needs design discussion and maintainer alignment before PR work starts. Please either come by Discord to discuss the feature/design with us, or open a proposal in the repo’s Discussions area before doing further implementation work. The proposal should cover the intended user flow, broker/server/worker lifecycle model, protocol changes, UI states, rollout plan, and verification strategy. This is not a rejection of the tournament idea. It may be worth doing, but it needs to start as a design discussion first, then be split into small reviewable PRs once the direction is agreed. |
2 participants
Summary
lobby-broker: registration, Swiss pairing with rematch avoidance, Magic tiebreaker standings, and self-reported match results.server-core,phase-server, and the Cloudflarelobby-workerWASM shell (sharedConnState+ DO snapshot)./tournament,/tournament/:code),tournamentClient.ts, and a link from Multiplayer.Closes #4612.
Test plan
cargo test -p lobby-broker(90 tests)cargo test -p server-core --test lobby_wire_contract(5 tests)cargo check -p phase-serverlobby-workerWASM before production (protocol v12 + broker snapshot shape)