Issue/#995 Improve player state tracking (#997)

* Refactor state checks in restore_game_session

* Add STARTING_GAME state and ensure GameConnections are cleaned up

We need to protect against double game_host/join messages which means we
need to establish a time window after a game_launch message is sent
during which we are waiting to see that the game has opened and don't
let the client do any other game related actions. This also means we
need to clean up the GameConnection objects which would previously stick
around forever if a client never responded to a game_launch message.

* Only clear PlayerOptions when player disconnects intentionally.

server/gameconnection.py

@@ -42,7 +42,8 @@ def __init__(
         protocol: Protocol,
         player_service: PlayerService,
         games: GameService,
-        state: GameConnectionState = GameConnectionState.INITIALIZING
+        state: GameConnectionState = GameConnectionState.INITIALIZING,
+        setup_timeout: int = 60,
     ):
         """
         Construct a new GameConnection
@@ -52,38 +53,35 @@ def __init__(
             f"{self.__class__.__qualname__}.{game.id}"
         )
         self._db = database
-        self._logger.debug("GameConnection initializing")
 
         self.protocol = protocol
         self._state = state
         self.game_service = games
         self.player_service = player_service
 
-        self._player = player
+        self.player = player
         player.game_connection = self  # Set up weak reference to self
-        self._game = game
+        self.game = game
 
-        self.finished_sim = False
+        self.setup_timeout = setup_timeout
 
-    @property
-    def state(self) -> GameConnectionState:
-        return self._state
+        self.finished_sim = False
 
-    @property
-    def game(self) -> Game:
-        return self._game
+        self._logger.debug("GameConnection initializing")
+        if self.state is GameConnectionState.INITIALIZING:
+            asyncio.get_event_loop().create_task(
+                self.timeout_game_connection(setup_timeout)
+            )
 
-    @game.setter
-    def game(self, val: Game):
-        self._game = val
+    async def timeout_game_connection(self, timeout):
+        await asyncio.sleep(timeout)
+        if self.state is GameConnectionState.INITIALIZING:
+            self._logger.debug("GameConection timed out...")
+            await self.abort("Player took too long to start the game")
 
     @property
-    def player(self) -> Player:
-        return self._player
-
-    @player.setter
-    def player(self, val: Player):
-        self._player = val
+    def state(self) -> GameConnectionState:
+        return self._state
 
     def is_host(self) -> bool:
         if not self.game or not self.player:
@@ -122,6 +120,7 @@ async def _handle_idle_state(self):
             self.game.add_game_connection(self)
             self.player.state = PlayerState.HOSTING
         else:
+            self._state = GameConnectionState.INITIALIZED
             self.player.state = PlayerState.JOINING
 
     async def _handle_lobby_state(self):
@@ -486,10 +485,10 @@ async def handle_game_state(self, state: str):
                     )
         # Signals that the FA executable has been closed
         elif state == "Ended":
-            await self.on_connection_lost()
+            await self.on_connection_closed()
         self._mark_dirty()
 
-    async def handle_game_ended(self, *args:  list[Any]):
+    async def handle_game_ended(self, *args: list[Any]):
         """
         Signals that the simulation has ended.
         """
@@ -592,7 +591,21 @@ async def disconnect_all_peers(self):
                     exc_info=True
                 )
 
+    async def on_connection_closed(self):
+        """
+        The connection is closed by the player.
+        """
+        try:
+            await self.game.disconnect_player(self.player)
+        except Exception as e:  # pragma: no cover
+            self._logger.exception(e)
+        finally:
+            await self.abort()
+
     async def on_connection_lost(self):
+        """
+        The connection is lost due to a disconnect from the lobby server.
+        """
         try:
             await self.game.remove_game_connection(self)
         except Exception as e:  # pragma: no cover

server/games/game.py

@@ -103,6 +103,7 @@ def __init__(
         self.displayed_rating_range = displayed_rating_range or InclusiveRange()
         self.enforce_rating_range = enforce_rating_range
         self.matchmaker_queue_id = matchmaker_queue_id
+        self.setup_timeout = setup_timeout
         self.state = GameState.INITIALIZING
         self._connections = {}
         self._configured_player_ids: set[int] = set()
@@ -390,6 +391,17 @@ def add_game_connection(self, game_connection):
         self._logger.info("Added game connection %s", game_connection)
         self._connections[game_connection.player] = game_connection
 
+    async def disconnect_player(self, player: Player):
+        if player.game_connection not in self._connections.values():
+            return
+
+        self._configured_player_ids.discard(player.id)
+
+        if self.state is GameState.LOBBY and player.id in self._player_options:
+            del self._player_options[player.id]
+
+        await self.remove_game_connection(player.game_connection)
+
     async def remove_game_connection(self, game_connection):
         """
         Remove a game connection from this game.
@@ -403,10 +415,6 @@ async def remove_game_connection(self, game_connection):
         player = game_connection.player
         del self._connections[player]
         del player.game
-        self._configured_player_ids.discard(player.id)
-
-        if self.state is GameState.LOBBY and player.id in self._player_options:
-            del self._player_options[player.id]
 
         self._logger.info("Removed game connection %s", game_connection)
 

