From 37f4d8c8fe7fff76547fab0e8b84bc1f3e32627c Mon Sep 17 00:00:00 2001 From: Ben Date: Wed, 23 Sep 2026 18:11:56 -0600 Subject: [PATCH 1/6] Keep timed permission expiry on platform threads --- .../api/permissions/PermissionHandler.java | 51 +++++++++++++-- .../permissions/PlayerPermissionHandler.java | 56 +++++++++++----- .../PlayerPermissionHandlerThreadingTest.java | 65 +++++++++++++++++++ 3 files changed, 151 insertions(+), 21 deletions(-) create mode 100644 AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java index 14ab45b03c..f5424729c7 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java @@ -6,6 +6,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Pattern; import org.bukkit.Bukkit; @@ -49,6 +50,7 @@ public class PermissionHandler { @Getter private final ScheduledExecutorService timer = Executors.newScheduledThreadPool(1); + private final AtomicBoolean acceptingExpirations = new AtomicBoolean(true); public PermissionHandler(AdvancedCorePlugin plugin) { this.plugin = plugin; @@ -88,6 +90,42 @@ public PermissionHandler(AdvancedCorePlugin plugin) { } } + + void scheduleExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt, long delayMillis) { + if (!acceptingExpirations.get()) return; + try { + timer.schedule(() -> dispatchExpiration(handle, permission, expectedExpireAt), + Math.max(0L, delayMillis), java.util.concurrent.TimeUnit.MILLISECONDS); + } catch (RuntimeException failure) { + plugin.debug(failure); + throw failure; + } + } + + private void dispatchExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { + if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; + try { + plugin.getBukkitScheduler().runTask(plugin, () -> { + if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; + Player player = Bukkit.getPlayer(handle.getUuid()); + if (player == null) { + handle.expirePermission(permission, expectedExpireAt, false); + return; + } + try { + plugin.getBukkitScheduler().runTask(plugin, () -> { + if (!acceptingExpirations.get()) return; + handle.expirePermission(permission, expectedExpireAt, true); + }, player); + } catch (RuntimeException failure) { + plugin.debug(failure); + } + }); + } catch (RuntimeException failure) { + plugin.debug(failure); + } + } + public void addPermission(Player player, String permission) { addPermission(player.getUniqueId(), permission); } @@ -273,6 +311,10 @@ public void removePermission(UUID uuid, String playerName, String permission) { * Persists timed permissions for both online + offline handlers. */ public void shutDown() { + // Fence every timer/global/entity callback before taking persistence snapshots. + // A callback already inside a handler monitor finishes before timedPermissionSnapshot(). + acceptingExpirations.set(false); + timer.shutdownNow(); saveTimedPerms(perms); saveTimedPerms(permsToAdd); plugin.getServerDataFile().saveData(); @@ -280,16 +322,13 @@ public void shutDown() { private void saveTimedPerms(ConcurrentHashMap map) { for (PlayerPermissionHandler handle : map.values()) { - if (handle.getTimedPermissions() == null || handle.getTimedPermissions().isEmpty()) { - continue; - } + java.util.Map snapshot = handle.timedPermissionSnapshot(); + if (snapshot.isEmpty()) continue; ArrayList list = new ArrayList<>(); - for (Entry entry : handle.getTimedPermissions().entrySet()) { - // Store absolute expireAtMillis + for (Entry entry : snapshot.entrySet()) { list.add(entry.getKey() + "%line%" + entry.getValue()); } - plugin.getServerDataFile().getData().set("TimedPermissions." + handle.getUuid(), list); } } diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java index b1c5a52dea..caea6a6a0a 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java @@ -5,7 +5,6 @@ import java.util.Map.Entry; import java.util.Set; import java.util.UUID; -import java.util.concurrent.TimeUnit; import org.bukkit.entity.Player; import org.bukkit.permissions.PermissionAttachment; @@ -70,7 +69,7 @@ public PlayerPermissionHandler(UUID uuid, PermissionAttachment attachment, Permi * @param duration How long the permission should last * @return this */ - public PlayerPermissionHandler addExpiration(String perm, ParsedDuration duration) { + public synchronized PlayerPermissionHandler addExpiration(String perm, ParsedDuration duration) { if (duration == null || duration.isEmpty()) { return addPerm(perm); } @@ -87,7 +86,7 @@ public PlayerPermissionHandler addExpiration(String perm, ParsedDuration duratio } long delayMillis = duration.delayMillisFromNow(); - handler.getTimer().schedule(() -> removePermission(perm), delayMillis, TimeUnit.MILLISECONDS); + handler.scheduleExpiration(this, perm, expireAt, delayMillis); return this; } @@ -106,7 +105,7 @@ public PlayerPermissionHandler addExpiration(String perm, long seconds) { * @param duration Duration; empty means permanent * @return this */ - public PlayerPermissionHandler addOfflinePerm(String perm, ParsedDuration duration) { + public synchronized PlayerPermissionHandler addOfflinePerm(String perm, ParsedDuration duration) { if (permsToAdd == null) { permsToAdd = new HashMap<>(); } @@ -128,7 +127,7 @@ public PlayerPermissionHandler addOfflinePerm(String perm, long seconds) { * @param perm Permission node * @return this */ - public PlayerPermissionHandler addPerm(String perm) { + public synchronized PlayerPermissionHandler addPerm(String perm) { persistentPermissions.add(perm); if (attachment != null) { @@ -141,7 +140,7 @@ public PlayerPermissionHandler addPerm(String perm) { /** * Re-applies stored permissions after an attachment is created on login. */ - public void onLogin(Player player) { + public synchronized void onLogin(Player player) { if (attachment == null) { return; } @@ -184,10 +183,44 @@ public void onLogout(Player player) { // intentionally empty } + + /** True only while this exact scheduled expiration still owns the timed grant. */ + synchronized java.util.Map timedPermissionSnapshot() { + return timedPermissions == null ? java.util.Map.of() : new java.util.HashMap<>(timedPermissions); + } + + synchronized boolean isExpirationCurrent(String perm, long expectedExpireAt) { + if (timedPermissions == null) return false; + Long current = timedPermissions.get(perm); + return current != null && current.longValue() == expectedExpireAt; + } + + /** + * Expire only the scheduled grant that still owns this permission. The + * attachment flag is true only from the player's owning platform scheduler. + */ + synchronized void expirePermission(String perm, long expectedExpireAt, boolean updateAttachment) { + if (timedPermissions == null) return; + Long current = timedPermissions.get(perm); + if (current == null || current.longValue() != expectedExpireAt) return; + if (!timedPermissions.remove(perm, current)) return; + persistentPermissions.remove(perm); + if (permsToAdd != null) permsToAdd.remove(perm); + if (updateAttachment && attachment != null) attachment.unsetPermission(perm); + removeHandlerIfEmpty(); + } + + private void removeHandlerIfEmpty() { + boolean noTracked = persistentPermissions.isEmpty() + && (timedPermissions == null || timedPermissions.isEmpty()) + && (permsToAdd == null || permsToAdd.isEmpty()); + if (noTracked && (attachment == null || attachment.getPermissions().isEmpty())) handler.removePermission(uuid); + } + /** * Removes a permission from both internal tracking and the live attachment (if online). */ - public void removePermission(String perm) { + public synchronized void removePermission(String perm) { persistentPermissions.remove(perm); if (timedPermissions != null) { timedPermissions.remove(perm); @@ -201,13 +234,6 @@ public void removePermission(String perm) { attachment.getPermissions().remove(perm); } - // If nothing tracked and attachment has nothing, drop handler state entirely - boolean noTracked = persistentPermissions.isEmpty() - && (timedPermissions == null || timedPermissions.isEmpty()) - && (permsToAdd == null || permsToAdd.isEmpty()); - - if (noTracked && attachment != null && attachment.getPermissions().isEmpty()) { - handler.removePermission(uuid); - } + removeHandlerIfEmpty(); } } diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java new file mode 100644 index 0000000000..bd7fffda22 --- /dev/null +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java @@ -0,0 +1,65 @@ +package com.bencodez.advancedcore.api.permissions; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.util.UUID; + +import org.bukkit.permissions.PermissionAttachment; +import org.junit.jupiter.api.Test; + +import com.bencodez.simpleapi.time.ParsedDuration; + +class PlayerPermissionHandlerThreadingTest { + + @Test + void staleExpirationCannotRevokeExtendedPermission() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + when(attachment.getPermissions()).thenReturn(new java.util.LinkedHashMap<>()); + PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); + + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long firstExpiry = handler.getTimedPermissions().get("example.use"); + handler.addExpiration("example.use", ParsedDuration.ofMillis(120_000)); + long extendedExpiry = handler.getTimedPermissions().get("example.use"); + assertTrue(extendedExpiry >= firstExpiry); + clearInvocations(attachment); + + handler.expirePermission("example.use", firstExpiry, true); + + assertEquals(extendedExpiry, handler.getTimedPermissions().get("example.use")); + verify(attachment, never()).unsetPermission("example.use"); + } + + @Test + void offOwnerExpirationUpdatesStateWithoutTouchingAttachment() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + when(attachment.getPermissions()).thenReturn(new java.util.LinkedHashMap<>()); + PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); + + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long expiry = handler.getTimedPermissions().get("example.use"); + clearInvocations(attachment); + handler.expirePermission("example.use", expiry, false); + + assertFalse(handler.getTimedPermissions().containsKey("example.use")); + verify(attachment, never()).setPermission("example.use", false); + } + @Test + void expirationClearsAttachmentEntryInsteadOfInstallingDenial() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + when(attachment.getPermissions()).thenReturn(new java.util.LinkedHashMap<>()); + PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long expiry = handler.getTimedPermissions().get("example.use"); + clearInvocations(attachment); + handler.expirePermission("example.use", expiry, true); + verify(attachment).unsetPermission("example.use"); + verify(attachment, never()).setPermission("example.use", false); + } + +} From 3adf3213b94f81b86b4e10819c520e7c0a86b00d Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:32:56 -0600 Subject: [PATCH 2/6] Fix offline timed permission cleanup --- .../permissions/PlayerPermissionHandler.java | 10 +++++---- .../listeners/PlayerJoinEvent.java | 6 ++--- .../PlayerPermissionHandlerThreadingTest.java | 22 ++++++++++++++++++- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java index caea6a6a0a..dcd9e14df4 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java @@ -207,14 +207,16 @@ synchronized void expirePermission(String perm, long expectedExpireAt, boolean u persistentPermissions.remove(perm); if (permsToAdd != null) permsToAdd.remove(perm); if (updateAttachment && attachment != null) attachment.unsetPermission(perm); - removeHandlerIfEmpty(); + removeHandlerIfEmpty(!updateAttachment); } - private void removeHandlerIfEmpty() { + private void removeHandlerIfEmpty(boolean attachmentIsOffline) { boolean noTracked = persistentPermissions.isEmpty() && (timedPermissions == null || timedPermissions.isEmpty()) && (permsToAdd == null || permsToAdd.isEmpty()); - if (noTracked && (attachment == null || attachment.getPermissions().isEmpty())) handler.removePermission(uuid); + if (noTracked && (attachmentIsOffline || attachment == null || attachment.getPermissions().isEmpty())) { + handler.removePermission(uuid); + } } /** @@ -234,6 +236,6 @@ public synchronized void removePermission(String perm) { attachment.getPermissions().remove(perm); } - removeHandlerIfEmpty(); + removeHandlerIfEmpty(false); } } diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java index 0f5c094fa5..540375e3dd 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java @@ -158,9 +158,9 @@ public void onPlayerQuit(PlayerQuitEvent event) { Player player = event.getPlayer(); plugin.debug("Logout: " + player.getName() + " (" + player.getUniqueId() + ")"); - if (plugin.getPermissionHandler() != null) { - plugin.getPermissionHandler().login(player); - } + if (plugin.getPermissionHandler() != null) { + plugin.getPermissionHandler().logout(player); + } plugin.getLoginTimer().execute(new Runnable() { diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java index bd7fffda22..f76902a24f 100644 --- a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java @@ -46,8 +46,28 @@ void offOwnerExpirationUpdatesStateWithoutTouchingAttachment() { handler.expirePermission("example.use", expiry, false); assertFalse(handler.getTimedPermissions().containsKey("example.use")); - verify(attachment, never()).setPermission("example.use", false); + verify(attachment, never()).unsetPermission(anyString()); + verify(attachment, never()).setPermission(anyString(), anyBoolean()); } + + @Test + void offlineExpirationDropsEmptyHandlerDespiteStaleAttachmentState() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + when(attachment.getPermissions()).thenReturn(java.util.Map.of("example.use", true)); + UUID uuid = UUID.randomUUID(); + PlayerPermissionHandler handler = new PlayerPermissionHandler(uuid, attachment, manager); + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long expiry = handler.getTimedPermissions().get("example.use"); + clearInvocations(attachment); + + handler.expirePermission("example.use", expiry, false); + + verify(manager).removePermission(uuid); + verify(attachment, never()).unsetPermission(anyString()); + verify(attachment, never()).setPermission(anyString(), anyBoolean()); + } + @Test void expirationClearsAttachmentEntryInsteadOfInstallingDenial() { PermissionHandler manager = mock(PermissionHandler.class); From c46336e22cdf3e6d2c5a73e717f159539eaef872 Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:43:10 -0600 Subject: [PATCH 3/6] Preserve timed permissions through handoff failures --- .../api/permissions/PermissionHandler.java | 28 +++++++-- .../permissions/PlayerPermissionHandler.java | 2 +- .../PlayerPermissionHandlerThreadingTest.java | 57 ++++++++++++++++++- 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java index f5424729c7..006de9efb7 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java @@ -102,7 +102,7 @@ void scheduleExpiration(PlayerPermissionHandler handle, String permission, long } } - private void dispatchExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { + void dispatchExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; try { plugin.getBukkitScheduler().runTask(plugin, () -> { @@ -119,13 +119,21 @@ private void dispatchExpiration(PlayerPermissionHandler handle, String permissio }, player); } catch (RuntimeException failure) { plugin.debug(failure); + retryExpiration(handle, permission, expectedExpireAt); } }); } catch (RuntimeException failure) { plugin.debug(failure); + retryExpiration(handle, permission, expectedExpireAt); } } + private void retryExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { + if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; + try { scheduleExpiration(handle, permission, expectedExpireAt, 1_000L); } + catch (RuntimeException ignored) { /* scheduleExpiration already reported the rejection */ } + } + public void addPermission(Player player, String permission) { addPermission(player.getUniqueId(), permission); } @@ -167,8 +175,11 @@ public void addPermission(UUID uuid, String permission) { PlayerPermissionHandler newHandle = new PlayerPermissionHandler(uuid, attachment, this).addPerm(perm); perms.put(uuid, newHandle); } else { - permsToAdd.put(uuid, - new PlayerPermissionHandler(uuid, null, this).addOfflinePerm(perm, ParsedDuration.empty())); + permsToAdd.compute(uuid, (ignored, pending) -> { + PlayerPermissionHandler target = pending == null + ? new PlayerPermissionHandler(uuid, null, this) : pending; + return target.addOfflinePerm(perm, ParsedDuration.empty()); + }); } } } @@ -205,7 +216,11 @@ public void addPermission(UUID uuid, String permission, ParsedDuration duration) .addExpiration(perm, duration); perms.put(uuid, newHandle); } else { - permsToAdd.put(uuid, new PlayerPermissionHandler(uuid, null, this).addOfflinePerm(perm, duration)); + permsToAdd.compute(uuid, (ignored, pending) -> { + PlayerPermissionHandler target = pending == null + ? new PlayerPermissionHandler(uuid, null, this) : pending; + return target.addOfflinePerm(perm, duration); + }); } } } @@ -273,6 +288,11 @@ public void removePermission(UUID uuid) { permsToAdd.remove(uuid); } + void removePermission(UUID uuid, PlayerPermissionHandler expected) { + perms.remove(uuid, expected); + permsToAdd.remove(uuid, expected); + } + /** * Removes one or more permissions (split by "|") from a specific player. * diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java index dcd9e14df4..7253845f01 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java @@ -215,7 +215,7 @@ private void removeHandlerIfEmpty(boolean attachmentIsOffline) { && (timedPermissions == null || timedPermissions.isEmpty()) && (permsToAdd == null || permsToAdd.isEmpty()); if (noTracked && (attachmentIsOffline || attachment == null || attachment.getPermissions().isEmpty())) { - handler.removePermission(uuid); + handler.removePermission(uuid, this); } } diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java index f76902a24f..e15cadb931 100644 --- a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java @@ -6,12 +6,67 @@ import java.util.UUID; +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.bukkit.entity.Player; import org.bukkit.permissions.PermissionAttachment; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; +import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.simpleapi.scheduler.BukkitScheduler; import com.bencodez.simpleapi.time.ParsedDuration; class PlayerPermissionHandlerThreadingTest { + @Test + void offlineGrantsReuseTheHandlerPreservedByLogout() { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.getServerDataFile().getData()).thenReturn(null); + PermissionHandler manager = new PermissionHandler(plugin); + UUID uuid = UUID.randomUUID(); + PlayerPermissionHandler preserved = new PlayerPermissionHandler(uuid, null, manager) + .addOfflinePerm("existing.use", ParsedDuration.empty()); + manager.getPermsToAdd().put(uuid, preserved); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(null); + manager.addPermission(uuid, "new.use"); + assertSame(preserved, manager.getPermsToAdd().get(uuid)); + PermissionAttachment attachment = mock(PermissionAttachment.class); + preserved.setAttachment(attachment); + preserved.onLogin(mock(Player.class)); + verify(attachment).setPermission("existing.use", true); + verify(attachment).setPermission("new.use", true); + } finally { manager.getTimer().shutdownNow(); } + } + + @Test + void entitySchedulerRejectionRetriesCurrentExpiration() { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.getServerDataFile().getData()).thenReturn(null); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + when(plugin.getBukkitScheduler()).thenReturn(scheduler); + PermissionHandler manager = spy(new PermissionHandler(plugin)); + PlayerPermissionHandler handle = mock(PlayerPermissionHandler.class); + UUID uuid = UUID.randomUUID(); + long expiry = System.currentTimeMillis(); + when(handle.getUuid()).thenReturn(uuid); + when(handle.isExpirationCurrent("example.use", expiry)).thenReturn(true); + Player player = mock(Player.class); + doAnswer(call -> { ((Runnable) call.getArgument(1)).run(); return null; }) + .when(scheduler).runTask(eq(plugin), any(Runnable.class)); + doThrow(new IllegalStateException("entity retired")) + .when(scheduler).runTask(eq(plugin), any(Runnable.class), eq(player)); + doNothing().when(manager).scheduleExpiration(handle, "example.use", expiry, 1_000L); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(Bukkit::getServer).thenReturn(mock(Server.class)); + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(player); + manager.dispatchExpiration(handle, "example.use", expiry); + verify(manager).scheduleExpiration(handle, "example.use", expiry, 1_000L); + verify(handle, never()).expirePermission(anyString(), anyLong(), anyBoolean()); + } finally { manager.getTimer().shutdownNow(); } + } @Test void staleExpirationCannotRevokeExtendedPermission() { @@ -63,7 +118,7 @@ void offlineExpirationDropsEmptyHandlerDespiteStaleAttachmentState() { handler.expirePermission("example.use", expiry, false); - verify(manager).removePermission(uuid); + verify(manager).removePermission(uuid, handler); verify(attachment, never()).unsetPermission(anyString()); verify(attachment, never()).setPermission(anyString(), anyBoolean()); } From f604d36195caa068a05188d5cbfc0b1543e19100 Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:00:27 -0600 Subject: [PATCH 4/6] Close timed permission handoff races --- .../api/permissions/PermissionHandler.java | 188 +++++++++++------- .../permissions/PlayerPermissionHandler.java | 70 ++++--- .../PlayerPermissionHandlerThreadingTest.java | 93 ++++++++- 3 files changed, 244 insertions(+), 107 deletions(-) diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java index 006de9efb7..295dd0565b 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java @@ -51,9 +51,11 @@ public class PermissionHandler { @Getter private final ScheduledExecutorService timer = Executors.newScheduledThreadPool(1); private final AtomicBoolean acceptingExpirations = new AtomicBoolean(true); + private final Object[] stateLocks = new Object[64]; public PermissionHandler(AdvancedCorePlugin plugin) { this.plugin = plugin; + for (int i = 0; i < stateLocks.length; i++) stateLocks[i] = new Object(); // Restore timed permissions from previous shutdown (stored as expireAtMillis) if (plugin.getServerDataFile().getData() != null @@ -161,25 +163,32 @@ public void addPermission(UUID uuid, String permission) { return; } - for (String perm : permission.split(Pattern.quote("|"))) { - PlayerPermissionHandler handle = perms.get(uuid); + synchronized (stateLock(uuid)) { + for (String perm : permission.split(Pattern.quote("|"))) { + PlayerPermissionHandler handle = perms.get(uuid); - if (handle != null) { - handle.addPerm(perm); - continue; - } + if (handle != null) { + handle.addPerm(perm); + continue; + } + PlayerPermissionHandler pending = permsToAdd.get(uuid); + if (pending != null) { + pending.addOfflinePerm(perm, ParsedDuration.empty()); + continue; + } - Player p = Bukkit.getPlayer(uuid); - if (p != null) { - PermissionAttachment attachment = p.addAttachment(plugin); - PlayerPermissionHandler newHandle = new PlayerPermissionHandler(uuid, attachment, this).addPerm(perm); - perms.put(uuid, newHandle); - } else { - permsToAdd.compute(uuid, (ignored, pending) -> { - PlayerPermissionHandler target = pending == null - ? new PlayerPermissionHandler(uuid, null, this) : pending; - return target.addOfflinePerm(perm, ParsedDuration.empty()); - }); + Player p = Bukkit.getPlayer(uuid); + if (p != null) { + PermissionAttachment attachment = p.addAttachment(plugin); + PlayerPermissionHandler newHandle = new PlayerPermissionHandler(uuid, attachment, this).addPerm(perm); + perms.put(uuid, newHandle); + } else { + permsToAdd.compute(uuid, (ignored, existing) -> { + PlayerPermissionHandler target = existing == null + ? new PlayerPermissionHandler(uuid, null, this) : existing; + return target.addOfflinePerm(perm, ParsedDuration.empty()); + }); + } } } } @@ -201,26 +210,33 @@ public void addPermission(UUID uuid, String permission, ParsedDuration duration) return; } - for (String perm : permission.split(Pattern.quote("|"))) { - PlayerPermissionHandler handle = perms.get(uuid); + synchronized (stateLock(uuid)) { + for (String perm : permission.split(Pattern.quote("|"))) { + PlayerPermissionHandler handle = perms.get(uuid); - if (handle != null) { - handle.addExpiration(perm, duration); - continue; - } + if (handle != null) { + handle.addExpiration(perm, duration); + continue; + } + PlayerPermissionHandler pending = permsToAdd.get(uuid); + if (pending != null) { + pending.addOfflinePerm(perm, duration); + continue; + } - Player p = Bukkit.getPlayer(uuid); - if (p != null) { - PermissionAttachment attachment = p.addAttachment(plugin); - PlayerPermissionHandler newHandle = new PlayerPermissionHandler(uuid, attachment, this) - .addExpiration(perm, duration); - perms.put(uuid, newHandle); - } else { - permsToAdd.compute(uuid, (ignored, pending) -> { - PlayerPermissionHandler target = pending == null - ? new PlayerPermissionHandler(uuid, null, this) : pending; - return target.addOfflinePerm(perm, duration); - }); + Player p = Bukkit.getPlayer(uuid); + if (p != null) { + PermissionAttachment attachment = p.addAttachment(plugin); + PlayerPermissionHandler newHandle = new PlayerPermissionHandler(uuid, attachment, this) + .addExpiration(perm, duration); + perms.put(uuid, newHandle); + } else { + permsToAdd.compute(uuid, (ignored, existing) -> { + PlayerPermissionHandler target = existing == null + ? new PlayerPermissionHandler(uuid, null, this) : existing; + return target.addOfflinePerm(perm, duration); + }); + } } } } @@ -241,19 +257,20 @@ public void addPermission(UUID uuid, String permission, long seconds) { */ public void login(Player player) { UUID uuid = player.getUniqueId(); + synchronized (stateLock(uuid)) { + PlayerPermissionHandler handle = perms.get(uuid); + if (handle != null) { + handle.setAttachment(player.addAttachment(plugin)); + handle.onLogin(player); + return; + } - PlayerPermissionHandler handle = perms.get(uuid); - if (handle != null) { - handle.setAttachment(player.addAttachment(plugin)); - handle.onLogin(player); - return; - } - - PlayerPermissionHandler pending = permsToAdd.remove(uuid); - if (pending != null) { - pending.setAttachment(player.addAttachment(plugin)); - pending.onLogin(player); - perms.put(uuid, pending); + PlayerPermissionHandler pending = permsToAdd.remove(uuid); + if (pending != null) { + pending.setAttachment(player.addAttachment(plugin)); + pending.onLogin(player); + perms.put(uuid, pending); + } } } @@ -266,31 +283,46 @@ public void login(Player player) { *

*/ public void logout(Player player) { - PlayerPermissionHandler handle = perms.remove(player.getUniqueId()); - if (handle == null) { - return; - } + UUID uuid = player.getUniqueId(); + synchronized (stateLock(uuid)) { + PlayerPermissionHandler handle = perms.remove(uuid); + if (handle == null) return; - try { - if (handle.getAttachment() != null) { - player.removeAttachment(handle.getAttachment()); + try { + if (handle.getAttachment() != null) player.removeAttachment(handle.getAttachment()); + } catch (Throwable ignored) { } - } catch (Throwable ignored) { - } - handle.setAttachment(null); - handle.onLogout(player); - permsToAdd.put(player.getUniqueId(), handle); + handle.setAttachment(null); + handle.onLogout(player); + permsToAdd.merge(uuid, handle, (pending, moved) -> { + java.util.Map queued = pending.offlinePermissionSnapshot(); + moved.mergeOfflinePermissions(queued); + return moved; + }); + } } public void removePermission(UUID uuid) { - perms.remove(uuid); - permsToAdd.remove(uuid); + synchronized (stateLock(uuid)) { + perms.remove(uuid); + permsToAdd.remove(uuid); + } } void removePermission(UUID uuid, PlayerPermissionHandler expected) { - perms.remove(uuid, expected); - permsToAdd.remove(uuid, expected); + synchronized (stateLock(uuid)) { + perms.remove(uuid, expected); + permsToAdd.remove(uuid, expected); + } + } + + void removePermissionIfEmpty(UUID uuid, PlayerPermissionHandler expected, boolean attachmentIsOffline) { + synchronized (stateLock(uuid)) { + if (!expected.isHandlerEmpty(attachmentIsOffline)) return; + perms.remove(uuid, expected); + permsToAdd.remove(uuid, expected); + } } /** @@ -309,24 +341,26 @@ public void removePermission(UUID uuid, String playerName, String permission) { return; } - PlayerPermissionHandler handle = perms.get(uuid); - if (handle == null) { - handle = permsToAdd.get(uuid); - } - if (handle == null) { - return; - } - - for (String perm : permission.split(Pattern.quote("|"))) { - handle.removePermission(perm); - if (playerName != null && !playerName.isEmpty()) { - plugin.debug("Removing temp permission " + perm + " from " + playerName); - } else { - plugin.debug("Removing temp permission " + perm + " from " + uuid); + synchronized (stateLock(uuid)) { + PlayerPermissionHandler handle = perms.get(uuid); + if (handle == null) handle = permsToAdd.get(uuid); + if (handle == null) return; + + for (String perm : permission.split(Pattern.quote("|"))) { + handle.removePermission(perm); + if (playerName != null && !playerName.isEmpty()) { + plugin.debug("Removing temp permission " + perm + " from " + playerName); + } else { + plugin.debug("Removing temp permission " + perm + " from " + uuid); + } } } } + private Object stateLock(UUID uuid) { + return stateLocks[(uuid.hashCode() & Integer.MAX_VALUE) % stateLocks.length]; + } + /** * Persists timed permissions for both online + offline handlers. */ diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java index 7253845f01..658d7a71b3 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java @@ -114,6 +114,16 @@ public synchronized PlayerPermissionHandler addOfflinePerm(String perm, ParsedDu return this; } + synchronized void mergeOfflinePermissions(java.util.Map queued) { + if (queued.isEmpty()) return; + if (permsToAdd == null) permsToAdd = new HashMap<>(); + permsToAdd.putAll(queued); + } + + synchronized java.util.Map offlinePermissionSnapshot() { + return permsToAdd == null ? java.util.Map.of() : new HashMap<>(permsToAdd); + } + /** * Backwards-compatible offline queue. */ @@ -199,43 +209,51 @@ synchronized boolean isExpirationCurrent(String perm, long expectedExpireAt) { * Expire only the scheduled grant that still owns this permission. The * attachment flag is true only from the player's owning platform scheduler. */ - synchronized void expirePermission(String perm, long expectedExpireAt, boolean updateAttachment) { - if (timedPermissions == null) return; - Long current = timedPermissions.get(perm); - if (current == null || current.longValue() != expectedExpireAt) return; - if (!timedPermissions.remove(perm, current)) return; - persistentPermissions.remove(perm); - if (permsToAdd != null) permsToAdd.remove(perm); - if (updateAttachment && attachment != null) attachment.unsetPermission(perm); - removeHandlerIfEmpty(!updateAttachment); + void expirePermission(String perm, long expectedExpireAt, boolean updateAttachment) { + long remaining; + boolean removeHandler; + synchronized (this) { + if (timedPermissions == null) return; + Long current = timedPermissions.get(perm); + if (current == null || current.longValue() != expectedExpireAt) return; + remaining = current.longValue() - System.currentTimeMillis(); + if (remaining > 0L) { + removeHandler = false; + } else { + if (!timedPermissions.remove(perm, current)) return; + persistentPermissions.remove(perm); + if (updateAttachment && attachment != null) attachment.unsetPermission(perm); + removeHandler = isHandlerEmpty(!updateAttachment); + } + } + if (remaining > 0L) handler.scheduleExpiration(this, perm, expectedExpireAt, remaining); + else if (removeHandler) handler.removePermissionIfEmpty(uuid, this, !updateAttachment); } - private void removeHandlerIfEmpty(boolean attachmentIsOffline) { + synchronized boolean isHandlerEmpty(boolean attachmentIsOffline) { boolean noTracked = persistentPermissions.isEmpty() && (timedPermissions == null || timedPermissions.isEmpty()) && (permsToAdd == null || permsToAdd.isEmpty()); - if (noTracked && (attachmentIsOffline || attachment == null || attachment.getPermissions().isEmpty())) { - handler.removePermission(uuid, this); - } + return noTracked && (attachmentIsOffline || attachment == null || attachment.getPermissions().isEmpty()); } /** * Removes a permission from both internal tracking and the live attachment (if online). */ - public synchronized void removePermission(String perm) { - persistentPermissions.remove(perm); - if (timedPermissions != null) { - timedPermissions.remove(perm); - } - if (permsToAdd != null) { - permsToAdd.remove(perm); - } + public void removePermission(String perm) { + boolean removeHandler; + synchronized (this) { + persistentPermissions.remove(perm); + if (timedPermissions != null) timedPermissions.remove(perm); + if (permsToAdd != null) permsToAdd.remove(perm); + + if (attachment != null) { + attachment.setPermission(perm, false); + attachment.getPermissions().remove(perm); + } - if (attachment != null) { - attachment.setPermission(perm, false); - attachment.getPermissions().remove(perm); + removeHandler = isHandlerEmpty(false); } - - removeHandlerIfEmpty(false); + if (removeHandler) handler.removePermissionIfEmpty(uuid, this, false); } } diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java index e15cadb931..ea97e1b8db 100644 --- a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java @@ -96,7 +96,8 @@ void offOwnerExpirationUpdatesStateWithoutTouchingAttachment() { PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); - long expiry = handler.getTimedPermissions().get("example.use"); + long expiry = System.currentTimeMillis() - 1L; + handler.getTimedPermissions().put("example.use", expiry); clearInvocations(attachment); handler.expirePermission("example.use", expiry, false); @@ -113,12 +114,13 @@ void offlineExpirationDropsEmptyHandlerDespiteStaleAttachmentState() { UUID uuid = UUID.randomUUID(); PlayerPermissionHandler handler = new PlayerPermissionHandler(uuid, attachment, manager); handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); - long expiry = handler.getTimedPermissions().get("example.use"); + long expiry = System.currentTimeMillis() - 1L; + handler.getTimedPermissions().put("example.use", expiry); clearInvocations(attachment); handler.expirePermission("example.use", expiry, false); - verify(manager).removePermission(uuid, handler); + verify(manager).removePermissionIfEmpty(uuid, handler, true); verify(attachment, never()).unsetPermission(anyString()); verify(attachment, never()).setPermission(anyString(), anyBoolean()); } @@ -130,11 +132,94 @@ void expirationClearsAttachmentEntryInsteadOfInstallingDenial() { when(attachment.getPermissions()).thenReturn(new java.util.LinkedHashMap<>()); PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); - long expiry = handler.getTimedPermissions().get("example.use"); + long expiry = System.currentTimeMillis() - 1L; + handler.getTimedPermissions().put("example.use", expiry); clearInvocations(attachment); handler.expirePermission("example.use", expiry, true); verify(attachment).unsetPermission("example.use"); verify(attachment, never()).setPermission("example.use", false); } + @Test + void earlyTimerReschedulesInsteadOfRevokingPermission() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), attachment, manager); + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long expiry = handler.getTimedPermissions().get("example.use"); + clearInvocations(manager, attachment); + + handler.expirePermission("example.use", expiry, true); + + assertTrue(handler.isExpirationCurrent("example.use", expiry)); + verify(manager).scheduleExpiration(eq(handler), eq("example.use"), eq(expiry), longThat(delay -> delay > 0)); + verify(attachment, never()).unsetPermission(anyString()); + } + + @Test + void oldTimedExpirationPreservesQueuedRegrant() { + PermissionHandler manager = mock(PermissionHandler.class); + PermissionAttachment attachment = mock(PermissionAttachment.class); + PlayerPermissionHandler handler = new PlayerPermissionHandler(UUID.randomUUID(), null, manager); + handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long expiry = System.currentTimeMillis() - 1L; + handler.getTimedPermissions().put("example.use", expiry); + handler.addOfflinePerm("example.use", ParsedDuration.empty()); + + handler.expirePermission("example.use", expiry, false); + handler.setAttachment(attachment); + handler.onLogin(mock(Player.class)); + + verify(attachment).setPermission("example.use", true); + } + + @Test + void logoutHandoffIsAtomicWithOfflineGrant() throws Exception { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.getServerDataFile().getData()).thenReturn(null); + PermissionHandler manager = new PermissionHandler(plugin); + UUID uuid = UUID.randomUUID(); + Player player = mock(Player.class); + PermissionAttachment oldAttachment = mock(PermissionAttachment.class); + when(player.getUniqueId()).thenReturn(uuid); + PlayerPermissionHandler active = new PlayerPermissionHandler(uuid, oldAttachment, manager).addPerm("existing.use"); + manager.getPerms().put(uuid, active); + java.util.concurrent.CountDownLatch logoutEntered = new java.util.concurrent.CountDownLatch(1); + java.util.concurrent.CountDownLatch releaseLogout = new java.util.concurrent.CountDownLatch(1); + doAnswer(call -> { + logoutEntered.countDown(); + releaseLogout.await(); + return null; + }).when(player).removeAttachment(oldAttachment); + java.util.concurrent.atomic.AtomicBoolean grantFinished = new java.util.concurrent.atomic.AtomicBoolean(); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(null); + Thread logout = new Thread(() -> manager.logout(player)); + Thread grant = new Thread(() -> { + manager.addPermission(uuid, "new.use"); + grantFinished.set(true); + }); + logout.start(); + assertTrue(logoutEntered.await(1, java.util.concurrent.TimeUnit.SECONDS)); + grant.start(); + Thread.sleep(50L); + assertFalse(grantFinished.get(), "grant must wait for the map handoff"); + releaseLogout.countDown(); + logout.join(1_000L); + grant.join(1_000L); + assertFalse(logout.isAlive()); + assertFalse(grant.isAlive()); + assertSame(active, manager.getPermsToAdd().get(uuid)); + PermissionAttachment newAttachment = mock(PermissionAttachment.class); + active.setAttachment(newAttachment); + active.onLogin(player); + verify(newAttachment).setPermission("existing.use", true); + verify(newAttachment).setPermission("new.use", true); + } finally { + releaseLogout.countDown(); + manager.getTimer().shutdownNow(); + } + } + } From 68f13be7dee16614994edc913d3bc0b9db83642f Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:20:17 -0600 Subject: [PATCH 5/6] Fence offline permission expiry against login --- .../api/permissions/PermissionHandler.java | 15 ++++++++- .../PlayerPermissionHandlerThreadingTest.java | 31 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java index 295dd0565b..e5502f55e4 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java @@ -111,7 +111,7 @@ void dispatchExpiration(PlayerPermissionHandler handle, String permission, long if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; Player player = Bukkit.getPlayer(handle.getUuid()); if (player == null) { - handle.expirePermission(permission, expectedExpireAt, false); + expireOfflineOrRetry(handle, permission, expectedExpireAt); return; } try { @@ -130,6 +130,19 @@ void dispatchExpiration(PlayerPermissionHandler handle, String permission, long } } + private void expireOfflineOrRetry(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { + boolean active; + UUID uuid = handle.getUuid(); + synchronized (stateLock(uuid)) { + if (permsToAdd.get(uuid) == handle) { + handle.expirePermission(permission, expectedExpireAt, false); + return; + } + active = perms.get(uuid) == handle; + } + if (active) retryExpiration(handle, permission, expectedExpireAt); + } + private void retryExpiration(PlayerPermissionHandler handle, String permission, long expectedExpireAt) { if (!acceptingExpirations.get() || !handle.isExpirationCurrent(permission, expectedExpireAt)) return; try { scheduleExpiration(handle, permission, expectedExpireAt, 1_000L); } diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java index ea97e1b8db..fdd035cf48 100644 --- a/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandlerThreadingTest.java @@ -222,4 +222,35 @@ void logoutHandoffIsAtomicWithOfflineGrant() throws Exception { } } + @Test + void offlineExpirationCannotRaceACompletedLogin() { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.getServerDataFile().getData()).thenReturn(null); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + when(plugin.getBukkitScheduler()).thenReturn(scheduler); + PermissionHandler manager = spy(new PermissionHandler(plugin)); + UUID uuid = UUID.randomUUID(); + PlayerPermissionHandler handle = mock(PlayerPermissionHandler.class); + when(handle.getUuid()).thenReturn(uuid); + long expiry = System.currentTimeMillis() - 1L; + when(handle.isExpirationCurrent("example.use", expiry)).thenReturn(true); + manager.getPermsToAdd().put(uuid, handle); + java.util.concurrent.atomic.AtomicReference global = new java.util.concurrent.atomic.AtomicReference<>(); + doAnswer(call -> { global.set(call.getArgument(1)); return null; }) + .when(scheduler).runTask(eq(plugin), any(Runnable.class)); + doNothing().when(manager).scheduleExpiration(handle, "example.use", expiry, 1_000L); + Player player = mock(Player.class); + when(player.getUniqueId()).thenReturn(uuid); + when(player.addAttachment(plugin)).thenReturn(mock(PermissionAttachment.class)); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(null); + manager.dispatchExpiration(handle, "example.use", expiry); + manager.login(player); + global.get().run(); + verify(handle, never()).expirePermission("example.use", expiry, false); + verify(manager).scheduleExpiration(handle, "example.use", expiry, 1_000L); + } finally { manager.getTimer().shutdownNow(); } + } + } From d0f9815e33c65f8b5819d4d8a5b1db1a0082d197 Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:35:11 -0600 Subject: [PATCH 6/6] Reject stale delayed login callbacks --- .../listeners/PlayerJoinEvent.java | 52 ++++++++++++------- .../listeners/PlayerJoinEventSessionTest.java | 46 ++++++++++++++++ 2 files changed, 79 insertions(+), 19 deletions(-) create mode 100644 AdvancedCore/src/test/java/com/bencodez/advancedcore/listeners/PlayerJoinEventSessionTest.java diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java index 540375e3dd..6e102c45fa 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/listeners/PlayerJoinEvent.java @@ -1,6 +1,7 @@ package com.bencodez.advancedcore.listeners; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.ConcurrentHashMap; import org.bukkit.Bukkit; import org.bukkit.entity.Player; @@ -17,7 +18,14 @@ public class PlayerJoinEvent implements Listener { - private final AdvancedCorePlugin plugin; + private final AdvancedCorePlugin plugin; + private final ConcurrentHashMap pendingLoginSessions = new ConcurrentHashMap<>(); + private final Object[] loginSessionLocks = java.util.stream.IntStream.range(0, 64) + .mapToObj(ignored -> new Object()).toArray(); + + private Object loginSessionLock(java.util.UUID uuid) { + return loginSessionLocks[(uuid.hashCode() & Integer.MAX_VALUE) % loginSessionLocks.length]; + } public PlayerJoinEvent(AdvancedCorePlugin plugin) { this.plugin = plugin; @@ -58,7 +66,7 @@ public void onJoin(AdvancedCoreLoginEvent event) { } @EventHandler(priority = EventPriority.HIGHEST, ignoreCancelled = true) - public void onPlayerLogin(final org.bukkit.event.player.PlayerJoinEvent event) { + public void onPlayerLogin(final org.bukkit.event.player.PlayerJoinEvent event) { if (plugin == null || !plugin.isEnabled() || !plugin.isLoadUserData()) { return; } @@ -70,7 +78,11 @@ public void onPlayerLogin(final org.bukkit.event.player.PlayerJoinEvent event) { plugin.debug("Login: " + event.getPlayer().getName() + " (" + event.getPlayer().getUniqueId() + ")"); } - plugin.getLoginTimer().schedule(new Runnable() { + Player joiningPlayer = event.getPlayer(); + synchronized (loginSessionLock(joiningPlayer.getUniqueId())) { + pendingLoginSessions.put(joiningPlayer.getUniqueId(), joiningPlayer); + } + plugin.getLoginTimer().schedule(new Runnable() { @Override public void run() { @@ -86,10 +98,10 @@ public void run() { return; } - Player player = event.getPlayer(); - if (player == null) { - return; - } + Player player = event.getPlayer(); + if (player == null || pendingLoginSessions.get(player.getUniqueId()) != player) { + return; + } // Vanish metadata support for (MetadataValue meta : player.getMetadata("vanished")) { @@ -115,9 +127,10 @@ public void run() { plugin.debug("Login: " + player.getName() + " (" + player.getUniqueId() + ")"); - if (plugin.getPermissionHandler() != null) { - plugin.getPermissionHandler().login(player); - } + synchronized (loginSessionLock(player.getUniqueId())) { + if (pendingLoginSessions.get(player.getUniqueId()) != player) return; + if (plugin.getPermissionHandler() != null) plugin.getPermissionHandler().login(player); + } // Resolve UUID BEFORE constructing/getting the user final String resolvedUuid = plugin.getOptions().isOnlineMode() ? player.getUniqueId().toString() @@ -142,9 +155,9 @@ public void run() { return; } - } catch (Exception e) { - e.printStackTrace(); - } + } catch (Exception e) { + e.printStackTrace(); + } finally { pendingLoginSessions.remove(joiningPlayer.getUniqueId(), joiningPlayer); } } }, 1500 + plugin.getOptions().getDelayLoginEventMs(), TimeUnit.MILLISECONDS); } @@ -155,11 +168,12 @@ public void onPlayerQuit(PlayerQuitEvent event) { return; } - Player player = event.getPlayer(); - plugin.debug("Logout: " + player.getName() + " (" + player.getUniqueId() + ")"); - - if (plugin.getPermissionHandler() != null) { - plugin.getPermissionHandler().logout(player); + Player player = event.getPlayer(); + plugin.debug("Logout: " + player.getName() + " (" + player.getUniqueId() + ")"); + + synchronized (loginSessionLock(player.getUniqueId())) { + pendingLoginSessions.remove(player.getUniqueId(), player); + if (plugin.getPermissionHandler() != null) plugin.getPermissionHandler().logout(player); } plugin.getLoginTimer().execute(new Runnable() { diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/listeners/PlayerJoinEventSessionTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/listeners/PlayerJoinEventSessionTest.java new file mode 100644 index 0000000000..a019453273 --- /dev/null +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/listeners/PlayerJoinEventSessionTest.java @@ -0,0 +1,46 @@ +package com.bencodez.advancedcore.listeners; + +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.util.UUID; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +import org.bukkit.entity.Player; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.advancedcore.api.permissions.PermissionHandler; + +class PlayerJoinEventSessionTest { + @Test + void delayedLoginDoesNotRestorePermissionsAfterThePlayerQuit() { + AdvancedCorePlugin plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + ScheduledExecutorService loginTimer = mock(ScheduledExecutorService.class); + PermissionHandler permissions = mock(PermissionHandler.class); + when(plugin.isEnabled()).thenReturn(true); + when(plugin.isLoadUserData()).thenReturn(true); + when(plugin.getOptions().isHideLoginMessage()).thenReturn(true); + when(plugin.getLoginTimer()).thenReturn(loginTimer); + when(plugin.getPermissionHandler()).thenReturn(permissions); + Player player = mock(Player.class); + when(player.getUniqueId()).thenReturn(UUID.randomUUID()); + when(player.getName()).thenReturn("Player"); + org.bukkit.event.player.PlayerJoinEvent join = mock(org.bukkit.event.player.PlayerJoinEvent.class); + when(join.getPlayer()).thenReturn(player); + org.bukkit.event.player.PlayerQuitEvent quit = mock(org.bukkit.event.player.PlayerQuitEvent.class); + when(quit.getPlayer()).thenReturn(player); + ArgumentCaptor delayed = ArgumentCaptor.forClass(Runnable.class); + + PlayerJoinEvent listener = new PlayerJoinEvent(plugin); + listener.onPlayerLogin(join); + verify(loginTimer).schedule(delayed.capture(), anyLong(), eq(TimeUnit.MILLISECONDS)); + listener.onPlayerQuit(quit); + delayed.getValue().run(); + + verify(permissions).logout(player); + verify(permissions, never()).login(any()); + } +}