Skip to content

LuckPerms compatibility audit: fix event subscription lifecycle, coalesce resyncs#1

Merged
THEFricadelle merged 2 commits into
mainfrom
audit/luckperms-compatibility
Jun 10, 2026
Merged

LuckPerms compatibility audit: fix event subscription lifecycle, coalesce resyncs#1
THEFricadelle merged 2 commits into
mainfrom
audit/luckperms-compatibility

Conversation

@Blushister

Copy link
Copy Markdown
Collaborator

Summary

Full audit of every LuckPerms touchpoint in the mod (backend selection, runtime checks, event integration, version gating, mod metadata, tests, docs). Two concrete bugs are fixed in this PR; five additional findings are documented in docs/LUCKPERMS_COMPATIBILITY_AUDIT.md without behaviour changes, since they reflect deliberate 1.0.3 policy or low-risk inconsistencies.

Fixed

HIGH - LP event subscription leaked across server lifecycles

LuckPermsService.initServerHooks() subscribed to UserDataRecalculateEvent without keeping the EventSubscription, guarded by a one-way hooksReady flag. After a server stop + start in the same JVM (integrated server opening another world, embedded restart):

  • the old subscription stayed registered on the LP event bus with a lambda retaining the dead MinecraftServer (memory leak of the whole server graph);
  • hooksReady stayed true, so the new server never re-subscribed - live command-tree resync silently stopped working until JVM restart.

The subscription is now stored, closed on ServerStoppedEvent (closeServerHooks()), and re-created on the next ServerStartedEvent.

MEDIUM - Resync storm on UserDataRecalculateEvent

LP fires this event in bursts for the same user (login, group-inheritance recalculation, messaging sync, web-editor apply). Each event triggered one full ClientboundCommandsPacket re-send plus an INFO log line. Resyncs are now coalesced per player (one packet per burst) and the per-event log is DEBUG.

Documented, intentionally not changed (see audit doc)

  • A1 (MEDIUM): permanent degraded flip on the first LP exception (documented invariant) - one transient hiccup disables the LP backend until restart, default fallback deny. A re-probe on /customperm reload is suggested as follow-up.
  • A2 (MEDIUM): with LP installed, direct command exposure is disabled, but LuckPerms on NeoForge does not gate vanilla Brigadier commands - aliases become the only way to grant vanilla commands to non-ops. The README workflow about managing normal command perms in LP does not apply to vanilla commands.
  • A3 (LOW): version gate rejects qualified versions (5.4.150-SNAPSHOT, 5.5.0-beta) regardless of numeric value - documented as deliberate.
  • A4 (LOW): neoforge.mods.toml declares [5.4,) while code enforces 5.4.150+ - LP below 5.4 hard-fails at the loader instead of the graceful fallback.
  • A5 (LOW): OP short-circuit asymmetry - internal backend grants any op-2 source; LP backend queries LP verbatim, so /customperm test can show DENIED for an op.

Validation

  • ./gradlew compileJava test - build and full pure-Java unit suite pass.
  • GameTests are unaffected (LP is compileOnly, the changed code paths only run with LP present); the two fixes require a dev server with the LP jar in run/mods/ per the README manual-validation procedure.

- Close the UserDataRecalculateEvent subscription on ServerStoppedEvent and
  re-subscribe on the next server start, instead of leaking the subscription
  (and a stale MinecraftServer reference) across server lifecycles in the
  same JVM, which also silently broke live command-tree resync after an
  embedded restart.
- Coalesce LP-triggered command-tree resyncs per player so a burst of
  recalculation events sends a single ClientboundCommandsPacket.
- Downgrade the per-event recalculation log from INFO to DEBUG.
- Add docs/LUCKPERMS_COMPATIBILITY_AUDIT.md covering every LP touchpoint,
  including documented-but-unchanged findings.
@THEFricadelle THEFricadelle merged commit bd481a6 into main Jun 10, 2026
1 check passed
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.

2 participants