From 75c93f2fdea0611da76daee2ecef9b59f3e1f985 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 12 Nov 2026 12:07:03 +0100 Subject: [PATCH] fix: read inactive members on the main thread for async income previews Since 4.28.0 (#238) every law, trade and favour income estimate on Main showed "Income estimate unavailable". Member shares in stability and upkeep skip members 28+ days offline, and whether such a member is online asks TLibs, which refuses lookups off the main thread, so the preview worker threw on the first guild with an inactive member. FrozenReads now also captures the inactive members with each preview copy: one pass over the offline day counts, minus the characters being played now (one session lookup per online player, none when nobody is past the threshold). Inside a frozen read isMemberInactive is a set lookup, so the worker no longer calls TLibs or reads the day-count map the server thread writes. The live check outside previews is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../simplefactions/economy/FrozenReads.java | 28 ++- .../inactivity/InactivityService.java | 34 +++ .../simplefactions/utils/FactionCleanup.java | 13 ++ .../inactivity/FrozenInactiveMembersTest.java | 197 ++++++++++++++++++ 4 files changed, 266 insertions(+), 6 deletions(-) create mode 100644 src/test/java/net/tfminecraft/simplefactions/inactivity/FrozenInactiveMembersTest.java diff --git a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java index c96133d7..3a6653f9 100644 --- a/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java +++ b/src/main/java/net/tfminecraft/simplefactions/economy/FrozenReads.java @@ -3,6 +3,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.function.Supplier; @@ -11,14 +12,15 @@ import net.tfminecraft.simplefactions.government.StabilityModifier; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.guild.income.Ledger; +import net.tfminecraft.simplefactions.inactivity.InactivityService; import net.tfminecraft.simplefactions.managers.FactionManager; import net.tfminecraft.simplefactions.objects.Bank; import net.tfminecraft.simplefactions.objects.Faction; /** - * Guild treasuries, held tax, 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. + * Guild treasuries, held tax, node upkeep, espionage stability and inactive members 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<>(); @@ -27,17 +29,23 @@ public final class FrozenReads { private final Map> taxContributors; private final Map nodeUpkeep; private final Map> espionage; + private final Set inactive; private FrozenReads( Map cents, Map> taxContributors, - Map nodeUpkeep, Map> espionage) { + Map nodeUpkeep, Map> espionage, + Set inactive) { this.cents = Map.copyOf(cents); this.taxContributors = Map.copyOf(taxContributors); this.nodeUpkeep = Map.copyOf(nodeUpkeep); this.espionage = Map.copyOf(espionage); + this.inactive = Set.copyOf(inactive); } - /** Reads guild treasuries, held tax and node upkeep and realm espionage modifiers now. Call on the main thread. */ + /** + * Reads guild treasuries, held tax, node upkeep, realm espionage modifiers and inactive + * members now. Call on the main thread. + */ public static FrozenReads capture() { long now = System.currentTimeMillis(); Map> espionage = new HashMap<>(); @@ -68,7 +76,9 @@ public static FrozenReads capture() { nodeUpkeep.put(incarnation, Ledger.liveNodeUpkeep(guild)); } } - return new FrozenReads(cents, taxContributors, nodeUpkeep, espionage); + // Member shares (stability, upkeep) skip inactive members; whether one is online asks TLibs. + Set inactive = InactivityService.captureInactiveMembers(); + return new FrozenReads(cents, taxContributors, nodeUpkeep, espionage, inactive); } /** Runs the work with these reads standing in for the live state on this thread. */ @@ -113,6 +123,12 @@ public static double nodeUpkeep(Guild guild) { return Ledger.liveNodeUpkeep(guild); } + /** Inside {@link #read}, the members who were inactive at capture; otherwise null, so callers check live. */ + public static Set inactiveMembers() { + FrozenReads current = CURRENT.get(); + return current == null ? null : current.inactive; + } + /** 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/inactivity/InactivityService.java b/src/main/java/net/tfminecraft/simplefactions/inactivity/InactivityService.java index 4b25af29..2dbbdbfc 100644 --- a/src/main/java/net/tfminecraft/simplefactions/inactivity/InactivityService.java +++ b/src/main/java/net/tfminecraft/simplefactions/inactivity/InactivityService.java @@ -4,8 +4,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.logging.Level; import org.bukkit.Bukkit; @@ -15,6 +17,7 @@ import net.tfminecraft.simplefactions.SimpleFactions; import net.tfminecraft.simplefactions.database.Database; import net.tfminecraft.simplefactions.database.JsonUtil; +import net.tfminecraft.simplefactions.economy.FrozenReads; import net.tfminecraft.simplefactions.guild.Guild; import net.tfminecraft.simplefactions.inactivity.DecayClock.Event; import net.tfminecraft.simplefactions.managers.BackgroundRecalculation; @@ -110,11 +113,42 @@ public static int prestigePercent(Faction faction) { public static boolean isMemberInactive(String name) { if (!InactivityRules.isTrackedMember(name)) return false; + // Async income previews: TLibs refuses the online lookup off the server thread, and the + // day counts change on it, so they read the set captured with the preview copy. + Set frozen = FrozenReads.inactiveMembers(); + if (frozen != null) return frozen.contains(name); // The day count is a map read; only a member already past the threshold needs the online lookup. if (!InactivityRules.isInactive(FactionCleanup.daysOffline(name))) return false; return !isOnline(name); } + /** + * Every tracked member inactive right now: past the offline threshold and not playing. + * Server thread only. One pass over the day counts, and one character lookup per online + * player instead of one per member past the threshold. + */ + public static Set captureInactiveMembers() { + List away = FactionCleanup.membersOffline(InactivityRules::isInactive); + if (away.isEmpty()) return Set.of(); + Set online = onlineMemberKeys(); + Set inactive = new HashSet<>(); + for (String member : away) { + if (InactivityRules.isTrackedMember(member) && !online.contains(member)) inactive.add(member); + } + return inactive; + } + + /** The character keys being played now, which is what {@link Members#isOnline} checks per key. */ + private static Set onlineMemberKeys() { + if (Bukkit.getServer() == null) return Set.of(); + Set keys = new HashSet<>(); + for (Player player : Bukkit.getOnlinePlayers()) { + String key = Members.key(player); + if (key != null) keys.add(key); + } + return keys; + } + /** The members who still count. Inactive members stay in their guild but add nothing to counts. */ public static List activeOnly(java.util.Collection members) { List active = new ArrayList<>(); diff --git a/src/main/java/net/tfminecraft/simplefactions/utils/FactionCleanup.java b/src/main/java/net/tfminecraft/simplefactions/utils/FactionCleanup.java index 8c6c5e09..0ab8b8d9 100644 --- a/src/main/java/net/tfminecraft/simplefactions/utils/FactionCleanup.java +++ b/src/main/java/net/tfminecraft/simplefactions/utils/FactionCleanup.java @@ -5,11 +5,13 @@ import java.io.FileWriter; import java.io.IOException; import java.lang.reflect.Type; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.IntPredicate; import java.util.logging.Level; import org.bukkit.Bukkit; @@ -97,6 +99,17 @@ public static int daysOffline(String member) { return days == null ? 0 : days; } + /** Members whose offline day count matches, in one pass over the day counts. Server thread. */ + public static List membersOffline(IntPredicate days) { + if (!ensureLoaded()) return List.of(); + List members = new ArrayList<>(); + for (Map.Entry entry : offlineDays.entrySet()) { + Integer count = entry.getValue(); + if (days.test(count == null ? 0 : count)) members.add(entry.getKey()); + } + return members; + } + public static void ping(String member) { if (member == null || member.isBlank()) return; try { diff --git a/src/test/java/net/tfminecraft/simplefactions/inactivity/FrozenInactiveMembersTest.java b/src/test/java/net/tfminecraft/simplefactions/inactivity/FrozenInactiveMembersTest.java new file mode 100644 index 00000000..89790087 --- /dev/null +++ b/src/test/java/net/tfminecraft/simplefactions/inactivity/FrozenInactiveMembersTest.java @@ -0,0 +1,197 @@ +package net.tfminecraft.simplefactions.inactivity; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.bukkit.entity.Player; +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.simplefactions.database.JsonUtil; +import net.tfminecraft.simplefactions.economy.FrozenReads; +import net.tfminecraft.simplefactions.identity.Members; +import net.tfminecraft.simplefactions.managers.FactionManager; +import net.tfminecraft.simplefactions.objects.Faction; +import net.tfminecraft.simplefactions.testsupport.PersistenceFilesFixture; +import net.tfminecraft.simplefactions.utils.FactionCleanup; +import net.tfminecraft.tlibs.character.CharacterInfo; +import net.tfminecraft.tlibs.character.CharacterSession; + +/** + * Async income previews count member shares (stability, upkeep). Whether an inactive member + * is online asks TLibs, which refuses other threads, so previews read the set captured with + * their copy on the main thread (Main, 11-11: every trade, law and favour estimate failed). + */ +class FrozenInactiveMembersTest { + private static final int AWAY = InactivityRules.DAYS_UNTIL_INACTIVE; + + private final List savedFactions = new ArrayList<>(); + private PersistenceFilesFixture disk; + private Field history; + private Object savedHistory; + private Characters characters; + + @BeforeEach + void setUp() throws Exception { + disk = new PersistenceFilesFixture(); + history = FactionCleanup.class.getDeclaredField("offlineDays"); + history.setAccessible(true); + savedHistory = history.get(null); + // No realms, so capture reads no treasuries and only the inactive members matter here. + savedFactions.addAll(FactionManager.factions); + FactionManager.factions.clear(); + characters = new Characters(); + Members.setResolver(characters); + } + + @AfterEach + void tearDown() throws Exception { + try { + Members.setResolver(null); + FactionManager.factions.clear(); + FactionManager.factions.addAll(savedFactions); + history.set(null, savedHistory); + } finally { + disk.close(); + } + } + + private void days(Map days) throws Exception { + disk.write("Cache/logins.json", JsonUtil.GSON.toJson(days)); + history.set(null, null); + } + + private static MockedStatic server(Player... online) { + MockedStatic bukkit = mockStatic(Bukkit.class); + Server server = mock(Server.class); + bukkit.when(Bukkit::getServer).thenReturn(server); + bukkit.when(Bukkit::getOnlinePlayers).thenAnswer(call -> List.of(online)); + return bukkit; + } + + @Test + void previewsReadTheInactiveMembersCapturedOnTheMainThread() throws Exception { + days(Map.of("bob", AWAY, "carl", AWAY + 10, "dana", AWAY - 1, "dummy_ghost", AWAY + 40)); + Player carl = mock(Player.class); + characters.playing.put(carl, "carl"); + List realm = List.of("alice", "bob", "carl", "dana", "dummy_ghost"); + + FrozenReads frozen; + try (var bukkit = server(carl)) { + frozen = FrozenReads.capture(); + // The copy answers as the live check does for each kind of member. + for (String member : List.of("bob", "carl", "dana", "dummy_ghost", "erin")) { + assertEquals(InactivityService.isMemberInactive(member), + frozen.read(() -> InactivityService.isMemberInactive(member)), member); + } + + // Off the main thread TLibs refuses the online lookup, as it did on Main. + characters.refuse = true; + assertThrows(IllegalStateException.class, () -> InactivityService.isMemberInactive("bob")); + + assertTrue(frozen.read(() -> InactivityService.isMemberInactive("bob"))); + assertFalse(frozen.read(() -> InactivityService.isMemberInactive("carl")), "online at capture"); + assertFalse(frozen.read(() -> InactivityService.isMemberInactive("dana")), "under the threshold"); + assertFalse(frozen.read(() -> InactivityService.isMemberInactive("dummy_ghost")), "not tracked"); + assertFalse(frozen.read(() -> InactivityService.isMemberInactive("erin")), "never counted"); + assertFalse(frozen.read(() -> InactivityService.isMemberInactive(null))); + } + + // Day counts written after the copy belong to the next preview, not this one. + days(Map.of()); + List active = List.of("alice", "carl", "dana", "dummy_ghost"); + assertEquals(active, CompletableFuture.supplyAsync(() -> frozen.read(() -> InactivityService.activeOnly(realm))).get()); + assertFalse(InactivityService.isMemberInactive("bob")); + assertNull(FrozenReads.inactiveMembers()); + } + + @Test + void noOneAwayNeedsNoCharacterLookup() throws Exception { + days(Map.of("bob", AWAY - 1)); + characters.refuse = true; + try (var bukkit = server(mock(Player.class))) { + assertEquals(Set.of(), InactivityService.captureInactiveMembers()); + } + } + + @Test + void playersWithoutACharacterAreNobodyOnline() throws Exception { + days(Map.of("bob", AWAY)); + try (var bukkit = server(mock(Player.class))) { + assertEquals(Set.of("bob"), InactivityService.captureInactiveMembers()); + } + } + + @Test + void withoutAServerNobodyIsOnline() throws Exception { + // Raw JSON: Gson leaves null entries out when it writes. + disk.write("Cache/logins.json", "{\"bob\":" + AWAY + ",\"carl\":null}"); + history.set(null, null); + assertEquals(List.of("carl"), FactionCleanup.membersOffline(days -> days == 0)); + assertEquals(Set.of("bob"), InactivityService.captureInactiveMembers()); + } + + @Test + void unreadableDayCountsMarkNobodyInactive() throws Exception { + disk.write("Cache/logins.json", "{"); + history.set(null, null); + assertEquals(List.of(), FactionCleanup.membersOffline(days -> true)); + assertEquals(Set.of(), InactivityService.captureInactiveMembers()); + } + + /** Plays the given characters and, once told to, refuses lookups like TLibs off the main thread. */ + private static final class Characters implements Members.Resolver { + private final Map playing = new HashMap<>(); + private volatile boolean refuse; + + private void guard() { + if (refuse) throw new IllegalStateException("Character identity must be used on the main thread"); + } + + @Override + public String key(Player player) { + guard(); + return playing.get(player); + } + + @Override + public Optional online(String key) { + guard(); + return playing.entrySet().stream().filter(entry -> entry.getValue().equals(key)).map(Map.Entry::getKey).findFirst(); + } + + @Override + public Optional info(String key) { + guard(); + return Optional.empty(); + } + + @Override + public Optional session(Player player) { + return Optional.empty(); + } + + @Override + public boolean isCurrent(CharacterSession session) { + return false; + } + } +}