delay teardown ownership until duplicate login check passes

[ZECHEESELORD]

Fixes #1740.

Prevents ConnectedPlayer teardown from running for a rejected duplicate login attempt by only taking teardown ownership after the duplicate login gate passes.

Tests:

• Added a regression test to verify a rejected duplicate login does not reach teardown/unregister logic
• Added a test to verify owned players still teardown on disconnect

[WouterGritter]

I think this can be fixed even better by only ever instantiating ConnectedPlayer after VelocityServer#canRegisterConnection returns true. This avoid ever creating a BossBarManager/ChatQueue/whatever (this is done in ConnectedPlayer's constructor).

VelocityServer#canRegisterConnection currently consumes the ConnectedPlayer, but only ever checks its username and UUID. We can consume a com.velocitypowered.api.proxy.player.PlayerInfo instead, which is effectively a record of String username, UUID uuid, or just consume the username/UUID to perform the check.

Edit:
The only issue is that we won't have access to ConnectedPlayer#disconnect0 then. This method only references the VelocityServer instance and the connection (mcConnection in AuthSessionHandler's context). Maybe we can pull this method into MinecraftConnection, and have ConnectedPlayer#disconnect0 just call this method on it's own connection to avoid duplicate code?

Edit # 2: The canonical solution is to use GameProfile, which contains the player's name and uuid, as we already have access to this through profileEvent.getGameProfile() here. See #1748

[ZECHEESELORD]

I think this can be fixed even better by only ever instantiating ConnectedPlayer after VelocityServer#canRegisterConnection returns true. This avoid ever creating a BossBarManager/ChatQueue/whatever (this is done in ConnectedPlayer's constructor).

VelocityServer#canRegisterConnection currently consumes the ConnectedPlayer, but only ever checks its username and UUID. We can consume a com.velocitypowered.api.proxy.player.PlayerInfo instead, which is effectively a record of String username, UUID uuid, or just consume the username/UUID to perform the check.

Edit: The only issue is that we won't have access to ConnectedPlayer#disconnect0 then. This method only references the VelocityServer instance and the connection (mcConnection in AuthSessionHandler's context). Maybe we can pull this method into MinecraftConnection, and have ConnectedPlayer#disconnect0 just call this method on it's own connection to avoid duplicate code?

Edit # 2: The canonical solution is to use GameProfile, which contains the player's name and uuid, as we already have access to this through profileEvent.getGameProfile() here. See #1748

this is definitely cleaner than my version-

#1744 does add regression coverage. If you feel that it is necessary, feel free to cherrypick the test commit from here, or I can adapt the tests to #1748’s shape.

happy to defer to #1748 if that’s the preferred direction.

[WouterGritter]

I'll leave it up to the maintainers whether or not we need unit tests in the second PR, or @ZECHEESELORD if you have the time feel free to cherry pick my commits into this PR and adapt the tests that you already have in place.

[ZECHEESELORD]

I'll leave it up to the maintainers whether or not we need unit tests in the second PR, or @ZECHEESELORD if you have the time feel free to cherry pick my commits into this PR and adapt the tests that you already have in place.

Thanks- I’ll fold your commits into #1744 when I get a chance (and then adapt the tests, so we have regression coverage)

[4drian3d]

The problem with this is that disconnections caused by a duplicate login are represented in the API, specifically in the DisconnectEvent (https://jd.papermc.io/velocity/3.5.0/com/velocitypowered/api/event/connection/DisconnectEvent.LoginStatus.html#CONFLICTING_LOGIN), so by preventing the creation of the player object and its teardown, the event would not be triggered

[ZECHEESELORD]

That’s a fair point. I think I got a bit too focused on the internal cleanup path and not enough on the API surface.
I’m going to move this PR back to the original ownership based fix (which keeps that behavior intact and still fixes #1740).

[WouterGritter]

The problem with this is that disconnections caused by a duplicate login are represented in the API, specifically in the DisconnectEvent (https://jd.papermc.io/velocity/3.5.0/com/velocitypowered/api/event/connection/DisconnectEvent.LoginStatus.html#CONFLICTING_LOGIN), so by preventing the creation of the player object and its teardown, the event would not be triggered

The problem is now that we are already exposing the Player object to plugins through this event. Plugins can do whatever they want with the Player, including registering bossbars. Currently disconnected() only cleans up the bossbars, which could be omitted if we didn't ever expose the ConnectedPlayer object, but because we do we must consider a plugin registering bossbars on player disconnect. This sounds like weird behavior, but it's something that's possible nevertheless.

Also, it's interesting that a DisconnectEvent can be called on a player who was never connected, as far as the plugins go. Is this intentional?