Concurrency guarantees of connection events

I'm raising this issue to get clarification on what the concurrency guarantees are supposed to be and showing the possible problems you could face with the current behavior due to the current implementations possible racy behavior. I'm focusing here for the cases where the kick-existing-players has been turned off as that is the default behavior. However turning that on has multiple additional quirks that could be discussed.

Currently we are unregistering the connection on teardown even before the DisconnectEvent has been fired as can be seen here. Technically it is now possible for the player to login and trigger events related to post Mojang authorization like PermissionsSetupEvent or LoginEvent. This could lead to situations where we initialize some data on login event but the disconnect event ends up incorrectly clearing them. The DisconnectEvent event seems to have note about that "Velocity typically fires this event asynchronously and does not wait for a response." and the current behavior seems somewhat intended but in my opinion this doesn't make sense for post Mojang authorization.

The solution is quite simple, move the connection unregistration after the event has fired. But this also has some implications like what if some bad behaving plugin doesn't complete the event? This would block all further logins from the player until the proxy would be restarted but this argument seems moot to me as correctness should be preferred. Another argument could be made that this is very unlikely due to the high delay between user actions. Dealing with this in a plugin would require comparing the actual Player references which is very inconvenient.

Fixing just the disconnection isn't enough as the connection registration is done after LoginEvent as can be seen here. This could result in somewhat similar race where the player logins, cancels it and then reconnects immediately. This is also somewhat unlikely due to the default login rate limiting of 3s.

The solution here is to move it to when the AuthSessionHandler gets activated. This also has some implications like the getPlayer API would return the Player much earlier and plugins might assume all the state has been already initialized at this point which is no longer true. This could be fixed by adding additional conditions where we would not return the player even if its present in the collection. Additionally previously cancelling the LoginEvent when kick-existing-players is set to true would not kick the currently active session but now it would.

Well that was a lot! I might have missed something but hopefully that sums up the current state for at least when the kick-existing-players is turned off..

Start with the cited teardown in proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java and registration flow in proxy/src/main/java/com/velocitypowered/proxy/connection/client/AuthSessionHandler.java. Trace event ordering for disconnect, login, cancellation, and reconnection, then establish the intended concurrency guarantees and account for the implications described in the issue. Done requires an agreed behavior and an implementation plan.