diff --git a/src/main/java/studio/magemonkey/fabled/api/util/FlagManager.java b/src/main/java/studio/magemonkey/fabled/api/util/FlagManager.java index e4596380a8..debeba9c37 100644 --- a/src/main/java/studio/magemonkey/fabled/api/util/FlagManager.java +++ b/src/main/java/studio/magemonkey/fabled/api/util/FlagManager.java @@ -29,13 +29,22 @@ import org.bukkit.entity.LivingEntity; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; /** * The manager for temporary entity flag data */ public class FlagManager { - private static final Map data = new HashMap<>(); + private static final Map data = new HashMap<>(); + /** + * Guards against reentrant clearFlags calls on the same entity. + * Prevents infinite recursion when FlagExpireTrigger skills add new flags + * during flag-clearing (e.g., on player death), which would re-create a + * FlagData entry and cause clearFlags → clear → removeFlag → clearFlags → ∞. + */ + private static final Set clearingEntities = new HashSet<>(); /** * Retrieves the flag data for an entity. This creates new data if @@ -128,9 +137,21 @@ public static void clearFlags(LivingEntity entity) { if (entity == null) { return; } - FlagData result = data.remove(entity.getEntityId()); - if (result != null) { - result.clear(); + // Guard against reentrant calls on the same entity. + // FlagExpireTrigger skills may call addFlag during clearing, + // re-creating a FlagData entry; without this guard that would + // cause infinite recursion (clearFlags → clear → removeFlag → clearFlags …). + int id = entity.getEntityId(); + if (!clearingEntities.add(id)) { + return; // already being cleared — skip + } + try { + FlagData result = data.remove(id); + if (result != null) { + result.clear(); + } + } finally { + clearingEntities.remove(id); } } } diff --git a/src/test/java/studio/magemonkey/fabled/api/util/FlagManagerReentrancyTest.java b/src/test/java/studio/magemonkey/fabled/api/util/FlagManagerReentrancyTest.java new file mode 100644 index 0000000000..fcc50bc605 --- /dev/null +++ b/src/test/java/studio/magemonkey/fabled/api/util/FlagManagerReentrancyTest.java @@ -0,0 +1,87 @@ +package studio.magemonkey.fabled.api.util; + +import org.bukkit.entity.LivingEntity; +import org.bukkit.event.EventPriority; +import org.bukkit.event.Listener; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import studio.magemonkey.fabled.api.event.FlagExpireEvent; +import studio.magemonkey.fabled.testutil.MockedTest; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers the infinite-recursion fix in FlagManager.clearFlags: a FlagExpireTrigger-driven + * skill can add a new flag to an entity while that entity's flags are being cleared (e.g. + * on death), which previously re-entered clearFlags for the same entity and recursed forever. + */ +public class FlagManagerReentrancyTest extends MockedTest implements Listener { + private LivingEntity entity; + + @BeforeEach + void setup() { + entity = genPlayer("ReentrancyTester"); + } + + private void reflagOnExpire(String expiredFlag, String newFlag) { + server.getPluginManager().registerEvent(FlagExpireEvent.class, this, EventPriority.NORMAL, + (listener, event) -> { + FlagExpireEvent e = (FlagExpireEvent) event; + if (e.getEntity().equals(entity) && e.getFlag().equals(expiredFlag)) { + FlagManager.addFlag(entity, newFlag, 100); + } + }, plugin, true); + } + + @Test + void clearFlags_reentrantAddDuringExpire_doesNotRecurseForever() { + reflagOnExpire("initial", "reflag"); + + FlagManager.addFlag(entity, "initial", 100); + + assertDoesNotThrow(() -> FlagManager.clearFlags(entity)); + } + + @Test + void clearFlags_reentrantAdd_leavesNewlyAddedFlagIntact() { + reflagOnExpire("initial", "reflag"); + + FlagManager.addFlag(entity, "initial", 100); + FlagManager.clearFlags(entity); + + assertFalse(FlagManager.hasFlag(entity, "initial")); + assertTrue(FlagManager.hasFlag(entity, "reflag")); + } + + @Test + void clearFlags_selfReflagLoop_doesNotRecurseForever() { + // A skill that keeps re-adding the same flag on every expire would, without the + // guard, cause clear() -> removeFlag() -> expire event -> addFlag() -> (flags now + // empty again) -> clearFlags() -> clear() -> ... indefinitely for the same entity. + reflagOnExpire("loopy", "loopy"); + + FlagManager.addFlag(entity, "loopy", 100); + + assertDoesNotThrow(() -> FlagManager.clearFlags(entity)); + // The flag re-added mid-clear should survive the clear rather than being dropped. + assertTrue(FlagManager.hasFlag(entity, "loopy")); + } + + @Test + void clearFlags_normalCase_clearsAllFlags() { + FlagManager.addFlag(entity, "a", 100); + FlagManager.addFlag(entity, "b", 100); + + FlagManager.clearFlags(entity); + + assertFalse(FlagManager.hasFlag(entity, "a")); + assertFalse(FlagManager.hasFlag(entity, "b")); + } + + @Test + void clearFlags_nullEntity_doesNotThrow() { + assertDoesNotThrow(() -> FlagManager.clearFlags(null)); + } +}