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.

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):

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()

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

@@ -1069,6 +1069,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:
@@ -1080,9 +1084,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:

tests/integration_tests/test_game.py

@@ -632,7 +632,12 @@ async def test_double_host_message(lobby_server):
         "mod": "faf",
         "visibility": "public",
     })
-    await read_until_command(proto, "game_launch", timeout=10)
+    msg = await read_until_command(proto, "notice", timeout=10)
+    assert msg == {
+        "command": "notice",
+        "style": "error",
+        "text": "Can't host a game while in state STARTING_GAME",
+    }
 
 
 @fast_forward(30)
@@ -661,7 +666,12 @@ async def test_double_join_message(lobby_server):
         "command": "game_join",
         "uid": game_id
     })
-    await read_until_command(guest_proto, "game_launch", timeout=10)
+    msg = await read_until_command(guest_proto, "notice", timeout=10)
+    assert msg == {
+        "command": "notice",
+        "style": "error",
+        "text": "Can't join a game while in state STARTING_GAME",
+    }
 
 
 @fast_forward(30)

tests/integration_tests/test_matchmaker_violations.py

@@ -120,12 +120,24 @@ async def test_violation_persisted_across_logins(mocker, lobby_server):
 
     await read_until_command(host, "match_cancelled", timeout=120)
     await read_until_command(guest, "match_cancelled", timeout=10)
+    await read_until_command(host, "game_info", timeout=10, state="closed")
+
+    # DEPRECATED: Because the game sends a game_launch to the guest after the
+    # host times out, we need to simulate opening and closing the game before
+    # we can queue again. If we wait for the timeout, our violations expire.
+    await open_fa(guest)
+    await guest.send_message({
+        "target": "game",
+        "command": "GameState",
+        "args": ["Ended"]
+    })
 
     # Second time searching there is no ban
     await start_search(host)
     await start_search(guest)
     await read_until_command(host, "match_cancelled", timeout=120)
     await read_until_command(guest, "match_cancelled", timeout=10)
+    await read_until_command(host, "game_info", timeout=10, state="closed")
 
     # Third time searching there is a short ban
     await host.send_message({
@@ -177,13 +189,34 @@ async def test_violation_persisted_across_parties(mocker, lobby_server):
 
     await read_until_command(host, "match_cancelled", timeout=120)
     await read_until_command(guest, "match_cancelled", timeout=10)
+    await read_until_command(host, "game_info", timeout=10, state="closed")
+
+    # DEPRECATED: Because the game sends a game_launch to the guest after the
+    # host times out, we need to simulate opening and closing the game before
+    # we can queue again. If we wait for the timeout, our violations expire.
+    await open_fa(guest)
+    await guest.send_message({
+        "target": "game",
+        "command": "GameState",
+        "args": ["Ended"]
+    })
 
     # Second time searching there is no ban
     await start_search(host)
     await start_search(guest)
     await read_until_command(host, "match_cancelled", timeout=120)
     await read_until_command(guest, "match_cancelled", timeout=10)
 
+    # DEPRECATED: Because the game sends a game_launch to the guest after the
+    # host times out, we need to simulate opening and closing the game before
+    # we can queue again. If we wait for the timeout, our violations expire.
+    await open_fa(guest)
+    await guest.send_message({
+        "target": "game",
+        "command": "GameState",
+        "args": ["Ended"]
+    })
+
     # Third time searching there is a short ban
     await host.send_message({
         "command": "game_matchmaking",

tests/integration_tests/test_server.py

@@ -733,6 +733,35 @@ async def test_game_host_name_non_ascii(lobby_server):
     }
 
 
+@fast_forward(30)
+async def test_game_host_then_queue(lobby_server):
+    player_id, session, proto = await connect_and_sign_in(
+        ("test", "test_password"),
+        lobby_server
+    )
+    await read_until_command(proto, "game_info")
+
+    # Send game_host but don't send GameState: Idle, then immediately queue
+    await proto.send_message({
+        "command": "game_host",
+        "title": "My Game",
+        "mod": "faf",
+        "visibility": "public",
+    })
+    await proto.send_message({
+        "command": "game_matchmaking",
+        "state": "start",
+        "faction": "uef"
+    })
+
+    msg = await read_until_command(proto, "notice", timeout=10)
+    assert msg == {
+        "command": "notice",
+        "style": "error",
+        "text": "Can't join a queue while test is in state STARTING_GAME",
+    }
+
+
 @fast_forward(10)
 async def test_play_game_while_queueing(lobby_server):
     player_id, session, proto = await connect_and_sign_in(

tests/unit_tests/conftest.py

@@ -74,7 +74,7 @@ async def violation_service():
 
 
 @pytest.fixture
-def game_connection(
+async def game_connection(
     request,
     database,
     game,

tests/unit_tests/test_gameconnection.py

@@ -62,12 +62,12 @@ async def fake_send_dc(player_id):
         return "OK"
 
     # Set up a peer that will disconnect without error
-    ok_disconnect = mock.create_autospec(GameConnection)
+    ok_disconnect = mock.create_autospec(game_connection)
     ok_disconnect.state = GameConnectionState.CONNECTED_TO_HOST
     ok_disconnect.send_DisconnectFromPeer = fake_send_dc
 
     # Set up a peer that will throw an exception
-    fail_disconnect = mock.create_autospec(GameConnection)
+    fail_disconnect = mock.create_autospec(game_connection)
     fail_disconnect.send_DisconnectFromPeer.return_value = Exception("Test exception")
     fail_disconnect.state = GameConnectionState.CONNECTED_TO_HOST
 
@@ -81,7 +81,7 @@ async def fake_send_dc(player_id):
 
 
 async def test_connect_to_peer(game_connection):
-    peer = mock.create_autospec(GameConnection)
+    peer = mock.create_autospec(game_connection)
 
     await game_connection.connect_to_peer(peer)
 
@@ -92,7 +92,7 @@ async def test_connect_to_peer_disconnected(game_connection):
     # Weak reference has dissapeared
     await game_connection.connect_to_peer(None)
 
-    peer = mock.create_autospec(GameConnection)
+    peer = mock.create_autospec(game_connection)
     peer.send_ConnectToPeer.side_effect = DisconnectedError("Test error")
 
     # The client disconnects right as we send the message

tests/unit_tests/test_lobbyconnection.py

@@ -264,7 +264,7 @@ async def test_launch_game(lobbyconnection, game, player_factory):
     assert lobbyconnection.player.game == game
     assert lobbyconnection.player.game_connection == lobbyconnection.game_connection
     assert lobbyconnection.game_connection.player == lobbyconnection.player
-    assert lobbyconnection.player.state == PlayerState.IDLE
+    assert lobbyconnection.player.state == PlayerState.STARTING_GAME
     lobbyconnection.send.assert_called_once()
 
 
@@ -791,7 +791,6 @@ async def test_game_connection_not_restored_if_game_state_prohibits(
     game_service: GameService,
     game_stats_service,
     game_state,
-    mocker,
     database
 ):
     del lobbyconnection.player.game_connection
@@ -998,8 +997,9 @@ async def test_command_matchmaker_info(
     })
 
 
-async def test_connection_lost(lobbyconnection):
+async def test_connection_lost(lobbyconnection, players):
     lobbyconnection.game_connection = mock.create_autospec(GameConnection)
+    lobbyconnection.game_connection.player = players.hosting
     await lobbyconnection.on_connection_lost()
 
     lobbyconnection.game_connection.on_connection_lost.assert_called_once()