Restore world saving, fix five server crashes, and persist dropped items

OpenHellion/IO/PersistenceJsonConverter.cs

@@ -7,6 +7,11 @@ namespace OpenHellion.IO;
 
 public class PersistenceJsonConverter : JsonConverter
 {
+	// This converter only resolves the concrete type when reading. Writing is left to the default
+	// serialiser, which stores the type in PersistenceData.__ObjectType. Serialising here would
+	// re-enter this converter through the same JsonSerializer and recurse until the stack overflows.
+	public override bool CanWrite => false;
+
 	public override bool CanConvert(Type objectType)
 	{
 		return objectType == typeof(PersistenceData) || objectType == typeof(PersistenceObjectData);
@@ -24,6 +29,6 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
 
 	public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
 	{
-		serializer.Serialize(writer, value);
+		throw new NotSupportedException("PersistenceJsonConverter is read-only.");
 	}
 }

ZeroGravity/BulletPhysics/BulletPhysicsController.cs

@@ -292,18 +292,30 @@ public bool RemoveRigidBody(SpaceObjectVessel ship)
 	{
 		try
 		{
-			lock (_dynamicsWorld)
+			// Walk up the docking tree iteratively, remembering where we have been. A vessel loaded
+			// from persistence can end up in a docking cycle (A docked to B, B docked back to A), and
+			// following it recursively overflows the stack and takes the whole server down.
+			HashSet<SpaceObjectVessel> visited = [];
+			while (ship != null && visited.Add(ship))
 			{
-				if (ship.RigidBody != null && _dynamicsWorld.CollisionObjectArray.Contains(ship.RigidBody))
+				lock (_dynamicsWorld)
+				{
+					if (ship.RigidBody != null && _dynamicsWorld.CollisionObjectArray.Contains(ship.RigidBody))
+					{
+						_dynamicsWorld.RemoveRigidBody(ship.RigidBody);
+						ship.RigidBody = null;
+						return true;
+					}
+				}
+				if (!ship.IsDocked)
 				{
-					_dynamicsWorld.RemoveRigidBody(ship.RigidBody);
-					ship.RigidBody = null;
 					return true;
 				}
+				ship = ship.DockedToMainVessel as Ship;
 			}
-			if (ship.IsDocked)
+			if (ship != null)
 			{
-				return RemoveRigidBody(ship.DockedToMainVessel as Ship);
+				Debug.LogError("Cycle in docking tree while removing rigid body", ship.Guid);
 			}
 			return true;
 		}

ZeroGravity/Objects/DynamicObject.cs

@@ -22,7 +22,10 @@
 
 	private long _MasterClientID;
 
-	private DateTime lastSenderTime;
+	// Starts counting from when the object comes into existence. Left at DateTime.MinValue, an
+	// object restored from persistence looks abandoned for two millennia and SelfDestructCheck
+	// deletes it on the first tick, before any client ever gets the chance to touch it.
+	private DateTime lastSenderTime = DateTime.UtcNow;
 
 	private DateTime takeoverTime;
 
@@ -122,7 +125,7 @@
 		dosm.Info.Stats = StatsNew;
 		if (Parent != null)
 		{
 			await NetworkController.SendToClientsSubscribedToParents(dosm, Parent, -1L);
 		}
 		StatsChanged = false;
 		LastStatsSendTime = Server.SolarSystemTime;
@@ -413,16 +416,45 @@
 				else if (message.AttachData.ParentType is SpaceObjectType.PlayerPivot or SpaceObjectType.CorpsePivot or SpaceObjectType.DynamicObjectPivot)
 				{
 					ArtificialBody refObject = GetParent<ArtificialBody>(oldParent);
+
+					// The module the item was released in, before refObject is moved up to the station
+					// it is docked into. Null means it was released in open space.
+					SpaceObjectVessel releasedInside = refObject as SpaceObjectVessel;
+
 					if (refObject is SpaceObjectVessel vessel)
 					{
 						refObject = vessel.MainVessel;
 					}
-					newParent = new Pivot(this, refObject);
-					removeFromOldParent();
-					if (message.AttachData.LocalPosition != null && message.AttachData.LocalRotation != null)
+
+					if (releasedInside != null)
 					{
-						LocalPosition = message.AttachData.LocalPosition.ToVector3D();
-						LocalRotation = message.AttachData.LocalRotation.ToQuaternionD();
+						// Let go of inside a vessel, so it stays the vessel's own, and the reply
+						// below carries that back to the client.
+						//
+						// The client says otherwise, over and over: an item resting on the floor
+						// leaves and re-enters its room trigger about once a second, because every
+						// answer re-parents it and re-parenting fires the trigger again. Believing
+						// it puts the item on a pivot of its own, and a pivot's contents are only
+						// ever described once, in the message announcing the drop - the movement
+						// message walks vessels, and a pivot is not a vessel. So the item becomes
+						// invisible to anyone who arrives afterwards, including the player who
+						// dropped it once they reconnect, and there is nothing under any vessel to
+						// write down when the world is saved.
+						//
+						// The shipped game hides all of this: items let go of in a vessel are
+						// cleaned up after five minutes and were never saved, so nobody could tell
+						// they had already stopped existing for everyone else.
+						newParent = releasedInside;
+					}
+					else
+					{
+					  newParent = new Pivot(this, refObject);
+					  removeFromOldParent();
+					  if (message.AttachData.LocalPosition != null && message.AttachData.LocalRotation != null)
+					  {
+              LocalPosition = message.AttachData.LocalPosition.ToVector3D();
+					  	LocalRotation = message.AttachData.LocalRotation.ToQuaternionD();
+            }
 					}
 				}
 				else if (message.AttachData.ParentType == SpaceObjectType.DynamicObject)

ZeroGravity/Objects/Player.cs

@@ -190,7 +190,9 @@ public bool PlayerReady
 		}
 		private set
 		{
-			if (_playerReady = value)
+			// This compared with a single '=', so the body ran on every assignment instead of only when
+			// the value actually changed, firing whatever hangs off it many times a second.
+			if (_playerReady != value)
 			{
 				_playerReady = value;
 				if (PlayerReady && EnvironmentReady)

ZeroGravity/Objects/QuestTrigger.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
@@ -24,9 +25,12 @@ public override bool Equals(object obj)
 
 		public override int GetHashCode()
 		{
-			return new object[(int)checked((nint)PlayerGUID), QuestID, ID].GetHashCode();
+			return HashCode.Combine(PlayerGUID, QuestID, ID);
 		}
 
+		// Most vessels carry no quest trigger id at all, so both sides have to tolerate null. Without
+		// this, comparing against them throws, and since the comparison happens while a player is being
+		// killed the exception escapes an async void path and takes the whole server down.
 		public static bool operator ==(QuestTriggerID x, QuestTriggerID y)
 		{
 			if (ReferenceEquals(x, y))

ZeroGravity/Objects/Ship.cs

@@ -1616,52 +1616,52 @@
 		}
 		if (data.ResourceTanks != null)
 		{
-			await Parallel.ForEachAsync(data.ResourceTanks, async (rtd, ct) =>
+			foreach (var rtd in data.ResourceTanks)
 			{
 				await DistributionManager.GetResourceContainer(new VesselObjectID(Guid, rtd.InSceneID))?.LoadPersistenceData(rtd);
-			});
+			}
 		}
 		if (data.Generators != null)
 		{
-			await Parallel.ForEachAsync(data.Generators, async (vc, ct) =>
+			foreach (var vc in data.Generators)
 			{
 				await DistributionManager.GetGenerator(new VesselObjectID(Guid, vc.InSceneID))?.LoadPersistenceData(vc);
-			});
+			}
 		}
 		if (data.SubSystems != null)
 		{
-			await Parallel.ForEachAsync(data.SubSystems, async (subSystem, ct) =>
+			foreach (var subSystem in data.SubSystems)
 			{
 				await DistributionManager.GetSubSystem(new VesselObjectID(Guid, subSystem.InSceneID))?.LoadPersistenceData(subSystem);
-			});
+			}
 		}
 		if (data.Rooms != null)
 		{
-			await Parallel.ForEachAsync(data.Rooms, async (room, ct) =>
+			foreach (var room in data.Rooms)
 			{
 				await DistributionManager.GetRoom(new VesselObjectID(Guid, room.InSceneID))?.LoadPersistenceData(room);
-			});
+			}
 		}
 		if (data.Doors != null)
 		{
-			await Parallel.ForEachAsync(data.Doors, async (door, ct) =>
+			foreach (var door in data.Doors)
 			{
 				await Doors.Find((Door x) => x.ID.InSceneID == door.InSceneID)?.LoadPersistenceData(door);
-			});
+			}
 		}
 		if (data.DockingPorts != null)
 		{
-			await Parallel.ForEachAsync(data.DockingPorts, async (dp, ct) =>
+			foreach (var dp in data.DockingPorts)
 			{
-				await DockingPorts.First((VesselDockingPort m) => m.ID.InSceneID == dp.InSceneID)?.LoadPersistenceData(dp);
-			});
+				await DockingPorts.FirstOrDefault((VesselDockingPort m) => m.ID.InSceneID == dp.InSceneID)?.LoadPersistenceData(dp);
+			}
 		}
 		if (data.Executors != null)
 		{
-			await Parallel.ForEachAsync(data.Executors, async (executor, ct) =>
+			foreach (var executor in data.Executors)
 			{
 				await SceneTriggerExecutors.Find(x => x.InSceneID == executor.InSceneID)?.LoadPersistenceData(executor);
-			});
+			}
 		}
 		if (data.NameTags != null)
 		{
@@ -1676,10 +1676,10 @@
 		}
 		if (data.RepairPoints is { Count: > 0 })
 		{
-			await Parallel.ForEachAsync(data.RepairPoints, async (rp, ct) =>
+			foreach (var rp in data.RepairPoints)
 			{
 				await RepairPoints.Find((VesselRepairPoint x) => x.ID.InSceneID == rp.InSceneID)?.LoadPersistenceData(rp);
-			});
+			}
 		}
 		await MainDistributionManager.UpdateSystems();
 		if (data.OrbitData != null)
