fix: read inactive members on the main thread for async income previews

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<FrozenReads> CURRENT = new ThreadLocal<>();
@@ -27,17 +29,23 @@ public final class FrozenReads {
     private final Map<UUID, Map<UUID, Long>> taxContributors;
     private final Map<UUID, Double> nodeUpkeep;
     private final Map<String, List<StabilityModifier>> espionage;
+    private final Set<String> inactive;
 
     private FrozenReads(
             Map<String, Long> cents, Map<UUID, Map<UUID, Long>> taxContributors,
-            Map<UUID, Double> nodeUpkeep, Map<String, List<StabilityModifier>> espionage) {
+            Map<UUID, Double> nodeUpkeep, Map<String, List<StabilityModifier>> espionage,
+            Set<String> 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<String, List<StabilityModifier>> 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<String> 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<String> 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<StabilityModifier> espionage(Faction faction, long now) {
         FrozenReads current = CURRENT.get();

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<String> 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<String> captureInactiveMembers() {
+		List<String> away = FactionCleanup.membersOffline(InactivityRules::isInactive);
+		if (away.isEmpty()) return Set.of();
+		Set<String> online = onlineMemberKeys();
+		Set<String> 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<String> onlineMemberKeys() {
+		if (Bukkit.getServer() == null) return Set.of();
+		Set<String> 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<String> activeOnly(java.util.Collection<String> members) {
 		List<String> active = new ArrayList<>();

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<String> membersOffline(IntPredicate days) {
+        if (!ensureLoaded()) return List.of();
+        List<String> members = new ArrayList<>();
+        for (Map.Entry<String, Integer> 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 {

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<Faction> 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<String, Integer> days) throws Exception {
+        disk.write("Cache/logins.json", JsonUtil.GSON.toJson(days));
+        history.set(null, null);
+    }
+
+    private static MockedStatic<Bukkit> server(Player... online) {
+        MockedStatic<Bukkit> 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<String> 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<String> 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<Player, String> 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<Player> online(String key) {
+            guard();
+            return playing.entrySet().stream().filter(entry -> entry.getValue().equals(key)).map(Map.Entry::getKey).findFirst();
+        }
+
+        @Override
+        public Optional<CharacterInfo> info(String key) {
+            guard();
+            return Optional.empty();
+        }
+
+        @Override
+        public Optional<CharacterSession> session(Player player) {
+            return Optional.empty();
+        }
+
+        @Override
+        public boolean isCurrent(CharacterSession session) {
+            return false;
+        }
+    }
+}