server/ladder_service/ladder_service.py

@@ -611,8 +611,6 @@ def make_game_options(player: Player) -> GameLaunchOptions:
             game_id = game.id if game else None
             msg = {"command": "match_cancelled", "game_id": game_id}
             for player in all_players:
-                if player.state == PlayerState.STARTING_AUTOMATCH:
-                    player.state = PlayerState.IDLE
                 player.write_message(msg)
 
             if abandoning_players:

server/lobbyconnection.py

@@ -216,6 +216,35 @@ async def on_message_received(self, message):
             self._logger.exception(e)
             await self.abort("Error processing command")
 
+    def ice_only(func):
+        """
+        Ensures that a handler function is not invoked from a non ICE client.
+        """
+        @wraps(func)
+        async def wrapper(self, message):
+            if self._attempted_connectivity_test:
+                raise ClientError("Cannot join game. Please update your client to the newest version.")
+            return await func(self, message)
+        return wrapper
+
+    def player_idle(state_text):
+        """
+        Ensures that a handler function is not invoked unless the player state
+        is IDLE.
+        """
+        def decorator(func):
+            @wraps(func)
+            async def wrapper(self, message):
+                if self.player.state != PlayerState.IDLE:
+                    raise ClientError(
+                        f"Can't {state_text} while in state "
+                        f"{self.player.state.name}",
+                        recoverable=True
+                    )
+                return await func(self, message)
+            return wrapper
+        return decorator
+
     async def command_ping(self, msg):
         await self.send({"command": "pong"})
 
@@ -716,22 +745,35 @@ async def on_player_login(
 
         await self.send_game_list()
 
+    @ice_only
+    @player_idle("reconnect to a game")
     async def command_restore_game_session(self, message):
         assert self.player is not None
 
-        game_id = int(message.get("game_id"))
+        game_id = int(message["game_id"])
 
         # Restore the player's game connection, if the game still exists and is live
         if not game_id or game_id not in self.game_service:
-            await self.send_warning("The game you were connected to does no longer exist")
+            await self.send_warning("The game you were connected to no longer exists")
             return
 
-        game = self.game_service[game_id]  # type: Game
-        if game.state is not GameState.LOBBY and game.state is not GameState.LIVE:
+        game: Game = self.game_service[game_id]
+
+        if game.state not in (GameState.LOBBY, GameState.LIVE):
+            # NOTE: Getting here is only possible if you join within the
+            # 1 second window between the game ending and the game being removed
+            # from the game service.
             await self.send_warning("The game you were connected to is no longer available")
             return
 
-        self._logger.debug("Restoring game session of player %s to game %s", self.player, game)
+        if (
+            game.state is GameState.LIVE
+            and self.player.id not in (player.id for player in game.players)
+        ):
+            await self.send_warning("You are not part of this game")
+            return
+
+        self._logger.info("Restoring game session of player %s to game %s", self.player, game)
         self.game_connection = GameConnection(
             database=self._db,
             game=game,
@@ -829,35 +871,6 @@ async def command_avatar(self, message):
         else:
             raise KeyError("invalid action")
 
-    def ice_only(func):
-        """
-        Ensures that a handler function is not invoked from a non ICE client.
-        """
-        @wraps(func)
-        async def wrapper(self, message):
-            if self._attempted_connectivity_test:
-                raise ClientError("Cannot join game. Please update your client to the newest version.")
-            return await func(self, message)
-        return wrapper
-
-    def player_idle(state_text):
-        """
-        Ensures that a handler function is not invoked unless the player state
-        is IDLE.
-        """
-        def decorator(func):
-            @wraps(func)
-            async def wrapper(self, message):
-                if self.player.state != PlayerState.IDLE:
-                    raise ClientError(
-                        f"Can't {state_text} while in state "
-                        f"{self.player.state.name}",
-                        recoverable=True
-                    )
-                return await func(self, message)
-            return wrapper
-        return decorator
-
     @ice_only
     @player_idle("join a game")
     async def command_game_join(self, message):
@@ -1052,6 +1065,10 @@ def _prepare_launch_game(
     ):
         assert self.player is not None
         assert self.game_connection is None
+        assert self.player.state in (
+            PlayerState.IDLE,
+            PlayerState.STARTING_AUTOMATCH,
+        )
 
         # TODO: Fix setting up a ridiculous amount of cyclic pointers here
         if is_host:
@@ -1063,9 +1080,13 @@ def _prepare_launch_game(
             player=self.player,
             protocol=self.protocol,
             player_service=self.player_service,
-            games=self.game_service
+            games=self.game_service,
+            setup_timeout=game.setup_timeout,
         )
 
+        if self.player.state is PlayerState.IDLE:
+            self.player.state = PlayerState.STARTING_GAME
+
         self.player.game = game
         cmd = {
             "command": "game_launch",

server/players.py

@@ -21,6 +21,7 @@ class PlayerState(Enum):
     JOINING = 4
     SEARCHING_LADDER = 5
     STARTING_AUTOMATCH = 6
+    STARTING_GAME = 7
 
 
 class Player: