From f3ea4fa65a918326ecbb2e4b831fd2f455e61f61 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:53:02 +0100 Subject: [PATCH 1/4] perf: reuse unchanged live character snapshots in identity lookups CharacterIdentityService.info overlays every loaded account holding the character before each lookup, and that rebuilt a fresh CharacterInfo for each of the account's characters every time (colour stripping, trait sets, record copies). On Main it cost about 5 s an hour on the main thread, mostly from CompanionPets and SimpleFactions lookups through TLibs Characters.info. The directory now keeps each live character's last snapshot together with every value it was built from (owner, id, name, status, race id, trait ids, playtime, last played) and reuses it while all of them are unchanged. The check reads the live character on every lookup, so any edit, however it is made, is seen at once; no invalidation hooks are needed. Directory updates, quarantine and duplicate handling run exactly as before. The per-lookup scans of loaded accounts use plain loops instead of stream pipelines. Co-Authored-By: Claude Opus 5.5 --- .../identity/CharacterDirectory.java | 18 ++++- .../identity/CharacterIdentityService.java | 28 +++++-- .../rpcharacters/identity/LiveSnapshot.java | 68 ++++++++++++++++ .../CharacterIdentityServiceTest.java | 77 +++++++++++++++++++ 4 files changed, 183 insertions(+), 8 deletions(-) create mode 100644 src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java index a9d5f7be..1fd54afa 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java @@ -21,6 +21,7 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; +import java.util.WeakHashMap; import java.util.function.Consumer; import org.json.simple.JSONArray; @@ -44,6 +45,11 @@ public final class CharacterDirectory { private final Set unavailable = new HashSet<>(); /** Characters saved with the legacy {@code active} flag, which predates session timestamps. */ private final Set flaggedActive = new HashSet<>(); + /** + * Each live character's last snapshot, reused while nothing it was built from has changed. Keyed by + * object identity and weakly, so characters of players who left drop out on their own. + */ + private final Map live = new WeakHashMap<>(); private final Consumer warn; private CharacterDirectory(Consumer warn) { @@ -144,7 +150,7 @@ public void upsert(UUID ownerId, RPCharacter character) { Objects.requireNonNull(ownerId, "ownerId"); CharacterInfo info; try { - info = snapshot(ownerId, character); + info = liveSnapshot(ownerId, character); } catch (RuntimeException error) { quarantine(ownerId, "Invalid live character: " + error.getMessage()); return; @@ -152,6 +158,16 @@ public void upsert(UUID ownerId, RPCharacter character) { upsert(ownerId, info, Boolean.TRUE.equals(character.isActive()), character.getDeathId()); } + /** {@link #snapshot}, reusing the last one while every value it read is unchanged. */ + private CharacterInfo liveSnapshot(UUID ownerId, RPCharacter character) { + LiveSnapshot last = live.get(character); + if (last != null && last.matches(ownerId, character)) return last.info(); + CharacterInfo info = snapshot(ownerId, character); + // It succeeded, so the character, its trait list and every trait in it are present. + live.put(character, new LiveSnapshot(ownerId, character, info)); + return info; + } + /** Updates from a snapshot taken earlier, such as when a save finished off the main thread. */ public void upsert(UUID ownerId, CharacterInfo info, boolean active) { upsert(ownerId, info, active, null); diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java index f23c28e6..c88e612b 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java @@ -38,28 +38,42 @@ public CharacterIdentityService(CharacterDirectory directory, CharacterTransitio */ private void refreshOwner(UUID ownerId) { CharacterTransitions.requireMainThread(); + List online = PlayerManager.getOnlineData(); Set ids = new HashSet<>(); - for (PlayerData data : PlayerManager.getOnlineData()) { + for (PlayerData data : online) { if (!ownerId.equals(data.getUniqueId())) continue; for (RPCharacter character : data.getCharacters()) { if (character.getId() != null) ids.add(character.getId().toLowerCase(Locale.ROOT)); } } - for (PlayerData data : PlayerManager.getOnlineData()) { - if (ownerId.equals(data.getUniqueId()) || data.getCharacters().stream().anyMatch( - character -> character.getId() != null && ids.contains(character.getId().toLowerCase(Locale.ROOT)))) { - refresh(data); - } + for (PlayerData data : online) { + if (ownerId.equals(data.getUniqueId()) || sharesId(data, ids)) refresh(data); } } + private static boolean sharesId(PlayerData data, Set ids) { + if (ids.isEmpty()) return false; + for (RPCharacter character : data.getCharacters()) { + if (character.getId() != null && ids.contains(character.getId().toLowerCase(Locale.ROOT))) return true; + } + return false; + } + /** Overlays every loaded account holding this character, so live duplicates still quarantine. */ private void refreshCharacter(UUID characterId) { CharacterTransitions.requireMainThread(); String id = characterId.toString(); for (PlayerData data : PlayerManager.getOnlineData()) { - if (data.getCharacters().stream().anyMatch(character -> id.equalsIgnoreCase(character.getId()))) refresh(data); + if (holds(data, id)) refresh(data); + } + } + + // Plain loops: these run for every lookup, and stream pipelines here cost more than the compares. + private static boolean holds(PlayerData data, String id) { + for (RPCharacter character : data.getCharacters()) { + if (id.equalsIgnoreCase(character.getId())) return true; } + return false; } private void refresh(PlayerData data) { diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java new file mode 100644 index 00000000..4e8b67b4 --- /dev/null +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java @@ -0,0 +1,68 @@ +package net.tfminecraft.rpcharacters.identity; + +import java.time.Instant; +import java.util.List; +import java.util.Objects; +import java.util.UUID; + +import net.tfminecraft.rpcharacters.enums.Status; +import net.tfminecraft.rpcharacters.objects.RPCharacter; +import net.tfminecraft.rpcharacters.objects.races.Race; +import net.tfminecraft.rpcharacters.objects.trait.Trait; +import net.tfminecraft.tlibs.character.CharacterInfo; + +/** + * A live character's last {@link CharacterDirectory#snapshot} with every value that snapshot read. + * While those values are unchanged the snapshot comes out the same, so it is reused, not rebuilt. + * Main thread only, like the directory. + */ +final class LiveSnapshot { + private final UUID ownerId; + private final String id; + private final String name; + private final Status status; + private final boolean hasRace; + private final String raceId; + private final String[] traitIds; + private final int playtimeSeconds; + private final Instant lastPlayedAt; + private final CharacterInfo info; + + /** Records the values {@code info} was just built from; the build succeeded, so none is missing. */ + LiveSnapshot(UUID ownerId, RPCharacter character, CharacterInfo info) { + this.ownerId = ownerId; + this.id = character.getId(); + this.name = character.getName(); + this.status = character.getStatus(); + Race race = character.getRace(); + this.hasRace = race != null; + this.raceId = race == null ? null : race.getId(); + List traits = character.getTraits(); + this.traitIds = new String[traits.size()]; + for (int i = 0; i < traitIds.length; i++) traitIds[i] = traits.get(i).getId(); + this.playtimeSeconds = character.getOnlinePlaytimeSeconds(); + this.lastPlayedAt = character.getLastPlayedAt(); + this.info = Objects.requireNonNull(info, "info"); + } + + CharacterInfo info() { + return info; + } + + /** True when every value the snapshot read still matches, so it would come out the same. */ + boolean matches(UUID ownerId, RPCharacter character) { + if (!this.ownerId.equals(ownerId) || character.getOnlinePlaytimeSeconds() != playtimeSeconds + || character.getStatus() != status || !Objects.equals(character.getId(), id) + || !Objects.equals(character.getName(), name) + || !Objects.equals(character.getLastPlayedAt(), lastPlayedAt)) return false; + Race race = character.getRace(); + if (race == null ? hasRace : !hasRace || !Objects.equals(race.getId(), raceId)) return false; + List traits = character.getTraits(); + if (traits == null || traits.size() != traitIds.length) return false; + for (int i = 0; i < traitIds.length; i++) { + Trait trait = traits.get(i); + if (trait == null || !Objects.equals(trait.getId(), traitIds[i])) return false; + } + return true; + } +} diff --git a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java index 30c82e3f..0808f31b 100644 --- a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java +++ b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java @@ -395,6 +395,83 @@ void lookupsOnlyRebuildTheAccountsTheyNeed() { assertEquals("Renamed", service.characters(other.getUniqueId()).orElseThrow().getFirst().name()); } + @Test + void liveSnapshotsAreReusedUntilAnyValueTheyReadChangesAndAlwaysMatchAFreshSnapshot() { + UUID owner = player.getUniqueId(); + UUID id = UUID.fromString(alice.getId()); + Race elf = mock(Race.class); + when(elf.getId()).thenReturn("elf"); + Race human = mock(Race.class); + when(human.getId()).thenReturn("human"); + Trait strong = mock(Trait.class); + when(strong.getId()).thenReturn("STRONG"); + Trait swift = mock(Trait.class); + when(swift.getId()).thenReturn("swift"); + List edits = List.of( + () -> alice.setName("&cRenamed"), + () -> alice.setStatus(Status.MISSING), + () -> alice.setStatus(Status.ALIVE), + () -> alice.setRace(elf), + () -> when(elf.getId()).thenReturn("high_elf"), + () -> alice.setRace(human), + () -> alice.setRace(null), + () -> alice.setTraits(new ArrayList<>(List.of(strong))), + () -> alice.getTraits().add(swift), + () -> when(swift.getId()).thenReturn("nimble"), + () -> alice.getTraits().remove(strong), + () -> alice.setOnlinePlaytimeSeconds(60), + () -> alice.addOnlinePlaytimeSeconds(60), + () -> alice.setLastPlayedAt(Instant.ofEpochMilli(1000)), + () -> alice.setLastPlayedAt(Instant.ofEpochMilli(2000)), + () -> alice.setLastPlayedAt(null)); + CharacterInfo previous = service.info(id).orElseThrow(); + assertSame(previous, service.info(id).orElseThrow()); + for (Runnable edit : edits) { + edit.run(); + CharacterInfo now = service.info(id).orElseThrow(); + assertEquals(CharacterDirectory.snapshot(owner, alice), now); + assertNotEquals(previous, now); + // Unchanged values reuse the snapshot; every lookup method sees the same one. + assertSame(now, service.info(id).orElseThrow()); + assertSame(now, service.characters(owner).orElseThrow().getFirst()); + assertSame(now, service.lastPlayed(owner).orElseThrow()); + previous = now; + } + assertEquals(List.of(previous), service.findByName("Renamed")); + assertEquals(Set.of("nimble"), previous.traits()); + // An unchanged edit (same name again) keeps the reused snapshot. + alice.setName("Renamed"); + assertSame(previous, service.info(id).orElseThrow()); + } + + @Test + void aLiveCharacterThatBreaksAfterBeingSnapshottedStillQuarantinesItsAccount() { + UUID id = UUID.fromString(alice.getId()); + assertTrue(service.info(id).isPresent()); + List broken = new ArrayList<>(); + broken.add(null); + alice.setTraits(broken); + assertTrue(service.info(id).isEmpty()); + assertTrue(service.characters(player.getUniqueId()).isEmpty()); + alice.setTraits(null); + assertTrue(service.info(id).isEmpty()); + } + + @Test + void characterLookupsCatchALiveDuplicateWhoseIdDiffersOnlyInCase() { + UUID id = UUID.fromString(alice.getId()); + assertTrue(service.info(id).isPresent()); + PlayerMock other = server.addPlayer("Other"); + PlayerData otherData = new PlayerData(other); + loaded.put(other, otherData); + RPCharacter duplicate = new RPCharacter(other); + duplicate.setId(alice.getId().toUpperCase(java.util.Locale.ROOT)); + duplicate.setName("Different owner"); + otherData.addCharacter(duplicate); + assertTrue(service.info(id).isEmpty()); + assertTrue(service.characters(other.getUniqueId()).isEmpty()); + } + private void install() throws Exception { directory = CharacterDirectory.load(root, ignored -> {}); runtime = new CharacterTransitions(database, directory); From c215b58f1e960e6f1a28399a9e16fdf23569c0c3 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 5 Nov 2026 12:40:33 +0100 Subject: [PATCH 2/4] Keep identity reuse exact when getters fail; test warnings, id and owner edits Review fixes: a failing read while checking a reused snapshot now just rebuilds it (so the rebuild reports the failure as before), failing to record a fresh snapshot only skips reuse, and the owner scan no longer skips reading other accounts' ids when the owner holds none. Tests now check the exact quarantine warnings, id case and owner changes against a directory fed fresh snapshots only. Co-Authored-By: Claude Opus 5.5 --- .../identity/CharacterDirectory.java | 10 +- .../identity/CharacterIdentityService.java | 1 - .../rpcharacters/identity/LiveSnapshot.java | 28 +++++- .../CharacterIdentityServiceTest.java | 91 ++++++++++++++++++- 4 files changed, 118 insertions(+), 12 deletions(-) diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java index 1fd54afa..e81485d9 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java @@ -46,8 +46,9 @@ public final class CharacterDirectory { /** Characters saved with the legacy {@code active} flag, which predates session timestamps. */ private final Set flaggedActive = new HashSet<>(); /** - * Each live character's last snapshot, reused while nothing it was built from has changed. Keyed by - * object identity and weakly, so characters of players who left drop out on their own. + * Each live character's last snapshot, reused while nothing it was built from has changed. Weak keys, + * so characters of players who left drop out on their own; RPCharacter keeps Object's identity + * equals and hashCode, which keeps each key findable while its fields change. */ private final Map live = new WeakHashMap<>(); private final Consumer warn; @@ -163,8 +164,9 @@ private CharacterInfo liveSnapshot(UUID ownerId, RPCharacter character) { LiveSnapshot last = live.get(character); if (last != null && last.matches(ownerId, character)) return last.info(); CharacterInfo info = snapshot(ownerId, character); - // It succeeded, so the character, its trait list and every trait in it are present. - live.put(character, new LiveSnapshot(ownerId, character, info)); + LiveSnapshot next = LiveSnapshot.capture(ownerId, character, info); + if (next == null) live.remove(character); + else live.put(character, next); return info; } diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java index c88e612b..c69da215 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityService.java @@ -52,7 +52,6 @@ private void refreshOwner(UUID ownerId) { } private static boolean sharesId(PlayerData data, Set ids) { - if (ids.isEmpty()) return false; for (RPCharacter character : data.getCharacters()) { if (character.getId() != null && ids.contains(character.getId().toLowerCase(Locale.ROOT))) return true; } diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java index 4e8b67b4..00f3f896 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java @@ -28,8 +28,19 @@ final class LiveSnapshot { private final Instant lastPlayedAt; private final CharacterInfo info; - /** Records the values {@code info} was just built from; the build succeeded, so none is missing. */ - LiveSnapshot(UUID ownerId, RPCharacter character, CharacterInfo info) { + /** + * Records the values {@code info} was just built from, or null if reading them again fails. Getters + * of real characters are plain field reads, so this only guards odd test doubles. + */ + static LiveSnapshot capture(UUID ownerId, RPCharacter character, CharacterInfo info) { + try { + return new LiveSnapshot(ownerId, character, info); + } catch (RuntimeException unreadable) { + return null; + } + } + + private LiveSnapshot(UUID ownerId, RPCharacter character, CharacterInfo info) { this.ownerId = ownerId; this.id = character.getId(); this.name = character.getName(); @@ -49,8 +60,19 @@ CharacterInfo info() { return info; } - /** True when every value the snapshot read still matches, so it would come out the same. */ + /** + * True when every value the snapshot read still matches, so it would come out the same. A read that + * fails counts as a change: the caller then rebuilds, and the rebuild reports the failure as before. + */ boolean matches(UUID ownerId, RPCharacter character) { + try { + return unchanged(ownerId, character); + } catch (RuntimeException unreadable) { + return false; + } + } + + private boolean unchanged(UUID ownerId, RPCharacter character) { if (!this.ownerId.equals(ownerId) || character.getOnlinePlaytimeSeconds() != playtimeSeconds || character.getStatus() != status || !Objects.equals(character.getId(), id) || !Objects.equals(character.getName(), name) diff --git a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java index 0808f31b..90a8b71f 100644 --- a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java +++ b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java @@ -9,6 +9,7 @@ import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -62,6 +63,7 @@ class CharacterIdentityServiceTest { private MockedStatic masks; private final Map loaded = new LinkedHashMap<>(); private final List boundaries = new ArrayList<>(); + private final List warnings = new ArrayList<>(); @BeforeEach void setup() throws Exception { @@ -445,16 +447,82 @@ void liveSnapshotsAreReusedUntilAnyValueTheyReadChangesAndAlwaysMatchAFreshSnaps } @Test - void aLiveCharacterThatBreaksAfterBeingSnapshottedStillQuarantinesItsAccount() { + void aLiveCharacterThatBreaksAfterBeingSnapshottedQuarantinesItsAccountWithTheSameWarning() throws Exception { + UUID owner = player.getUniqueId(); UUID id = UUID.fromString(alice.getId()); assertTrue(service.info(id).isPresent()); List broken = new ArrayList<>(); broken.add(null); alice.setTraits(broken); + String nullTrait = invalidLiveCharacter(owner, alice); assertTrue(service.info(id).isEmpty()); - assertTrue(service.characters(player.getUniqueId()).isEmpty()); + assertTrue(service.characters(owner).isEmpty()); + assertEquals(nullTrait, warnings.getLast()); + + install(); + alice.setTraits(new ArrayList<>()); + assertTrue(service.info(id).isPresent()); alice.setTraits(null); + String nullList = invalidLiveCharacter(owner, alice); assertTrue(service.info(id).isEmpty()); + assertEquals(nullList, warnings.getLast()); + } + + @Test + void aGetterThatFailsOnLaterReadsIsReportedOrToleratedAsAFreshSnapshotWouldBe() { + UUID owner = UUID.randomUUID(); + RPCharacter odd = mock(RPCharacter.class); + UUID oddId = UUID.randomUUID(); + when(odd.getId()).thenReturn(oddId.toString()); + when(odd.getName()).thenReturn("Odd"); + when(odd.getStatus()).thenReturn(Status.ALIVE); + when(odd.getTraits()).thenReturn(List.of()); + directory.upsert(owner, odd); + assertEquals("Odd", directory.info(oddId).orElseThrow().name()); + // A failing read while checking the reused snapshot rebuilds it, and the rebuild reports the failure. + when(odd.getName()).thenThrow(new IllegalStateException("name unavailable")); + directory.upsert(owner, odd); + assertTrue(directory.characters(owner).isEmpty()); + assertEquals("Character directory unavailable for " + owner + ": Invalid live character: name unavailable", + warnings.getLast()); + + UUID other = UUID.randomUUID(); + RPCharacter flaky = mock(RPCharacter.class); + UUID flakyId = UUID.randomUUID(); + when(flaky.getId()).thenReturn(flakyId.toString()); + when(flaky.getName()).thenReturn("Flaky").thenThrow(new IllegalStateException("second read")); + when(flaky.getStatus()).thenReturn(Status.ALIVE); + when(flaky.getTraits()).thenReturn(List.of()); + // Only the snapshot itself reads the name successfully; failing to record it only skips reuse. + int before = warnings.size(); + directory.upsert(other, flaky); + assertEquals("Flaky", directory.info(flakyId).orElseThrow().name()); + assertEquals(before, warnings.size()); + } + + @Test + void idCaseAndOwnerChangesRebuildExactlyAsWithoutReuse() { + UUID owner = player.getUniqueId(); + UUID id = UUID.fromString(alice.getId()); + CharacterInfo before = service.info(id).orElseThrow(); + alice.setId(alice.getId().toUpperCase(Locale.ROOT)); + CharacterInfo after = service.info(id).orElseThrow(); + assertEquals(CharacterDirectory.snapshot(owner, alice), after); + assertNotSame(before, after); + assertSame(after, service.info(id).orElseThrow()); + + // The same character object under another owner: identical to a directory without reuse. + CharacterDirectory reference = reference(); + UUID other = UUID.randomUUID(); + RPCharacter moved = character("Moved", false); + UUID movedId = UUID.fromString(moved.getId()); + for (UUID holder : List.of(other, owner, other)) { + directory.upsert(holder, moved); + reference.upsert(holder, CharacterDirectory.snapshot(holder, moved), false, null); + assertEquals(reference.info(movedId), directory.info(movedId)); + assertEquals(reference.characters(other), directory.characters(other)); + } + assertTrue(directory.info(movedId).isEmpty()); } @Test @@ -465,7 +533,7 @@ void characterLookupsCatchALiveDuplicateWhoseIdDiffersOnlyInCase() { PlayerData otherData = new PlayerData(other); loaded.put(other, otherData); RPCharacter duplicate = new RPCharacter(other); - duplicate.setId(alice.getId().toUpperCase(java.util.Locale.ROOT)); + duplicate.setId(alice.getId().toUpperCase(Locale.ROOT)); duplicate.setName("Different owner"); otherData.addCharacter(duplicate); assertTrue(service.info(id).isEmpty()); @@ -473,11 +541,26 @@ void characterLookupsCatchALiveDuplicateWhoseIdDiffersOnlyInCase() { } private void install() throws Exception { - directory = CharacterDirectory.load(root, ignored -> {}); + directory = CharacterDirectory.load(root, warnings::add); runtime = new CharacterTransitions(database, directory); service = new CharacterIdentityService(directory, runtime); } + /** A directory fed fresh snapshots only, as every update was before snapshots were reused. */ + private CharacterDirectory reference() { + try { + return CharacterDirectory.load(root.resolve("reference-absent"), ignored -> {}); + } catch (java.io.IOException error) { + throw new AssertionError(error); + } + } + + /** The warning the directory gives for a live character whose snapshot fails, built from that failure. */ + private static String invalidLiveCharacter(UUID owner, RPCharacter character) { + RuntimeException error = assertThrows(RuntimeException.class, () -> CharacterDirectory.snapshot(owner, character)); + return "Character directory unavailable for " + owner + ": Invalid live character: " + error.getMessage(); + } + private RPCharacter character(String name, boolean active) { RPCharacter character = new RPCharacter(player); character.setName(name); From d8c835bcb74fd14428906e7fd5cbdeefcdeba928 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:05:24 +0100 Subject: [PATCH 3/4] Record identity snapshot inputs before building; cover same-size trait edits Review round 2: the values a reused snapshot is checked against are now read before the snapshot is built, so an edit landing between the two reads can only cost a rebuild, never a stale reuse. Tests add a same-size trait list edit, compare quarantine warnings with a directory fed fresh snapshots only, and show that a character whose values cannot be recorded is rebuilt each time with the same result. Co-Authored-By: Claude Opus 5.5 --- .../identity/CharacterDirectory.java | 5 ++- .../rpcharacters/identity/LiveSnapshot.java | 17 ++++--- .../CharacterIdentityServiceTest.java | 44 ++++++++++++------- 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java index e81485d9..8b3a8202 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java @@ -163,10 +163,11 @@ public void upsert(UUID ownerId, RPCharacter character) { private CharacterInfo liveSnapshot(UUID ownerId, RPCharacter character) { LiveSnapshot last = live.get(character); if (last != null && last.matches(ownerId, character)) return last.info(); + // Values first, so an edit landing between the two reads can only cost a rebuild, never a stale reuse. + LiveSnapshot next = LiveSnapshot.read(ownerId, character); CharacterInfo info = snapshot(ownerId, character); - LiveSnapshot next = LiveSnapshot.capture(ownerId, character, info); if (next == null) live.remove(character); - else live.put(character, next); + else live.put(character, next.built(info)); return info; } diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java index 00f3f896..4cb08710 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/LiveSnapshot.java @@ -26,21 +26,21 @@ final class LiveSnapshot { private final String[] traitIds; private final int playtimeSeconds; private final Instant lastPlayedAt; - private final CharacterInfo info; + private CharacterInfo info; /** - * Records the values {@code info} was just built from, or null if reading them again fails. Getters - * of real characters are plain field reads, so this only guards odd test doubles. + * The values a snapshot reads, recorded before it is built, or null if reading them fails (the + * snapshot then reports the failure itself). Getters of real characters are plain field reads. */ - static LiveSnapshot capture(UUID ownerId, RPCharacter character, CharacterInfo info) { + static LiveSnapshot read(UUID ownerId, RPCharacter character) { try { - return new LiveSnapshot(ownerId, character, info); + return new LiveSnapshot(ownerId, character); } catch (RuntimeException unreadable) { return null; } } - private LiveSnapshot(UUID ownerId, RPCharacter character, CharacterInfo info) { + private LiveSnapshot(UUID ownerId, RPCharacter character) { this.ownerId = ownerId; this.id = character.getId(); this.name = character.getName(); @@ -53,7 +53,12 @@ private LiveSnapshot(UUID ownerId, RPCharacter character, CharacterInfo info) { for (int i = 0; i < traitIds.length; i++) traitIds[i] = traits.get(i).getId(); this.playtimeSeconds = character.getOnlinePlaytimeSeconds(); this.lastPlayedAt = character.getLastPlayedAt(); + } + + /** Attaches the snapshot built after these values were read; called once. */ + LiveSnapshot built(CharacterInfo info) { this.info = Objects.requireNonNull(info, "info"); + return this; } CharacterInfo info() { diff --git a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java index 90a8b71f..1921a2df 100644 --- a/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java +++ b/src/test/java/net/tfminecraft/rpcharacters/identity/CharacterIdentityServiceTest.java @@ -450,10 +450,12 @@ void liveSnapshotsAreReusedUntilAnyValueTheyReadChangesAndAlwaysMatchAFreshSnaps void aLiveCharacterThatBreaksAfterBeingSnapshottedQuarantinesItsAccountWithTheSameWarning() throws Exception { UUID owner = player.getUniqueId(); UUID id = UUID.fromString(alice.getId()); + Trait strong = mock(Trait.class); + when(strong.getId()).thenReturn("strong"); + alice.setTraits(new ArrayList<>(List.of(strong))); assertTrue(service.info(id).isPresent()); - List broken = new ArrayList<>(); - broken.add(null); - alice.setTraits(broken); + // Same list size, so only the per-trait check sees the change. + alice.getTraits().set(0, null); String nullTrait = invalidLiveCharacter(owner, alice); assertTrue(service.info(id).isEmpty()); assertTrue(service.characters(owner).isEmpty()); @@ -486,17 +488,26 @@ void aGetterThatFailsOnLaterReadsIsReportedOrToleratedAsAFreshSnapshotWouldBe() assertEquals("Character directory unavailable for " + owner + ": Invalid live character: name unavailable", warnings.getLast()); + // A trait list whose indexed reads fail: the snapshot only iterates it, so it still succeeds as + // before, and the failed recording only skips reuse. + RPCharacter listed = character("Listed", false); + Trait swift = mock(Trait.class); + when(swift.getId()).thenReturn("swift"); + listed.setTraits(new ArrayList<>(List.of(swift)) { + @Override + public Trait get(int index) { + throw new IllegalStateException("no indexed reads"); + } + }); UUID other = UUID.randomUUID(); - RPCharacter flaky = mock(RPCharacter.class); - UUID flakyId = UUID.randomUUID(); - when(flaky.getId()).thenReturn(flakyId.toString()); - when(flaky.getName()).thenReturn("Flaky").thenThrow(new IllegalStateException("second read")); - when(flaky.getStatus()).thenReturn(Status.ALIVE); - when(flaky.getTraits()).thenReturn(List.of()); - // Only the snapshot itself reads the name successfully; failing to record it only skips reuse. + UUID listedId = UUID.fromString(listed.getId()); int before = warnings.size(); - directory.upsert(other, flaky); - assertEquals("Flaky", directory.info(flakyId).orElseThrow().name()); + directory.upsert(other, listed); + CharacterInfo first = directory.info(listedId).orElseThrow(); + assertEquals(CharacterDirectory.snapshot(other, listed), first); + directory.upsert(other, listed); + assertEquals(first, directory.info(listedId).orElseThrow()); + assertNotSame(first, directory.info(listedId).orElseThrow()); assertEquals(before, warnings.size()); } @@ -512,15 +523,18 @@ void idCaseAndOwnerChangesRebuildExactlyAsWithoutReuse() { assertSame(after, service.info(id).orElseThrow()); // The same character object under another owner: identical to a directory without reuse. - CharacterDirectory reference = reference(); + List referenceWarnings = new ArrayList<>(); + CharacterDirectory reference = reference(referenceWarnings); UUID other = UUID.randomUUID(); RPCharacter moved = character("Moved", false); UUID movedId = UUID.fromString(moved.getId()); + int start = warnings.size(); for (UUID holder : List.of(other, owner, other)) { directory.upsert(holder, moved); reference.upsert(holder, CharacterDirectory.snapshot(holder, moved), false, null); assertEquals(reference.info(movedId), directory.info(movedId)); assertEquals(reference.characters(other), directory.characters(other)); + assertEquals(referenceWarnings, warnings.subList(start, warnings.size())); } assertTrue(directory.info(movedId).isEmpty()); } @@ -547,9 +561,9 @@ private void install() throws Exception { } /** A directory fed fresh snapshots only, as every update was before snapshots were reused. */ - private CharacterDirectory reference() { + private CharacterDirectory reference(List warned) { try { - return CharacterDirectory.load(root.resolve("reference-absent"), ignored -> {}); + return CharacterDirectory.load(root.resolve("reference-absent"), warned::add); } catch (java.io.IOException error) { throw new AssertionError(error); } From 3bfea218d641042f6161e53113c3da6febaa7026 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:14:27 +0100 Subject: [PATCH 4/4] Reword the snapshot-order comment (review round 3) Co-Authored-By: Claude Opus 5.5 --- .../tfminecraft/rpcharacters/identity/CharacterDirectory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java index 8b3a8202..0854aaa8 100644 --- a/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java +++ b/src/main/java/net/tfminecraft/rpcharacters/identity/CharacterDirectory.java @@ -163,7 +163,7 @@ public void upsert(UUID ownerId, RPCharacter character) { private CharacterInfo liveSnapshot(UUID ownerId, RPCharacter character) { LiveSnapshot last = live.get(character); if (last != null && last.matches(ownerId, character)) return last.info(); - // Values first, so an edit landing between the two reads can only cost a rebuild, never a stale reuse. + // Values first, so an edit landing between the two reads costs a rebuild instead of a stale reuse. LiveSnapshot next = LiveSnapshot.read(ownerId, character); CharacterInfo info = snapshot(ownerId, character); if (next == null) live.remove(character);