From 4dc6668105685980cb47b55e268a57c713eb0a30 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Wed, 21 Oct 2026 16:48:59 +0200 Subject: [PATCH 1/5] Stop guild menus from stalling the server with trade recalculations Opening a guild that earns no trade reran the whole province trade flood every time, and the first guild view after a restart repeated the startup run. Each flood also rebuilt every province owner's stability, with a treasury read, once per foreign guild it reached. - A guild menu now refreshes trade at most once a minute; settlement, staff topology changes and inactivity still recalculate every time. - A full recalculation counts as the latest one, so the startup run satisfies the first menu open. - Stability reports are kept for the length of one recalculation only. Nothing the flood writes feeds stability, so the results are the same. - Guild lookup by id walks each realm's guilds without copying the list, in the same order and with the same case-insensitive match. Co-Authored-By: Claude Opus 5.5 --- .../simplefactions/government/Government.java | 2 +- .../government/stability/StabilityMemo.java | 43 +++++++++++++++ .../managers/FactionManager.java | 5 +- .../managers/ProvinceManager.java | 27 ++++++++++ .../managers/inventory/GuildView.java | 2 +- .../objects/handler/GuildHandler.java | 8 +++ .../stability/StabilityMemoTest.java | 52 +++++++++++++++++++ .../guild/income/MercenarySettlementTest.java | 4 ++ .../managers/FactionManagerCoverageTest.java | 10 ++++ .../managers/ProvinceEconomyCoverageTest.java | 34 ++++++++++++ .../inventory/GuildMenusCoverageTest.java | 3 +- .../handler/GuildHandlerFindGuildTest.java | 41 +++++++++++++++ 12 files changed, 225 insertions(+), 6 deletions(-) create mode 100644 src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java create mode 100644 src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java create mode 100644 src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java diff --git a/src/main/java/net/tfminecraft/simplefactions/government/Government.java b/src/main/java/net/tfminecraft/simplefactions/government/Government.java index 3e65d984..64481bd0 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/Government.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/Government.java @@ -595,7 +595,7 @@ public double getBaseStability() { } public StabilityReport stateReport() { - return StateStability.of(f); + return net.tfminecraft.simplefactions.government.stability.StabilityMemo.report(f, () -> StateStability.of(f)); } public boolean isIllegitimate() { diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java new file mode 100644 index 00000000..aa3ba2a2 --- /dev/null +++ b/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java @@ -0,0 +1,43 @@ +package net.tfminecraft.simplefactions.government.stability; + +import java.util.IdentityHashMap; +import java.util.Map; +import java.util.function.Supplier; + +import net.tfminecraft.simplefactions.objects.Faction; + +/** + * One stability report per realm for the length of a trade recalculation. The flood asks for a + * province owner's stability once per foreign guild it reaches, and each ask reads the realm + * guild's treasury. Nothing the flood writes feeds stability, so every ask in one run gets the + * same report. + */ +public final class StabilityMemo { + private static final ThreadLocal> RUN = new ThreadLocal<>(); + + private StabilityMemo() {} + + /** Starts a run on this thread. False when one is already running, so only the outer run ends it. */ + public static boolean begin() { + if (RUN.get() != null) return false; + RUN.set(new IdentityHashMap<>()); + return true; + } + + public static void end(boolean began) { + if (began) RUN.remove(); + } + + /** The faction's report for this run, or a fresh one outside a run. */ + public static StabilityReport report(Faction faction, Supplier assess) { + Map run = RUN.get(); + if (run == null || faction == null) return assess.get(); + StabilityReport report = run.get(faction); + if (report == null) { + // get/put rather than computeIfAbsent: assessing one realm may ask about another. + report = assess.get(); + run.put(faction, report); + } + return report; + } +} diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java index 58f8f231..2757740d 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java @@ -642,9 +642,8 @@ public static Guild getGuildByLeader(String leader) { public static Guild getGuildByString(String id) { for(Faction f : factions) { - for(Guild guild : f.getGuildHandler().getGuilds()) { - if(guild.getId().equalsIgnoreCase(id)) return guild; - } + Guild guild = f.getGuildHandler().findGuild(id); + if(guild != null) return guild; } return null; } diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java index 254b9e5b..a29c72fa 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java @@ -7,6 +7,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.function.LongSupplier; import org.bukkit.Bukkit; @@ -33,6 +34,7 @@ import net.tfminecraft.simplefactions.war.resolution.PillageTradeHit; import net.tfminecraft.simplefactions.enums.GuildModifier; import net.tfminecraft.simplefactions.government.proposal.TaxTarget; +import net.tfminecraft.simplefactions.government.stability.StabilityMemo; import net.tfminecraft.simplefactions.objects.handler.TaxHandler; import net.tfminecraft.simplefactions.laws.Law; import net.tfminecraft.simplefactions.laws.LawGroup; @@ -47,6 +49,12 @@ public class ProvinceManager { private Map> installationAccess = new HashMap<>(); private long stateVersion = 0; private long lastCalculatedVersion = -1; + /** A menu open refreshes trade at most this often. Settlement and staff changes always recalculate. */ + static final long MENU_REFRESH_MILLIS = 60_000L; + /** Monotonic milliseconds, so a wall-clock change cannot hold off menu refreshes. */ + static LongSupplier clock = () -> System.nanoTime() / 1_000_000L; + private boolean calculated; + private long lastCalculatedAt; public void markDirty() { stateVersion++; @@ -59,6 +67,15 @@ public void recalculateIfNeeded() { lastCalculatedVersion = stateVersion; } + /** + * A menu's refresh. Skipped when a full recalculation finished within the last minute, so + * opening a guild that earns no trade does not flood every province each time. + */ + public void recalculateUnlessRecent() { + if (calculated && clock.getAsLong() - lastCalculatedAt < MENU_REFRESH_MILLIS) return; + recalculate(); + } + public List getProvinces() { return new ArrayList<>(provinces.values()); } public Province get(int id) { @@ -116,6 +133,7 @@ public void recalculate() { return; } InstallationAccess.beginRecalculation(); + boolean memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -129,7 +147,12 @@ public void recalculate() { } recalculateProsperity(); for(Guild guild : FactionManager.getAllGuilds()) getIncome(guild); + // The startup run counts, so the first guild view after a restart does not repeat it. + lastCalculatedVersion = stateVersion; + calculated = true; + lastCalculatedAt = clock.getAsLong(); } finally { + StabilityMemo.end(memo); InstallationAccess.endRecalculation(); } } @@ -145,6 +168,7 @@ public void recalculateQuiet(List guilds, List factions) { previewGuilds = guilds == null ? List.of() : List.copyOf(guilds); previewFactions = factions == null ? List.of() : List.copyOf(factions); InstallationAccess.beginRecalculation(); + boolean memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -161,6 +185,7 @@ public void recalculateQuiet(List guilds, List factions) { } recalculateProsperity(); } finally { + StabilityMemo.end(memo); InstallationAccess.endRecalculation(); } } @@ -176,6 +201,7 @@ public void recalculateForSingleGuild(Guild g, boolean save) { return; } InstallationAccess.beginRecalculation(); + boolean memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -189,6 +215,7 @@ public void recalculateForSingleGuild(Guild g, boolean save) { } } } finally { + StabilityMemo.end(memo); InstallationAccess.endRecalculation(); } } diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java index 5c1a8e44..e5d8f0af 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java @@ -201,7 +201,7 @@ public void guildView(Player player, Guild guild, Inventory i) { } if(manager == null) setProvinceManager(SimpleFactions.getInstance().getProvinceManager()); if(guild.hasCapital()) { - if(guild.getTradeBreakdown().getIncome() == 0) manager.recalculate(); + if(guild.getTradeBreakdown().getIncome() == 0) manager.recalculateUnlessRecent(); manager.getIncome(guild); } manager.recalculateIfNeeded(); diff --git a/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java b/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java index db3e7033..39b6d118 100644 --- a/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java +++ b/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java @@ -23,6 +23,14 @@ public List getGuilds() { return new ArrayList<>(guilds.values()); } + /** The first guild in {@link #getGuilds()} order whose id matches, ignoring case, without copying the list. */ + public Guild findGuild(String id) { + for (Guild guild : guilds.values()) { + if (guild.getId().equalsIgnoreCase(id)) return guild; + } + return null; + } + public List getReleasableGuilds() { List releasable = new ArrayList<>(); for(Guild g : guilds.values()) { diff --git a/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java b/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java new file mode 100644 index 00000000..6523420d --- /dev/null +++ b/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java @@ -0,0 +1,52 @@ +package net.tfminecraft.simplefactions.government.stability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Test; + +import net.tfminecraft.simplefactions.objects.Faction; + +class StabilityMemoTest { + + @Test + void aRunAssessesEachRealmOnceAndOnlyTheOuterRunEndsIt() { + Faction realm = mock(Faction.class); + Faction other = mock(Faction.class); + AtomicInteger assessed = new AtomicInteger(); + boolean outer = StabilityMemo.begin(); + try { + assertTrue(outer); + StabilityReport first = StabilityMemo.report(realm, () -> fresh(assessed)); + assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); + boolean inner = StabilityMemo.begin(); + assertFalse(inner); + StabilityMemo.end(inner); + assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); + // Assessing one realm may ask about another while the first is still being assessed. + StabilityReport nested = StabilityMemo.report(other, () -> { + StabilityMemo.report(realm, () -> fresh(assessed)); + return fresh(assessed); + }); + assertSame(nested, StabilityMemo.report(other, () -> fresh(assessed))); + assertNotSame(StabilityMemo.report(null, () -> fresh(assessed)), StabilityMemo.report(null, () -> fresh(assessed))); + assertEquals(4, assessed.get()); + } finally { + StabilityMemo.end(outer); + } + assertNotSame(StabilityMemo.report(realm, () -> fresh(assessed)), StabilityMemo.report(realm, () -> fresh(assessed))); + assertTrue(StabilityMemo.begin()); + StabilityMemo.end(true); + } + + private static StabilityReport fresh(AtomicInteger assessed) { + assessed.incrementAndGet(); + return new StabilityReport(); + } +} diff --git a/src/test/java/net/tfminecraft/simplefactions/guild/income/MercenarySettlementTest.java b/src/test/java/net/tfminecraft/simplefactions/guild/income/MercenarySettlementTest.java index 98796d77..719b1da0 100644 --- a/src/test/java/net/tfminecraft/simplefactions/guild/income/MercenarySettlementTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/guild/income/MercenarySettlementTest.java @@ -302,6 +302,10 @@ private static final class Party { when(faction.getOrCreateMainGuild()).thenReturn(guild); GuildHandler handler = mock(GuildHandler.class); when(handler.getGuilds()).thenReturn(new ArrayList<>(List.of(guild))); + when(handler.findGuild(org.mockito.ArgumentMatchers.any())).thenAnswer(call -> { + String id = call.getArgument(0); + return guildId.equalsIgnoreCase(id) ? guild : null; + }); when(faction.getGuildHandler()).thenReturn(handler); when(guild.getId()).thenReturn(guildId); diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java index 0d18678d..287a2273 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java @@ -133,6 +133,8 @@ private Faction faction(String id, String leader) { when(faction.getProvinces()).thenReturn(new ArrayList<>()); when(faction.getTitles()).thenReturn(new ArrayList<>()); when(faction.getGuildHandler().getGuilds()).thenReturn(new ArrayList<>()); + when(faction.getGuildHandler().findGuild(any())) + .thenAnswer(call -> firstGuild(faction.getGuildHandler().getGuilds(), call.getArgument(0))); when(faction.getGovernment().getFaction()).thenReturn(faction); when(faction.getGovernment().getMovementById(anyString())).thenReturn(null); when(faction.getGovernment().getMovementByLeader(anyString())).thenReturn(null); @@ -149,6 +151,14 @@ private Faction faction(String id, String leader) { return faction; } + /** The real {@code GuildHandler.findGuild} scan over the stubbed guild list. */ + private static Guild firstGuild(List guilds, String id) { + for (Guild guild : guilds) { + if (guild.getId().equalsIgnoreCase(id)) return guild; + } + return null; + } + private Guild guild(Faction faction, String id, String leader) { Guild guild = mock(Guild.class, RETURNS_DEEP_STUBS); when(guild.getId()).thenReturn(id); diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java index bb457603..4cfd3b97 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java @@ -299,6 +299,40 @@ void dirtyRecalculationReplacesEconomicsOnlyWhenTheModelChanges() { assertEquals(prosperity, guild.getTradeBreakdown().getIncome(), 0.01); } + @Test + void menuRefreshSkipsARecentRecalculationAndTheStartupRunSatisfiesTheDirtyCheck() { + java.util.function.LongSupplier oldClock = ProvinceManager.clock; + long[] now = {1_000_000L}; + ProvinceManager.clock = () -> now[0]; + try { + productiveBranch(); + Faction home = fixture.saved("home", "Alice"); + Guild guild = home.getOrCreateMainGuild(); + Province source = province(1); + home.addProvince(1); + home.setCapital(1, true); + fixture.provincesEnabled(true); + manager.recalculateUnlessRecent(); + ProvinceDataEntry first = source.getData(guild.getId()); + assertNotNull(first); + manager.recalculateIfNeeded(); + assertSame(first, source.getData(guild.getId())); + now[0] += ProvinceManager.MENU_REFRESH_MILLIS - 1; + manager.recalculateUnlessRecent(); + assertSame(first, source.getData(guild.getId())); + now[0] += 1; + manager.recalculateUnlessRecent(); + ProvinceDataEntry second = source.getData(guild.getId()); + assertNotSame(first, second); + now[0] += 5; + manager.recalculate(); + assertNotSame(second, source.getData(guild.getId())); + assertEquals(source.getProsperity(), guild.getTradeBreakdown().getIncome(), 0.01); + } finally { + ProvinceManager.clock = oldClock; + } + } + @Test void disabledIncomeQueriesAndRecalculationRespectWhetherSavingWasRequested() { productiveBranch(); diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java index 49253dd0..b831298e 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java @@ -869,7 +869,8 @@ void exactGuildRenderingUsesRealBranchControlsAndCurrentMembership() { assertNotNull(inventory.getItem(17)); assertNotNull(inventory.getItem(8)); assertNotNull(inventory.getItem(19)); - verify(fixture.provinces).recalculate(); + verify(fixture.provinces).recalculateUnlessRecent(); + verify(fixture.provinces, never()).recalculate(); verify(fixture.provinces).getIncome(display); verify(fixture.provinces).recalculateIfNeeded(); previewTasks.verify( diff --git a/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java b/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java new file mode 100644 index 00000000..5bb8c81a --- /dev/null +++ b/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java @@ -0,0 +1,41 @@ +package net.tfminecraft.simplefactions.objects.handler; + +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.junit.jupiter.api.Test; + +import net.tfminecraft.simplefactions.guild.Guild; +import net.tfminecraft.simplefactions.objects.Faction; + +class GuildHandlerFindGuildTest { + + @Test + void findGuildMatchesTheFirstListedIdIgnoringCase() { + GuildHandler handler = new GuildHandler(mock(Faction.class)); + Guild merchants = guild("merchants"); + Guild smiths = guild("Smiths"); + handler.addGuild(merchants); + handler.addGuild(smiths); + Guild expected = null; + for (Guild guild : handler.getGuilds()) { + if (guild.getId().equalsIgnoreCase("SMITHS")) { + expected = guild; + break; + } + } + assertSame(smiths, expected); + assertSame(expected, handler.findGuild("SMITHS")); + assertSame(merchants, handler.findGuild("merchants")); + assertNull(handler.findGuild("bakers")); + assertNull(handler.findGuild(null)); + } + + private static Guild guild(String id) { + Guild guild = mock(Guild.class); + when(guild.getId()).thenReturn(id); + return guild; + } +} From 500ce78cf9972559c2b1cccf7a6c80b93a41aeb0 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Wed, 21 Oct 2026 17:10:55 +0200 Subject: [PATCH 2/5] Keep every zero-income guild menu recalculating and copy lists off-thread Review follow-up. The minute-long menu throttle could leave trade and production at stale values (for example right after a deposit clears a bankruptcy) for movement power, civil war and pillage, so the guild view recalculates on every zero-income open again, exactly as before. Each run is still cheaper because stability is assessed once per realm per run. - Guild lookups off the server thread read a copy of each realm's guilds, as before, so async previews cannot hit a concurrent modification. - Each recalculation, including a nested preview with its own law overlay, keeps its own stability reports; runs stay confined to their thread. Co-Authored-By: Claude Opus 5.5 --- .../government/stability/StabilityMemo.java | 27 +++++--- .../managers/ProvinceManager.java | 24 +------ .../managers/inventory/GuildView.java | 2 +- .../objects/handler/GuildHandler.java | 15 ++++- .../stability/StabilityMemoTest.java | 23 +++---- .../managers/ProvinceEconomyCoverageTest.java | 51 ++++++--------- .../inventory/GuildMenusCoverageTest.java | 6 +- .../handler/GuildHandlerFindGuildTest.java | 63 +++++++++++++++---- 8 files changed, 123 insertions(+), 88 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java index aa3ba2a2..7a107c0d 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java @@ -10,22 +10,35 @@ * One stability report per realm for the length of a trade recalculation. The flood asks for a * province owner's stability once per foreign guild it reaches, and each ask reads the realm * guild's treasury. Nothing the flood writes feeds stability, so every ask in one run gets the - * same report. + * same report. Runs belong to one thread, so an async preview never sees the server's reports. */ public final class StabilityMemo { private static final ThreadLocal> RUN = new ThreadLocal<>(); private StabilityMemo() {} - /** Starts a run on this thread. False when one is already running, so only the outer run ends it. */ - public static boolean begin() { - if (RUN.get() != null) return false; + /** What {@link #begin} replaced, so {@link #end} can put it back. */ + public static final class Run { + private final Map outer; + + private Run(Map outer) { + this.outer = outer; + } + } + + /** + * Starts a run on this thread. A nested run, such as a preview with its own law overlay, + * gets its own reports rather than the outer run's. + */ + public static Run begin() { + Run run = new Run(RUN.get()); RUN.set(new IdentityHashMap<>()); - return true; + return run; } - public static void end(boolean began) { - if (began) RUN.remove(); + public static void end(Run run) { + if (run.outer == null) RUN.remove(); + else RUN.set(run.outer); } /** The faction's report for this run, or a fresh one outside a run. */ diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java index a29c72fa..723b0645 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java @@ -7,7 +7,6 @@ import java.util.Locale; import java.util.Map; import java.util.Set; -import java.util.function.LongSupplier; import org.bukkit.Bukkit; @@ -49,12 +48,6 @@ public class ProvinceManager { private Map> installationAccess = new HashMap<>(); private long stateVersion = 0; private long lastCalculatedVersion = -1; - /** A menu open refreshes trade at most this often. Settlement and staff changes always recalculate. */ - static final long MENU_REFRESH_MILLIS = 60_000L; - /** Monotonic milliseconds, so a wall-clock change cannot hold off menu refreshes. */ - static LongSupplier clock = () -> System.nanoTime() / 1_000_000L; - private boolean calculated; - private long lastCalculatedAt; public void markDirty() { stateVersion++; @@ -67,15 +60,6 @@ public void recalculateIfNeeded() { lastCalculatedVersion = stateVersion; } - /** - * A menu's refresh. Skipped when a full recalculation finished within the last minute, so - * opening a guild that earns no trade does not flood every province each time. - */ - public void recalculateUnlessRecent() { - if (calculated && clock.getAsLong() - lastCalculatedAt < MENU_REFRESH_MILLIS) return; - recalculate(); - } - public List getProvinces() { return new ArrayList<>(provinces.values()); } public Province get(int id) { @@ -133,7 +117,7 @@ public void recalculate() { return; } InstallationAccess.beginRecalculation(); - boolean memo = StabilityMemo.begin(); + StabilityMemo.Run memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -149,8 +133,6 @@ public void recalculate() { for(Guild guild : FactionManager.getAllGuilds()) getIncome(guild); // The startup run counts, so the first guild view after a restart does not repeat it. lastCalculatedVersion = stateVersion; - calculated = true; - lastCalculatedAt = clock.getAsLong(); } finally { StabilityMemo.end(memo); InstallationAccess.endRecalculation(); @@ -168,7 +150,7 @@ public void recalculateQuiet(List guilds, List factions) { previewGuilds = guilds == null ? List.of() : List.copyOf(guilds); previewFactions = factions == null ? List.of() : List.copyOf(factions); InstallationAccess.beginRecalculation(); - boolean memo = StabilityMemo.begin(); + StabilityMemo.Run memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -201,7 +183,7 @@ public void recalculateForSingleGuild(Guild g, boolean save) { return; } InstallationAccess.beginRecalculation(); - boolean memo = StabilityMemo.begin(); + StabilityMemo.Run memo = StabilityMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java index e5d8f0af..5c1a8e44 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildView.java @@ -201,7 +201,7 @@ public void guildView(Player player, Guild guild, Inventory i) { } if(manager == null) setProvinceManager(SimpleFactions.getInstance().getProvinceManager()); if(guild.hasCapital()) { - if(guild.getTradeBreakdown().getIncome() == 0) manager.recalculateUnlessRecent(); + if(guild.getTradeBreakdown().getIncome() == 0) manager.recalculate(); manager.getIncome(guild); } manager.recalculateIfNeeded(); diff --git a/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java b/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java index 39b6d118..1b839266 100644 --- a/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java +++ b/src/main/java/net/tfminecraft/simplefactions/objects/handler/GuildHandler.java @@ -6,6 +6,8 @@ import java.util.List; import java.util.Map; +import org.bukkit.Bukkit; + import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.managers.ProvinceManager; import net.tfminecraft.simplefactions.objects.Faction; @@ -23,14 +25,23 @@ public List getGuilds() { return new ArrayList<>(guilds.values()); } - /** The first guild in {@link #getGuilds()} order whose id matches, ignoring case, without copying the list. */ + /** + * The first guild in {@link #getGuilds()} order whose id matches, ignoring case. The server + * thread reads the map directly; async previews reach this too, so they read a copy, as + * before, and a guild created or removed meanwhile cannot break their iteration. + */ public Guild findGuild(String id) { - for (Guild guild : guilds.values()) { + Iterable each = onServerThread() ? guilds.values() : getGuilds(); + for (Guild guild : each) { if (guild.getId().equalsIgnoreCase(id)) return guild; } return null; } + private static boolean onServerThread() { + return Bukkit.getServer() != null && Bukkit.isPrimaryThread(); + } + public List getReleasableGuilds() { List releasable = new ArrayList<>(); for(Guild g : guilds.values()) { diff --git a/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java b/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java index 6523420d..f0daf7e5 100644 --- a/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java @@ -1,10 +1,8 @@ package net.tfminecraft.simplefactions.government.stability; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import java.util.concurrent.atomic.AtomicInteger; @@ -16,18 +14,23 @@ class StabilityMemoTest { @Test - void aRunAssessesEachRealmOnceAndOnlyTheOuterRunEndsIt() { + void aRunAssessesEachRealmOnceAndANestedRunKeepsItsOwnReports() { Faction realm = mock(Faction.class); Faction other = mock(Faction.class); AtomicInteger assessed = new AtomicInteger(); - boolean outer = StabilityMemo.begin(); + StabilityMemo.Run outer = StabilityMemo.begin(); try { - assertTrue(outer); StabilityReport first = StabilityMemo.report(realm, () -> fresh(assessed)); assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); - boolean inner = StabilityMemo.begin(); - assertFalse(inner); - StabilityMemo.end(inner); + StabilityMemo.Run inner = StabilityMemo.begin(); + StabilityReport nestedRun; + try { + nestedRun = StabilityMemo.report(realm, () -> fresh(assessed)); + assertNotSame(first, nestedRun); + assertSame(nestedRun, StabilityMemo.report(realm, () -> fresh(assessed))); + } finally { + StabilityMemo.end(inner); + } assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); // Assessing one realm may ask about another while the first is still being assessed. StabilityReport nested = StabilityMemo.report(other, () -> { @@ -36,13 +39,11 @@ void aRunAssessesEachRealmOnceAndOnlyTheOuterRunEndsIt() { }); assertSame(nested, StabilityMemo.report(other, () -> fresh(assessed))); assertNotSame(StabilityMemo.report(null, () -> fresh(assessed)), StabilityMemo.report(null, () -> fresh(assessed))); - assertEquals(4, assessed.get()); + assertEquals(5, assessed.get()); } finally { StabilityMemo.end(outer); } assertNotSame(StabilityMemo.report(realm, () -> fresh(assessed)), StabilityMemo.report(realm, () -> fresh(assessed))); - assertTrue(StabilityMemo.begin()); - StabilityMemo.end(true); } private static StabilityReport fresh(AtomicInteger assessed) { diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java index 4cfd3b97..1c422539 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java @@ -300,37 +300,26 @@ void dirtyRecalculationReplacesEconomicsOnlyWhenTheModelChanges() { } @Test - void menuRefreshSkipsARecentRecalculationAndTheStartupRunSatisfiesTheDirtyCheck() { - java.util.function.LongSupplier oldClock = ProvinceManager.clock; - long[] now = {1_000_000L}; - ProvinceManager.clock = () -> now[0]; - try { - productiveBranch(); - Faction home = fixture.saved("home", "Alice"); - Guild guild = home.getOrCreateMainGuild(); - Province source = province(1); - home.addProvince(1); - home.setCapital(1, true); - fixture.provincesEnabled(true); - manager.recalculateUnlessRecent(); - ProvinceDataEntry first = source.getData(guild.getId()); - assertNotNull(first); - manager.recalculateIfNeeded(); - assertSame(first, source.getData(guild.getId())); - now[0] += ProvinceManager.MENU_REFRESH_MILLIS - 1; - manager.recalculateUnlessRecent(); - assertSame(first, source.getData(guild.getId())); - now[0] += 1; - manager.recalculateUnlessRecent(); - ProvinceDataEntry second = source.getData(guild.getId()); - assertNotSame(first, second); - now[0] += 5; - manager.recalculate(); - assertNotSame(second, source.getData(guild.getId())); - assertEquals(source.getProsperity(), guild.getTradeBreakdown().getIncome(), 0.01); - } finally { - ProvinceManager.clock = oldClock; - } + void aFullRecalculationSatisfiesTheDirtyCheckSoTheFirstMenuOpenDoesNotRepeatIt() { + productiveBranch(); + Faction home = fixture.saved("home", "Alice"); + Guild guild = home.getOrCreateMainGuild(); + Province source = province(1); + home.addProvince(1); + home.setCapital(1, true); + fixture.provincesEnabled(true); + manager.recalculate(); + ProvinceDataEntry first = source.getData(guild.getId()); + assertNotNull(first); + manager.recalculateIfNeeded(); + assertSame(first, source.getData(guild.getId())); + manager.recalculate(); + ProvinceDataEntry second = source.getData(guild.getId()); + assertNotSame(first, second); + manager.markDirty(); + manager.recalculateIfNeeded(); + assertNotSame(second, source.getData(guild.getId())); + assertEquals(source.getProsperity(), guild.getTradeBreakdown().getIncome(), 0.01); } @Test diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java index b831298e..07c0b06a 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/inventory/GuildMenusCoverageTest.java @@ -869,8 +869,7 @@ void exactGuildRenderingUsesRealBranchControlsAndCurrentMembership() { assertNotNull(inventory.getItem(17)); assertNotNull(inventory.getItem(8)); assertNotNull(inventory.getItem(19)); - verify(fixture.provinces).recalculateUnlessRecent(); - verify(fixture.provinces, never()).recalculate(); + verify(fixture.provinces).recalculate(); verify(fixture.provinces).getIncome(display); verify(fixture.provinces).recalculateIfNeeded(); previewTasks.verify( @@ -881,6 +880,9 @@ void exactGuildRenderingUsesRealBranchControlsAndCurrentMembership() { () -> BranchIncomePreviewService.schedule( fixture.player, inventory, 38, prepared, display, display.getBranch(0), -1)); + // A guild that still earns no trade recalculates on every open, however soon it reopens. + view.guildView(fixture.player, display, inventory); + verify(fixture.provinces, times(2)).recalculate(); display.setLeader("Other"); display.kick("Leader"); display.setCapital(-1); diff --git a/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java b/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java index 5bb8c81a..9f6675d7 100644 --- a/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/objects/handler/GuildHandlerFindGuildTest.java @@ -3,9 +3,15 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.bukkit.Bukkit; +import org.bukkit.Server; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.objects.Faction; @@ -13,24 +19,55 @@ class GuildHandlerFindGuildTest { @Test - void findGuildMatchesTheFirstListedIdIgnoringCase() { + void findGuildMatchesTheFirstListedIdIgnoringCaseOnTheServerThread() { + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(Bukkit::getServer).thenReturn(mock(Server.class)); + bukkit.when(Bukkit::isPrimaryThread).thenReturn(true); + GuildHandler handler = new GuildHandler(mock(Faction.class)); + Guild merchants = guild("merchants"); + Guild smiths = guild("Smiths"); + handler.addGuild(merchants); + handler.addGuild(smiths); + Guild expected = null; + for (Guild guild : handler.getGuilds()) { + if (guild.getId().equalsIgnoreCase("SMITHS")) { + expected = guild; + break; + } + } + assertSame(smiths, expected); + assertSame(expected, handler.findGuild("SMITHS")); + assertSame(merchants, handler.findGuild("merchants")); + assertNull(handler.findGuild("bakers")); + assertNull(handler.findGuild(null)); + } + } + + @Test + void anAsyncLookupReadsACopySoAGuildAddedMeanwhileCannotBreakIt() { GuildHandler handler = new GuildHandler(mock(Faction.class)); Guild merchants = guild("merchants"); - Guild smiths = guild("Smiths"); handler.addGuild(merchants); + Guild smiths = guild("smiths"); handler.addGuild(smiths); - Guild expected = null; - for (Guild guild : handler.getGuilds()) { - if (guild.getId().equalsIgnoreCase("SMITHS")) { - expected = guild; - break; - } + AtomicBoolean added = new AtomicBoolean(); + // The server thread founds a guild while the preview is still walking the realm. + for (Guild guild : new Guild[] {merchants, smiths}) { + String id = guild.getId(); + when(guild.getId()).thenAnswer(call -> { + if (added.compareAndSet(false, true)) handler.addGuild(guild("bakers")); + return id; + }); + } + try (MockedStatic bukkit = mockStatic(Bukkit.class)) { + bukkit.when(Bukkit::getServer).thenReturn(mock(Server.class)); + bukkit.when(Bukkit::isPrimaryThread).thenReturn(false); + assertNull(handler.findGuild("nobody")); } - assertSame(smiths, expected); - assertSame(expected, handler.findGuild("SMITHS")); - assertSame(merchants, handler.findGuild("merchants")); - assertNull(handler.findGuild("bakers")); - assertNull(handler.findGuild(null)); + // No server at all (unit tests, shutdown) reads a copy too. + assertSame(merchants, handler.findGuild("MERCHANTS")); + assertSame(handler.getGuilds().stream().filter(g -> g.getId().equals("bakers")).findFirst().orElseThrow(), + handler.findGuild("bakers")); } private static Guild guild(String id) { From a0aff74f1b6879a896e447c6537c8c26e81ad748 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Wed, 21 Oct 2026 17:25:36 +0200 Subject: [PATCH 3/5] Let the first guild view after a restart recalculate again Review follow-up. Counting the startup run as the latest one meant changes made between startup and the first guild view (a deposit, for example) waited for the next recalculation, so recalculate() no longer marks the dirty check. Results now match the old behaviour exactly; only the cost of each run drops, through the per-run stability reports. Co-Authored-By: Claude Opus 5.5 --- .../managers/ProvinceManager.java | 2 -- .../managers/ProvinceEconomyCoverageTest.java | 23 ------------------- 2 files changed, 25 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java index 723b0645..c4e9f419 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java @@ -131,8 +131,6 @@ public void recalculate() { } recalculateProsperity(); for(Guild guild : FactionManager.getAllGuilds()) getIncome(guild); - // The startup run counts, so the first guild view after a restart does not repeat it. - lastCalculatedVersion = stateVersion; } finally { StabilityMemo.end(memo); InstallationAccess.endRecalculation(); diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java index 1c422539..bb457603 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/ProvinceEconomyCoverageTest.java @@ -299,29 +299,6 @@ void dirtyRecalculationReplacesEconomicsOnlyWhenTheModelChanges() { assertEquals(prosperity, guild.getTradeBreakdown().getIncome(), 0.01); } - @Test - void aFullRecalculationSatisfiesTheDirtyCheckSoTheFirstMenuOpenDoesNotRepeatIt() { - productiveBranch(); - Faction home = fixture.saved("home", "Alice"); - Guild guild = home.getOrCreateMainGuild(); - Province source = province(1); - home.addProvince(1); - home.setCapital(1, true); - fixture.provincesEnabled(true); - manager.recalculate(); - ProvinceDataEntry first = source.getData(guild.getId()); - assertNotNull(first); - manager.recalculateIfNeeded(); - assertSame(first, source.getData(guild.getId())); - manager.recalculate(); - ProvinceDataEntry second = source.getData(guild.getId()); - assertNotSame(first, second); - manager.markDirty(); - manager.recalculateIfNeeded(); - assertNotSame(second, source.getData(guild.getId())); - assertEquals(source.getProsperity(), guild.getTradeBreakdown().getIncome(), 0.01); - } - @Test void disabledIncomeQueriesAndRecalculationRespectWhetherSavingWasRequested() { productiveBranch(); From 506a95928d229518304de11f4ce6ba8d2635f490 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Wed, 21 Oct 2026 17:45:33 +0200 Subject: [PATCH 4/5] Read each realm's treasury once per recalculation, not its whole stability Review follow-up. Office unrest wears off continuously, so a realm's stability can cross a threshold between two provinces of one run; keeping the whole report would have frozen it. Only the bankruptcy check, a ledger read that nothing in a recalculation changes, is now kept for the run. Modifiers, unrest, laws and everything else in stability are read live on every ask, exactly as before. Co-Authored-By: Claude Opus 5.5 --- .../simplefactions/government/Government.java | 2 +- .../government/stability/BankruptcyMemo.java | 53 ++++++++++ .../government/stability/StabilityMemo.java | 56 ----------- .../government/stability/StateStability.java | 2 +- .../managers/ProvinceManager.java | 14 +-- .../stability/BankruptcyMemoTest.java | 99 +++++++++++++++++++ .../stability/StabilityMemoTest.java | 53 ---------- 7 files changed, 161 insertions(+), 118 deletions(-) create mode 100644 src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java delete mode 100644 src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java create mode 100644 src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java delete mode 100644 src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java diff --git a/src/main/java/net/tfminecraft/simplefactions/government/Government.java b/src/main/java/net/tfminecraft/simplefactions/government/Government.java index 64481bd0..3e65d984 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/Government.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/Government.java @@ -595,7 +595,7 @@ public double getBaseStability() { } public StabilityReport stateReport() { - return net.tfminecraft.simplefactions.government.stability.StabilityMemo.report(f, () -> StateStability.of(f)); + return StateStability.of(f); } public boolean isIllegitimate() { diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java new file mode 100644 index 00000000..7448a8b1 --- /dev/null +++ b/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java @@ -0,0 +1,53 @@ +package net.tfminecraft.simplefactions.government.stability; + +import java.util.IdentityHashMap; +import java.util.Map; + +import net.tfminecraft.simplefactions.guild.Guild; + +/** + * Whether each realm guild is bankrupt, read once per trade recalculation. The flood asks for a + * province owner's stability once per foreign guild it reaches, and each ask reads the realm + * guild's treasury from the ledger. A treasury only changes through a ledger posting, and nothing + * in a recalculation posts one, so every ask in one run gets the same answer. Everything else in + * stability, including unrest that decays over time, is still read live on every ask. + * Runs belong to one thread, so an async preview never sees the server's answers. + */ +public final class BankruptcyMemo { + private static final ThreadLocal> RUN = new ThreadLocal<>(); + + private BankruptcyMemo() {} + + /** What {@link #begin} replaced, so {@link #end} can put it back. */ + public static final class Run { + private final Map outer; + + private Run(Map outer) { + this.outer = outer; + } + } + + /** Starts a run on this thread. A nested run, such as a preview, gets its own answers. */ + public static Run begin() { + Run run = new Run(RUN.get()); + RUN.set(new IdentityHashMap<>()); + return run; + } + + public static void end(Run run) { + if (run.outer == null) RUN.remove(); + else RUN.set(run.outer); + } + + /** {@link Guild#isBankrupt()}, read once per guild in a run and live outside one. */ + public static boolean isBankrupt(Guild guild) { + Map run = RUN.get(); + if (run == null) return guild.isBankrupt(); + Boolean bankrupt = run.get(guild); + if (bankrupt == null) { + bankrupt = guild.isBankrupt(); + run.put(guild, bankrupt); + } + return bankrupt; + } +} diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java deleted file mode 100644 index 7a107c0d..00000000 --- a/src/main/java/net/tfminecraft/simplefactions/government/stability/StabilityMemo.java +++ /dev/null @@ -1,56 +0,0 @@ -package net.tfminecraft.simplefactions.government.stability; - -import java.util.IdentityHashMap; -import java.util.Map; -import java.util.function.Supplier; - -import net.tfminecraft.simplefactions.objects.Faction; - -/** - * One stability report per realm for the length of a trade recalculation. The flood asks for a - * province owner's stability once per foreign guild it reaches, and each ask reads the realm - * guild's treasury. Nothing the flood writes feeds stability, so every ask in one run gets the - * same report. Runs belong to one thread, so an async preview never sees the server's reports. - */ -public final class StabilityMemo { - private static final ThreadLocal> RUN = new ThreadLocal<>(); - - private StabilityMemo() {} - - /** What {@link #begin} replaced, so {@link #end} can put it back. */ - public static final class Run { - private final Map outer; - - private Run(Map outer) { - this.outer = outer; - } - } - - /** - * Starts a run on this thread. A nested run, such as a preview with its own law overlay, - * gets its own reports rather than the outer run's. - */ - public static Run begin() { - Run run = new Run(RUN.get()); - RUN.set(new IdentityHashMap<>()); - return run; - } - - public static void end(Run run) { - if (run.outer == null) RUN.remove(); - else RUN.set(run.outer); - } - - /** The faction's report for this run, or a fresh one outside a run. */ - public static StabilityReport report(Faction faction, Supplier assess) { - Map run = RUN.get(); - if (run == null || faction == null) return assess.get(); - StabilityReport report = run.get(faction); - if (report == null) { - // get/put rather than computeIfAbsent: assessing one realm may ask about another. - report = assess.get(); - run.put(faction, report); - } - return report; - } -} diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/StateStability.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/StateStability.java index b44d3a3e..d474f26a 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/stability/StateStability.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/stability/StateStability.java @@ -26,7 +26,7 @@ public static StabilityFacts facts(Faction faction) { facts.government = law(faction, "government", "autocracy"); facts.electedLeadership = "elected".equalsIgnoreCase(law(faction, "leadership", "fixed")); facts.provinces = faction.getProvinces() == null ? 0 : faction.getProvinces().size(); - facts.bankrupt = faction.getOrCreateMainGuild() != null && faction.getOrCreateMainGuild().isBankrupt(); + facts.bankrupt = faction.getOrCreateMainGuild() != null && BankruptcyMemo.isBankrupt(faction.getOrCreateMainGuild()); if (faction.getGovernment() != null && faction.getGovernment().getStabilityModifiers() != null) { faction.getGovernment().getStabilityModifiers().forEach(modifier -> { if (modifier != null) { diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java index c4e9f419..5a580c5e 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java @@ -33,7 +33,7 @@ import net.tfminecraft.simplefactions.war.resolution.PillageTradeHit; import net.tfminecraft.simplefactions.enums.GuildModifier; import net.tfminecraft.simplefactions.government.proposal.TaxTarget; -import net.tfminecraft.simplefactions.government.stability.StabilityMemo; +import net.tfminecraft.simplefactions.government.stability.BankruptcyMemo; import net.tfminecraft.simplefactions.objects.handler.TaxHandler; import net.tfminecraft.simplefactions.laws.Law; import net.tfminecraft.simplefactions.laws.LawGroup; @@ -117,7 +117,7 @@ public void recalculate() { return; } InstallationAccess.beginRecalculation(); - StabilityMemo.Run memo = StabilityMemo.begin(); + BankruptcyMemo.Run memo = BankruptcyMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -132,7 +132,7 @@ public void recalculate() { recalculateProsperity(); for(Guild guild : FactionManager.getAllGuilds()) getIncome(guild); } finally { - StabilityMemo.end(memo); + BankruptcyMemo.end(memo); InstallationAccess.endRecalculation(); } } @@ -148,7 +148,7 @@ public void recalculateQuiet(List guilds, List factions) { previewGuilds = guilds == null ? List.of() : List.copyOf(guilds); previewFactions = factions == null ? List.of() : List.copyOf(factions); InstallationAccess.beginRecalculation(); - StabilityMemo.Run memo = StabilityMemo.begin(); + BankruptcyMemo.Run memo = BankruptcyMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -165,7 +165,7 @@ public void recalculateQuiet(List guilds, List factions) { } recalculateProsperity(); } finally { - StabilityMemo.end(memo); + BankruptcyMemo.end(memo); InstallationAccess.endRecalculation(); } } @@ -181,7 +181,7 @@ public void recalculateForSingleGuild(Guild g, boolean save) { return; } InstallationAccess.beginRecalculation(); - StabilityMemo.Run memo = StabilityMemo.begin(); + BankruptcyMemo.Run memo = BankruptcyMemo.begin(); try { installationAccess = new HashMap<>(); TradeGraph.refreshIfLive(this); @@ -195,7 +195,7 @@ public void recalculateForSingleGuild(Guild g, boolean save) { } } } finally { - StabilityMemo.end(memo); + BankruptcyMemo.end(memo); InstallationAccess.endRecalculation(); } } diff --git a/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java b/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java new file mode 100644 index 00000000..a2c7c794 --- /dev/null +++ b/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java @@ -0,0 +1,99 @@ +package net.tfminecraft.simplefactions.government.stability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.jupiter.api.Test; + +import net.tfminecraft.simplefactions.government.Government; +import net.tfminecraft.simplefactions.government.StabilityModifier; +import net.tfminecraft.simplefactions.guild.Guild; +import net.tfminecraft.simplefactions.objects.Faction; + +class BankruptcyMemoTest { + + @Test + void aRunReadsEachTreasuryOnceAndANestedRunReadsItsOwn() { + Guild realm = mock(Guild.class); + AtomicBoolean bankrupt = new AtomicBoolean(true); + when(realm.isBankrupt()).thenAnswer(call -> bankrupt.get()); + BankruptcyMemo.Run outer = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(realm)); + bankrupt.set(false); + assertTrue(BankruptcyMemo.isBankrupt(realm)); + BankruptcyMemo.Run inner = BankruptcyMemo.begin(); + try { + assertFalse(BankruptcyMemo.isBankrupt(realm)); + bankrupt.set(true); + assertFalse(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(inner); + } + assertTrue(BankruptcyMemo.isBankrupt(realm)); + verify(realm, times(2)).isBankrupt(); + } finally { + BankruptcyMemo.end(outer); + } + bankrupt.set(false); + assertFalse(BankruptcyMemo.isBankrupt(realm)); + bankrupt.set(true); + assertTrue(BankruptcyMemo.isBankrupt(realm)); + verify(realm, times(4)).isBankrupt(); + } + + @Test + void unrestThatDecaysDuringARunStillChangesStabilityBetweenProvinces() { + long[] now = {0}; + Guild realm = mock(Guild.class); + Faction faction = realmDecaying(realm, now); + + // Unmemoised: what each province would have seen before. + StabilityReport liveEarly = StateStability.of(faction); + now[0] = 1; + StabilityReport liveLate = StateStability.of(faction); + assertNotEquals(liveEarly.stability, liveLate.stability); + + now[0] = 0; + BankruptcyMemo.Run run = BankruptcyMemo.begin(); + try { + StabilityReport early = StateStability.of(faction); + now[0] = 1; + StabilityReport late = StateStability.of(faction); + assertEquals(liveEarly.stability, early.stability); + assertEquals(liveLate.stability, late.stability); + assertEquals(liveEarly.status, early.status); + assertEquals(liveLate.status, late.status); + } finally { + BankruptcyMemo.end(run); + } + // Two live asks read the treasury twice; the run read it once for both of its asks. + verify(realm, times(3)).isBankrupt(); + } + + /** A realm whose office unrest wears off as {@code now} advances. */ + private static Faction realmDecaying(Guild realm, long[] now) { + Faction faction = mock(Faction.class); + Government government = mock(Government.class); + when(faction.getId()).thenReturn("decaying"); + when(faction.getProvinces()).thenReturn(new ArrayList<>(List.of(1))); + when(faction.getRelations()).thenReturn(new HashMap<>()); + when(faction.getOrCreateMainGuild()).thenReturn(realm); + when(faction.getGovernment()).thenReturn(government); + when(government.getStabilityModifiers()) + .thenAnswer(call -> List.of(new StabilityModifier("Office unrest", -50 + 7.5 * now[0], 0))); + when(realm.isBankrupt()).thenReturn(false); + return faction; + } +} diff --git a/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java b/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java deleted file mode 100644 index f0daf7e5..00000000 --- a/src/test/java/net/tfminecraft/simplefactions/government/stability/StabilityMemoTest.java +++ /dev/null @@ -1,53 +0,0 @@ -package net.tfminecraft.simplefactions.government.stability; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotSame; -import static org.junit.jupiter.api.Assertions.assertSame; -import static org.mockito.Mockito.mock; - -import java.util.concurrent.atomic.AtomicInteger; - -import org.junit.jupiter.api.Test; - -import net.tfminecraft.simplefactions.objects.Faction; - -class StabilityMemoTest { - - @Test - void aRunAssessesEachRealmOnceAndANestedRunKeepsItsOwnReports() { - Faction realm = mock(Faction.class); - Faction other = mock(Faction.class); - AtomicInteger assessed = new AtomicInteger(); - StabilityMemo.Run outer = StabilityMemo.begin(); - try { - StabilityReport first = StabilityMemo.report(realm, () -> fresh(assessed)); - assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); - StabilityMemo.Run inner = StabilityMemo.begin(); - StabilityReport nestedRun; - try { - nestedRun = StabilityMemo.report(realm, () -> fresh(assessed)); - assertNotSame(first, nestedRun); - assertSame(nestedRun, StabilityMemo.report(realm, () -> fresh(assessed))); - } finally { - StabilityMemo.end(inner); - } - assertSame(first, StabilityMemo.report(realm, () -> fresh(assessed))); - // Assessing one realm may ask about another while the first is still being assessed. - StabilityReport nested = StabilityMemo.report(other, () -> { - StabilityMemo.report(realm, () -> fresh(assessed)); - return fresh(assessed); - }); - assertSame(nested, StabilityMemo.report(other, () -> fresh(assessed))); - assertNotSame(StabilityMemo.report(null, () -> fresh(assessed)), StabilityMemo.report(null, () -> fresh(assessed))); - assertEquals(5, assessed.get()); - } finally { - StabilityMemo.end(outer); - } - assertNotSame(StabilityMemo.report(realm, () -> fresh(assessed)), StabilityMemo.report(realm, () -> fresh(assessed))); - } - - private static StabilityReport fresh(AtomicInteger assessed) { - assessed.incrementAndGet(); - return new StabilityReport(); - } -} From 076002dc5136145120cac889d6a76ed5ae6686f7 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Wed, 21 Oct 2026 18:04:39 +0200 Subject: [PATCH 5/5] Keep treasury answers only on the server thread with an open ledger Review follow-up. An async preview shares live guilds with the server, so a bank removed meanwhile must be seen: previews now read every bankruptcy check live, as before. A failed ledger read closes the ledger and every balance then reads as zero, so a closed or missing ledger drops the run's answers and reads live too. Co-Authored-By: Claude Opus 5.5 --- .../government/stability/BankruptcyMemo.java | 42 ++++-- .../stability/BankruptcyMemoTest.java | 134 +++++++++++++++++- 2 files changed, 166 insertions(+), 10 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java b/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java index 7448a8b1..ad36cf19 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemo.java @@ -3,15 +3,23 @@ import java.util.IdentityHashMap; import java.util.Map; +import org.bukkit.Bukkit; + +import net.tfminecraft.denareconomy.DenarEconomy; +import net.tfminecraft.denareconomy.ledger.Ledger; import net.tfminecraft.simplefactions.guild.Guild; /** - * Whether each realm guild is bankrupt, read once per trade recalculation. The flood asks for a - * province owner's stability once per foreign guild it reaches, and each ask reads the realm - * guild's treasury from the ledger. A treasury only changes through a ledger posting, and nothing - * in a recalculation posts one, so every ask in one run gets the same answer. Everything else in - * stability, including unrest that decays over time, is still read live on every ask. - * Runs belong to one thread, so an async preview never sees the server's answers. + * Whether each realm guild is bankrupt, read once per trade recalculation on the server thread. + * The flood asks for a province owner's stability once per foreign guild it reaches, and each ask + * reads the realm guild's treasury from the ledger. During a run on the server thread nothing + * else can post to a treasury or remove a bank, and nothing in a recalculation does, so every ask + * in that run gets the same answer. Everything else in stability, including unrest that decays + * over time, is still read live on every ask. + * + *

Runs off the server thread (async previews) read live as before: the server can change a + * guild's bank while they run. If the ledger is closed, every ask reads live too, because a + * closed ledger reads every balance as zero. */ public final class BankruptcyMemo { private static final ThreadLocal> RUN = new ThreadLocal<>(); @@ -30,7 +38,8 @@ private Run(Map outer) { /** Starts a run on this thread. A nested run, such as a preview, gets its own answers. */ public static Run begin() { Run run = new Run(RUN.get()); - RUN.set(new IdentityHashMap<>()); + if (onServerThread()) RUN.set(new IdentityHashMap<>()); + else RUN.remove(); return run; } @@ -39,15 +48,30 @@ public static void end(Run run) { else RUN.set(run.outer); } - /** {@link Guild#isBankrupt()}, read once per guild in a run and live outside one. */ + /** {@link Guild#isBankrupt()}, read once per guild in a server-thread run and live otherwise. */ public static boolean isBankrupt(Guild guild) { Map run = RUN.get(); if (run == null) return guild.isBankrupt(); + if (!ledgerOpen()) { + // A failed read closes the ledger for good, and answers from before it are stale. + run.clear(); + return guild.isBankrupt(); + } Boolean bankrupt = run.get(guild); if (bankrupt == null) { bankrupt = guild.isBankrupt(); - run.put(guild, bankrupt); + // Keep only answers the ledger stayed open for; this very read may have closed it. + if (ledgerOpen()) run.put(guild, bankrupt); } return bankrupt; } + + private static boolean onServerThread() { + return Bukkit.getServer() != null && Bukkit.isPrimaryThread(); + } + + private static boolean ledgerOpen() { + Ledger ledger = DenarEconomy.getLedger(); + return ledger != null && ledger.isOpen(); + } } diff --git a/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java b/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java index a2c7c794..c562503f 100644 --- a/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/government/stability/BankruptcyMemoTest.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -14,17 +15,45 @@ import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; +import net.tfminecraft.denareconomy.DenarEconomy; +import net.tfminecraft.denareconomy.ledger.Ledger; import net.tfminecraft.simplefactions.government.Government; import net.tfminecraft.simplefactions.government.StabilityModifier; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.objects.Faction; class BankruptcyMemoTest { + private MockedStatic bukkit; + private Ledger previousLedger; + private final AtomicBoolean serverThread = new AtomicBoolean(true); + private final AtomicBoolean ledgerOpen = new AtomicBoolean(true); + + @BeforeEach + void setUp() { + bukkit = mockStatic(Bukkit.class); + bukkit.when(Bukkit::getServer).thenReturn(mock(Server.class)); + bukkit.when(Bukkit::isPrimaryThread).thenAnswer(call -> serverThread.get()); + previousLedger = DenarEconomy.getLedger(); + Ledger ledger = mock(Ledger.class); + when(ledger.isOpen()).thenAnswer(call -> ledgerOpen.get()); + DenarEconomy.setLedger(ledger); + } + + @AfterEach + void tearDown() { + DenarEconomy.setLedger(previousLedger); + bukkit.close(); + } @Test - void aRunReadsEachTreasuryOnceAndANestedRunReadsItsOwn() { + void aServerRunReadsEachTreasuryOnceAndANestedRunReadsItsOwn() { Guild realm = mock(Guild.class); AtomicBoolean bankrupt = new AtomicBoolean(true); when(realm.isBankrupt()).thenAnswer(call -> bankrupt.get()); @@ -53,6 +82,109 @@ void aRunReadsEachTreasuryOnceAndANestedRunReadsItsOwn() { verify(realm, times(4)).isBankrupt(); } + @Test + void anAsyncRunReadsLiveSoABankRemovedMeanwhileIsSeen() { + serverThread.set(false); + Guild realm = mock(Guild.class); + AtomicBoolean hasBank = new AtomicBoolean(true); + // isBankrupt() is false once the bank is gone, as Guild.isBankrupt() reads it. + when(realm.isBankrupt()).thenAnswer(call -> hasBank.get()); + BankruptcyMemo.Run run = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(realm)); + // The server thread runs /f delbank while the preview is still flooding. + hasBank.set(false); + assertFalse(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(run); + } + verify(realm, times(2)).isBankrupt(); + } + + @Test + void anAsyncRunInsideAServerRunStillReadsLiveAndTheServerRunResumes() { + Guild realm = mock(Guild.class); + AtomicBoolean bankrupt = new AtomicBoolean(true); + when(realm.isBankrupt()).thenAnswer(call -> bankrupt.get()); + BankruptcyMemo.Run outer = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(realm)); + serverThread.set(false); + BankruptcyMemo.Run inner = BankruptcyMemo.begin(); + try { + bankrupt.set(false); + assertFalse(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(inner); + } + assertTrue(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(outer); + } + } + + @Test + void aClosedLedgerDropsTheRunsAnswersAndEveryAskReadsLive() { + Guild first = mock(Guild.class); + Guild second = mock(Guild.class); + AtomicBoolean firstBankrupt = new AtomicBoolean(true); + when(first.isBankrupt()).thenAnswer(call -> firstBankrupt.get()); + // Reading the second treasury fails, and the ledger closes for good. + when(second.isBankrupt()).thenAnswer(call -> { + ledgerOpen.set(false); + return false; + }); + BankruptcyMemo.Run run = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(first)); + assertFalse(BankruptcyMemo.isBankrupt(second)); + // A closed ledger reads every balance as zero, so the first realm is no longer bankrupt. + firstBankrupt.set(false); + assertFalse(BankruptcyMemo.isBankrupt(first)); + assertFalse(BankruptcyMemo.isBankrupt(second)); + // Answers from before the close are gone even if the ledger came back. + ledgerOpen.set(true); + firstBankrupt.set(true); + assertTrue(BankruptcyMemo.isBankrupt(first)); + firstBankrupt.set(false); + assertTrue(BankruptcyMemo.isBankrupt(first)); + } finally { + BankruptcyMemo.end(run); + } + verify(first, times(3)).isBankrupt(); + verify(second, times(2)).isBankrupt(); + } + + @Test + void noLedgerAtAllReadsLive() { + DenarEconomy.setLedger(null); + Guild realm = mock(Guild.class); + when(realm.isBankrupt()).thenReturn(true); + BankruptcyMemo.Run run = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(realm)); + assertTrue(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(run); + } + verify(realm, times(2)).isBankrupt(); + } + + @Test + void noServerAtAllReadsLive() { + bukkit.when(Bukkit::getServer).thenReturn(null); + Guild realm = mock(Guild.class); + when(realm.isBankrupt()).thenReturn(true); + BankruptcyMemo.Run run = BankruptcyMemo.begin(); + try { + assertTrue(BankruptcyMemo.isBankrupt(realm)); + assertTrue(BankruptcyMemo.isBankrupt(realm)); + } finally { + BankruptcyMemo.end(run); + } + verify(realm, times(2)).isBankrupt(); + } + @Test void unrestThatDecaysDuringARunStillChangesStabilityBetweenProvinces() { long[] now = {0};