@@ -1735,6 +1735,43 @@
 		}
 	}
 
+	/// <summary>
+	/// 	Restores docking and stabilisation, which both refer to other vessels by GUID.
+	/// 	Must run after every vessel has been created, and one vessel at a time: docking rewrites
+	/// 	the shared docked-vessels tree, so doing it concurrently corrupts that tree into cycles.
+	/// </summary>
+	public async Task LoadDockingPersistenceData(PersistenceObjectData persistenceData)
+	{
+		PersistenceObjectDataShip data = persistenceData as PersistenceObjectDataShip;
+		if (data.DockedToShipGUID.HasValue)
+		{
+			if (Server.Instance.GetVessel(data.DockedToShipGUID.Value) is not Ship dockToShip)
+			{
+				Debug.LogError("Could not find vessel to dock to", Guid, data.DockedToShipGUID.Value);
+			}
+			else
+			{
+				VesselDockingPort myPort = DockingPorts.FirstOrDefault((VesselDockingPort m) => m.ID.InSceneID == data.DockedPortID.Value);
+				VesselDockingPort dockedToPort = dockToShip.DockingPorts.FirstOrDefault((VesselDockingPort m) => m.ID.InSceneID == data.DockedToPortID.Value);
+				if (myPort == null || dockedToPort == null)
+				{
+					Debug.LogError("Could not find docking port", Guid, data.DockedPortID, data.DockedToPortID);
+				}
+				else
+				{
+					await DockToVessel(myPort, dockedToPort, dockToShip, disableStabilization: false, useCurrentSolarSystemTime: true, buildingStation: true);
+				}
+			}
+		}
+		if (data.StabilizeToTargetGUID.HasValue)
+		{
+			SpaceObjectVessel ab = Server.Instance.GetObject(data.StabilizeToTargetGUID.Value) as SpaceObjectVessel;
+			StabilizeToTarget(ab, forceStabilize: true);
+			StabilizeToTargetRelPosition = data.StabilizeToTargetPosition.ToVector3D();
+			await UpdateStabilization();
+		}
+	}
+
 	public async void VesselRequestListener(NetworkData data)
 	{
 		var request = data as VesselRequest;

ZeroGravity/Persistence.cs

@@ -99,7 +99,7 @@ public static void Save(string filename = null, SaveFileAuxData auxData = null)
 			Players = new HashSet<PersistenceObjectData>(),
 			RespawnObjects = new HashSet<PersistenceObjectData>(),
 			SpawnPoints = new HashSet<PersistenceObjectData>(),
-			ArenaControllers = new HashSet<PersistenceObjectData>()
+			ArenaControllers = new HashSet<PersistenceObjectData>(),
 		};
 
 		foreach (SpaceObjectVessel ves in Server.Instance.AllVessels)
