From 44deabc56b4f45df028d8bfe9a69882055f964f3 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:15:30 +0000 Subject: [PATCH 1/2] fix: read Spymaster deaths on the main thread for async income previews Main still showed "income estimate unavailable" after 4.4.1. Stability's Spymaster vacancy check asks TLibs whether the holder's character is dead, and TLibs refuses character lookups off the main thread, so every branch and economic impact preview failed on its worker. FrozenBalances becomes FrozenReads: each preview copy now also captures every realm's espionage stability modifiers on the main thread, and Government reads those inside a frozen read. Co-Authored-By: Claude Opus 5.5 --- .../economy/FrozenBalances.java | 68 ------ .../simplefactions/economy/FrozenReads.java | 90 ++++++++ .../simplefactions/government/Government.java | 2 +- .../guild/income/BranchIncomePreview.java | 10 +- .../guild/income/EconomicPreview.java | 10 +- .../inventory/BranchIncomePreviewService.java | 2 +- .../simplefactions/objects/Bank.java | 4 +- .../utils/EconomicImpactService.java | 2 +- .../economy/FrozenBalancesTest.java | 108 ---------- .../economy/FrozenReadsTest.java | 193 ++++++++++++++++++ .../utils/EconomicImpactCoverageTest.java | 8 +- .../EconomicImpactServiceCoverageTest.java | 8 +- 12 files changed, 306 insertions(+), 199 deletions(-) delete mode 100644 src/main/java/net/tfminecraft/simplefactions/economy/FrozenBalances.java create mode 100644 src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java delete mode 100644 src/test/java/net/tfminecraft/simplefactions/economy/FrozenBalancesTest.java create mode 100644 src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java diff --git a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenBalances.java b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenBalances.java deleted file mode 100644 index eebc3685..00000000 --- a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenBalances.java +++ /dev/null @@ -1,68 +0,0 @@ -package net.tfminecraft.simplefactions.economy; - -import java.util.HashMap; -import java.util.Map; -import java.util.function.Supplier; - -import net.tfminecraft.denareconomy.api.Denars; -import net.tfminecraft.simplefactions.guild.Guild; -import net.tfminecraft.simplefactions.managers.FactionManager; -import net.tfminecraft.simplefactions.objects.Bank; -import net.tfminecraft.simplefactions.objects.Faction; - -/** - * Guild treasury balances read once on the main thread. DenarEconomy refuses ledger reads - * from other threads, so income previews that run async read these instead. - */ -public final class FrozenBalances { - private static final ThreadLocal CURRENT = new ThreadLocal<>(); - - private final Map cents; - - private FrozenBalances(Map cents) { - this.cents = Map.copyOf(cents); - } - - /** Reads every guild treasury now. Call on the main thread. */ - public static FrozenBalances capture() { - // Stability asks for each realm guild, so create any missing one here rather than on a worker. - for (Faction faction : FactionManager.factions) { - if (faction != null && faction.getGuildHandler() != null) { - faction.getOrCreateMainGuild(); - } - } - Map cents = new HashMap<>(); - for (Guild guild : FactionManager.getAllGuilds()) { - Bank bank = guild == null ? null : guild.getBank(); - if (bank != null) { - String account = bank.accountId(); - cents.put(account, Denars.balance(account)); - } - } - return new FrozenBalances(cents); - } - - /** Runs the work with these balances standing in for the ledger on this thread. */ - public T read(Supplier work) { - FrozenBalances previous = CURRENT.get(); - CURRENT.set(this); - try { - return work.get(); - } finally { - if (previous == null) { - CURRENT.remove(); - } else { - CURRENT.set(previous); - } - } - } - - /** The frozen balance inside {@link #read}, otherwise the live ledger balance. */ - public static long balance(String account) { - FrozenBalances current = CURRENT.get(); - if (current != null) { - return current.cents.getOrDefault(account, 0L); - } - return Denars.balance(account); - } -} diff --git a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java new file mode 100644 index 00000000..5f06731a --- /dev/null +++ b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java @@ -0,0 +1,90 @@ +package net.tfminecraft.simplefactions.economy; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; + +import net.tfminecraft.denareconomy.api.Denars; +import net.tfminecraft.simplefactions.espionage.EspionageService; +import net.tfminecraft.simplefactions.government.StabilityModifier; +import net.tfminecraft.simplefactions.guild.Guild; +import net.tfminecraft.simplefactions.managers.FactionManager; +import net.tfminecraft.simplefactions.objects.Bank; +import net.tfminecraft.simplefactions.objects.Faction; + +/** + * Guild treasuries and espionage stability read once on the main thread. DenarEconomy and + * TLibs character identity refuse reads from other threads, so income previews that run + * async read these instead. + */ +public final class FrozenReads { + private static final ThreadLocal CURRENT = new ThreadLocal<>(); + + private final Map cents; + private final Map> espionage; + + private FrozenReads(Map cents, Map> espionage) { + this.cents = Map.copyOf(cents); + this.espionage = Map.copyOf(espionage); + } + + /** Reads every guild treasury and realm espionage modifier now. Call on the main thread. */ + public static FrozenReads capture() { + long now = System.currentTimeMillis(); + Map> espionage = new HashMap<>(); + for (Faction faction : FactionManager.factions) { + // Stability asks for each realm guild, so create any missing one here rather than on a worker. + if (faction != null && faction.getGuildHandler() != null) { + faction.getOrCreateMainGuild(); + } + // Spymaster vacancy asks TLibs whether the holder's character is dead. + if (faction != null && faction.getId() != null && faction.getEspionage() != null) { + espionage.put(faction.getId(), List.copyOf(EspionageService.stabilityModifiers(faction, now))); + } + } + Map cents = new HashMap<>(); + for (Guild guild : FactionManager.getAllGuilds()) { + Bank bank = guild == null ? null : guild.getBank(); + if (bank != null) { + String account = bank.accountId(); + cents.put(account, Denars.balance(account)); + } + } + return new FrozenReads(cents, espionage); + } + + /** Runs the work with these reads standing in for the live state on this thread. */ + public T read(Supplier work) { + FrozenReads previous = CURRENT.get(); + CURRENT.set(this); + try { + return work.get(); + } finally { + if (previous == null) { + CURRENT.remove(); + } else { + CURRENT.set(previous); + } + } + } + + /** The frozen balance inside {@link #read}, otherwise the live ledger balance. */ + public static long balance(String account) { + FrozenReads current = CURRENT.get(); + if (current != null) { + return current.cents.getOrDefault(account, 0L); + } + return Denars.balance(account); + } + + /** The frozen espionage modifiers inside {@link #read}, otherwise the live ones at {@code now}. */ + public static List espionage(Faction faction, long now) { + FrozenReads current = CURRENT.get(); + if (current != null) { + String id = faction.getId(); + return id == null ? List.of() : current.espionage.getOrDefault(id, List.of()); + } + return EspionageService.stabilityModifiers(faction, now); + } +} diff --git a/src/main/java/net/tfminecraft/simplefactions/government/Government.java b/src/main/java/net/tfminecraft/simplefactions/government/Government.java index d092f270..94c18f35 100644 --- a/src/main/java/net/tfminecraft/simplefactions/government/Government.java +++ b/src/main/java/net/tfminecraft/simplefactions/government/Government.java @@ -232,7 +232,7 @@ public void addStabilityModifier(StabilityModifier modifier) { public List getStabilityModifiers() { if (f == null || f.getEspionage() == null) return stabilityModifiers; - var active = net.tfminecraft.simplefactions.espionage.EspionageService.stabilityModifiers(f, System.currentTimeMillis()); + var active = net.tfminecraft.simplefactions.economy.FrozenReads.espionage(f, System.currentTimeMillis()); if (active.isEmpty()) return stabilityModifiers; List result = new ArrayList<>(stabilityModifiers); result.addAll(active); diff --git a/src/main/java/net/tfminecraft/simplefactions/guild/income/BranchIncomePreview.java b/src/main/java/net/tfminecraft/simplefactions/guild/income/BranchIncomePreview.java index 64c434d8..b8e8ef70 100644 --- a/src/main/java/net/tfminecraft/simplefactions/guild/income/BranchIncomePreview.java +++ b/src/main/java/net/tfminecraft/simplefactions/guild/income/BranchIncomePreview.java @@ -8,7 +8,7 @@ import java.util.Map; import java.util.Set; -import net.tfminecraft.simplefactions.economy.FrozenBalances; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.enums.GuildModifier; import net.tfminecraft.simplefactions.government.proposal.TaxTarget; import net.tfminecraft.simplefactions.guild.Guild; @@ -31,15 +31,15 @@ private BranchIncomePreview() {} public static final class Prepared { private final ProvinceManager snapshot; - private final FrozenBalances balances = FrozenBalances.capture(); + private final FrozenReads reads = FrozenReads.capture(); private Prepared(ProvinceManager snapshot) { this.snapshot = snapshot; } - /** Guild treasuries as they were when this copy was made, for work off the main thread. */ - public FrozenBalances balances() { - return balances; + /** Main-thread state as it was when this copy was made, for work off the main thread. */ + public FrozenReads reads() { + return reads; } } diff --git a/src/main/java/net/tfminecraft/simplefactions/guild/income/EconomicPreview.java b/src/main/java/net/tfminecraft/simplefactions/guild/income/EconomicPreview.java index ff04418c..1b5ad036 100644 --- a/src/main/java/net/tfminecraft/simplefactions/guild/income/EconomicPreview.java +++ b/src/main/java/net/tfminecraft/simplefactions/guild/income/EconomicPreview.java @@ -7,7 +7,7 @@ import net.tfminecraft.simplefactions.SimpleFactions; import net.tfminecraft.simplefactions.diplomacy.RelationType; -import net.tfminecraft.simplefactions.economy.FrozenBalances; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.government.proposal.TaxTarget; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.laws.Law; @@ -25,15 +25,15 @@ private EconomicPreview() {} public static final class Prepared { private final ProvinceManager snapshot; - private final FrozenBalances balances = FrozenBalances.capture(); + private final FrozenReads reads = FrozenReads.capture(); private Prepared(ProvinceManager snapshot) { this.snapshot = snapshot; } - /** Guild treasuries as they were when this copy was made, for work off the main thread. */ - public FrozenBalances balances() { - return balances; + /** Main-thread state as it was when this copy was made, for work off the main thread. */ + public FrozenReads reads() { + return reads; } } diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/BranchIncomePreviewService.java b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/BranchIncomePreviewService.java index 4b676d68..1a7fd3cb 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/inventory/BranchIncomePreviewService.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/inventory/BranchIncomePreviewService.java @@ -58,7 +58,7 @@ public static void schedule( Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> { BranchIncomePreview.Estimate estimate; try { - estimate = prepared.balances().read( + estimate = prepared.reads().read( () -> BranchIncomePreview.estimate(prepared, guild, current, hypothetical)); } catch (RuntimeException ex) { plugin.getLogger().log( diff --git a/src/main/java/net/tfminecraft/simplefactions/objects/Bank.java b/src/main/java/net/tfminecraft/simplefactions/objects/Bank.java index 13fe3fb1..0659c61c 100644 --- a/src/main/java/net/tfminecraft/simplefactions/objects/Bank.java +++ b/src/main/java/net/tfminecraft/simplefactions/objects/Bank.java @@ -4,7 +4,7 @@ import org.bukkit.Chunk; import net.tfminecraft.denareconomy.api.Denars; import net.tfminecraft.simplefactions.Cache; -import net.tfminecraft.simplefactions.economy.FrozenBalances; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.economy.Money; import net.tfminecraft.simplefactions.guild.Guild; @@ -24,7 +24,7 @@ public Bank(Guild guild, double amount, Chunk chunk) { this.chunk = chunk; } public String accountId() { return Denars.guild(guild.getIncarnationId()); } - public Double getWealth() { return Money.denars(FrozenBalances.balance(accountId())); } + public Double getWealth() { return Money.denars(FrozenReads.balance(accountId())); } /** Read only by the one-time, ledger-keyed legacy importer during startup. */ public double legacyBalance() { return legacyBalance; } public Chunk getChunk() { return chunk; } diff --git a/src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpactService.java b/src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpactService.java index 7146e8c7..cc000c9c 100644 --- a/src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpactService.java +++ b/src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpactService.java @@ -92,7 +92,7 @@ private static void start(SimpleFactions plugin, Player player, long token) { Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> { Map deltas; try { - deltas = pending.prepared.balances().read(() -> pending.calculator.calculate(pending.prepared)); + deltas = pending.prepared.reads().read(() -> pending.calculator.calculate(pending.prepared)); } catch (RuntimeException ex) { plugin.getLogger().log(Level.WARNING, "Economic impact preview failed", ex); deltas = null; diff --git a/src/test/java/net/tfminecraft/simplefactions/economy/FrozenBalancesTest.java b/src/test/java/net/tfminecraft/simplefactions/economy/FrozenBalancesTest.java deleted file mode 100644 index 91145a5c..00000000 --- a/src/test/java/net/tfminecraft/simplefactions/economy/FrozenBalancesTest.java +++ /dev/null @@ -1,108 +0,0 @@ -package net.tfminecraft.simplefactions.economy; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.CALLS_REAL_METHODS; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockStatic; -import static org.mockito.Mockito.when; - -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.UUID; -import java.util.concurrent.CompletableFuture; - -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -import net.tfminecraft.denareconomy.api.Denars; -import net.tfminecraft.simplefactions.guild.Guild; -import net.tfminecraft.simplefactions.guild.income.BranchIncomePreview; -import net.tfminecraft.simplefactions.guild.income.EconomicPreview; -import net.tfminecraft.simplefactions.managers.FactionManager; -import net.tfminecraft.simplefactions.managers.ProvinceManager; -import net.tfminecraft.simplefactions.objects.Bank; -import net.tfminecraft.simplefactions.objects.Faction; -import net.tfminecraft.simplefactions.objects.handler.GuildHandler; - -class FrozenBalancesTest { - private final List savedFactions = new ArrayList<>(); - private Guild guild; - - @BeforeEach - void setUp() { - savedFactions.addAll(FactionManager.factions); - FactionManager.factions.clear(); - guild = mock(Guild.class); - when(guild.getIncarnationId()).thenReturn(UUID.fromString("00000000-0000-0000-0000-000000000042")); - when(guild.getBank()).thenAnswer(invocation -> new Bank(guild, null)); - GuildHandler handler = mock(GuildHandler.class); - when(handler.getGuilds()).thenReturn(new ArrayList<>(List.of(guild))); - Faction faction = mock(Faction.class); - when(faction.getGuildHandler()).thenReturn(handler); - FactionManager.factions.add(faction); - } - - @AfterEach - void tearDown() { - FactionManager.factions.clear(); - FactionManager.factions.addAll(savedFactions); - } - - @Test - void asyncReadsUseBalancesCapturedOnTheMainThread() throws Exception { - FrozenBalances frozen; - try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { - denars.when(() -> Denars.balance(anyString())).thenReturn(1234L); - frozen = FrozenBalances.capture(); - } - - // Static mocks stay on the test thread, so only the frozen balance can give the worker 12.34. - assertEquals(12.34, CompletableFuture.supplyAsync(() -> frozen.read(() -> guild.getBank().getWealth())).get()); - assertEquals(0L, frozen.read(() -> FrozenBalances.balance("unknown"))); - } - - @Test - void readRestoresTheLiveLedgerAfterwards() { - try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { - denars.when(() -> Denars.balance(anyString())).thenReturn(500L); - FrozenBalances frozen = FrozenBalances.capture(); - denars.when(() -> Denars.balance(anyString())).thenReturn(700L); - - assertEquals(5.0, frozen.read(() -> guild.getBank().getWealth())); - assertEquals(7.0, guild.getBank().getWealth()); - } - } - - @Test - void nestedReadsRestoreTheOuterBalances() { - try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { - denars.when(() -> Denars.balance(anyString())).thenReturn(100L); - FrozenBalances outer = FrozenBalances.capture(); - denars.when(() -> Denars.balance(anyString())).thenReturn(200L); - FrozenBalances inner = FrozenBalances.capture(); - - assertEquals(1.0, outer.read(() -> { - assertEquals(2.0, inner.read(() -> guild.getBank().getWealth())); - return guild.getBank().getWealth(); - })); - } - } - - @Test - void previewCopiesCarryTheBalancesFromWhenTheyWereMade() { - ProvinceManager live = new ProvinceManager(); - live.start(Map.of()); - try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { - denars.when(() -> Denars.balance(anyString())).thenReturn(-300L); - EconomicPreview.Prepared economic = EconomicPreview.prepare(live); - BranchIncomePreview.Prepared branch = BranchIncomePreview.prepare(live); - denars.when(() -> Denars.balance(anyString())).thenReturn(0L); - - assertEquals(-3.0, economic.balances().read(() -> guild.getBank().getWealth())); - assertEquals(-3.0, branch.balances().read(() -> guild.getBank().getWealth())); - } - } -} diff --git a/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java b/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java new file mode 100644 index 00000000..4e0a089d --- /dev/null +++ b/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java @@ -0,0 +1,193 @@ +package net.tfminecraft.simplefactions.economy; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; + +import org.bukkit.entity.Player; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import net.tfminecraft.denareconomy.api.Denars; +import net.tfminecraft.simplefactions.espionage.EspionageService; +import net.tfminecraft.simplefactions.espionage.EspionageState; +import net.tfminecraft.simplefactions.espionage.SpecialPositionAssignment; +import net.tfminecraft.simplefactions.government.StabilityModifier; +import net.tfminecraft.simplefactions.guild.Guild; +import net.tfminecraft.simplefactions.guild.income.BranchIncomePreview; +import net.tfminecraft.simplefactions.guild.income.EconomicPreview; +import net.tfminecraft.simplefactions.identity.Members; +import net.tfminecraft.simplefactions.managers.FactionManager; +import net.tfminecraft.simplefactions.managers.ProvinceManager; +import net.tfminecraft.simplefactions.objects.Bank; +import net.tfminecraft.simplefactions.objects.Faction; +import net.tfminecraft.simplefactions.objects.handler.GuildHandler; +import net.tfminecraft.tlibs.character.CharacterInfo; +import net.tfminecraft.tlibs.character.CharacterRef; +import net.tfminecraft.tlibs.character.CharacterSession; +import net.tfminecraft.tlibs.character.CharacterStatus; + +class FrozenReadsTest { + private final List savedFactions = new ArrayList<>(); + private Guild guild; + + @BeforeEach + void setUp() { + savedFactions.addAll(FactionManager.factions); + FactionManager.factions.clear(); + guild = mock(Guild.class); + when(guild.getIncarnationId()).thenReturn(UUID.fromString("00000000-0000-0000-0000-000000000042")); + when(guild.getBank()).thenAnswer(invocation -> new Bank(guild, null)); + GuildHandler handler = mock(GuildHandler.class); + when(handler.getGuilds()).thenReturn(new ArrayList<>(List.of(guild))); + Faction faction = mock(Faction.class); + when(faction.getGuildHandler()).thenReturn(handler); + FactionManager.factions.add(faction); + } + + @AfterEach + void tearDown() { + FactionManager.factions.clear(); + FactionManager.factions.addAll(savedFactions); + } + + @Test + void asyncReadsUseBalancesCapturedOnTheMainThread() throws Exception { + FrozenReads frozen; + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(1234L); + frozen = FrozenReads.capture(); + } + + // Static mocks stay on the test thread, so only the frozen balance can give the worker 12.34. + assertEquals(12.34, CompletableFuture.supplyAsync(() -> frozen.read(() -> guild.getBank().getWealth())).get()); + assertEquals(0L, frozen.read(() -> FrozenReads.balance("unknown"))); + } + + @Test + void readRestoresTheLiveLedgerAfterwards() { + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(500L); + FrozenReads frozen = FrozenReads.capture(); + denars.when(() -> Denars.balance(anyString())).thenReturn(700L); + + assertEquals(5.0, frozen.read(() -> guild.getBank().getWealth())); + assertEquals(7.0, guild.getBank().getWealth()); + } + } + + @Test + void nestedReadsRestoreTheOuterBalances() { + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(100L); + FrozenReads outer = FrozenReads.capture(); + denars.when(() -> Denars.balance(anyString())).thenReturn(200L); + FrozenReads inner = FrozenReads.capture(); + + assertEquals(1.0, outer.read(() -> { + assertEquals(2.0, inner.read(() -> guild.getBank().getWealth())); + return guild.getBank().getWealth(); + })); + } + } + + @Test + void previewCopiesCarryTheBalancesFromWhenTheyWereMade() { + ProvinceManager live = new ProvinceManager(); + live.start(Map.of()); + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(-300L); + EconomicPreview.Prepared economic = EconomicPreview.prepare(live); + BranchIncomePreview.Prepared branch = BranchIncomePreview.prepare(live); + denars.when(() -> Denars.balance(anyString())).thenReturn(0L); + + assertEquals(-3.0, economic.reads().read(() -> guild.getBank().getWealth())); + assertEquals(-3.0, branch.reads().read(() -> guild.getBank().getWealth())); + } + } + + @Test + void asyncStabilityUsesSpymasterDeathsCapturedOnTheMainThread() throws Exception { + CharacterRef spy = new CharacterRef(UUID.randomUUID(), UUID.randomUUID()); + Members.setResolver(new MainThreadCharacters(Thread.currentThread(), spy)); + SpecialPositionAssignment holder = new SpecialPositionAssignment(); + holder.playerId = spy.ownerId(); + holder.characterId = spy.characterId().toString(); + EspionageState state = new EspionageState(); + state.appoint(holder, 80); + Faction realm = mock(Faction.class); + when(realm.getId()).thenReturn("realm"); + when(realm.getEspionage()).thenReturn(state); + when(realm.isMember(Members.key(spy))).thenReturn(true); + FactionManager.factions.add(realm); + long now = System.currentTimeMillis(); + + // Off the main thread the live check hits the character guard, as it did on Main. + ExecutionException live = assertThrows(ExecutionException.class, + () -> CompletableFuture.supplyAsync(() -> EspionageService.stabilityModifiers(realm, now)).get()); + assertInstanceOf(IllegalStateException.class, live.getCause()); + + FrozenReads frozen; + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(0L); + frozen = FrozenReads.capture(); + } + List modifiers = CompletableFuture + .supplyAsync(() -> frozen.read(() -> FrozenReads.espionage(realm, now))).get(); + assertEquals(1, modifiers.size()); + assertEquals("Vacant Spymaster", modifiers.getFirst().getName()); + + Faction later = mock(Faction.class); + when(later.getId()).thenReturn("later"); + assertEquals(List.of(), frozen.read(() -> FrozenReads.espionage(later, now))); + } + + /** Knows one dead character and refuses lookups off the capturing thread, like TLibs. */ + private record MainThreadCharacters(Thread main, CharacterRef dead) implements Members.Resolver { + @Override + public Optional info(String key) { + if (Thread.currentThread() != main) { + throw new IllegalStateException("Character identity must be used on the main thread"); + } + return Optional.of(new CharacterInfo(dead, "Spy", CharacterStatus.DEAD, "human", Set.of(), 0L, Instant.EPOCH)) + .filter(info -> key.equals(Members.key(info.ref()))); + } + + @Override + public Optional session(Player player) { + return Optional.empty(); + } + + @Override + public boolean isCurrent(CharacterSession session) { + return false; + } + + @Override + public String key(Player player) { + return null; + } + + @Override + public Optional online(String key) { + return Optional.empty(); + } + } +} diff --git a/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactCoverageTest.java index a8a323f2..c9cc613c 100644 --- a/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactCoverageTest.java @@ -10,7 +10,7 @@ import java.util.function.Supplier; import java.util.stream.Stream; import net.tfminecraft.simplefactions.diplomacy.RelationType; -import net.tfminecraft.simplefactions.economy.FrozenBalances; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.government.proposal.TaxTarget; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.guild.income.EconomicPreview; @@ -74,9 +74,9 @@ void setUp() { law = mock(Law.class); agreement = mock(RelationType.class); prepared = mock(EconomicPreview.Prepared.class); - FrozenBalances balances = mock(FrozenBalances.class); - when(balances.read(any())).thenAnswer(call -> call.>getArgument(0).get()); - when(prepared.balances()).thenReturn(balances); + FrozenReads reads = mock(FrozenReads.class); + when(reads.read(any())).thenAnswer(call -> call.>getArgument(0).get()); + when(prepared.reads()).thenReturn(reads); when(gui.plugin.getProvinceManager()).thenReturn(provinces); when(faction.getTaxHandler()).thenReturn(taxes); factions = mockStatic(FactionManager.class); diff --git a/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactServiceCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactServiceCoverageTest.java index d4dd1f3a..21ef240b 100644 --- a/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactServiceCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/utils/EconomicImpactServiceCoverageTest.java @@ -11,7 +11,7 @@ import java.util.function.Supplier; import java.util.stream.Stream; import net.tfminecraft.simplefactions.SimpleFactions; -import net.tfminecraft.simplefactions.economy.FrozenBalances; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.guild.income.EconomicPreview; import net.tfminecraft.simplefactions.keys.Keys; @@ -71,9 +71,9 @@ void setUp() { when(viewer.getName()).thenReturn("Our Guild"); previews = mockStatic(EconomicPreview.class); prepared = mock(EconomicPreview.Prepared.class); - FrozenBalances balances = mock(FrozenBalances.class); - when(balances.read(any())).thenAnswer(call -> call.>getArgument(0).get()); - when(prepared.balances()).thenReturn(balances); + FrozenReads reads = mock(FrozenReads.class); + when(reads.read(any())).thenAnswer(call -> call.>getArgument(0).get()); + when(prepared.reads()).thenReturn(reads); previews.when(EconomicPreview::current).thenReturn(prepared); calculations = new AtomicInteger(); } From e4aefabec19e974d56ed70970f4d85ea5d5544e3 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:26:03 +0000 Subject: [PATCH 2/2] fix: read Dowsing node upkeep on the main thread for async previews Sol review: law, treaty and tax previews read net income on the worker, and the NODES line calls Dowsing's upkeep lookup, which asks SimpleFactionsApi.ready and so TLibs. Each capture now records every guild's node upkeep by incarnation, and the ledger reads that inside a frozen read. Co-Authored-By: Claude Opus 5.5 --- .../simplefactions/economy/FrozenReads.java | 32 +++++++++++++++---- .../simplefactions/guild/income/Ledger.java | 9 +++++- .../economy/FrozenReadsTest.java | 29 +++++++++++++++++ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java index 5f06731a..a1b58e50 100644 --- a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java +++ b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java @@ -3,33 +3,38 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.UUID; import java.util.function.Supplier; import net.tfminecraft.denareconomy.api.Denars; import net.tfminecraft.simplefactions.espionage.EspionageService; import net.tfminecraft.simplefactions.government.StabilityModifier; import net.tfminecraft.simplefactions.guild.Guild; +import net.tfminecraft.simplefactions.guild.income.Ledger; import net.tfminecraft.simplefactions.managers.FactionManager; import net.tfminecraft.simplefactions.objects.Bank; import net.tfminecraft.simplefactions.objects.Faction; /** - * Guild treasuries and espionage stability read once on the main thread. DenarEconomy and - * TLibs character identity refuse reads from other threads, so income previews that run - * async read these instead. + * Guild treasuries, node upkeep and espionage stability read once on the main thread. + * DenarEconomy, Dowsing and TLibs character identity refuse reads from other threads, so + * income previews that run async read these instead. */ public final class FrozenReads { private static final ThreadLocal CURRENT = new ThreadLocal<>(); private final Map cents; + private final Map nodeUpkeep; private final Map> espionage; - private FrozenReads(Map cents, Map> espionage) { + private FrozenReads( + Map cents, Map nodeUpkeep, Map> espionage) { this.cents = Map.copyOf(cents); + this.nodeUpkeep = Map.copyOf(nodeUpkeep); this.espionage = Map.copyOf(espionage); } - /** Reads every guild treasury and realm espionage modifier now. Call on the main thread. */ + /** Reads every guild treasury and node upkeep and every realm espionage modifier now. Call on the main thread. */ public static FrozenReads capture() { long now = System.currentTimeMillis(); Map> espionage = new HashMap<>(); @@ -44,14 +49,19 @@ public static FrozenReads capture() { } } Map cents = new HashMap<>(); + Map nodeUpkeep = new HashMap<>(); for (Guild guild : FactionManager.getAllGuilds()) { Bank bank = guild == null ? null : guild.getBank(); if (bank != null) { String account = bank.accountId(); cents.put(account, Denars.balance(account)); } + UUID incarnation = guild == null ? null : guild.getIncarnationId(); + if (incarnation != null) { + nodeUpkeep.put(incarnation, Ledger.liveNodeUpkeep(guild)); + } } - return new FrozenReads(cents, espionage); + return new FrozenReads(cents, nodeUpkeep, espionage); } /** Runs the work with these reads standing in for the live state on this thread. */ @@ -78,6 +88,16 @@ public static long balance(String account) { return Denars.balance(account); } + /** The frozen node upkeep inside {@link #read}, otherwise the live upkeep. */ + public static double nodeUpkeep(Guild guild) { + FrozenReads current = CURRENT.get(); + if (current != null) { + UUID incarnation = guild.getIncarnationId(); + return incarnation == null ? 0.0 : current.nodeUpkeep.getOrDefault(incarnation, 0.0); + } + return Ledger.liveNodeUpkeep(guild); + } + /** The frozen espionage modifiers inside {@link #read}, otherwise the live ones at {@code now}. */ public static List espionage(Faction faction, long now) { FrozenReads current = CURRENT.get(); diff --git a/src/main/java/net/tfminecraft/simplefactions/guild/income/Ledger.java b/src/main/java/net/tfminecraft/simplefactions/guild/income/Ledger.java index 0c9d64c2..b7b8e41d 100644 --- a/src/main/java/net/tfminecraft/simplefactions/guild/income/Ledger.java +++ b/src/main/java/net/tfminecraft/simplefactions/guild/income/Ledger.java @@ -8,6 +8,8 @@ import java.util.Set; import java.util.function.ToDoubleFunction; +import net.tfminecraft.simplefactions.economy.FrozenReads; + import net.tfminecraft.simplefactions.vehicles.pool.FactionVehiclePoolService; import org.bukkit.Bukkit; @@ -71,6 +73,11 @@ public static void setNodeUpkeepLookup(ToDoubleFunction lookup) { nodeUpkeepLookup = lookup == null ? guild -> 0.0 : lookup; } + /** The registered node upkeep, read live. Dowsing answers only on the main thread. */ + public static double liveNodeUpkeep(Guild guild) { + return nodeUpkeepLookup.applyAsDouble(guild); + } + // Pool vehicle upkeep is withdrawn in VehicleUpkeepService. This lookup is the ledger line. private static ToDoubleFunction factionVehicleUpkeep = FactionVehiclePoolService::dailyUpkeepOf; @@ -399,7 +406,7 @@ public double getIncome(Cashflow cashflow) { amount *= net.tfminecraft.simplefactions.inactivity.InactivityService.outputFactor(guild); break; case NODES: - amount = -nodeUpkeepLookup.applyAsDouble(guild); + amount = -FrozenReads.nodeUpkeep(guild); break; //Mercenary contracts case MERCENARY_CONTRACT: diff --git a/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java b/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java index 4e0a089d..3716ca65 100644 --- a/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/economy/FrozenReadsTest.java @@ -32,7 +32,9 @@ import net.tfminecraft.simplefactions.government.StabilityModifier; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.guild.income.BranchIncomePreview; +import net.tfminecraft.simplefactions.guild.income.Cashflow; import net.tfminecraft.simplefactions.guild.income.EconomicPreview; +import net.tfminecraft.simplefactions.guild.income.Ledger; import net.tfminecraft.simplefactions.identity.Members; import net.tfminecraft.simplefactions.managers.FactionManager; import net.tfminecraft.simplefactions.managers.ProvinceManager; @@ -64,6 +66,7 @@ void setUp() { @AfterEach void tearDown() { + Ledger.setNodeUpkeepLookup(null); FactionManager.factions.clear(); FactionManager.factions.addAll(savedFactions); } @@ -159,6 +162,32 @@ void asyncStabilityUsesSpymasterDeathsCapturedOnTheMainThread() throws Exception assertEquals(List.of(), frozen.read(() -> FrozenReads.espionage(later, now))); } + @Test + void asyncLedgerUsesNodeUpkeepCapturedOnTheMainThread() throws Exception { + Thread main = Thread.currentThread(); + Ledger.setNodeUpkeepLookup(g -> { + if (Thread.currentThread() != main) { + throw new IllegalStateException("Character identity must be used on the main thread"); + } + return g == guild ? 200 : 0; + }); + when(guild.getFaction()).thenReturn(FactionManager.factions.getFirst()); + Ledger ledger = new Ledger(guild); + + // Dowsing's lookup asks SimpleFactions' API for readiness, which only answers on the main thread. + ExecutionException live = assertThrows(ExecutionException.class, + () -> CompletableFuture.supplyAsync(() -> ledger.getIncome(Cashflow.NODES)).get()); + assertInstanceOf(IllegalStateException.class, live.getCause()); + + FrozenReads frozen; + try (var denars = mockStatic(Denars.class, CALLS_REAL_METHODS)) { + denars.when(() -> Denars.balance(anyString())).thenReturn(0L); + frozen = FrozenReads.capture(); + } + assertEquals(-200.0, CompletableFuture.supplyAsync(() -> frozen.read(() -> ledger.getIncome(Cashflow.NODES))).get()); + assertEquals(-200.0, ledger.getIncome(Cashflow.NODES)); + } + /** Knows one dead character and refuses lookups off the capturing thread, like TLibs. */ private record MainThreadCharacters(Thread main, CharacterRef dead) implements Members.Resolver { @Override