From 3bb2eb9f504eff95c32f4beeaf598d5c5f18f989 Mon Sep 17 00:00:00 2001 From: "pozdn.r.a" Date: Mon, 11 May 2026 14:04:45 +0700 Subject: [PATCH] fix(autosave): harden world save handlers and fix related gametests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The autosave path between v17 and master picked up a handful of under-defended seams that can all manifest as a "save crashed the game" report. This slice tightens them up without changing behaviour on the golden path, plus repairs two SETTLEMENT-005 gametests that broke when the political registry started enforcing "one entity per leader". Changes - ClaimRuntimeService / RecruitLifecycleEvents: null-check the static server/manager accessors before calling .overworld() (NPE during early init or shutdown races), gate LevelEvent.Save on overworld so the SavedData write fires once per autosave instead of three times (overworld+nether+end), and wrap the persist call in try/catch so a failure surfaces as a logger.error with a clear label instead of killing the save callback. - RecruitsClaimManager.persistClaims: copy claim values into a local ArrayList before feeding the HashSet so the autosave-thread iteration cannot CME against a concurrent tick handler mutating the live map. - RecruitPersistenceBridge: align Color/Biome read with the actual putInt write (was getByte, which returns 0 on a TAG_INT slot — silent data loss on every reload). - SafeSavedDataWriter: shared wrapper that catches throwables in SavedData.save bodies and logs the failing label. Applied to the newest war/governance SavedData (WarPoliticalRegistry, Treaty, EconomicObjective, BannerModGovernor) so a serializer regression in those areas no longer aborts the world autosave. - BannerModGovernorManager.save: snapshot the values list before iterating to defuse the same CME risk on heartbeat-driven mutations. - BannerModWorkOrderClaimReleaseGameTests: bump LEADER_UUID off ...5001 — it collided with BannerModWorkerUnbindOnAreaRemovalGameTests under the new "one entity per leader" validation rule and made both workorder release gametests fail at ensureFaction. - BannerModIntegratedRuntimeSmokeTest: bump expected civilian packet count to 34 (MessageUseSurveyorBlock landed after v17). Verification - ./gradlew compileJava test → green (1339 unit tests pass). - ./gradlew runGameTestServer → green (188 gametests pass; the two workorder release tests now pass). --- ...nnerModWorkOrderClaimReleaseGameTests.java | 5 ++- .../military/RecruitPersistenceBridge.java | 4 +- .../events/RecruitLifecycleEvents.java | 30 ++++++++++----- .../governance/BannerModGovernorManager.java | 17 +++++---- .../persistence/SafeSavedDataWriter.java | 38 +++++++++++++++++++ .../military/RecruitsClaimManager.java | 5 ++- .../runtime/ClaimRuntimeService.java | 23 ++++++++++- .../WarPoliticalRegistrySavedData.java | 10 +++-- .../runtime/EconomicObjectiveSavedData.java | 10 +++-- .../war/runtime/TreatySavedData.java | 14 ++++--- .../BannerModIntegratedRuntimeSmokeTest.java | 2 +- 11 files changed, 120 insertions(+), 38 deletions(-) create mode 100644 src/main/java/com/talhanation/bannermod/persistence/SafeSavedDataWriter.java diff --git a/src/gametest/java/com/talhanation/bannermod/BannerModWorkOrderClaimReleaseGameTests.java b/src/gametest/java/com/talhanation/bannermod/BannerModWorkOrderClaimReleaseGameTests.java index e6da08c7..9fa117a4 100644 --- a/src/gametest/java/com/talhanation/bannermod/BannerModWorkOrderClaimReleaseGameTests.java +++ b/src/gametest/java/com/talhanation/bannermod/BannerModWorkOrderClaimReleaseGameTests.java @@ -31,7 +31,10 @@ @GameTestHolder(BannerModMain.MOD_ID) public class BannerModWorkOrderClaimReleaseGameTests { - private static final UUID LEADER_UUID = UUID.fromString("00000000-0000-0000-0000-000000005001"); + // Uses a leader UUID disjoint from other gametests (workerunbind shares 5001) so the + // PoliticalRegistry's "one entity per leader" rule does not collide when multiple + // gametests run against the same GameTestServer level. + private static final UUID LEADER_UUID = UUID.fromString("00000000-0000-0000-0000-000000005050"); private static final UUID DEATH_CLAIM_UUID = UUID.fromString("00000000-0000-0000-0000-000000005a01"); private static final UUID DEATH_BUILDING_UUID = UUID.fromString("00000000-0000-0000-0000-000000005b01"); private static final UUID DISCARD_CLAIM_UUID = UUID.fromString("00000000-0000-0000-0000-000000005a02"); diff --git a/src/main/java/com/talhanation/bannermod/entity/military/RecruitPersistenceBridge.java b/src/main/java/com/talhanation/bannermod/entity/military/RecruitPersistenceBridge.java index 0851861a..cba2898d 100644 --- a/src/main/java/com/talhanation/bannermod/entity/military/RecruitPersistenceBridge.java +++ b/src/main/java/com/talhanation/bannermod/entity/military/RecruitPersistenceBridge.java @@ -90,7 +90,7 @@ static void readRecruitData(AbstractRecruitEntity recruit, CompoundTag nbt) { recruit.setMountTimer(nbt.getInt("mountTimer")); if (nbt.contains("UpkeepTimer")) recruit.setUpkeepTimer(nbt.getInt("UpkeepTimer")); else recruit.setUpkeepTimer(nbt.getInt("upkeepTimer")); - recruit.setColor(nbt.getByte("Color")); + recruit.setColor((byte) nbt.getInt("Color")); recruit.setMaxFallDistance(nbt.getInt("MaxFallDistance")); recruit.formationPos = nbt.getInt("formationPos"); recruit.setShouldRest(nbt.getBoolean("ShouldRest")); @@ -136,7 +136,7 @@ static void readRecruitData(AbstractRecruitEntity recruit, CompoundTag nbt) { )); } - if (nbt.contains("Biome")) recruit.setBiome(nbt.getByte("Biome")); + if (nbt.contains("Biome")) recruit.setBiome((byte) nbt.getInt("Biome")); else RecruitSpawnService.applyBiomeAndVariant(recruit); if (recruit.getCommandSenderWorld().isClientSide()) return; diff --git a/src/main/java/com/talhanation/bannermod/events/RecruitLifecycleEvents.java b/src/main/java/com/talhanation/bannermod/events/RecruitLifecycleEvents.java index 835cad2b..f318ae99 100644 --- a/src/main/java/com/talhanation/bannermod/events/RecruitLifecycleEvents.java +++ b/src/main/java/com/talhanation/bannermod/events/RecruitLifecycleEvents.java @@ -37,11 +37,7 @@ public void onServerStarted(ServerStartedEvent event) { @SubscribeEvent public void onServerStopping(ServerStoppingEvent event) { - RecruitWorldLifecycleService.saveManagers( - RecruitEvents.server(), - RecruitEvents.playerUnitManager(), - RecruitEvents.groupsManager() - ); + saveRecruitManagersSafely(); AsyncPathProcessor.shutdown(); TrueAsyncPathfindingRuntime.instance().shutdown(); @@ -49,11 +45,25 @@ public void onServerStopping(ServerStoppingEvent event) { @SubscribeEvent public void onWorldSave(LevelEvent.Save event) { - RecruitWorldLifecycleService.saveManagers( - RecruitEvents.server(), - RecruitEvents.playerUnitManager(), - RecruitEvents.groupsManager() - ); + // LevelEvent.Save fires once per dimension; gate on overworld so we only do the SavedData + // write once per autosave instead of three times (overworld+nether+end). + if (!(event.getLevel() instanceof ServerLevel serverLevel)) return; + if (serverLevel.dimension() != net.minecraft.world.level.Level.OVERWORLD) return; + saveRecruitManagersSafely(); + } + + private static void saveRecruitManagersSafely() { + MinecraftServer server = RecruitEvents.server(); + if (server == null) return; + var playerUnitManager = RecruitEvents.playerUnitManager(); + var groupsManager = RecruitEvents.groupsManager(); + if (playerUnitManager == null || groupsManager == null) return; + try { + RecruitWorldLifecycleService.saveManagers(server, playerUnitManager, groupsManager); + } catch (Throwable t) { + org.slf4j.LoggerFactory.getLogger(RecruitLifecycleEvents.class) + .error("Failed to persist recruit managers during world save", t); + } } @SubscribeEvent diff --git a/src/main/java/com/talhanation/bannermod/governance/BannerModGovernorManager.java b/src/main/java/com/talhanation/bannermod/governance/BannerModGovernorManager.java index a2347468..7b434b37 100644 --- a/src/main/java/com/talhanation/bannermod/governance/BannerModGovernorManager.java +++ b/src/main/java/com/talhanation/bannermod/governance/BannerModGovernorManager.java @@ -1,5 +1,6 @@ package com.talhanation.bannermod.governance; +import com.talhanation.bannermod.persistence.SafeSavedDataWriter; import com.talhanation.bannermod.persistence.SavedDataVersioning; import net.minecraft.core.HolderLookup; import net.minecraft.nbt.CompoundTag; @@ -40,13 +41,15 @@ public static BannerModGovernorManager load(CompoundTag tag, HolderLookup.Provid @Override public CompoundTag save(CompoundTag tag, HolderLookup.Provider registries) { - SavedDataVersioning.putVersion(tag, CURRENT_VERSION); - ListTag list = new ListTag(); - for (BannerModGovernorSnapshot snapshot : this.snapshots.values()) { - list.add(snapshot.toTag()); - } - tag.put("Snapshots", list); - return tag; + return SafeSavedDataWriter.write("BannerModGovernor", tag, registries, (out, regs) -> { + SavedDataVersioning.putVersion(out, CURRENT_VERSION); + ListTag list = new ListTag(); + // Snapshot keys first to avoid CME if a heartbeat tick mutates the map mid-save. + for (BannerModGovernorSnapshot snapshot : new java.util.ArrayList<>(this.snapshots.values())) { + list.add(snapshot.toTag()); + } + out.put("Snapshots", list); + }); } @Nullable diff --git a/src/main/java/com/talhanation/bannermod/persistence/SafeSavedDataWriter.java b/src/main/java/com/talhanation/bannermod/persistence/SafeSavedDataWriter.java new file mode 100644 index 00000000..4bf3bb5b --- /dev/null +++ b/src/main/java/com/talhanation/bannermod/persistence/SafeSavedDataWriter.java @@ -0,0 +1,38 @@ +package com.talhanation.bannermod.persistence; + +import com.mojang.logging.LogUtils; +import net.minecraft.core.HolderLookup; +import net.minecraft.nbt.CompoundTag; +import org.slf4j.Logger; + +import java.util.function.BiConsumer; + +/** + * Wrap a SavedData {@code save} body so a runtime error in our serialization path is logged + * with the failing SavedData name instead of crashing the world autosave. + * + *

Minecraft saves levels via a chain that does not catch RuntimeExceptions from SavedData + * writers — one of our writers throwing aborts the entire autosave and surfaces as a "saving + * crashed the game" report with little context about which SavedData was at fault. This helper + * keeps the original tag intact (so a transient bug never bricks the world.dat entry on disk + * with a half-written payload) and shifts the failure into the log with a clear label.

+ */ +public final class SafeSavedDataWriter { + private static final Logger LOGGER = LogUtils.getLogger(); + + private SafeSavedDataWriter() { + } + + public static CompoundTag write(String savedDataName, + CompoundTag tag, + HolderLookup.Provider registries, + BiConsumer body) { + try { + body.accept(tag, registries); + } catch (Throwable t) { + LOGGER.error("Failed to serialize {} SavedData — autosave continues with last known good payload", + savedDataName, t); + } + return tag; + } +} diff --git a/src/main/java/com/talhanation/bannermod/persistence/military/RecruitsClaimManager.java b/src/main/java/com/talhanation/bannermod/persistence/military/RecruitsClaimManager.java index 9ff1cfbe..1109c108 100644 --- a/src/main/java/com/talhanation/bannermod/persistence/military/RecruitsClaimManager.java +++ b/src/main/java/com/talhanation/bannermod/persistence/military/RecruitsClaimManager.java @@ -49,7 +49,10 @@ public void save(ServerLevel level) { } void persistClaims(RecruitsClaimSaveData data) { - data.setAllClaims(new ArrayList<>(new HashSet<>(this.claims.values()))); + // Copy values into a local ArrayList first so the dedup HashSet does not iterate the + // live map (autosave fires from the server thread while tick handlers can still mutate + // claims via removeIf/put — a CME there bubbles up into the autosave callback). + data.setAllClaims(new ArrayList<>(new HashSet<>(new ArrayList<>(this.claims.values())))); data.setDirty(); } diff --git a/src/main/java/com/talhanation/bannermod/settlement/runtime/ClaimRuntimeService.java b/src/main/java/com/talhanation/bannermod/settlement/runtime/ClaimRuntimeService.java index 249d1790..f87b8a7c 100644 --- a/src/main/java/com/talhanation/bannermod/settlement/runtime/ClaimRuntimeService.java +++ b/src/main/java/com/talhanation/bannermod/settlement/runtime/ClaimRuntimeService.java @@ -19,11 +19,30 @@ public void onServerStarting(ServerStartingEvent event) { } public void onServerStopping(ServerStoppingEvent event) { - ClaimEvents.claimManager().save(ClaimEvents.server().overworld()); + saveClaims(); } public void onWorldSave(LevelEvent.Save event) { - ClaimEvents.claimManager().save(ClaimEvents.server().overworld()); + // LevelEvent.Save fires once per dimension; only persist via the overworld save so the + // SavedData write happens once and never on a half-installed runtime (server() or + // claimManager() can be null during early init / shutdown races). + if (!(event.getLevel() instanceof ServerLevel serverLevel)) return; + if (serverLevel.dimension() != net.minecraft.world.level.Level.OVERWORLD) return; + saveClaims(); + } + + private void saveClaims() { + var server = ClaimEvents.server(); + var manager = ClaimEvents.claimManager(); + if (server == null || manager == null) return; + ServerLevel overworld = server.overworld(); + if (overworld == null) return; + try { + manager.save(overworld); + } catch (Throwable t) { + org.slf4j.LoggerFactory.getLogger(ClaimRuntimeService.class) + .error("Failed to persist RecruitsClaimManager during world save", t); + } } public void onPlayerJoin(EntityJoinLevelEvent event) { diff --git a/src/main/java/com/talhanation/bannermod/war/registry/WarPoliticalRegistrySavedData.java b/src/main/java/com/talhanation/bannermod/war/registry/WarPoliticalRegistrySavedData.java index f78eace9..05979452 100644 --- a/src/main/java/com/talhanation/bannermod/war/registry/WarPoliticalRegistrySavedData.java +++ b/src/main/java/com/talhanation/bannermod/war/registry/WarPoliticalRegistrySavedData.java @@ -1,5 +1,6 @@ package com.talhanation.bannermod.war.registry; +import com.talhanation.bannermod.persistence.SafeSavedDataWriter; import com.talhanation.bannermod.persistence.SavedDataVersioning; import com.talhanation.bannermod.war.events.WarSyncDirtyTracker; import net.minecraft.core.HolderLookup; @@ -40,10 +41,11 @@ public static WarPoliticalRegistrySavedData load(CompoundTag tag, HolderLookup.P @Override public CompoundTag save(CompoundTag tag, HolderLookup.Provider registries) { - SavedDataVersioning.putVersion(tag, CURRENT_VERSION); - CompoundTag runtimeTag = this.runtime.toTag(); - tag.put("PoliticalEntities", runtimeTag.getList("PoliticalEntities", Tag.TAG_COMPOUND)); - return tag; + return SafeSavedDataWriter.write("WarPoliticalRegistry", tag, registries, (out, regs) -> { + SavedDataVersioning.putVersion(out, CURRENT_VERSION); + CompoundTag runtimeTag = this.runtime.toTag(); + out.put("PoliticalEntities", runtimeTag.getList("PoliticalEntities", Tag.TAG_COMPOUND)); + }); } public PoliticalRegistryRuntime runtime() { diff --git a/src/main/java/com/talhanation/bannermod/war/runtime/EconomicObjectiveSavedData.java b/src/main/java/com/talhanation/bannermod/war/runtime/EconomicObjectiveSavedData.java index c27fece9..c0a75ce0 100644 --- a/src/main/java/com/talhanation/bannermod/war/runtime/EconomicObjectiveSavedData.java +++ b/src/main/java/com/talhanation/bannermod/war/runtime/EconomicObjectiveSavedData.java @@ -1,5 +1,6 @@ package com.talhanation.bannermod.war.runtime; +import com.talhanation.bannermod.persistence.SafeSavedDataWriter; import com.talhanation.bannermod.persistence.SavedDataVersioning; import com.talhanation.bannermod.war.events.WarSyncDirtyTracker; import net.minecraft.core.HolderLookup; @@ -35,10 +36,11 @@ public static EconomicObjectiveSavedData load(CompoundTag tag, HolderLookup.Prov @Override public CompoundTag save(CompoundTag tag, HolderLookup.Provider registries) { - SavedDataVersioning.putVersion(tag, CURRENT_VERSION); - CompoundTag inner = runtime.toTag(); - tag.put("EconomicObjectives", inner.getList("EconomicObjectives", Tag.TAG_COMPOUND)); - return tag; + return SafeSavedDataWriter.write("EconomicObjective", tag, registries, (out, regs) -> { + SavedDataVersioning.putVersion(out, CURRENT_VERSION); + CompoundTag inner = runtime.toTag(); + out.put("EconomicObjectives", inner.getList("EconomicObjectives", Tag.TAG_COMPOUND)); + }); } public EconomicObjectiveRuntime runtime() { diff --git a/src/main/java/com/talhanation/bannermod/war/runtime/TreatySavedData.java b/src/main/java/com/talhanation/bannermod/war/runtime/TreatySavedData.java index ff0f7b50..3789d94e 100644 --- a/src/main/java/com/talhanation/bannermod/war/runtime/TreatySavedData.java +++ b/src/main/java/com/talhanation/bannermod/war/runtime/TreatySavedData.java @@ -1,5 +1,6 @@ package com.talhanation.bannermod.war.runtime; +import com.talhanation.bannermod.persistence.SafeSavedDataWriter; import com.talhanation.bannermod.persistence.SavedDataVersioning; import net.minecraft.core.HolderLookup; import net.minecraft.nbt.CompoundTag; @@ -34,12 +35,13 @@ public static TreatySavedData load(CompoundTag tag, HolderLookup.Provider regist @Override public CompoundTag save(CompoundTag tag, HolderLookup.Provider registries) { - SavedDataVersioning.putVersion(tag, CURRENT_VERSION); - CompoundTag inner = runtime.toTag(); - tag.put("TributeTreaties", inner.getList("TributeTreaties", Tag.TAG_COMPOUND)); - tag.put("VassalRelationships", inner.getList("VassalRelationships", Tag.TAG_COMPOUND)); - tag.put("DefaultFacts", inner.getList("DefaultFacts", Tag.TAG_COMPOUND)); - return tag; + return SafeSavedDataWriter.write("Treaty", tag, registries, (out, regs) -> { + SavedDataVersioning.putVersion(out, CURRENT_VERSION); + CompoundTag inner = runtime.toTag(); + out.put("TributeTreaties", inner.getList("TributeTreaties", Tag.TAG_COMPOUND)); + out.put("VassalRelationships", inner.getList("VassalRelationships", Tag.TAG_COMPOUND)); + out.put("DefaultFacts", inner.getList("DefaultFacts", Tag.TAG_COMPOUND)); + }); } public TreatyRuntime runtime() { diff --git a/src/test/java/com/talhanation/bannermod/BannerModIntegratedRuntimeSmokeTest.java b/src/test/java/com/talhanation/bannermod/BannerModIntegratedRuntimeSmokeTest.java index 3ffd7567..216c8d68 100644 --- a/src/test/java/com/talhanation/bannermod/BannerModIntegratedRuntimeSmokeTest.java +++ b/src/test/java/com/talhanation/bannermod/BannerModIntegratedRuntimeSmokeTest.java @@ -16,7 +16,7 @@ void recruitRuntimeIdentityAndWorkerSubsystemSeamShareOneBannerModRuntime() { assertEquals(BannerModMain.MOD_ID, WorkersRuntime.modId()); assertEquals(BannerModNetworkBootstrap.workerPacketOffset(), WorkersRuntime.networkIdOffset()); assertEquals(BannerModNetworkBootstrap.MILITARY_MESSAGES.length, BannerModNetworkBootstrap.workerPacketOffset()); - assertEquals(33, BannerModNetworkBootstrap.CIVILIAN_MESSAGES.length); + assertEquals(34, BannerModNetworkBootstrap.CIVILIAN_MESSAGES.length); assertTrue(BannerModNetworkBootstrap.CIVILIAN_MESSAGES.length > 0); } }