@@ -139,6 +139,7 @@ public static void Save(string filename = null, SaveFileAuxData auxData = null)
 		{
 			per.ArenaControllers.Add(dmac.GetPersistenceData());
 		}
+
 		per.DoomControllerData = Server.Instance.DoomedShipController.GetPersistenceData();
 		per.SpawnManagerData = SpawnManager.GetPersistenceData();
 		DirectoryInfo d = new DirectoryInfo(Path.Combine(Path.GetDirectoryName(Assembly.GetEntryAssembly().Location), Server.ConfigDir));
@@ -157,9 +158,17 @@ public static void Save(string filename = null, SaveFileAuxData auxData = null)
 			filename = string.Format(PersistanceFileName, DateTime.UtcNow.ToString("yyyy-MM-dd-HH-mm-ss"));
 		}
 
-		// TODO: Fix saving
-		// JsonSerialiser.SerializeToFile(per, Path.Combine(Server.ConfigDir, filename), JsonSerialiser.Formatting.None);
-		Debug.Log("Saved world...");
+		string savePath = Path.Combine(d.FullName, filename);
+		try
+		{
+			JsonSerialiser.SerializeToFile(per, savePath, JsonSerialiser.Formatting.None);
+		  Debug.Log("Saved world...");
+		}
+		catch (Exception ex)
+		{
+			Debug.LogError("Failed to save world", savePath, ex.Message);
+			return;
+		}
 	}
 
 	private static void LoadRespawnObjectPersistence(PersistenceObjectDataRespawnObject data)
