Sponge: Tab-completing commands causes a RuntimeException

[Rasgnarok]

I am currently running

• SpongeForge version: 3291
• SpongeVanilla version: N/A
• Java version: 8
• Operating System: CentOS 7

• Plugins/Mods: https://timings.aikar.co/?id=10b048d7ff6a4667acc46b1b432f7377

Issue Description
Tab-completing a command like GP's /playerinfo or EconomyLite's /pay causes a RuntimeException error. Works from console and in-game. I personally can reproduce it always by trying to tab-complete H. Does not seem to happen with Nucleus' /seen or /gamemode, nor with Luckperms' /lp user command.

EconomyLite: https://gist.github.com/Rasgnarok/1fe1afd5c8c808cca28617cbb006509f
GriefPrevention: https://gist.github.com/Rasgnarok/62684fe336b22bc0f592bc1aff022b8d

[dualspiral]

I think there are instances where the GameProfile exists in the user storage service, but there was never a corresponding User created. This may make the correct fix a lot trickier.

To explain, the PatternMatchingCommandElement of which this is a type, has two key methods, getChoices and getValue. getChoices is always run before getValue and getChoices contains all the possible strings that can select a value, so the idea is that getValue is guaranteed to return successfully.

The user element has always used UserStorageService#getAll() for getChoices and UserStorageService#get() for getValue(). The problem is that getChoices uses a collection of GameProfile objects - they are not Users and my guess is that not all of them will have an associated user. Of course, this will need investigation, as I am not on my computer right now!

The correct fix is to find a way to return a collection of strings that doesn’t kill performance in the first place, for large servers the get all check can already be overbearing - can we cache the game profiles of the available users in our user storage service somehow and expose that?

The workaround fix for now will be to catch the IAE in the parsing and rethrow it as a ArgumentParseException.

To answer for Nucleus, it uses a different argument to parse the user on that occassion. LP uses its own arg parsing too.

[dualspiral]

I think the issue is linked to here, when doing a getAll check - it gets game profiles from the cache when scanning the available player files, but the check doesn't scan the Sponge user files. As far as I can see, there is no guarantee that the profiles picked up here have a Sponge User created for them.

So, getChoices fishes it out because it's basing the check on GameProfiles which might not have a user, getValue makes the assumption that all the choices are good and can be turned into Users, but, in reality, some of them don't.

Either:

• We turn the get check in getValue into getOrCreate (simplest solution, but may lead to unwanted user entries that get selected when they shouldn't be, if there is a reason why GameProfiles shouldn't have an associated User)
• We trap the IAE and ignore it, so if no users are selected in the end, the element says no choices work (ugly, means tab completion and reality are still out of sync)
• We add a way to get all profiles with a User only (probably not performant)

Other things I've noticed, putting here now as food for thought:

• We might want to limit the number of entries coming back for a tab completion, there is a fair amount of processing going on in the user element, I've seen reports of server hangs on tab completion where servers have had tens of thousands of users.
• The UserDiscoverer#getAllProfiles method also does seem like it could be a bottleneck given the filesystem scanning it does - is there a way we could cache the game profiles and listen/have callbacks for when they change so this method uses a cache unless things change?

[dualspiral]

Debugging this with @Rasgnarok it turns out that my initial thoughts are a little off. It's a desync between names on the ban list and names in the profile cache where players have since changed their names.

[dualspiral]

To sum up what's going on: there was an incorrect game profile returned by UserDiscoverer#getAll(). Ras had a banned user from a while ago, under an old name. Since the user changed their name, I guess what happened is that they attempted to log on and the player cache updated with their new name, but the ban list didn't.

UserDiscoverer does the following:

• Check the user discoverer cache for game profiles
• Scans the playerdata directory and obtains profiles based on the file names in there
• Adds the game profiles from the whitelist and ban list to the set of profiles, if they are not already there

The problem is that the GameProfiles were being added to a set, presumably to avoid duplicates. However, equality on GameProfiles depends on both the UUID and the name. So, the duplicate UUIDs with the names were added anyway when only the "valid" one (from the player cache) should have been there.

In most use cases, this issue is transparent, but it blows up the UserCommandElement. This is because of how the element works, it creates a set of choices from the usernames in the given list, which includes the wrong ones. The wrong choice then gets passed back to UserDiscoverer.findByUsername returning null when the name can't be found from the profile manager. The command element assumes all choices are valid - so it causes an exception, originally a NSEE, later a IAE - happening on both tab completion and general usage.

I'm preparing to push a fix by only getting missing profiles from the whitelist/ban lists, and ensuring they are synced up with the cache. It's been tested by @Rasgnarok on their server, as well as me in a minimal, engineered test case.