Skip to content

fix: skip premium username migration when the username already matches - #48

Open
sk8erboi17 wants to merge 1 commit into
Navio1430:mainfrom
sk8erboi17:fix/premium-rename-login-race
Open

fix: skip premium username migration when the username already matches#48
sk8erboi17 wants to merge 1 commit into
Navio1430:mainfrom
sk8erboi17:fix/premium-rename-login-race

Conversation

@sk8erboi17

Copy link
Copy Markdown

Problem

When several logins of the same renamed premium account overlap, they all read the pre-rename state, so existingUserIgnoreCase is null for each of them. The first connection commits the username migration; the later ones then find that row — already carrying the Mojang username — through findUserByMojangUuid, and ask for the very same migration again. migrateUsernameNoTx rejects it, and the exception escapes the PreLoginEvent handler:

ERROR: Couldn't pass PreLoginEvent to navauth
java.sql.SQLException: OrmLite SQL transaction failed
	at ...TransactionServiceImpl$inTransaction$1.call(TransactionServiceImpl.kt:36)
	at ...UserService.migrateUsername-NOTGgSg(UserService.kt:171)
	at ...UsernameResolutionService.resolveUsernameConflicts-xYvRu8A(UsernameResolutionService.kt:46)
	at ...LoginListeners.onPreLogin(LoginListeners.kt:84)
Caused by: java.lang.IllegalArgumentException: username cannot be the same
	at ...UserService.migrateUsernameNoTx-NOTGgSg(UserService.kt:217)

resolveUsernameConflicts catches only UsernameAlreadyTakenException, so this one propagates. NavAuth still fails closed — the handshake session is never created, and the onPostLogin / PlayerChooseInitialServer / OnServerConnect guards 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, and onGameProfile additionally throws an NPE at LoginListeners.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 migrateUsernameNoTx throws on (Username is a value class over String, so the comparison is case-sensitive and a letter-case rename is still migrated), and it leaves the documented UserService contract untouched.

Tests

Two cases added to UsernameResolutionServiceIntegrationTests:

  • premium user already renamed by a concurrent login resolves without failing — resolves with the stale null against 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 main and pass with the fix. Full navauth-common suite: 114 tests, 0 failures. spotlessCheck is 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 main and are gone with this change.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant