fix(matchmaking): widen the join window and cancel short-handed games - #4762
Conversation
Matchmade games froze their roster ~9s after match-assignment (7s startsAt + 2s prestart gap), while the client join pipeline (exists poll, Turnstile re-challenge, join auth) takes 4-6s on a good day — and far longer from a throttled background tab. Players who missed the window were left out of gameStartInfo, so ranked games started short-handed: a void 2v2 or an uncontested 1v1 walkover. Two changes: - Give players 15s instead of 7s to connect. Full lobbies are unaffected: hasReachedMaxPlayerCount flips the game Active as soon as everyone joins. - If the deadline still arrives short-handed, cancel the game instead of starting it: kick the connected players with kick_reason.match_cancelled (alert + leave-lobby + matchmaking modal close client-side) and end the unstarted game without archiving. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughRanked matchmade games now cancel when players are missing at the start deadline. Connected clients receive a localized message, leave the lobby, and re-enter matchmaking. Unit and end-to-end tests cover cancellation, requeue, pruning, and no-show behavior. ChangesMatch Cancellation Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MatchmakingWorker
participant GameManager
participant GameServer
participant ClientGameRunner
participant Main
participant MatchmakingModal
MatchmakingWorker->>GameServer: Create match with 15-second roster deadline
GameManager->>GameServer: Check short-handed match at start deadline
GameServer->>ClientGameRunner: Send match-cancelled kick reason
ClientGameRunner->>Main: Dispatch matchmaking-requeue
Main->>MatchmakingModal: Call requeue()
MatchmakingModal->>MatchmakingWorker: Start matchmaking connection
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/GameManager.ts (1)
141-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecheck ranked attendance immediately before
start().After
prestart(),hasStarted()becomes true. If a player disconnects during the two-second delay, the callback still starts the ranked game short-handed because cancellation is never re-evaluated.Proposed fix
setTimeout(() => { try { - game.start(); + if (!game.cancelShortHandedMatch()) { + game.start(); + } } catch (error) { this.log.error(`error starting game ${id}: ${error}`); } }, 2000);Add a GameManager-level test for a full ranked lobby that loses a player after prestart but before the delayed start.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/GameManager.ts` around lines 141 - 153, Recheck ranked attendance in the delayed callback before calling game.start(), using the existing GameManager/game cancellation logic to cancel a match that loses a player after prestart instead of starting it short-handed. Preserve normal startup for full lobbies, and add a GameManager-level test covering a full ranked lobby that loses a player during the delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/GameManager.ts`:
- Around line 141-153: Recheck ranked attendance in the delayed callback before
calling game.start(), using the existing GameManager/game cancellation logic to
cancel a match that loses a player after prestart instead of starting it
short-handed. Preserve normal startup for full lobbies, and add a
GameManager-level test covering a full ranked lobby that loses a player during
the delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 89738c93-ee28-4584-9538-eaae8e21c31c
📒 Files selected for processing (6)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tstests/server/MatchmakingCancel.test.ts
When the server cancels a matchmade game because a player never connected, the players who did connect are put straight back into the queue: the matchmaking modal (still open on "waiting for game") resets to searching and reconnects in the same mode, with a toast explaining why. A blocking alert would keep them out of the queue until dismissed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Matchmaking.ts`:
- Around line 124-133: Update the requeue/reset flow in Matchmaking to detach or
invalidate the previous socket before resetting intentionalClose and gameID and
calling connect(). Ensure any delayed onclose handler from the old socket cannot
treat its closure as unexpected or initiate another connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cf80586-db5f-4592-a6f2-dcafb1cc7ce9
📒 Files selected for processing (2)
src/client/ClientGameRunner.tssrc/client/Matchmaking.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Four browser players queue for 2v2 against the real API worker; one has its /exists polls blocked so it never connects to the created game. Asserts the server cancels at the start deadline instead of starting 3-handed, the connected players get the toast + leave-lobby and are requeued automatically, the no-show is untouched, and the game is pruned without an archive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/matchmaking/e2e-cancel.mjs`:
- Around line 226-232: Replace the fixed 2-second delay and one-shot
fetchGameInfo check in the cancelled-game pruning assertion with the file’s
existing waitFor polling helper, using a generous timeout and polling until
fetchGameInfo(gameId) returns null. Preserve the existing check message and
cancellation-pruning expectation.
- Around line 234-238: Update the final cancellation-log check in the e2e test
to read the actual server log for this test, or catch read failures and make the
check evaluate false so cleanup and c.finish() still run. Also match the
complete emitted cancellation log message rather than only the shorter
substring, using the existing c.check flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe2aa701-a865-4ba3-b818-98e463e474df
📒 Files selected for processing (3)
package.jsontests/matchmaking/README.mdtests/matchmaking/e2e-cancel.mjs
| // The cancelled game must be gone from the game server (pruned, never | ||
| // started, nothing archived). | ||
| await new Promise((r) => setTimeout(r, 2000)); | ||
| c.check( | ||
| "cancelled game pruned from the server", | ||
| (await fetchGameInfo(gameId)) === null, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pruning check uses a fixed sleep instead of polling.
Every other multi-step assertion in this file uses waitFor with a generous timeout to avoid flakiness, but the pruning check relies on a single fixed 2s sleep + one-shot fetchGameInfo call. If pruning takes slightly longer under load, this fails spuriously instead of surfacing a clear timeout message.
♻️ Suggested fix: poll with waitFor like the rest of the file
- await new Promise((r) => setTimeout(r, 2000));
- c.check(
- "cancelled game pruned from the server",
- (await fetchGameInfo(gameId)) === null,
- );
+ const pruned = await waitFor(async () => (await fetchGameInfo(gameId)) === null, {
+ timeoutMs: 10000,
+ intervalMs: 1000,
+ label: "cancelled game to be pruned",
+ }).catch(() => false);
+ c.check("cancelled game pruned from the server", pruned);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The cancelled game must be gone from the game server (pruned, never | |
| // started, nothing archived). | |
| await new Promise((r) => setTimeout(r, 2000)); | |
| c.check( | |
| "cancelled game pruned from the server", | |
| (await fetchGameInfo(gameId)) === null, | |
| ); | |
| // The cancelled game must be gone from the game server (pruned, never | |
| // started, nothing archived). | |
| const pruned = await waitFor(async () => (await fetchGameInfo(gameId)) === null, { | |
| timeoutMs: 10000, | |
| intervalMs: 1000, | |
| label: "cancelled game to be pruned", | |
| }).catch(() => false); | |
| c.check("cancelled game pruned from the server", pruned); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/matchmaking/e2e-cancel.mjs` around lines 226 - 232, Replace the fixed
2-second delay and one-shot fetchGameInfo check in the cancelled-game pruning
assertion with the file’s existing waitFor polling helper, using a generous
timeout and polling until fetchGameInfo(gameId) returns null. Preserve the
existing check message and cancellation-pruning expectation.
| const devLog = readFileSync("/tmp/dev.log", "utf8"); | ||
| c.check( | ||
| "server logged the cancellation", | ||
| devLog.includes("cancelling matchmade game"), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the dev.log convention and the exact cancellation log string.
rg -n "dev\.log" tests/matchmaking -A2 -B2
rg -n "cancelling matchmade game" src/serverRepository: openfrontio/OpenFrontIO
Length of output: 564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate file outline/size ---\n'
wc -l tests/matchmaking/e2e-cancel.mjs
printf '\n--- relevant test block ---\n'
sed -n '1,270p' tests/matchmaking/e2e-cancel.mjs | cat -n
printf '\n--- exact server log context ---\n'
sed -n '1055,1085p' src/server/GameServer.ts | cat -n
printf '\n--- other dev.log reads in repo ---\n'
rg -n "readFileSync\\((?:\"|')/tmp/dev\\.log|\"/tmp/dev\\.log|'/tmp/dev\\.log" .Repository: openfrontio/OpenFrontIO
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- candidate file outline/size ---\n'
wc -l tests/matchmaking/e2e-cancel.mjs
printf -- '\n--- relevant test block ---\n'
sed -n '1,270p' tests/matchmaking/e2e-cancel.mjs | cat -n
printf -- '\n--- exact server log context ---\n'
sed -n '1055,1085p' src/server/GameServer.ts | cat -n
printf -- '\n--- other dev.log reads in repo ---\n'
rg -n "readFileSync\\((?:\"|')/tmp/dev\\.log|\"/tmp/dev\\.log|'/tmp/dev\\.log" .Repository: openfrontio/OpenFrontIO
Length of output: 10891
Make the final log check fail instead of throwing
readFileSync("/tmp/dev.log", "utf8") can throw if the log does not exist or the path convention changes, which skips the finally cleanup and c.finish() summary. Capture the log from the actual server log produced for this test, or wrap this read in try/catch so the check reports false instead of crashing. The expected server log text is longer than "cancelling matchmade game"; use the full emitted string if this is intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/matchmaking/e2e-cancel.mjs` around lines 234 - 238, Update the final
cancellation-log check in the e2e test to read the actual server log for this
test, or catch read failures and make the check evaluate false so cleanup and
c.finish() still run. Also match the complete emitted cancellation log message
rather than only the shorter substring, using the existing c.check flow.
Replace the duck-typed matchmaking-modal lookup in ClientGameRunner with a matchmaking-requeue document event handled by Main, and route WinModal's post-game requeue button through the same event so Main owns the mechanism (in-place modal rejoin pre-start; reload with the requeue param after a finished game). cancelShortHandedMatch now checks rankedType is exactly OneVOne/TwoVTwo so a future ranked type must opt in to pre-start cancellation rather than inherit it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
requeue()/onOpen() reset intentionalClose and gameID before calling connect(), so a delayed close event from the previous socket would pass the unexpected-close guard and schedule a duplicate connection — which the server then resolves by kicking the newer socket as "replaced by newer connection", closing the modal on the innocent player. connect() now detaches and closes any previous socket and clears a pending reconnect timer before dialing, serializing connection attempts. Also reset the queue-size display on requeue so a stale count from the previous queue session doesn't show until the first broadcast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
1 participant
Problem
Sometimes a matchmade player never makes it into the game — the classic symptom is a ranked 2v2 starting with 3 players (then voided by the WinCheck cancellation), or a 1v1 decided by a walkover the absent player never contested.
Root cause: matchmade games were created with
startsAt = now + 7s, andstart()freezesgameStartInfo.playersfrom whoever is connected ~9s after match-assignment (7s + the 2s prestart gap + tick granularity). The client join pipeline can't reliably fit in that window:verifyClientToken,verifyJoin,getUserMe).Changes
1. Widen the deadline: 7s → 15s (
Worker.ts). Full lobbies are NOT delayed:hasReachedMaxPlayerCountflips the game to Active as soon as everyone has joined, so the deadline only matters for underfilled lobbies — raising it costs nothing in the common case.2. Cancel instead of starting short-handed (
GameServer.cancelShortHandedMatch, wired inGameManager.tick). If the deadline still arrives with fewer thanmaxPlayersconnected on a game whoserankedTypeis exactlyOneVOneorTwoVTwo(explicit gate — a future ranked type must opt in rather than inherit pre-start cancellation), the game is cancelled: connected players are kicked with a newkick_reason.match_cancelledslug, mirroring the existingkick_reason.host_leftflow (_hasEndedset,phase()reports Finished, GameManager prunes next tick, no archive).3. Auto-requeue the players who did connect — wired through a new
matchmaking-requeuedocument event handled by Main (no direct DOM coupling). Onkick_reason.match_cancelledthe client dispatchesleave-lobbyto tear down the dead lobby, thenmatchmaking-requeue; Main callsMatchmakingModal.requeue(), which puts the modal — still open on "waiting for game" — straight back into the searching state, reconnecting to the queue in the same mode (1v1 or 2v2). A non-blocking toast explains why; a blocking alert would keep the player out of the queue until dismissed. If the modal was closed in the meantime, the modeless dispatch is a no-op — a player who left the queue isn't forced back in. WinModal's post-game requeue button now fires the same event (with the mode), so Main owns the whole mechanism; its behavior is unchanged (reload with the/?requeueparam, which the page consumes to reopen the queue after teardown).This is deliberately server-side, not core: the game never starts, so there is no simulation to decide anything — it's lobby lifecycle, complementing the core-side WinCheck cancellation (#4655) which covers players who connect but never spawn.
Notes for reviewers
Test plan
tests/server/MatchmakingCancel.test.ts: cancels short-handed 2v2 (kick message + close + Finished phase), cancels 1v1 no-show, leaves full ranked games, non-ranked games, and unknown ranked types alonetests/serversuite passes (292 tests), ESLint/Prettier/tsc --noEmitcleannpm run test:matchmaking:cancelharness — 4 browsers queue for 2v2, one has its/existspolls blocked; the game is cancelled 17.1s after assignment (re-verified 9/9 after the event-based requeue wiring), the three connected players get the toast +leave-lobbyand are automatically requeued (modal back to searching), the no-show is untouched, and the game is pruned without an archive. 9/9 checks pass.MM_MODE=2v2 npm run test:matchmaking:e2estill passes (8/8) — a full lobby starts normally and rides into the game with a deterministic 2v2 split.🤖 Generated with Claude Code