diff --git a/client/src/scenes/game-scene.js b/client/src/scenes/game-scene.js index 97b7332..e4dc9c4 100644 --- a/client/src/scenes/game-scene.js +++ b/client/src/scenes/game-scene.js @@ -12,6 +12,7 @@ import { eventBus } from '../services/event-bus.js'; import { gameState } from '../services/game-state-service.js'; import { connectionService } from '../services/connection-service.js'; import { ClientEventCode } from '../config/protocol-constants.js'; +import { isSelfExit } from '../services/client-exit-helpers.js'; import { showGameHud, updateTurnIndicator, @@ -218,11 +219,20 @@ export class GameScene extends Phaser.Scene { } /** - * Opponent disconnected or client exited — return to menu lobby. - * MenuScene.create() checks gameState.nickname and shows the lobby directly. + * ClientExitResponse received. Distinguish self vs peer: + * - self (exitClientId === 0 as an ack, OR === our clientId): transition to lobby + * - peer: stay in GameScene; the corresponding GameOverResponse will + * show the game-over modal via forfeit. Showing a toast here keeps + * the player informed without silently desyncing their server state. + * @param {{ exitClientId?: number, exitClientNickname?: string }} data * @private */ - _onClientExit() { + _onClientExit(data) { + if (!isSelfExit(data, gameState.clientId)) { + const who = (data && data.exitClientNickname) ? data.exitClientNickname : 'Opponent'; + showToast(`${who} left the room`, 'info'); + return; + } hideGameHud(); this._cleanup(); this.scene.start('MenuScene'); diff --git a/client/src/services/client-exit-helpers.js b/client/src/services/client-exit-helpers.js new file mode 100644 index 0000000..08ae400 --- /dev/null +++ b/client/src/services/client-exit-helpers.js @@ -0,0 +1,37 @@ +/** + * Helpers for interpreting ClientExitResponse. + * + * The server broadcasts ClientExitResponse to ALL players in the room when + * anyone leaves (so peers see "opponent left"), AND uses the same message + * type as a self-ack from homeState / watchingState. Clients must + * distinguish self from peer — otherwise a peer's exit forces the client + * to transition to lobby while the server keeps the client in gameoverState, + * causing a state desync where subsequent requests log as "unexpected". + * + * Rules: + * - exitClientId === 0 or missing → self ack (server path that sends a + * bare ClientExitResponse; e.g., home.ClientExit, watching.ClientExit, + * spectator room-closed eject). Treat as self. + * - exitClientId === this client's clientId → self (our own leaveRoom + * broadcast reached us through room.Players iteration). + * - Otherwise → a peer left; consumer should stay in state and render + * a "X left" notification. + * + * clientId is passed in explicitly (rather than reading gameState here) + * to keep this module free of circular imports with game-state-service. + * + * @module client-exit-helpers + */ + +/** + * Return true if the incoming ClientExitResponse refers to this client. + * @param {{ exitClientId?: number }|null|undefined} data + * @param {number|null} clientId - this client's server-assigned ID + * @returns {boolean} + */ +export function isSelfExit(data, clientId) { + if (!data) return true; // defensive: no data → treat as self to avoid ghost state + const id = data.exitClientId; + if (id == null || id === 0) return true; // bare ack from server + return id === clientId; +} diff --git a/client/src/services/game-state-service.js b/client/src/services/game-state-service.js index 1d296be..0236512 100644 --- a/client/src/services/game-state-service.js +++ b/client/src/services/game-state-service.js @@ -7,6 +7,7 @@ import { eventBus } from './event-bus.js'; import { ClientEventCode } from '../config/protocol-constants.js'; +import { isSelfExit } from './client-exit-helpers.js'; /** * @typedef {Object} MoveEntry @@ -141,7 +142,12 @@ class GameStateService { console.warn('[GameState] spectator cannot perform this action'); }); - eventBus.on(ClientEventCode.CLIENT_EXIT, () => this.reset()); + // Only reset when WE exited, not when a peer leaves a shared room. + // Otherwise a peer's exit would blow away our own roomId and board, + // desyncing us from the server's gameoverState. + eventBus.on(ClientEventCode.CLIENT_EXIT, (data) => { + if (isSelfExit(data, this.clientId)) this.reset(); + }); } } diff --git a/client/src/ui/menu-ui.js b/client/src/ui/menu-ui.js index 04c2d23..dbabe6c 100644 --- a/client/src/ui/menu-ui.js +++ b/client/src/ui/menu-ui.js @@ -11,6 +11,7 @@ import { ClientEventCode } from '../config/protocol-constants.js'; import { showToast } from './game-ui.js'; import { eventBus } from '../services/event-bus.js'; import { showRoomList, showPveDifficultyPanel, showWaiting } from './menu-ui-rooms.js'; +import { isSelfExit } from '../services/client-exit-helpers.js'; // Re-export showRoomList and showWaiting so menu-scene.js imports from one place export { showRoomList, showWaiting }; @@ -116,5 +117,8 @@ eventBus.on(ClientEventCode.NICKNAME_SET, (data) => { } }); -// Return to lobby when client exits or is kicked -eventBus.on(ClientEventCode.CLIENT_EXIT, showLobby); +// Return to lobby only when WE exit. Peer exits are rendered as toasts +// by game-scene._onClientExit; they must not blow away our menu state. +eventBus.on(ClientEventCode.CLIENT_EXIT, (data) => { + if (isSelfExit(data, gameState.clientId)) showLobby(); +}); diff --git a/server/state/flow_test.go b/server/state/flow_test.go index 764e4be..2054754 100644 --- a/server/state/flow_test.go +++ b/server/state/flow_test.go @@ -675,15 +675,17 @@ func TestFlow_CreateRoomAfterPvpRematchLeave(t *testing.T) { } } -// TestFlow_CreateRoomAfterOpponentForfeit: the other player disconnects -// during the game via ClientExit. The remaining player sees GameOver via -// forfeit broadcast + ClientExit notification. They end up in gameoverState -// and must be able to leave + create a new room. -func TestFlow_CreateRoomAfterOpponentForfeit(t *testing.T) { - black, white, room := setupFinishedPvpRoom(t) - _ = room +// TestFlow_OpponentExit_KicksPeerFromGameover verifies the regression fix +// for the server log "gameover player N: unexpected Request_CreateRoom": +// when one player leaves a finished PVP room, the remaining peer should +// NOT be left sitting in gameoverState — kickStaleRoomPeers injects a +// synthetic ClientExit so their goroutine auto-transitions to home. +// Both players must then be free to create fresh rooms. +func TestFlow_OpponentExit_KicksPeerFromGameover(t *testing.T) { + black, white, _ := setupFinishedPvpRoom(t) - // Black clicks Leave from gameoverState — room still has white inside. + // Black leaves the finished PVP room → kickStaleRoomPeers should push + // a synthetic ClientExit onto white.CmdCh. black.CmdCh <- &protocol.Request{ Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}, } @@ -694,21 +696,24 @@ func TestFlow_CreateRoomAfterOpponentForfeit(t *testing.T) { t.Errorf("black.RoomID = %d, want 0", black.RoomID) } - // White now sends ClientExit — room should be deleted. - white.CmdCh <- &protocol.Request{ - Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}, + // White's CmdCh should contain a synthetic ClientExit queued by + // kickStaleRoomPeers. Running gameoverState on white should consume + // it and transition to StateHome WITHOUT any test-driven input. + next, err := runState(consts.StateGameOver, white) + if err != nil { + t.Fatalf("white auto-exit from kick: %v", err) } - if _, err := runState(consts.StateGameOver, white); err != nil { - t.Fatalf("white gameover exit: %v", err) + if next != consts.StateHome { + t.Fatalf("white expected StateHome after synthetic kick, got %d", next) } if white.RoomID != 0 { - t.Errorf("white.RoomID = %d, want 0", white.RoomID) + t.Errorf("white.RoomID = %d after auto-kick, want 0", white.RoomID) } if len(lobby.GetAllRooms()) != 0 { - t.Errorf("expected 0 rooms after both players exit, got %d", len(lobby.GetAllRooms())) + t.Errorf("expected 0 rooms after both players kicked, got %d", len(lobby.GetAllRooms())) } - // Both players should now be able to create new rooms. + // Both players should now be able to create fresh rooms from home. for _, p := range []*lobby.Player{black, white} { drainSend(p) p.CmdCh <- &protocol.Request{ @@ -726,7 +731,6 @@ func TestFlow_CreateRoomAfterOpponentForfeit(t *testing.T) { } } - // Two new rooms in the store now. if len(lobby.GetAllRooms()) != 2 { t.Errorf("expected 2 rooms (one per player), got %d", len(lobby.GetAllRooms())) } diff --git a/server/state/gameover.go b/server/state/gameover.go index bbec59f..8f3bcba 100644 --- a/server/state/gameover.go +++ b/server/state/gameover.go @@ -60,6 +60,12 @@ func (*gameOverState) Next(player *lobby.Player) (consts.StateID, error) { case *protocol.Request_ClientExit: leaveRoom(player, room) + // If this left a PVP peer alone in a now-unplayable room, + // push a synthetic ClientExit onto their CmdCh so their + // gameoverState goroutine wakes up and transitions home too + // (instead of silently sitting in gameover while the client + // believes it's in the lobby — the exact desync reported). + kickStaleRoomPeers(room) return consts.StateHome, nil default: diff --git a/server/state/waiting.go b/server/state/waiting.go index 37b9678..80a2521 100644 --- a/server/state/waiting.go +++ b/server/state/waiting.go @@ -96,7 +96,8 @@ func assignColors(room *lobby.Room) { // leaveRoom removes player from their current room cleanly. // Broadcasts ClientExitResponse to everyone in the room BEFORE removing the // player, so the exiting player also receives their own exit confirmation -// (used by the client to transition UI back to the lobby). +// (used by the client to transition UI back to the lobby). The client +// distinguishes self vs peer via exit_client_id. func leaveRoom(player *lobby.Player, room *lobby.Room) { exitResp := &protocol.Response{ Payload: &protocol.Response_ClientExit{ @@ -112,3 +113,41 @@ func leaveRoom(player *lobby.Player, room *lobby.Room) { // Then remove from the room. lobby.LeaveRoom(player) } + +// kickStaleRoomPeers pushes a synthetic ClientExitRequest onto every remaining +// player's CmdCh if the room is now unplayable (PVP with fewer than 2 human +// players). Used after leaveRoom in the gameover path to prevent peers from +// sitting in gameoverState forever once the game cannot continue. +// +// Safe to call with a room that was already deleted from the store — the +// caller holds a reference; we just drain room.Players for peers still around. +func kickStaleRoomPeers(room *lobby.Room) { + room.RLock() + roomType := room.RoomType + peers := make([]*lobby.Player, 0, len(room.Players)) + for _, p := range room.Players { + peers = append(peers, p) + } + room.RUnlock() + + if roomType != lobby.RoomTypePvp { + return + } + if len(peers) >= 2 { + return // still playable + } + + syntheticExit := &protocol.Request{ + Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}, + } + for _, p := range peers { + if p.CmdCh == nil { + continue + } + select { + case p.CmdCh <- syntheticExit: + default: + // CmdCh full — idle reaper will clean up the room eventually. + } + } +}