LuckPerms compatibility audit: fix event subscription lifecycle, coalesce resyncs

.gitignore

@@ -112,6 +112,7 @@ logo.jpg
 # ============================================================
 docs/*
 !docs/server-setup-guide.html
+!docs/LUCKPERMS_COMPATIBILITY_AUDIT.md
 
 # ============================================================
 # BlueJ

CHANGELOG.md

@@ -7,6 +7,26 @@ Versioning: [Semantic Versioning](https://semver.org/)
 
 ---
 
+## [Unreleased]
+
+LuckPerms compatibility audit.
+
+### Fixed
+
+- The LuckPerms `UserDataRecalculateEvent` subscription is now closed on server stop and re-created on the next server start, instead of leaking across server lifecycles in the same JVM (stale `MinecraftServer` reference, broken live resync after an embedded restart).
+- Command-tree resyncs triggered by LuckPerms recalculation events are now coalesced per player, so a burst of recalculations (login, group inheritance, sync) sends a single command-tree packet instead of one per event.
+- Pending resync state is now isolated per server lifecycle, preventing an event racing with shutdown from suppressing resyncs after an embedded restart.
+
+### Changed
+
+- The per-event LuckPerms recalculation log line is now DEBUG instead of INFO.
+
+### Added
+
+- `docs/LUCKPERMS_COMPATIBILITY_AUDIT.md` — full audit of every LuckPerms touchpoint, including documented-but-unchanged findings (permanent degradation policy, vanilla-command policy with LP installed, version-gate behaviour).
+
+---
+
 ## [1.0.3] - 2026-06-10
 
 Backend policy update.

docs/LUCKPERMS_COMPATIBILITY_AUDIT.md

@@ -0,0 +1,123 @@
+# LuckPerms Compatibility Audit — CustomPerm 1.0.3
+
+Date: 2026-06-10
+Scope: full review of every LuckPerms touchpoint in the mod — backend selection,
+runtime permission checks, event integration, version gating, metadata, tests and docs.
+
+Severity scale: **HIGH** = broken behaviour in a realistic scenario · **MEDIUM** = degraded
+behaviour or misleading outcome · **LOW** = inconsistency / documentation / hardening note.
+
+---
+
+## Fixed in this PR
+
+### F1 — HIGH · LP event subscription leaks across server lifecycles
+
+`LuckPermsService.initServerHooks()` subscribed to `UserDataRecalculateEvent` without
+keeping the returned `EventSubscription`, guarded by a one-way `hooksReady` flag.
+
+Consequences when a server stops and another starts in the same JVM (integrated server
+opening a second world, or any embedded restart):
+
+1. The old subscription is never closed — its lambda retains the dead `MinecraftServer`
+   (memory leak: the full server object graph stays reachable from LP's event bus).
+2. `hooksReady` stays `true`, so the new server never re-subscribes — live command-tree
+   resync silently stops working until the JVM restarts.
+
+**Fix:** the subscription is stored, closed on `ServerStoppedEvent`
+(`LuckPermsService.closeServerHooks()`, wired in `CustomPerm.onServerStopped`), and
+re-created on the next `ServerStartedEvent`. Pending resync state is isolated in a
+per-server lifecycle coordinator; closing a lifecycle invalidates already queued work,
+so an event racing with shutdown cannot target the stopped server or suppress the same
+player's resync after restart.
+
+### F2 — MEDIUM · Resync storm on `UserDataRecalculateEvent`
+
+LuckPerms fires `UserDataRecalculateEvent` aggressively — typically several times in a
+burst for the same user (login, group inheritance recalculation, messaging-service sync,
+web-editor apply). Each event caused one full `ClientboundCommandsPacket` re-send plus
+one INFO log line.
+
+**Fix:** resyncs are coalesced per player (`pendingResyncs` set — one packet per burst),
+and the per-event log is downgraded from INFO to DEBUG. The one-time subscribe/unsubscribe
+logs remain at INFO.
+
+---
+
+## Findings documented, intentionally not changed
+
+### A1 — MEDIUM · Permanent degradation on the first LP exception
+
+`LuckPermsService.hasPermission()` flips a one-way `degraded` flag on **any** exception,
+including `IllegalStateException` from `LuckPermsProvider.get()`. This is the documented
+policy (AC1–AC3, INVARIANT-301), but note the consequence: a single transient hiccup
+(e.g. a permission check racing LP's startup, or one storage timeout surfacing through
+the cached-data layer) permanently disables the LuckPerms backend until server restart,
+with `deny` as the default fallback. In practice predicates are only evaluated after
+`ServerStartedEvent` (player login), when LP is registered, so the startup race is
+unlikely — but a recovery path (e.g. re-probe on the next `/customperm reload`) would be
+a worthwhile follow-up.
+
+### A2 — MEDIUM · With LuckPerms installed, vanilla commands cannot be granted at all
+
+The 1.0.3 policy disables direct command exposure whenever LP is present, deferring
+"normal command permissions" to LuckPerms. On NeoForge, however, **LuckPerms does not
+gate vanilla Brigadier commands** — it serves the PermissionAPI for mods that opt in;
+vanilla `/gamemode`, `/give`, etc. remain op-level-gated. So with LP installed, the only
+way to give a non-op access to a vanilla command is a CustomPerm alias (which elevates to
+op-4). The README's "Configure the server's normal /gamemode permission directly in
+LuckPerms" workflow does not work for vanilla commands on NeoForge. Operators should be
+aware that installing LP *removes* the `customperm.command.*` feature without LP providing
+an equivalent.
+
+### A3 — LOW · Version gate rejects qualified LP versions
+
+`VersionUtils.isVersionAtLeast` returns `false` for any version with more than three
+`[.\-]`-separated parts, so `5.4.150-SNAPSHOT`, `5.5.0-beta`, or build-metadata-qualified
+strings are rejected even when numerically above the minimum. This is documented as a
+deliberate safety stance (README: "prerelease-style versions are rejected"); release LP
+builds use plain `x.y.z` so impact is limited to dev/snapshot builds of LP.
+
+### A4 — LOW · `neoforge.mods.toml` range looser than the code gate
+
+The optional dependency declares `versionRange="[5.4,)"` while the code enforces
+`5.4.150+`. LP `5.4.0–5.4.149` passes the loader check and is then gracefully rejected by
+the code gate (deny/internal per `settings.json`) — fine. LP `< 5.4` instead fails the
+loader range and produces a hard NeoForge dependency error rather than the graceful
+fallback. Tightening the range to `[5.4.150,)` would convert *all* old-LP cases into hard
+loader errors, which is worse; the current split is acceptable but worth knowing.
+
+### A5 — LOW · OP short-circuit asymmetry between backends
+
+`InternalPermService` returns `true` for any op-level-2 source before consulting grades;
+`LuckPermsService` queries LP verbatim (no op short-circuit — call sites add
+`src.hasPermission(2) ||` themselves). Net effect on `/customperm test`: with the LP
+backend, an op player can show `DENIED` for a node LP doesn't grant, while the internal
+backend would show `GRANTED`. This matches LP semantics (LP itself doesn't special-case
+server ops) but can confuse diagnostics.
+
+### A6 — LOW · `getUser()` returning null is treated as deny, never loaded
+
+For an online player LP's user object is loaded during login, so `getUser(uuid)` should
+not be null in practice; if it ever is (plugin-managed unload, very early check), the
+check silently denies. No lookup/load fallback is attempted — acceptable for a hot path,
+noted for completeness.
+
+### A7 — LOW · Group-level changes rely on LP's cascade
+
+Only `UserDataRecalculateEvent` is subscribed. Group edits (`/lp group vip permission set
+…`) only trigger a client resync because LP invalidates and recalculates affected users'
+cached data, which fires the user event per affected online user. This holds with current
+LP versions; if LP ever changes that cascade, `NodeMutateEvent`/`GroupDataRecalculateEvent`
+would need explicit handling.
+
+---
+
+## Verification
+
+- `./gradlew test` — pure-Java unit suite (resolver, configs, version gate).
+- `ResyncCoordinatorTest` covers burst coalescing, queued-work invalidation on close,
+  and reuse of the same player UUID in a new server lifecycle.
+- `./gradlew runGameTestServer` — GameTests pass without LP at runtime (LP is
+  `compileOnly`); LP-specific behaviour (F1/F2) requires a dev server with the LP jar in
+  `run/mods/` as per README's manual validation section.

src/main/java/com/arcadia/customperm/CustomPerm.java

@@ -15,6 +15,7 @@
 import net.neoforged.fml.common.Mod;
 import net.neoforged.neoforge.common.NeoForge;
 import net.neoforged.neoforge.event.server.ServerStartedEvent;
+import net.neoforged.neoforge.event.server.ServerStoppedEvent;
 import org.slf4j.Logger;
 
 @Mod(CustomPerm.MODID)
@@ -77,6 +78,7 @@ public CustomPerm(IEventBus modBus, ModContainer container) {
 
         NeoForge.EVENT_BUS.register(CommandTreeRewriter.class);
         NeoForge.EVENT_BUS.addListener(CustomPerm::onServerStarted);
+        NeoForge.EVENT_BUS.addListener(CustomPerm::onServerStopped);
     }
 
     private static PermissionService unavailableLuckPermsBackend(InternalPermService internalBackend, String reason) {
@@ -104,6 +106,12 @@ private static void onServerStarted(ServerStartedEvent event) {
             backend, wrapped, exposed, aliases, grades);
     }
 
+    private static void onServerStopped(ServerStoppedEvent event) {
+        if (permissions instanceof LuckPermsService lps) {
+            lps.closeServerHooks();
+        }
+    }
+
     public static String backendLabel() {
         if (permissions instanceof LuckPermsService lps) {
             if (lps.isDegraded()) {

src/main/java/com/arcadia/customperm/perm/LuckPermsService.java

@@ -3,6 +3,7 @@
 import com.arcadia.customperm.CustomPerm;
 import net.luckperms.api.LuckPerms;
 import net.luckperms.api.LuckPermsProvider;
+import net.luckperms.api.event.EventSubscription;
 import net.luckperms.api.event.user.UserDataRecalculateEvent;
 import net.luckperms.api.model.user.User;
 import net.minecraft.commands.CommandSourceStack;
@@ -25,7 +26,14 @@ public class LuckPermsService implements PermissionService {
      */
     private final AtomicBoolean degraded = new AtomicBoolean(false);
 
