[n4aaa]

This pr prevents stale connections of clients who unexpectedly lose their network connection

[WouterGritter]

Had a look at this. The mechanics of the patch are fine — the lock is released exactly once on every path (the finally covers the early markDisconnectFired() == false return and both throw paths), and computeDisconnectStatus still runs before removeFromMaps, so LoginStatus is unchanged. But I don't think it fixes the reported problem, and it breaks a guarantee we deliberately added in #892.

It largely doesn't hit the target scenario

When a client's network dies, the proxy only notices the dead socket after read-timeout (default 25s). A player reconnecting before that goes through registerConnection → forciblyEvict(existing), which still fires DisconnectEvent and awaits it (up to 30s) while holding the identity lock. That path is untouched here, so the stall remains on the more common path. This PR only helps the narrow window where the disconnect was already detected and its handlers happen to still be running.

Related gap: PlayerIdentityLock.acquire has no timeout for waiters — only the lock holder is watchdogged (PlayerRegistry.java:227-245). That asymmetry is the actual reason a reconnect can hang long enough for the client to give up.

Regressions

1. Per-identity ordering guarantee is gone. The class javadoc (PlayerRegistry.java:48-58) and unregisterConnection's javadoc (:125-135) both state that DisconnectEvent firing is serialized per identity — that's the point of the lock, mirroring BungeeCord's fix for PaperMC#1696. After this change the old session's DisconnectEvent can run concurrently with, or entirely after, the new session's LoginEvent/PostLoginEvent. Any plugin doing save-on-quit / load-on-join will read stale data or overwrite fresh data. The javadocs still claim the old guarantee.

2. Concrete in-tree breakage: queue state. runCleanup → ConnectedPlayer.disconnected() → queueManager.onLocalPlayerDisconnect(old) now runs outside the lock. That method (VelocityQueueManager.java:359-385) is keyed purely by UUID with no identity guard: it calls entry.setOffline(...) + schedules removal, or removePlayerEntirely when timeout ≤ 0. Its counterpart onLocalPlayerConnect (:342) only clears the offline flag if a pending timeout task exists. In the reconnect race the new session's fullyConnected() → onLocalPlayerConnect runs first with nothing to clear, then the old session's cleanup marks the live session's queue entries offline — so the reconnected player silently drops out of the queue when the timeout elapses. That's a regression in exactly the scenario this PR targets.

For contrast, PlayerDepotService.onPlayerDisconnect (:141-158) does have a currentPlayer != player guard and survives the reordering. The queue path never needed one because the lock made it impossible.

3. Player is out of the registry during DisconnectEvent. proxy.getPlayer(uuid) is empty, getAllPlayers() excludes them, count already decremented while handlers run. This actually matches upstream (ConnectedPlayer.teardown() unregisters before firing), so it's defensible — but it's a behaviour change from current CTD that isn't mentioned in the PR or the docs.

Smaller things

• The first catch (Throwable t) { return failedFuture(t); } runs after markDisconnectFired() has been won, so a throw from computeDisconnectStatus/removeFromMaps means nobody runs runCleanup: player.disconnected() never executes and teardownFuture never completes, hanging forciblyEvict's wait (:259) and shutdown's allOf(getTeardownFuture()). Same hole existed before via withLockReleasedOnFailure, so not a regression — but the new explicit catch is the natural place to fix it (runCleanup(player, t, "player") before returning).
• The second catch can double-run cleanup: fireDisconnectAndCleanup may already have called runCleanup internally (the !isLoginEventFired() branch, :284-288), and disconnected() isn't idempotent. Practically unreachable, but it makes the defensive block a hazard.
• doUnregisterLocked is now a misnomer; the new .thenCompose( continuation is indented 8 spaces where the file uses 4.

Suggested direction

Preferred: keep the lock held across DisconnectEvent and bound the waiter instead — add a timeout to PlayerIdentityLock.acquire and/or lower DISCONNECT_EVENT_TIMEOUT_SECONDS, so a reconnect fails fast or proceeds rather than hanging, without reordering plugin events. (Worth fixing the misleading velocity.error.already-connected-proxy kick that the current failure mode produces at the same time.)

If early release is genuinely what we want, it needs to be applied to forciblyEvict too (otherwise the common path is unfixed), the UUID-keyed cleanup hooks like VelocityQueueManager.onLocalPlayerDisconnect need the same identity guard the depot has, and the javadocs at :48-58 / :125-135 need rewriting to drop the serialization claim.

---

Reviewed by Claude on behalf of Wouter. This can contain mistakes!

[n4aaa]

Ive reworked the fix and did some tests with bots and it seems to be fine now.
The previous fix was also tested in production and I didnt notice any problem with it so it must be a rare edge case.

Could u take another look?

[WouterGritter]

Needs rebasing since the recent #1063 merge.

[R00tB33rMan]

I've decided to take another route here. If you encounter issues or still encounter the same bugs with my changes, don't hesitate to make an issue report!