fix: reload LuckPerms users before /tfmc changes them

src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java

@@ -1,8 +1,13 @@
 package net.tfminecraft.tfmccore.tfmc;
 
 import java.util.List;
-import java.util.Optional;
+import java.util.Map;
+import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import java.util.logging.Logger;
 
 import org.bukkit.Bukkit;
 import org.bukkit.Material;
@@ -15,22 +20,32 @@
 
 import net.kyori.adventure.text.Component;
 import net.luckperms.api.LuckPerms;
+import net.luckperms.api.context.ContextSetFactory;
 import net.luckperms.api.context.DefaultContextKeys;
 import net.luckperms.api.context.ImmutableContextSet;
 import net.luckperms.api.model.user.User;
 import net.luckperms.api.node.NodeType;
-import net.luckperms.api.node.types.PermissionNode;
 import net.luckperms.api.track.Track;
 import net.luckperms.api.track.TrackManager;
 import net.tfminecraft.tfmccore.TFMCCore;
 
 /** Live implementation: Bukkit for commands and items, the LuckPerms API for permissions and tracks. */
 public final class BukkitTfmcActions implements TfmcActions {
 
+    private static final Logger LOGGER = Logger.getLogger("TFMCCore");
+
     private final TFMCCore plugin;
+    private final Supplier<LuckPerms> luckPerms;
+    // Tail of each player's LuckPerms changes, so a quick promote then demote cannot interleave
+    private final Map<UUID, CompletableFuture<?>> pending = new ConcurrentHashMap<>();
 
     public BukkitTfmcActions(TFMCCore plugin) {
+        this(plugin, BukkitTfmcActions::lookUpLuckPerms);
+    }
+
+    BukkitTfmcActions(TFMCCore plugin, Supplier<LuckPerms> luckPerms) {
         this.plugin = plugin;
+        this.luckPerms = luckPerms;
     }
 
     @Override
@@ -63,27 +78,29 @@ public void giveItems(Player player, List<Material> items) {
 
     @Override
     public CompletableFuture<Void> setPermission(Player player, String permission, boolean granted) {
-        LuckPerms api = luckPerms();
+        LuckPerms api = luckPerms.get();
         if (api == null) {
             return CompletableFuture.failedFuture(new IllegalStateException("LuckPerms is not available"));
         }
         // Matches the old "lp user <player> permission set|unset <node> server=<this server>"
         String server = api.getServerName();
+        ContextSetFactory contexts = api.getContextManager().getContextSetFactory();
         ImmutableContextSet context = "global".equalsIgnoreCase(server)
-                ? ImmutableContextSet.empty()
-                : ImmutableContextSet.of(DefaultContextKeys.SERVER_KEY, server);
-        return withUser(api, player).thenCompose(user -> {
+                ? contexts.immutableEmpty()
+                : contexts.immutableOf(DefaultContextKeys.SERVER_KEY, server);
+        return withFreshUser(api, player, user -> {
             user.data().clear(NodeType.PERMISSION.predicate(node ->
                     node.getPermission().equalsIgnoreCase(permission) && node.getContexts().equals(context)));
             // An explicit false overrides a true inherited from a group or the global context
-            user.data().add(PermissionNode.builder(permission).value(granted).context(context).build());
+            user.data().add(api.getNodeBuilderRegistry().forPermission()
+                    .permission(permission).value(granted).context(context).build());
             return save(api, user);
         });
     }
 
     @Override
-    public CompletableFuture<Optional<String>> stepTrack(Player player, String trackName, boolean promote) {
-        LuckPerms api = luckPerms();
+    public CompletableFuture<TrackStep> stepTrack(Player player, String trackName, boolean promote) {
+        LuckPerms api = luckPerms.get();
         if (api == null) {
             return CompletableFuture.failedFuture(new IllegalStateException("LuckPerms is not available"));
         }
@@ -93,19 +110,27 @@ public CompletableFuture<Optional<String>> stepTrack(Player player, String track
             return CompletableFuture.failedFuture(new IllegalStateException("No LuckPerms track " + trackName));
         }
         // Global context, like "lp user <player> promote|demote <track>"
-        return withUser(api, player).thenCompose(user -> {
-            Optional<String> group;
+        ImmutableContextSet global = api.getContextManager().getContextSetFactory().immutableEmpty();
+        return withFreshUser(api, player, user -> {
+            TrackStep step;
+            String status;
             if (promote) {
-                var result = track.promote(user, ImmutableContextSet.empty());
-                group = result.wasSuccessful() ? result.getGroupTo() : Optional.empty();
+                var result = track.promote(user, global);
+                step = new TrackStep(result.wasSuccessful(), result.getGroupTo());
+                status = result.getStatus().name();
             } else {
-                var result = track.demote(user, ImmutableContextSet.empty());
-                group = result.wasSuccessful() ? result.getGroupTo() : Optional.empty();
+                // Demoting past the first group succeeds with no destination: removed from the track
+                var result = track.demote(user, global);
+                step = new TrackStep(result.wasSuccessful(), result.getGroupTo());
+                status = result.getStatus().name();
             }
-            if (group.isEmpty()) {
-                return CompletableFuture.completedFuture(group);
+            if (!step.changed()) {
+                // e.g. AMBIGUOUS_CALL when the player holds more than one group on the track
+                LOGGER.warning("LuckPerms could not " + (promote ? "promote " : "demote ") + player.getName()
+                        + " on " + trackName + ": " + status);
+                return CompletableFuture.completedFuture(TrackStep.unchanged());
             }
-            return save(api, user).thenApply(done -> group);
+            return save(api, user).thenApply(done -> step);
         });
     }
 
@@ -133,11 +158,33 @@ public Runnable later(long ticks, Runnable task) {
         return scheduled::cancel;
     }
 
-    private static CompletableFuture<User> withUser(LuckPerms api, Player player) {
-        User loaded = api.getUserManager().getUser(player.getUniqueId());
-        return loaded != null
-                ? CompletableFuture.completedFuture(loaded)
-                : api.getUserManager().loadUser(player.getUniqueId());
+    /**
+     * Reloads the user from storage, then changes and saves them, one change per player at a time.
+     * This is what the lp commands and UserManager#modifyUser do. LuckPerms saves only the changes
+     * recorded since the last load, so changing the cached user instead can lose the removal of
+     * the old group and leave the player in two groups on one track.
+     */
+    private <T> CompletableFuture<T> withFreshUser(LuckPerms api, Player player, Function<User, CompletableFuture<T>> change) {
+        UUID id = player.getUniqueId();
+        // The queue waits on its own future, so callers cannot release the next change early
+        CompletableFuture<Void> finished = new CompletableFuture<>();
+        CompletableFuture<T> result = new CompletableFuture<>();
+        CompletableFuture<?> previous = pending.put(id, finished);
+        CompletableFuture<?> ready = previous == null ? CompletableFuture.completedFuture(null) : previous;
+        ready.thenCompose(ignored -> api.getUserManager().loadUser(id))
+                .thenCompose(change)
+                .whenComplete((value, error) -> {
+                    // Report this change before releasing the next, so replies arrive in order;
+                    // the queue entry stays until then so a new request still waits behind it
+                    if (error != null) {
+                        result.completeExceptionally(error);
+                    } else {
+                        result.complete(value);
+                    }
+                    pending.remove(id, finished);
+                    finished.complete(null);
+                });
+        return result;
     }
 
     private static CompletableFuture<Void> save(LuckPerms api, User user) {
@@ -146,7 +193,7 @@ private static CompletableFuture<Void> save(LuckPerms api, User user) {
                 .thenRun(() -> api.getMessagingService().ifPresent(service -> service.pushUserUpdate(user)));
     }
 
-    private static LuckPerms luckPerms() {
+    private static LuckPerms lookUpLuckPerms() {
         if (Bukkit.getPluginManager().getPlugin("LuckPerms") == null) {
             return null;
         }

src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcActions.java

@@ -15,6 +15,16 @@ public interface TfmcActions {
     /** A player's flight settings before a temporary change, so they can be put back. */
     record FlightState(boolean allowFlight, boolean flying, float flySpeed) {}
 
+    /**
+     * Outcome of a track step. {@code group} is empty when the step removed the player from the
+     * track (demoting past its first group) or when it did not change anything.
+     */
+    record TrackStep(boolean changed, Optional<String> group) {
+        public static TrackStep unchanged() {
+            return new TrackStep(false, Optional.empty());
+        }
+    }
+
     void consoleCommand(String command);
 
     void playerCommand(Player player, String command);
@@ -29,8 +39,8 @@ record FlightState(boolean allowFlight, boolean flying, float flySpeed) {}
     /** Sets a LuckPerms permission to true or false for the player in this server's context. */
     CompletableFuture<Void> setPermission(Player player, String permission, boolean granted);
 
-    /** Moves the player one step along a LuckPerms track; completes with the new group. */
-    CompletableFuture<Optional<String>> stepTrack(Player player, String track, boolean promote);
+    /** Moves the player one step along a LuckPerms track. */
+    CompletableFuture<TrackStep> stepTrack(Player player, String track, boolean promote);
 
     /** Lets the player fly at {@code speed} and returns their previous flight settings. */
     FlightState startFlight(Player player, float speed);

src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java

@@ -266,15 +266,19 @@ private int ban(CommandContext<CommandSourceStack> context, String rawReason) {
     }
 
     private void step(Player player, String track, boolean promote) {
-        actions.stepTrack(player, track, promote).whenCompleteAsync((group, error) -> {
+        actions.stepTrack(player, track, promote).whenCompleteAsync((step, error) -> {
             if (error != null) {
                 LOGGER.warning("Failed to " + (promote ? "promote " : "demote ") + player.getName() + " on " + track + ": " + error.getMessage());
             }
-            if (error != null || group.isEmpty()) {
+            if (error != null || !step.changed()) {
                 send(player, "track-messages.failed");
                 return;
             }
-            send(player, promote ? "track-messages.promoted" : "track-messages.demoted", Placeholder.unparsed("group", group.get()));
+            if (step.group().isEmpty()) {
+                send(player, "track-messages.removed", Placeholder.unparsed("track", track));
+                return;
+            }
+            send(player, promote ? "track-messages.promoted" : "track-messages.demoted", Placeholder.unparsed("group", step.group().get()));
         }, mainThread);
     }
 

src/main/resources/tfmc.yml

@@ -160,6 +160,7 @@ helper-tracks:
 track-messages:
   promoted: "<green>You are now <yellow><group></yellow>."
   demoted: "<green>You are now <yellow><group></yellow>."
+  removed: "<green>You are no longer on the <yellow><track></yellow> track."
   failed: "<red>Your helper status could not be changed."
 
 # /tfmc poster <name> runs the command as the player.