-    private volatile boolean hooksReady = false;
+    /**
+     * Abonnement LP actif — conservé pour pouvoir le fermer proprement à l'arrêt du serveur.
+     * Sans close(), l'abonnement survit au cycle de vie du serveur : la lambda garde une
+     * référence vers l'ancien MinecraftServer (fuite mémoire) et, au démarrage suivant dans
+     * la même JVM, le resync pointe vers un serveur mort.
+     */
+    private final Object hooksLock = new Object();
+    private volatile ServerHooks serverHooks;
 
     public LuckPermsService(InternalPermService fallback) {
         // P7 : fail-fast si fallback null — NPE tardif lors d'une vraie défaillance LP serait bien pire.
@@ -98,24 +106,60 @@ private boolean safeCallFallback(CommandSourceStack source, String node) {
      * until the player reconnects or the server reloads.
      */
     public void initServerHooks(MinecraftServer server) {
-        if (hooksReady) return;
-        try {
-            LuckPerms api = LuckPermsProvider.get();
-            api.getEventBus().subscribe(UserDataRecalculateEvent.class, event -> {
-                UUID uuid = event.getUser().getUniqueId();
-                // LP events fire off-thread; schedule the resync on the server thread.
-                server.execute(() -> {
-                    ServerPlayer player = server.getPlayerList().getPlayer(uuid);
-                    if (player != null) {
-                        CustomPerm.LOGGER.info("[CustomPerm] LP user data recalculated for {}, resending command tree.", player.getGameProfile().getName());
-                        server.getCommands().sendCommands(player);
-                    }
-                });
-            });
-            hooksReady = true;
-            CustomPerm.LOGGER.info("[CustomPerm] Subscribed to LuckPerms UserDataRecalculateEvent for live command tree resync.");
-        } catch (Throwable t) {
-            CustomPerm.LOGGER.warn("[CustomPerm] Could not subscribe LP events; permission checks may still work but live command tree resync is disabled.", t);
+        synchronized (hooksLock) {
+            if (serverHooks != null) return;
+            try {
+                LuckPerms api = LuckPermsProvider.get();
+                ResyncCoordinator coordinator = new ResyncCoordinator();
+                EventSubscription<UserDataRecalculateEvent> subscription =
+                    api.getEventBus().subscribe(UserDataRecalculateEvent.class, event -> {
+                        UUID uuid = event.getUser().getUniqueId();
+                        if (!coordinator.schedule(uuid)) return;
+                        // LP events fire off-thread; schedule the resync on the server thread.
+                        server.execute(() -> {
+                            if (!coordinator.complete(uuid)) return;
+                            ServerPlayer player = server.getPlayerList().getPlayer(uuid);
+                            if (player != null) {
+                                CustomPerm.LOGGER.debug("[CustomPerm] LP user data recalculated for {}, resending command tree.", player.getGameProfile().getName());
+                                server.getCommands().sendCommands(player);
+                            }
+                        });
+                    });
+                serverHooks = new ServerHooks(subscription, coordinator);
+                CustomPerm.LOGGER.info("[CustomPerm] Subscribed to LuckPerms UserDataRecalculateEvent for live command tree resync.");
+            } catch (Throwable t) {
+                CustomPerm.LOGGER.warn("[CustomPerm] Could not subscribe LP events; permission checks may still work but live command tree resync is disabled.", t);
+            }
         }
     }
+
+    /**
+     * Ferme l'abonnement LP au moment où le serveur s'arrête, pour que :
+     * 1) la lambda ne retienne pas l'ancien MinecraftServer (fuite mémoire),
+     * 2) un redémarrage de serveur dans la même JVM ré-abonne avec le bon serveur.
+     */
+    public void closeServerHooks() {
+        ServerHooks hooks;
+        synchronized (hooksLock) {
+            hooks = serverHooks;
+            serverHooks = null;
+            if (hooks != null) {
+                hooks.coordinator().close();
+            }
+        }
+
+        if (hooks != null) {
+            try {
+                hooks.subscription().close();
+                CustomPerm.LOGGER.info("[CustomPerm] Unsubscribed from LuckPerms UserDataRecalculateEvent (server stopped).");
+            } catch (Throwable t) {
+                CustomPerm.LOGGER.warn("[CustomPerm] Failed to close LuckPerms event subscription.", t);
+            }
+        }
+    }
+
+    private record ServerHooks(
+            EventSubscription<UserDataRecalculateEvent> subscription,
+            ResyncCoordinator coordinator) {
+    }
 }

src/main/java/com/arcadia/customperm/perm/ResyncCoordinator.java

@@ -0,0 +1,31 @@
+package com.arcadia.customperm.perm;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+
+/**
+ * Tracks command-tree resyncs for one server lifecycle.
+ *
+ * <p>All operations are synchronized so closing a lifecycle cannot race with
+ * an event adding work after the pending set has been cleared.</p>
+ */
+final class ResyncCoordinator {
+
+    private final Set<UUID> pending = new HashSet<>();
+    private boolean active = true;
+
+    synchronized boolean schedule(UUID uuid) {
+        return active && pending.add(uuid);
+    }
+
+    synchronized boolean complete(UUID uuid) {
+        pending.remove(uuid);
+        return active;
+    }
+
+    synchronized void close() {
+        active = false;
+        pending.clear();
+    }
+}

src/test/java/com/arcadia/customperm/perm/ResyncCoordinatorTest.java

@@ -0,0 +1,46 @@
+package com.arcadia.customperm.perm;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class ResyncCoordinatorTest {
+
+    @Test
+    void coalescesEventsUntilScheduledWorkCompletes() {
+        ResyncCoordinator coordinator = new ResyncCoordinator();
+        UUID uuid = UUID.randomUUID();
+
+        assertTrue(coordinator.schedule(uuid));
+        assertFalse(coordinator.schedule(uuid));
+        assertTrue(coordinator.complete(uuid));
+        assertTrue(coordinator.schedule(uuid));
+    }
+
+    @Test
+    void closingLifecycleInvalidatesQueuedWork() {
+        ResyncCoordinator coordinator = new ResyncCoordinator();
+        UUID uuid = UUID.randomUUID();
+
+        assertTrue(coordinator.schedule(uuid));
+        coordinator.close();
+
+        assertFalse(coordinator.complete(uuid));
+        assertFalse(coordinator.schedule(uuid));
+    }
+
+    @Test
+    void oldLifecycleCannotBlockSamePlayerOnNewLifecycle() {
+        UUID uuid = UUID.randomUUID();
+        ResyncCoordinator oldCoordinator = new ResyncCoordinator();
+
+        assertTrue(oldCoordinator.schedule(uuid));
+        oldCoordinator.close();
+
+        ResyncCoordinator newCoordinator = new ResyncCoordinator();
+        assertTrue(newCoordinator.schedule(uuid));
+    }
+}