diff --git a/server/lobby/lobby_flow_test.go b/server/lobby/lobby_flow_test.go new file mode 100644 index 0000000..f897b05 --- /dev/null +++ b/server/lobby/lobby_flow_test.go @@ -0,0 +1,220 @@ +package lobby + +import ( + "testing" +) + +// Flow-level tests exercising create/leave/find cycles reported as buggy. +// If any of these fail, the bug is in the lobby package. +// If they all pass, the bug is upstream (state machine or client). + +// TestFlow_CreateLeaveCreate_OneRoomVisible reproduces the user-reported +// scenario: create a room, leave it, create another, list rooms — expect +// only the second (active) room. Prior bug reports showed a "ghost" room. +func TestFlow_CreateLeaveCreate_OneRoomVisible(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + + // Create Room 1 and join it. + r1, err := CreatePvpRoom(alice) + if err != nil { + t.Fatalf("CreatePvpRoom 1: %v", err) + } + if err := JoinRoom(r1.ID, alice); err != nil { + t.Fatalf("JoinRoom r1: %v", err) + } + + // Leave Room 1 — should delete it (sole occupant). + LeaveRoom(alice) + + if _, ok := GetRoom(r1.ID); ok { + t.Errorf("Room %d still in store after leave as sole occupant", r1.ID) + } + if alice.RoomID != 0 { + t.Errorf("alice.RoomID = %d after leave, want 0", alice.RoomID) + } + + // Create Room 2 and join it. + r2, err := CreatePvpRoom(alice) + if err != nil { + t.Fatalf("CreatePvpRoom 2: %v", err) + } + if err := JoinRoom(r2.ID, alice); err != nil { + t.Fatalf("JoinRoom r2: %v", err) + } + if r2.ID == r1.ID { + t.Errorf("Room 2 reused ID %d — expected monotonic nextRoomID", r1.ID) + } + + // GetAllRooms should return exactly one room (the active r2). + rooms := GetAllRooms() + if len(rooms) != 1 { + ids := make([]int64, 0, len(rooms)) + for _, r := range rooms { + ids = append(ids, r.ID) + } + t.Fatalf("expected 1 room after create→leave→create, got %d rooms: %v", len(rooms), ids) + } + if rooms[0].ID != r2.ID { + t.Errorf("expected room ID %d, got %d", r2.ID, rooms[0].ID) + } +} + +// TestFlow_CreateLeave_DeletesRoom: basic sanity check that a sole-occupant +// leave removes the room from the store. +func TestFlow_CreateLeave_DeletesRoom(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + + r, _ := CreatePvpRoom(alice) + _ = JoinRoom(r.ID, alice) + + if len(GetAllRooms()) != 1 { + t.Fatalf("expected 1 room after create+join, got %d", len(GetAllRooms())) + } + + LeaveRoom(alice) + + if len(GetAllRooms()) != 0 { + t.Errorf("expected 0 rooms after sole-occupant leave, got %d", len(GetAllRooms())) + } +} + +// TestFlow_CreateLeaveLeaveLeave_NoGhost: repeated create/leave cycles must +// not leak rooms. +func TestFlow_CreateLeaveLeaveLeave_NoGhost(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + + for i := 0; i < 5; i++ { + r, err := CreatePvpRoom(alice) + if err != nil { + t.Fatalf("cycle %d: CreatePvpRoom: %v", i, err) + } + if err := JoinRoom(r.ID, alice); err != nil { + t.Fatalf("cycle %d: JoinRoom: %v", i, err) + } + LeaveRoom(alice) + + rooms := GetAllRooms() + if len(rooms) != 0 { + ids := make([]int64, 0, len(rooms)) + for _, r := range rooms { + ids = append(ids, r.ID) + } + t.Fatalf("cycle %d: expected 0 rooms after leave, got %d: %v", i, len(rooms), ids) + } + } +} + +// TestFlow_TwoPlayers_OneLeaves_RoomStays: with 2 players in a PVP room, one +// leaving should NOT delete the room — the other is still there. +func TestFlow_TwoPlayers_OneLeaves_RoomStays(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + bob := RegisterPlayer("bob") + + r, _ := CreatePvpRoom(alice) + _ = JoinRoom(r.ID, alice) + _ = JoinRoom(r.ID, bob) + + LeaveRoom(alice) + + if _, ok := GetRoom(r.ID); !ok { + t.Fatal("room deleted after only one of two players left") + } + r2, _ := GetRoom(r.ID) + if len(r2.Players) != 1 { + t.Errorf("expected 1 remaining player, got %d", len(r2.Players)) + } + if r2.OwnerID != bob.ID { + t.Errorf("ownership should have transferred to bob (%d), got %d", bob.ID, r2.OwnerID) + } + + // Bob leaves too — now room should be deleted. + LeaveRoom(bob) + if _, ok := GetRoom(r.ID); ok { + t.Error("room not deleted after both players left") + } +} + +// TestFlow_CreatePveLeave_DeletesRoom: PVE rooms also get deleted on leave. +func TestFlow_CreatePveLeave_DeletesRoom(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + + r, err := CreatePveRoom(alice, 2) + if err != nil { + t.Fatalf("CreatePveRoom: %v", err) + } + _ = JoinRoom(r.ID, alice) + + if len(GetAllRooms()) != 1 { + t.Fatalf("expected 1 PVE room, got %d", len(GetAllRooms())) + } + + LeaveRoom(alice) + + if len(GetAllRooms()) != 0 { + t.Errorf("expected 0 rooms after PVE leave, got %d", len(GetAllRooms())) + } +} + +// TestFlow_GetAllRooms_AfterLeaveReflectsStore: list retrieved from GetAllRooms +// must not include a room the caller just left. Catches stale-snapshot bugs. +func TestFlow_GetAllRooms_AfterLeaveReflectsStore(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + bob := RegisterPlayer("bob") + + // Alice creates her own room. + ra, _ := CreatePvpRoom(alice) + _ = JoinRoom(ra.ID, alice) + + // Bob creates his own room. + rb, _ := CreatePvpRoom(bob) + _ = JoinRoom(rb.ID, bob) + + if len(GetAllRooms()) != 2 { + t.Fatalf("expected 2 rooms, got %d", len(GetAllRooms())) + } + + // Alice leaves. + LeaveRoom(alice) + + rooms := GetAllRooms() + if len(rooms) != 1 { + t.Fatalf("expected 1 room after alice leaves, got %d", len(rooms)) + } + if rooms[0].ID != rb.ID { + t.Errorf("expected remaining room to be bob's (%d), got %d", rb.ID, rooms[0].ID) + } +} + +// TestFlow_CreateRoom_CreateAnother_OldOneDoesNotPersist: implicit test that +// a create without an intervening leave leaves the first room around (because +// it still has the player). The "leave" is what should delete it. +func TestFlow_CreateRoom_LeaveWhilePlayerIsStillMember_DeletesProperly(t *testing.T) { + resetStore() + alice := RegisterPlayer("alice") + + r, _ := CreatePvpRoom(alice) + _ = JoinRoom(r.ID, alice) + // Sanity: alice.RoomID is set. + if alice.RoomID != r.ID { + t.Fatalf("alice.RoomID = %d, want %d", alice.RoomID, r.ID) + } + + LeaveRoom(alice) + + // RoomID must be cleared so a follow-up LeaveRoom is a safe no-op. + if alice.RoomID != 0 { + t.Errorf("alice.RoomID = %d after leave, want 0", alice.RoomID) + } + + // Idempotent leave: calling again should not panic or create side effects. + LeaveRoom(alice) + if len(GetAllRooms()) != 0 { + t.Errorf("expected 0 rooms after idempotent leave, got %d", len(GetAllRooms())) + } +} diff --git a/server/lobby/room.go b/server/lobby/room.go index 9745bbe..db1d6ad 100644 --- a/server/lobby/room.go +++ b/server/lobby/room.go @@ -80,6 +80,12 @@ type Room struct { // Created in CreatePvpRoom; nil for PVE rooms (single player, no concurrent goroutine). GameOverCh chan struct{} + // RematchCh is closed by whichever player's gameoverState accepts a GameReset + // request first, signalling the other player's gameoverState to follow into + // the fresh game. Lazily created by the first goroutine entering gameoverState. + // Only used for PVP rooms; PVE has a single goroutine so no cross-sync needed. + RematchCh chan struct{} + CreatedAt time.Time LastActive time.Time } diff --git a/server/state/flow_test.go b/server/state/flow_test.go new file mode 100644 index 0000000..e968f8f --- /dev/null +++ b/server/state/flow_test.go @@ -0,0 +1,323 @@ +package state + +import ( + "testing" + "time" + + "github.com/tiennm99/gomoku/server/consts" + "github.com/tiennm99/gomoku/server/game" + "github.com/tiennm99/gomoku/server/lobby" + "github.com/tiennm99/gomoku/server/protocol" +) + +// End-to-end flow tests that drive the state machine through realistic user +// journeys. These catch bugs that unit tests on individual states miss — in +// particular ghosted rooms left behind after a leave, and broken rematch +// synchronisation between two player goroutines. + +// runState invokes a state's Next on the player and returns (nextID, err). +// Used where callers need to drive the machine one step at a time. +func runState(id consts.StateID, p *lobby.Player) (consts.StateID, error) { + s, ok := registry[id] + if !ok { + return 0, ErrClientExit + } + return s.Next(p) +} + +// TestFlow_HomeCreateLeaveCreate_OneRoomVisible simulates the exact reported +// bug: from home, create a PVP room, leave it from waiting state, create +// another, then assert lobby.GetAllRooms returns only the second room. +// If a "ghost" room is left behind anywhere in the state machine this fails. +func TestFlow_HomeCreateLeaveCreate_OneRoomVisible(t *testing.T) { + alice := makeRegisteredPlayer(t, "alice") + + // home → create room 1 → waiting + alice.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_CreateRoom{CreateRoom: &protocol.CreateRoomRequest{}}, + } + next, err := runState(consts.StateHome, alice) + if err != nil { + t.Fatalf("home create: %v", err) + } + if next != consts.StateWaiting { + t.Fatalf("expected StateWaiting, got %d", next) + } + + // waiting → client exit → home (room 1 should be deleted) + alice.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}, + } + next, err = runState(consts.StateWaiting, alice) + if err != nil { + t.Fatalf("waiting exit: %v", err) + } + if next != consts.StateHome { + t.Fatalf("expected StateHome after exit, got %d", next) + } + + // Critical check: after leaving, the store should contain 0 rooms. + rooms := lobby.GetAllRooms() + if len(rooms) != 0 { + ids := make([]int64, 0, len(rooms)) + for _, r := range rooms { + ids = append(ids, r.ID) + } + t.Fatalf("after leave: expected 0 rooms, got %d: %v", len(rooms), ids) + } + + // home → create room 2 → waiting + alice.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_CreateRoom{CreateRoom: &protocol.CreateRoomRequest{}}, + } + next, err = runState(consts.StateHome, alice) + if err != nil { + t.Fatalf("home create 2: %v", err) + } + if next != consts.StateWaiting { + t.Fatalf("expected StateWaiting, got %d", next) + } + + // GetRooms must return exactly 1 room (the fresh room 2). + rooms = lobby.GetAllRooms() + if len(rooms) != 1 { + ids := make([]int64, 0, len(rooms)) + for _, r := range rooms { + ids = append(ids, r.ID) + } + t.Fatalf("expected 1 room after second create, got %d: %v", len(rooms), ids) + } +} + +// TestFlow_RoomListReflectsCreateLeaveCycle verifies that after drive the +// state machine through create→leave→create, a follow-up GetAllRooms returns +// exactly the one currently-active room. Directly targets Bug 2. +func TestFlow_RoomListReflectsCreateLeaveCycle(t *testing.T) { + alice := makeRegisteredPlayer(t, "alice") + + // Baseline. + if n := len(lobby.GetAllRooms()); n != 0 { + t.Fatalf("precondition: store should be empty, got %d rooms", n) + } + + // Create → 1 room. + alice.CmdCh <- &protocol.Request{Payload: &protocol.Request_CreateRoom{CreateRoom: &protocol.CreateRoomRequest{}}} + if _, err := runState(consts.StateHome, alice); err != nil { + t.Fatalf("home create: %v", err) + } + if n := len(lobby.GetAllRooms()); n != 1 { + t.Fatalf("after create 1: expected 1 room, got %d", n) + } + + // Leave → 0 rooms. + alice.CmdCh <- &protocol.Request{Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}} + if _, err := runState(consts.StateWaiting, alice); err != nil { + t.Fatalf("waiting exit: %v", err) + } + if n := len(lobby.GetAllRooms()); n != 0 { + t.Fatalf("after leave: expected 0 rooms, got %d", n) + } + + // Create again → 1 room (not 2). + alice.CmdCh <- &protocol.Request{Payload: &protocol.Request_CreateRoom{CreateRoom: &protocol.CreateRoomRequest{}}} + if _, err := runState(consts.StateHome, alice); err != nil { + t.Fatalf("home create 2: %v", err) + } + if n := len(lobby.GetAllRooms()); n != 1 { + t.Fatalf("after create 2: expected 1 room, got %d — Bug 2 reproduced", n) + } +} + +// TestFlow_HomeExit_DoesNotKillSession verifies that ClientExit from home +// keeps the session alive (returns StateHome, not ErrClientExit) so the +// client isn't stuck after pressing a Back/Leave button in lobby. +func TestFlow_HomeExit_DoesNotKillSession(t *testing.T) { + alice := makeRegisteredPlayer(t, "alice") + alice.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_ClientExit{ClientExit: &protocol.ClientExitRequest{}}, + } + + next, err := runState(consts.StateHome, alice) + if err != nil { + t.Fatalf("home exit should not return error, got %v", err) + } + if next != consts.StateHome { + t.Fatalf("expected StateHome, got %d", next) + } + + // A ClientExitResponse should have been queued so the client knows to redraw. + found := false + for _, r := range drainSend(alice) { + if _, ok := r.Payload.(*protocol.Response_ClientExit); ok { + found = true + break + } + } + if !found { + t.Error("expected ClientExitResponse from home.ClientExit path") + } +} + +// TestFlow_Rematch_BothPlayersTransition is the guard for Bug 1. +// Two players finish a game and both land in gameoverState; only one clicks +// Play Again. Both player goroutines must transition to gamePvpState — +// otherwise the peer is stuck and cannot move when the game restarts. +func TestFlow_Rematch_BothPlayersTransition(t *testing.T) { + black, white, room := setupFinishedPvpRoom(t) + + // Assert preconditions: room is finished, both players wired. + if room.Status != lobby.RoomStatusFinished { + t.Fatalf("precondition: room should be Finished, got %d", room.Status) + } + + // Black clicks Play Again. + black.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_GameReset{GameReset: &protocol.GameResetRequest{}}, + } + + // Start white's gameoverState.Next in a goroutine — it should unblock via RematchCh. + whiteDone := make(chan consts.StateID, 1) + whiteErr := make(chan error, 1) + go func() { + next, err := runState(consts.StateGameOver, white) + whiteDone <- next + whiteErr <- err + }() + + // Let white's goroutine enter gameoverState (and create/publish RematchCh). + time.Sleep(20 * time.Millisecond) + + // Black processes its GameReset. + blackNext, err := runState(consts.StateGameOver, black) + if err != nil { + t.Fatalf("black gameover: %v", err) + } + if blackNext != consts.StateGamePvp { + t.Fatalf("black expected StateGamePvp, got %d", blackNext) + } + + // White must unblock and transition to gamePvp as well. + select { + case next := <-whiteDone: + if next != consts.StateGamePvp { + t.Errorf("white expected StateGamePvp after rematch, got %d", next) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("white goroutine did not transition to StateGamePvp within 500ms — rematch sync broken") + } + if err := <-whiteErr; err != nil { + t.Errorf("white error: %v", err) + } + + // Room state must be reset and ready for a fresh game. + room.RLock() + status := room.Status + nMoves := len(room.MoveHistory) + room.RUnlock() + if status != lobby.RoomStatusPlaying { + t.Errorf("room.Status after rematch = %d, want Playing", status) + } + if nMoves != 0 { + t.Errorf("room.MoveHistory should be empty after reset, got %d moves", nMoves) + } + + // Both players should have received a fresh GameStartingResponse. + for name, p := range map[string]*lobby.Player{"black": black, "white": white} { + found := false + for _, r := range drainSend(p) { + if _, ok := r.Payload.(*protocol.Response_GameStarting); ok { + found = true + break + } + } + if !found { + t.Errorf("%s player did not receive GameStartingResponse after rematch", name) + } + } +} + +// TestFlow_Rematch_NewGameAcceptsMovesFromBoth verifies that after rematch, +// the fresh game actually accepts moves. Regression guard for the reported +// bug where the board cleared but clicks did nothing. +// +// We drive black's gamePvpState in a goroutine, feed it a move, and then +// shut it down via a game-over signal so the goroutine exits cleanly. +func TestFlow_Rematch_NewGameAcceptsMovesFromBoth(t *testing.T) { + black, white, room := setupFinishedPvpRoom(t) + _ = white + + // Trigger rematch: reset room + broadcast fresh GameStartingResponse. + if _, err := handleGameReset(room); err != nil { + t.Fatalf("handleGameReset: %v", err) + } + + // Drain the GameStartingResponse sent via broadcast. + drainSend(black) + drainSend(white) + + // Precondition: fresh board, Playing status, Black's turn. + room.RLock() + if room.Status != lobby.RoomStatusPlaying { + room.RUnlock() + t.Fatalf("room not Playing after reset") + } + if room.CurrentTurn != game.Black { + room.RUnlock() + t.Errorf("CurrentTurn = %v, want Black", room.CurrentTurn) + } + if room.Board.MoveCount() != 0 { + room.RUnlock() + t.Errorf("Board.MoveCount = %d, want 0", room.Board.MoveCount()) + } + room.RUnlock() + + // Run black's gamePvpState in a goroutine. Feed it one GameMove, then + // trigger game-over to make the goroutine return. If the rematch-broken + // bug were still present, the state would silently drop the move. + done := make(chan consts.StateID, 1) + go func() { + next, _ := runState(consts.StateGamePvp, black) + done <- next + }() + + black.CmdCh <- &protocol.Request{ + Payload: &protocol.Request_GameMove{GameMove: &protocol.GameMoveRequest{Row: 7, Col: 7}}, + } + + // Give the goroutine a moment to process the move, then close the game + // so gamePvpState.Next returns (via GameOverCh select). + time.Sleep(50 * time.Millisecond) + signalGameOver(room) + + select { + case next := <-done: + if next != consts.StateGameOver { + t.Errorf("expected StateGameOver after signalGameOver, got %d", next) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("black's gamePvpState did not exit within 500ms after signalGameOver") + } + + // Move was applied: (7,7) has a black stone, turn flipped to White. + room.RLock() + piece := room.Board.GetPiece(7, 7) + nMoves := room.Board.MoveCount() + room.RUnlock() + if piece != game.Black { + t.Errorf("after move, (7,7) piece = %v, want Black", piece) + } + if nMoves != 1 { + t.Errorf("Board.MoveCount = %d, want 1", nMoves) + } + + // Black should have received exactly one GameMoveSuccessResponse. + gotMoveAck := 0 + for _, r := range drainSend(black) { + if _, ok := r.Payload.(*protocol.Response_GameMoveSuccess); ok { + gotMoveAck++ + } + } + if gotMoveAck != 1 { + t.Errorf("black received %d GameMoveSuccessResponse, want 1", gotMoveAck) + } +} diff --git a/server/state/gameover.go b/server/state/gameover.go index 0d9f34c..bbec59f 100644 --- a/server/state/gameover.go +++ b/server/state/gameover.go @@ -4,16 +4,23 @@ import ( "math/rand" "github.com/tiennm99/gomoku/server/consts" - "github.com/tiennm99/gomoku/server/lobby" "github.com/tiennm99/gomoku/server/game" + "github.com/tiennm99/gomoku/server/lobby" "github.com/tiennm99/gomoku/server/pkg/log" "github.com/tiennm99/gomoku/server/protocol" ) // gameOverState waits for the player to request a rematch or exit. -// On GameResetRequest: resets the board, broadcasts GameStartingResponse, -// and transitions back to the appropriate game state (PVP or PVE). -// On ClientExit: removes from room and goes home. +// +// In PVP both player goroutines enter this state after the game ends. When +// EITHER player sends GameResetRequest, the room is reset and both goroutines +// must transition to gamePvpState together — otherwise one player would keep +// acting in the fresh game while the other was still stuck in gameover, +// silently dropping their moves. +// +// The cross-goroutine sync uses room.RematchCh (pattern mirrors StartCh in +// waiting state). The first goroutine to enter gameover creates the channel; +// whoever accepts the GameReset closes it, waking the other goroutine. type gameOverState struct{} func (*gameOverState) Next(player *lobby.Player) (consts.StateID, error) { @@ -23,28 +30,47 @@ func (*gameOverState) Next(player *lobby.Player) (consts.StateID, error) { return consts.StateHome, nil } + // Lazily create RematchCh on first entry so both goroutines share it. + room.Lock() + if room.RematchCh == nil { + room.RematchCh = make(chan struct{}) + } + rematchCh := room.RematchCh + roomType := room.RoomType + room.Unlock() + for { - req, reqOk := <-player.CmdCh - if !reqOk { - leaveRoom(player, room) - return 0, ErrClientExit - } + select { + case <-rematchCh: + // Peer accepted a rematch — follow them into the new game. + if roomType == lobby.RoomTypePve { + return consts.StateGamePve, nil + } + return consts.StateGamePvp, nil - switch req.Payload.(type) { - case *protocol.Request_GameReset: - return handleGameReset(room) + case req, reqOk := <-player.CmdCh: + if !reqOk { + leaveRoom(player, room) + return 0, ErrClientExit + } - case *protocol.Request_ClientExit: - leaveRoom(player, room) - return consts.StateHome, nil + switch req.Payload.(type) { + case *protocol.Request_GameReset: + return handleGameReset(room) - default: - log.Errorf("[gameover] player %d: unexpected %T, ignoring\n", player.ID, req.Payload) + case *protocol.Request_ClientExit: + leaveRoom(player, room) + return consts.StateHome, nil + + default: + log.Errorf("[gameover] player %d: unexpected %T, ignoring\n", player.ID, req.Payload) + } } } } -// handleGameReset resets the room and starts a fresh game. +// handleGameReset resets the room, broadcasts a fresh GameStartingResponse, +// and closes RematchCh so the peer gameover goroutine transitions in lockstep. func handleGameReset(room *lobby.Room) (consts.StateID, error) { seed := rand.Int63() room.Lock() @@ -56,6 +82,11 @@ func handleGameReset(room *lobby.Room) (consts.StateID, error) { // Fresh GameOverCh for the new game round so player goroutines can sync again. room.GameOverCh = make(chan struct{}) } + // Wake the peer gameover goroutine so both transition into the new game. + if room.RematchCh != nil { + close(room.RematchCh) + room.RematchCh = nil + } room.Unlock() // Broadcast fresh GameStartingResponse to all players.