5-15 Zomby-games per day since server [re]start

[GrotheFAF]

A look at fafclient Replays - Live Games shows an increasing number of old games.
Last server restart was 2017-01-17

[Wesmania]

We've analysed an example zombie game (discussion on Slack in #general). The game was left hanging because of a GameConnection that wasn't removed. That GameConnection didn't belong to any player that played the game - it belonged to a player that tried and failed to connect to the lobby.

The logs show that an abort() method (used to clean up GameConnection) was called once for this player and was called before the only time this player's GameConnection was added to the list. This suggest it's either a race condition or we fail to call abort() in some situations.

[Wesmania]

It seems it's the latter - abort() is just called when STUN connectivity fails. We fail to remove GameConnection of a player after he leaves the lobby.

[Wesmania]

At least in one game case (id 6049916) a player tried to join a game, but failed to connect to host and quit before reaching the lobby. The server added his GameConnection to the game, but never removed it.

[GrotheFAF]

In the normal ended game there is an 'Removed game connection Player' before each 'Player.abort'.
(but I found 2 Player.abort were repeated (in under minute) in that game, didn't hurt in this case)
The 3 zombie games I looked at all had at least one Player.abort without a 'Removed game connection Player'.

[Wesmania]

I don't think that's the culprit - the examples I've seen had abort() called when a STUN connection to the host failed, so an abort() at that point would indeed not remove any GameConnection according to its code (we're not in the lobby yet). I think we rather fail to call abort() or _on_connection_lost() later.

[GrotheFAF]

you're talking about 6035382(?): (shortened the log for readability)

15:40:05: Connecting (Player(brodRga),Game(11)) to host (Player(MyriadDigits),Game(11)) using STUN  ## THE ACTUAL GAME HOST
15:40:07: (Player(brodRga),Game(11)).abort()  ## THAT SEEMS WRONG HERE
15:40:13: STUN between (Player(brodRga),Game(10)) (Player(MyriadDigits),Game(10)) failed
15:40:14: Added game connection (Player(brodRga),Game(10))  ## AND GOT ADDED ANYWAY
... connecting to other players (all with STUN), only 3 didn't fail:
15:40:14: Connecting (Player(brodRga),Game(11)) to host (Player(FalseProject),Game(11)) using STUN
15:40:14: Connecting (Player(brodRga),Game(11)) to host (Player(inZilcho),Game(11)) using STUN
15:40:14: Connecting (Player(brodRga),Game(11)) to host (Player(Mefistofel),Game(11)) using STUN
so not connected to game host, but added to the game and to 3 of the players?
and no disconnect/abort after that from this player <- that is the Problem

(same game other player) equal STUN fail without the abort:
15:40:39: Connecting (Player(Highsinger),Game(11)) to host (Player(MyriadDigits),Game(11)) using STUN
15:40:43: STUN between (Player(Highsinger),Game(11)) (Player(MyriadDigits),Game(11)) failed
15:40:43: Added game connection (Player(Highsinger),Game(11))
... connecting to other players, all STUN and mostly failed, but a disconnect/abort:
15:40:47: (Player(Highsinger),Game(12)).abort(Failed connecting to (Player(Achillesferse),Game(12))
15:40:47: Removed game connection (Player(Highsinger),Game(11))

that seems ok.
so... do we kinda agree?

[Wesmania]

Now that I look at the code again, calling abort() puts two important states into PlayerState.IDLE and GameConnectionState.ENDED, and within the game connection these two states block state changes completely. In particular that ENDED state means any further call to abort() does nothing (not even log itself).

I actually get the impression that while the method STUN is waiting, the client disconnects (and abort() is rightfully called), but the method that calls STUN (EstablishConnection) does not check for that and goes on (somehow connecting properly through TURN). Then it returns somewhere where we don't check if we already disconnected, either.

One solution would be to check if we disconnected after each method that can sleep in EstablishConnection, and fail appropriately. It's most likely the best one, since in such a situation all connections except the one we potentially created just now were cleaned up by abort() and we only need to clean up after ourselves - and once we return failure, functions that call it call abort() themselves.

[Wesmania]

IIRC this was fixed a while back :)

[louvenarde]

Apparently fixed as @Wesmania said, and there has been no recent occuring of ghost games swarms. Closing this for now.