Handle matchmaking socket close codes per API contract - #4595
Merged
Conversation
The matchmaking queue is in-memory on the API worker, so a deploy or restart drops queued players; previously the modal only logged the close and left the player on the searching spinner forever. Implements the close-code contract from the matchmaking handoff: - 1000 (replaced by newer connection): another tab/window took the queue slot - show a message and stop, no retry - 1008 (invalid session): reconnect and re-send join; getPlayToken() refreshes an expired token internally - any other close before assignment: reconnect and rejoin with exponential backoff (1s doubling to a 15s cap) Intentional closes (user backs out, assignment received) don't reconnect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Walkthrough
ChangesMatchmaking reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerOne
participant PlayerTwo
participant MatchmakingModal
participant MatchmakingService
participant GameServer
PlayerOne->>MatchmakingModal: connect to matchmaking
PlayerTwo->>MatchmakingModal: connect to matchmaking
MatchmakingModal->>MatchmakingService: join with session token
MatchmakingService-->>MatchmakingModal: match-assignment with gameId
MatchmakingModal->>GameServer: join assigned game
GameServer-->>PlayerOne: game lobby event
GameServer-->>PlayerTwo: game lobby event
Possibly related PRs
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}
Contributor
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 120-148: Reset reconnectAttempts to 0 in the socket onopen handler
after a connection succeeds, ensuring each independent reconnect cycle starts
with the initial backoff delay. Keep the existing onclose retry calculation
unchanged.
🪄 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
Run ID: 4ed28267-359e-4e2b-877b-7ad2971544f4
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/Matchmaking.ts
Comment on lines
+120
to
+148
| this.socket.onclose = (event: CloseEvent) => { | ||
| console.log( | ||
| `Matchmaking server closed connection: code=${event.code} reason=${event.reason}`, | ||
| ); | ||
| if (this.intentionalClose || this.gameID !== null) { | ||
| return; | ||
| } | ||
| if (event.code === 1000) { | ||
| // A newer connection for this account (e.g. a second tab) took the | ||
| // queue slot; this socket was replaced. Do not retry. | ||
| window.dispatchEvent( | ||
| new CustomEvent("show-message", { | ||
| detail: { | ||
| message: translateText("matchmaking_modal.replaced"), | ||
| color: "red", | ||
| duration: 5000, | ||
| }, | ||
| }), | ||
| ); | ||
| this.close(); | ||
| return; | ||
| } | ||
| // 1008: the jwt was rejected — getPlayToken() refreshes expired tokens, | ||
| // so rejoining sends a fresh one. Anything else is a server | ||
| // restart/deploy; the queue is in-memory only, so rejoin. Back off in | ||
| // case the failure repeats. | ||
| this.connected = false; | ||
| const delay = Math.min(1000 * 2 ** this.reconnectAttempts++, 15000); | ||
| this.reconnectTimeout = setTimeout(() => this.connect(), delay); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset reconnectAttempts on successful connection to avoid unbounded backoff growth.
reconnectAttempts is only reset in onOpen (line 194), not when the socket successfully reconnects via onopen. Across independent disconnect-reconnect cycles within the same session, the backoff delay keeps climbing toward 15 s even though each disconnect is a separate event. A user with intermittent connectivity could see 15 s waits after just a few cycles.
Add this.reconnectAttempts = 0 in the onopen handler so each successful connection restarts the backoff sequence:
🔄 Proposed fix
this.socket.onopen = async () => {
console.log("Connected to matchmaking server");
+ this.reconnectAttempts = 0;
this.connectTimeout = setTimeout(async () => {🤖 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/client/Matchmaking.ts` around lines 120 - 148, Reset reconnectAttempts to
0 in the socket onopen handler after a connection succeeds, ensuring each
independent reconnect cycle starts with the initial backoff delay. Keep the
existing onclose retry calculation unchanged.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Two runnable harnesses for the matchmaking client integration: - npm run test:matchmaking (contained): drives the real modal in a headless browser against a fake in-process matchmaking server that speaks the documented protocol, covering the full close-code contract (1006 deploy drop, 1008 invalid session, 1000 replaced, assignment, intentional close). - npm run test:matchmaking:e2e: real integration against the API worker on localhost:8787 - two browser players join the real queue, the dev game server receives the checkin assignment, and both clients end up in the same created game. Neither runs under npm test (they need a browser and, for e2e, a local API worker). See tests/matchmaking/README.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
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/contained.mjs`:
- Around line 93-120: Strengthen the invalid-session and replacement assertions
in the test flow around resetAndConnect, joinCountReaches, and
control("replace"): track and await a new getPlayToken() request after the 1008
rejection, then record window.__mmMessages.length immediately before replacement
and assert that a newly appended message is produced and specifically identifies
the replacement state rather than merely being untranslated or pre-existing.
- Around line 44-46: Guard partially initialized test resources so cleanup
cannot hang or be skipped: in tests/matchmaking/contained.mjs lines 44-46, move
launch inside a cleanup scope using a nullable browser handle; in lines 135-137,
make cleanup independent with nested finally blocks or Promise.allSettled so the
fake server always closes; in tests/matchmaking/fakeServer.mjs lines 93-100,
terminate WebSocket clients and close WebSocketServer before the HTTP server; in
tests/matchmaking/e2e.mjs lines 32-36, include browser context and page creation
within the cleanup scope.
🪄 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
Run ID: bf0cd3ee-2369-45bd-888d-8b3639bddd03
📒 Files selected for processing (7)
eslint.config.jspackage.jsontests/matchmaking/README.mdtests/matchmaking/contained.mjstests/matchmaking/e2e.mjstests/matchmaking/fakeServer.mjstests/matchmaking/util.mjs
Comment on lines
+44
to
+46
| const { browser, page } = await launch(); | ||
| const c = makeChecker(); | ||
| try { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard partially initialized resources so failures cannot hang the harness.
Browser/server setup happens before the cleanup scopes, and one rejected cleanup prevents later resources from closing.
tests/matchmaking/contained.mjs#L44-L46: move browser launch inside a guarded scope with a nullable handle.tests/matchmaking/contained.mjs#L135-L137: use nestedfinallyblocks orPromise.allSettled()so the fake server always closes.tests/matchmaking/fakeServer.mjs#L93-L100: terminate WebSocket clients and close theWebSocketServerbefore closing the HTTP server.tests/matchmaking/e2e.mjs#L32-L36: include browser context/page creation in the cleanup scope.
📍 Affects 3 files
tests/matchmaking/contained.mjs#L44-L46(this comment)tests/matchmaking/contained.mjs#L135-L137tests/matchmaking/fakeServer.mjs#L93-L100tests/matchmaking/e2e.mjs#L32-L36
🤖 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/contained.mjs` around lines 44 - 46, Guard partially
initialized test resources so cleanup cannot hang or be skipped: in
tests/matchmaking/contained.mjs lines 44-46, move launch inside a cleanup scope
using a nullable browser handle; in lines 135-137, make cleanup independent with
nested finally blocks or Promise.allSettled so the fake server always closes; in
tests/matchmaking/fakeServer.mjs lines 93-100, terminate WebSocket clients and
close WebSocketServer before the HTTP server; in tests/matchmaking/e2e.mjs lines
32-36, include browser context and page creation within the cleanup scope.
Comment on lines
+93
to
+120
| // 3. Invalid session: next join is closed 1008 -> client retries and rejoins | ||
| await control("reject-next"); | ||
| await control("kill"); // forces the reconnect whose join gets 1008 | ||
| await joinCountReaches(4, 20000); // join 3 rejected, join 4 accepted | ||
| c.check("1008 -> reconnected and rejoined with fresh token", true); | ||
|
|
||
| // 4. Assignment: modal records the gameId | ||
| await control("assign", { gameId: "FakeGame1" }); | ||
| await waitFor(() => modal(`return el.gameID === "FakeGame1";`), { | ||
| timeoutMs: 5000, | ||
| label: "modal to receive match-assignment", | ||
| }); | ||
| c.check("match-assignment received", true); | ||
| await modal(`el.onClose();`); // stop the game-exists polling | ||
|
|
||
| // 5. Replaced by newer connection: message shown, no retry | ||
| await resetAndConnect(); | ||
| await joinCountReaches(5, 8000); | ||
| await control("replace"); | ||
| await waitFor(() => page.evaluate(() => window.__mmMessages.length > 0), { | ||
| timeoutMs: 5000, | ||
| label: "replaced message", | ||
| }); | ||
| const msg = await page.evaluate(() => window.__mmMessages.at(-1)); | ||
| c.check( | ||
| `replaced -> message shown ("${msg}")`, | ||
| typeof msg === "string" && !msg.includes("matchmaking_modal."), | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the specific 1008 and replacement effects.
Join count does not prove that getPlayToken() ran again, and any existing translated message can satisfy the replacement check. Track the play-token request after 1008, capture the message count before replace, and assert the new replacement message specifically.
🤖 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/contained.mjs` around lines 93 - 120, Strengthen the
invalid-session and replacement assertions in the test flow around
resetAndConnect, joinCountReaches, and control("replace"): track and await a new
getPlayToken() request after the 1008 rejection, then record
window.__mmMessages.length immediately before replacement and assert that a
newly appended message is produced and specifically identifies the replacement
state rather than merely being untranslated or pre-existing.
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements the close-code contract from the matchmaking API handoff (API PR #419) in the matchmaking modal, plus two runnable integration harnesses. Previously
oncloseonly logged, so an API deploy while queued left the player on the "searching" spinner forever (the queue is in-memory on the API worker).Invalid sessionjoin—getPlayToken()refreshes an expired token internally, so the rejoin carries a fresh JWTReplaced by newer connectionmatchmaking_modal.replacedstring) and stop. No retryjoin, with exponential backoff (1s doubling to a 15s cap)Intentional closes (user backs out of the modal, assignment received) don't reconnect. A pending 2s join timer from a previous socket is cleared before each reconnect so it can't fire on the new socket.
Test harnesses (
tests/matchmaking/)npm run test:matchmaking(contained): drives the real modal in a headless browser against a fake in-process matchmaking server speaking the documented protocol; covers the whole close-code table over real WebSockets. Needs onlynpm run dev.npm run test:matchmaking:e2e: real integration against the API worker onlocalhost:8787— two browser players join the real queue, the dev game server receives the checkin assignment, and both clients end up in the same created game.Why now
This is required by the matchmaking API's client contract independent of the upcoming 2v2 work ("clients must already handle unexpected close → reconnect and rejoin"). The rest of the 2v2 integration is planned for the next version.
Verification
npm test(all 2,046 pass),npx tsc --noEmit, ESLint clean.🤖 Generated with Claude Code