@@ -207,6 +216,12 @@ private static void LoadRespawnObjectPersistence(PersistenceObjectDataRespawnObj
 		});
 	}
 
+	/// <summary>
+	/// 	Puts an item that a player left lying in a vessel back into that vessel, at the place it was
+	/// 	left. It comes back as an ordinary loose item in a room rather than on a pivot, which is what
+	/// 	keeps it saved, shown to clients that come near, and out of reach of the pivot cleanup timer.
+	/// </summary>
+
 	private static void LoadSpawnPointPeristence(PersistenceObjectDataSpawnPoint data)
 	{
 		try
@@ -244,11 +259,13 @@ public static async Task<bool> Load(string filename = null)
 			Server.Instance.SolarSystem.CalculatePositionsAfterTime(persistence.SolarSystemTime);
 			if (persistence.Asteroids != null)
 			{
-				await Parallel.ForEachAsync(persistence.Asteroids, async (asteroidData, ct) =>
+				// Loading is deliberately sequential: the game code it drives mutates shared state
+				// (rooms, docking trees, air consumers) through plain List<T>, which is not thread safe.
+				foreach (var asteroidData in persistence.Asteroids)
 				{
 					Asteroid ast = new Asteroid(asteroidData.GUID, initializeOrbit: false, Vector3D.Zero, Vector3D.One, QuaternionD.Identity);
 					await ast.LoadPersistenceData(asteroidData);
-				});
+				}
 			}
 			if (persistence.Ships != null)
 			{
@@ -272,12 +289,12 @@ await Parallel.ForEachAsync(persistence.Ships, async (shipData, ct) =>
 			}
 			if (persistence.Players != null)
 			{
-				await Parallel.ForEachAsync(persistence.Players, async (data, ct) =>
+				foreach (var data in persistence.Players)
 				{
 					var playerData = data as PersistenceObjectDataPlayer;
 					Player player = await Player.CreatePlayerAsync(playerData.GUID, Vector3D.Zero, QuaternionD.Identity, "PersistenceLoad", "", playerData.Gender, playerData.HeadType, playerData.HairType, addToServerList: false);
 					await player.LoadPersistenceData(playerData);
-				});
+				}
 			}
 			if (persistence.RespawnObjects != null)
 			{
@@ -295,12 +312,12 @@ await Parallel.ForEachAsync(persistence.Players, async (data, ct) =>
 			}
 			if (persistence.ArenaControllers != null)
 			{
-				await Parallel.ForEachAsync(persistence.ArenaControllers, async (data, ct) =>
+				foreach (var data in persistence.ArenaControllers)
 				{
 					var arenaControllerData = data as PersistenceArenaControllerData;
 					DeathMatchArenaController arenaController = new DeathMatchArenaController();
 					await arenaController.LoadPersistenceData(arenaControllerData);
-				});
+				}
 			}
 			if (persistence.DoomControllerData != null)
 			{
@@ -408,6 +425,7 @@ public struct PersistenceObject
 
 	public HashSet<PersistenceObjectData> ArenaControllers;
 
+
 	public PersistenceObjectData DoomControllerData;
 
 	public PersistenceObjectData SpawnManagerData;