fix: stop random login kicks and duplicated quit messages (1.21.11) - #596
Open
KostiaFed wants to merge 2 commits into
Open
fix: stop random login kicks and duplicated quit messages (1.21.11)#596KostiaFed wants to merge 2 commits into
KostiaFed wants to merge 2 commits into
Conversation
In offline mode vanilla handleHello() calls startClientVerification() itself, and
ServerLoginPacketListenerImplMixin injected at its TAIL to start the Bukkit
pre-login thread - which calls startClientVerification() a second time once the
events have run. So the login state machine was driven twice from two threads:
handleHello -> startClientVerification -> state = VERIFYING
tick -> verifyLoginAndFinishConnectionSetup
-> finishLoginAndWaitForClient
-> state = PROTOCOL_SWITCHING, sends ClientboundLoginFinished
authenticator -> startClientVerification -> state = VERIFYING again
tick -> verifyLoginAndFinishConnectionSetup a second time
-> a second ClientboundLoginFinishedPacket
Which of the three outcomes a player got depended purely on timing, so joining
took several attempts:
lost connection: Internal Exception: io.netty.handler.codec.EncoderException:
Pipeline has no outbound protocol configured, can't process packet
class_2901[gameProfile=GameProfile[id=..., name=Stas]]
lost connection: Internal Exception: java.lang.IllegalStateException:
Unexpected login acknowledgement packet
class_2901 is ClientboundLoginFinishedPacket: the second copy was written after
the client had already acknowledged the first and the pipeline had left the login
protocol. The other kick is the mirror image - the acknowledgement arrived while
the state had just been thrown back to VERIFYING.
Redirect the offline branch's startClientVerification() instead of injecting
after it, so vanilla's call is replaced rather than duplicated. The pre-login
events still run off the main thread, and the state is parked in AUTHENTICATING
while they do, exactly as the online path does. initUUID() takes and returns the
profile rather than mutating authenticatedProfile, which is not set yet at that
point, and the thread comes from the existing authenticator pool instead of a
fresh unbounded Thread per connection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ServerGamePacketListenerImplMixin overwrites disconnect(Component) to fire PlayerKickEvent. It then calls onDisconnect() directly so the quit event fires without waiting, and afterwards schedules Connection.handleDisconnection(), which calls onDisconnect() again. CraftBukkit guards both with a processedDisconnect flag; Cardboard only had it in commented-out code, so a single keep-alive timeout or kick ran the whole quit path twice: Stas lost connection: Timed out Stas left the game [voicechat] Disconnecting client Stas Stas lost connection: Timed out Stas left the game [voicechat] Disconnecting client Stas handleDisconnection() called twice Connection's own guard is what produces that last warning, and it only fires after both rounds have already run. Plugins saw PlayerQuitEvent twice, so in game this showed up as duplicated leave messages. Add the flag: disconnect() returns early if the disconnect was already processed, onDisconnect() is cancelled at HEAD on the second call, and isDisconnected() reports it, matching CraftBukkit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Two independent bugs in the connection lifecycle. Both are timing dependent, which is why they look like random server flakiness rather than a reproducible fault.
Logging in runs the client verification twice (offline mode). In offline mode vanilla
handleHello()callsstartClientVerification()itself, andServerLoginPacketListenerImplMixininjected at itsTAILto start the Bukkit pre-login thread — which callsstartClientVerification()a second time onceAsyncPlayerPreLoginEventhas run. So the login state machine is driven twice, from two threads:Which of the three possible outcomes a player gets is decided purely by where the tick lands, so joining takes several attempts:
class_2901isClientboundLoginFinishedPacket: the second copy is written after the client has already acknowledged the first and the pipeline has left the login protocol. The other kick is the mirror image — the acknowledgement arrives while the state has just been thrown back toVERIFYING, sohandleLoginAcknowledgement'sValidate.validState(state == PROTOCOL_SWITCHING)fails.Disconnecting runs the quit handling twice.
ServerGamePacketListenerImplMixinoverwritesdisconnect(Component)to firePlayerKickEvent. It then callsonDisconnect()directly so the quit event fires without waiting, and afterwards schedulesConnection.handleDisconnection(), which callsonDisconnect()again. CraftBukkit guards both with aprocessedDisconnectflag; here it only existed inside commented-out code, so a single keep-alive timeout or kick ran the whole quit path twice:Connection's own guard is what produces that last warning, and it only trips after both rounds have already run. Plugins seePlayerQuitEventtwice, so in game this shows up as duplicated leave messages.The fix
startClientVerification()call rather than injecting after it, so vanilla's call is replaced instead of duplicated. The pre-login events still run off the main thread, and the state is parked inAUTHENTICATINGwhile they do — exactly what the online path does.initUUID()now takes and returns the profile instead of mutatingauthenticatedProfile, which is not set yet at that point, and the thread comes from the existing authenticator pool instead of a fresh unboundedThreadper connection.processedDisconnectflag:disconnect()returns early if the disconnect was already processed,onDisconnect()is cancelled atHEADon the second call, andisDisconnected()reports it, matching CraftBukkit.The
@Atordinal = 1picks the offline-mode call site; ordinal 0 is the integrated-server owner shortcut. Verified against the bytecode of both1.21.11and26.1.2— the two call sites are in the same order in each.Testing
gradlew buildpasses on bothver/1.21.11andver/26.1.The diagnosis is from the server log above plus the vanilla bytecode:
handleHello's offline branch callingstartClientVerificationat the instruction the oldTAILinject ran after,tick()re-enteringverifyLoginAndFinishConnectionSetuponVERIFYING,Connection.handleDisconnectionlogginghandleDisconnection() called twiceonly after dispatchingonDisconnect, and the intermediary mapping identifyingclass_2901asClientboundLoginFinishedPacket.Not covered: mixin application and the in-game behaviour are not exercised here, since both bugs need a real client connecting and timing out. Worth a look from anyone who can reproduce the original kicks.