fix: skip premium username migration when the username already matches - #48
Open
sk8erboi17 wants to merge 1 commit into
Open
fix: skip premium username migration when the username already matches#48sk8erboi17 wants to merge 1 commit into
sk8erboi17 wants to merge 1 commit into
Conversation
Concurrent logins of a renamed premium account all read the pre-rename state, so every connection after the first asked UserService to migrate a row another connection had already migrated. migrateUsernameNoTx rejects that with IllegalArgumentException, which escaped the PreLoginEvent handler and left the account stuck in a kick loop.
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.
Problem
When several logins of the same renamed premium account overlap, they all read the pre-rename state, so
existingUserIgnoreCaseisnullfor each of them. The first connection commits the username migration; the later ones then find that row — already carrying the Mojang username — throughfindUserByMojangUuid, and ask for the very same migration again.migrateUsernameNoTxrejects it, and the exception escapes thePreLoginEventhandler:resolveUsernameConflictscatches onlyUsernameAlreadyTakenException, so this one propagates. NavAuth still fails closed — the handshake session is never created, and theonPostLogin/PlayerChooseInitialServer/OnServerConnectguards disconnect the player with "Auth session expired, please try again" — but a premium player who renamed and reconnects quickly can get stuck in a kick loop, andonGameProfileadditionally throws an NPE atLoginListeners.kt:187.This was first observed on a live proxy: one occurrence in ~60 logins, right after three reconnects of the same account within 13 seconds.
Fix
Migrate only when the stored username actually differs from the Mojang profile name. The guard mirrors the exact condition
migrateUsernameNoTxthrows on (Usernameis a value class overString, so the comparison is case-sensitive and a letter-case rename is still migrated), and it leaves the documentedUserServicecontract untouched.Tests
Two cases added to
UsernameResolutionServiceIntegrationTests:premium user already renamed by a concurrent login resolves without failing— resolves with the stalenullagainst an already-migrated row. Deterministic, and it reproduces the stack trace above exactly.concurrent logins of a renamed premium user never fail the resolution— two real threads over 32 rounds, both reading the pre-rename state and the second resolving after the first has committed. A latch makes that interleaving deterministic.Both fail on
mainand pass with the fix. Fullnavauth-commonsuite: 114 tests, 0 failures.spotlessCheckis green.The scenario was also verified end to end outside this repo, running the built plugin on a real headless Velocity with a seeded H2 database and eight simultaneous logins of a renamed premium account: the errors above appear on
mainand are gone with this change.