Fix ghost respawn bug (#17511)

Content.IntegrationTests/Tests/Minds/MindTests.Helpers.cs

@@ -18,6 +18,38 @@ namespace Content.IntegrationTests.Tests.Minds;
 [TestFixture]
 public sealed partial class MindTests
 {
+    /// <summary>
+    ///     Gets a server-client pair and ensures that the client is attached to a simple mind test entity.
+    /// </summary>
+    public async Task<PairTracker> SetupPair()
+    {
+        var pairTracker = await PoolManager.GetServerClient(new PoolSettings{ ExtraPrototypes = Prototypes });
+        var pair = pairTracker.Pair;
+
+        var entMan = pair.Server.ResolveDependency<IServerEntityManager>();
+        var playerMan = pair.Server.ResolveDependency<IPlayerManager>();
+        var mindSys = entMan.System<MindSystem>();
+
+        var player = playerMan.ServerSessions.Single();
+
+        EntityUid entity = default;
+        await pair.Server.WaitPost(() =>
+        {
+            entity = entMan.SpawnEntity("MindTestEntity", MapCoordinates.Nullspace);
+            mindSys.TransferTo(mindSys.CreateMind(player.UserId), entity);
+        });
+
+        await PoolManager.RunTicksSync(pair, 5);
+
+        var mind = player.ContentData()?.Mind;
+        Assert.NotNull(mind);
+        Assert.That(player.AttachedEntity, Is.EqualTo(entity));
+        Assert.That(player.AttachedEntity, Is.EqualTo(mind.CurrentEntity), "Player is not attached to the mind's current entity.");
+        Assert.That(entMan.EntityExists(mind.OwnedEntity), "The mind's current entity does not exist");
+        Assert.That(mind.VisitingEntity == null || entMan.EntityExists(mind.VisitingEntity), "The minds visited entity does not exist.");
+        return pairTracker;
+    }
+
     public async Task<EntityUid> BecomeGhost(Pair pair, bool visit = false)
     {
         var entMan = pair.Server.ResolveDependency<IServerEntityManager>();
@@ -74,9 +106,9 @@ public sealed partial class MindTests
 
         var mind = player.ContentData()!.Mind;
         Assert.NotNull(mind);
-        Assert.That(player.AttachedEntity, Is.EqualTo(mind.CurrentEntity));
+        Assert.That(player.AttachedEntity, Is.EqualTo(mind.CurrentEntity), "Player is not attached to the mind's current entity.");
-        Assert.That(entMan.EntityExists(mind.OwnedEntity));
+        Assert.That(entMan.EntityExists(mind.OwnedEntity), "The mind's current entity does not exist");
-        Assert.That(entMan.EntityExists(mind.CurrentEntity));
+        Assert.That(mind.VisitingEntity == null || entMan.EntityExists(mind.VisitingEntity), "The minds visited entity does not exist.");
 
         return mind;
     }

Content.IntegrationTests/Tests/Minds/MindTests.ReconnectTests.cs

@@ -20,21 +20,19 @@ public sealed partial class MindTests
     [Test]
     public async Task TestGhostsCanReconnect()
     {
-        await using var pairTracker = await PoolManager.GetServerClient();
+        await using var pairTracker = await SetupPair();
         var pair = pairTracker.Pair;
-
         var entMan = pair.Server.ResolveDependency<IEntityManager>();
-        await PoolManager.RunTicksSync(pair, 5);
         var mind = GetMind(pair);
 
         var ghost = await BecomeGhost(pair);
         await DisconnectReconnect(pair);
 
-        // Player in control of a NEW entity
+        // Player in control of a new ghost, but with the same mind
-        var newMind = GetMind(pair);
+        Assert.That(GetMind(pair) == mind);
-        Assert.That(newMind != mind);
         Assert.That(entMan.Deleted(ghost));
-        Assert.Null(newMind.VisitingEntity);
+        Assert.That(entMan.HasComponent<GhostComponent>(mind.OwnedEntity));
+        Assert.Null(mind.VisitingEntity);
 
         await pairTracker.CleanReturnAsync();
     }
@@ -47,11 +45,9 @@ public sealed partial class MindTests
     [Test]
     public async Task TestDeletedCanReconnect()
     {
-        await using var pairTracker = await PoolManager.GetServerClient();
+        await using var pairTracker = await SetupPair();
         var pair = pairTracker.Pair;
-
         var entMan = pair.Server.ResolveDependency<IEntityManager>();
-        await PoolManager.RunTicksSync(pair, 5);
         var mind = GetMind(pair);
 
         var playerMan = pair.Server.ResolveDependency<IPlayerManager>();
@@ -76,15 +72,12 @@ public sealed partial class MindTests
         // Reconnect
         await Connect(pair, name);
         player = playerMan.ServerSessions.Single();
-        Assert.That(user == player.UserId);
+        Assert.That(user, Is.EqualTo(player.UserId));
 
-        // Player is now a new entity
+        // Player is now a new ghost entity
-        var newMind = GetMind(pair);
+        Assert.That(GetMind(pair), Is.EqualTo(mind));
-        Assert.That(newMind != mind);
+        Assert.That(mind.OwnedEntity, Is.Not.EqualTo(entity));
-        Assert.Null(mind.UserId);
+        Assert.That(entMan.HasComponent<GhostComponent>(mind.OwnedEntity));
-        Assert.Null(mind.CurrentEntity);
-        Assert.NotNull(newMind.OwnedEntity);
-        Assert.That(entMan.EntityExists(newMind.OwnedEntity));
 
         await pairTracker.CleanReturnAsync();
     }
@@ -97,11 +90,10 @@ public sealed partial class MindTests
     [Test]
     public async Task TestVisitingGhostReconnect()
     {
-        await using var pairTracker = await PoolManager.GetServerClient();
+        await using var pairTracker = await SetupPair();
         var pair = pairTracker.Pair;
 
         var entMan = pair.Server.ResolveDependency<IEntityManager>();
-        await PoolManager.RunTicksSync(pair, 5);
         var mind = GetMind(pair);
 
         var original = mind.CurrentEntity;
@@ -109,8 +101,8 @@ public sealed partial class MindTests
         await DisconnectReconnect(pair);
 
         // Player now controls their original mob, mind was preserved
-        Assert.That(mind == GetMind(pair));
+        Assert.That(mind, Is.EqualTo(GetMind(pair)));
-        Assert.That(mind.CurrentEntity == original);
+        Assert.That(mind.CurrentEntity, Is.EqualTo(original));
         Assert.That(!entMan.Deleted(original));
         Assert.That(entMan.Deleted(ghost));
 
@@ -125,7 +117,7 @@ public sealed partial class MindTests
     [Test]
     public async Task TestVisitingReconnect()
     {
-        await using var pairTracker = await PoolManager.GetServerClient(new PoolSettings{ ExtraPrototypes = Prototypes });
+        await using var pairTracker = await SetupPair();
         var pair = pairTracker.Pair;
 
         var entMan = pair.Server.ResolveDependency<IEntityManager>();

Content.IntegrationTests/Tests/Minds/MindTests.cs

@@ -61,26 +61,6 @@ public sealed partial class MindTests
         - !type:GibBehavior { }
 ";
 
-    /// <summary>
-    ///     Exception handling for PlayerData and NetUserId invalid due to testing.
-    ///     Can be removed when Players can be mocked.
-    /// </summary>
-    /// <param name="func"></param>
-    private void CatchPlayerDataException(Action func)
-    {
-        try
-        {
-            func();
-        }
-        catch (ArgumentException e)
-        {
-            // Prevent exiting due to PlayerData not being initialized.
-            if (e.Message == "New owner must have previously logged into the server. (Parameter 'newOwner')")
-                return;
-            throw;
-        }
-    }
-
     [Test]
     public async Task TestCreateAndTransferMindToNewEntity()
     {
@@ -382,11 +362,11 @@ public sealed partial class MindTests
         await pairTracker.CleanReturnAsync();
     }
 
-    [Test]
+    // TODO Implement
+    /*[Test]
     public async Task TestPlayerCanReturnFromGhostWhenDead()
     {
-        // TODO Implement
+    }*/
-    }
 
     [Test]
     public async Task TestGhostDoesNotInfiniteLoop()

Content.Server/GameTicking/Commands/ObserveCommand.cs

@@ -30,7 +30,7 @@ namespace Content.Server.GameTicking.Commands
             if (ticker.PlayerGameStatuses.TryGetValue(player.UserId, out var status) &&
                 status != PlayerGameStatus.JoinedGame)
             {
-                ticker.MakeObserve(player);
+                ticker.JoinAsObserver(player);
             }
             else
             {

Content.Server/GameTicking/Commands/RespawnCommand.cs

@@ -22,7 +22,7 @@ namespace Content.Server.GameTicking.Commands
             }
 
             var playerMgr = IoCManager.Resolve<IPlayerManager>();
-            var sysMan = IoCManager.Resolve<EntitySystemManager>();
+            var sysMan = IoCManager.Resolve<IEntitySystemManager>();
             var ticker = sysMan.GetEntitySystem<GameTicker>();
             var mind = sysMan.GetEntitySystem<MindSystem>();
 

Content.Server/GameTicking/GameTicker.GamePreset.cs

@@ -3,13 +3,11 @@ using System.Linq;
 using System.Threading.Tasks;
 using Content.Server.GameTicking.Presets;
 using Content.Server.Ghost.Components;
-using Content.Server.Mind;
 using Content.Shared.CCVar;
 using Content.Shared.Damage;
 using Content.Shared.Damage.Prototypes;
 using Content.Shared.Database;
 using Content.Shared.Mobs.Components;
-using Content.Shared.Mobs.Systems;
 using JetBrains.Annotations;
 using Robust.Server.Player;
 
@@ -19,9 +17,6 @@ namespace Content.Server.GameTicking
     {
         public const float PresetFailedCooldownIncrease = 30f;
 
-        [Dependency] private readonly MobStateSystem _mobStateSystem = default!;
-        [Dependency] private readonly MindSystem _mindSystem = default!;
-
         public GamePresetPrototype? Preset { get; private set; }
 
         private bool StartPreset(IPlayerSession[] origReadyPlayers, bool force)
@@ -190,7 +185,7 @@ namespace Content.Server.GameTicking
 
             if (mind.VisitingEntity != default)
             {
-                _mindSystem.UnVisit(mind);
+                _mind.UnVisit(mind);
             }
 
             var position = Exists(playerEntity)
@@ -208,11 +203,11 @@ namespace Content.Server.GameTicking
             // + If we're in a mob that is critical, and we're supposed to be able to return if possible,
             //   we're succumbing - the mob is killed. Therefore, character is dead. Ghosting OK.
             //   (If the mob survives, that's a bug. Ghosting is kept regardless.)
-            var canReturn = canReturnGlobal && _mindSystem.IsCharacterDeadPhysically(mind);
+            var canReturn = canReturnGlobal && _mind.IsCharacterDeadPhysically(mind);
 
             if (canReturnGlobal && TryComp(playerEntity, out MobStateComponent? mobState))
             {
-                if (_mobStateSystem.IsCritical(playerEntity.Value, mobState))
+                if (_mobState.IsCritical(playerEntity.Value, mobState))
                 {
                     canReturn = true;
 
@@ -250,9 +245,9 @@ namespace Content.Server.GameTicking
             _ghosts.SetCanReturnToBody(ghostComponent, canReturn);
 
             if (canReturn)
-                _mindSystem.Visit(mind, ghost);
+                _mind.Visit(mind, ghost);
             else
-                _mindSystem.TransferTo(mind, ghost);
+                _mind.TransferTo(mind, ghost);
             return true;
         }
 

Content.Server/GameTicking/GameTicker.Player.cs

@@ -27,15 +27,16 @@ namespace Content.Server.GameTicking
         {
             var session = args.Session;
 
-            if (_mindSystem.TryGetMind(session.UserId, out var mind))
+            if (_mind.TryGetMind(session.UserId, out var mind))
             {
                 if (args.OldStatus == SessionStatus.Connecting && args.NewStatus == SessionStatus.Connected)
                     mind.Session = session;
 
                 DebugTools.Assert(mind.Session == session);
-                DebugTools.Assert(session.Data.ContentData()?.Mind is not {} dataMind || dataMind == mind);
             }
 
+            DebugTools.Assert(session.GetMind() == mind);
+
             switch (args.NewStatus)
             {
                 case SessionStatus.Connected:
@@ -75,32 +76,32 @@ namespace Content.Server.GameTicking
                 {
                     _userDb.ClientConnected(session);
 
-                    var data = session.ContentData();
+                    if (mind == null)
-
-                    DebugTools.AssertNotNull(data);
-
-                    if (data!.Mind == null)
                     {
                         if (LobbyEnabled)
-                        {
                             PlayerJoinLobby(session);
-                            return;
+                        else
-                        }
+                            SpawnWaitDb();
 
-                        SpawnWaitDb();
                         break;
                     }
 
-                    if (data.Mind.CurrentEntity == null || Deleted(data.Mind.CurrentEntity))
+                    if (mind.CurrentEntity == null || Deleted(mind.CurrentEntity))
                     {
-                        DebugTools.Assert(data.Mind.CurrentEntity == null, "a mind's current entity has been deleted");
+                        DebugTools.Assert(mind.CurrentEntity == null, "a mind's current entity was deleted without updating the mind");
-                        SpawnWaitDb();
+
+                        // This player is joining the game with an existing mind, but the mind has no entity.
+                        // Their entity was probably deleted sometime while they were disconnected, or they were an observer.
+                        // Instead of allowing them to spawn in, we will dump and their existing mind in an observer ghost.
+                        SpawnObserverWaitDb();
                     }
                     else
                     {
-                        session.AttachToEntity(data.Mind.CurrentEntity);
+                        // Simply re-attach to existing entity.
+                        session.AttachToEntity(mind.CurrentEntity);
                         PlayerJoinGame(session);
                     }
+
                     break;
                 }
 
@@ -123,6 +124,12 @@ namespace Content.Server.GameTicking
                 SpawnPlayer(session, EntityUid.Invalid);
             }
 
+            async void SpawnObserverWaitDb()
+            {
+                await _userDb.WaitLoadComplete(session);
+                JoinAsObserver(session);
+            }
+
             async void AddPlayerToDb(Guid id)
             {
                 if (RoundId != 0 && _runLevel != GameRunLevel.PreRoundLobby)

Content.Server/GameTicking/GameTicker.Spawning.cs

@@ -1,15 +1,12 @@
 using System.Globalization;
 using System.Linq;
 using Content.Server.Ghost;
-using Content.Server.Ghost.Components;
 using Content.Server.Players;
-using Content.Server.Shuttles.Systems;
 using Content.Server.Spawners.Components;
 using Content.Server.Speech.Components;
 using Content.Server.Station.Components;
 using Content.Shared.Database;
 using Content.Shared.GameTicking;
-using Content.Shared.Ghost;
 using Content.Shared.Preferences;
 using Content.Shared.Roles;
 using JetBrains.Annotations;
@@ -131,7 +128,7 @@ namespace Content.Server.GameTicking
 
             if (lateJoin && DisallowLateJoin)
             {
-                MakeObserve(player);
+                JoinAsObserver(player);
                 return;
             }
 
@@ -163,7 +160,7 @@ namespace Content.Server.GameTicking
             {
                 if (!LobbyEnabled)
                 {
-                    MakeObserve(player);
+                    JoinAsObserver(player);
                 }
                 _chatManager.DispatchServerMessage(player, Loc.GetString("game-ticker-player-no-jobs-available-when-joining"));
                 return;
@@ -175,12 +172,12 @@ namespace Content.Server.GameTicking
 
             DebugTools.AssertNotNull(data);
 
-            var newMind = _mindSystem.CreateMind(data!.UserId, character.Name);
+            var newMind = _mind.CreateMind(data!.UserId, character.Name);
-            _mindSystem.SetUserId(newMind, data.UserId);
+            _mind.SetUserId(newMind, data.UserId);
 
             var jobPrototype = _prototypeManager.Index<JobPrototype>(jobId);
             var job = new Job(newMind, jobPrototype);
-            _mindSystem.AddRole(newMind, job);
+            _mind.AddRole(newMind, job);
 
             _playTimeTrackings.PlayerRolesChanged(player);
 
@@ -189,7 +186,7 @@ namespace Content.Server.GameTicking
             DebugTools.AssertNotNull(mobMaybe);
             var mob = mobMaybe!.Value;
 
-            _mindSystem.TransferTo(newMind, mob);
+            _mind.TransferTo(newMind, mob);
 
             if (lateJoin)
             {
@@ -243,7 +240,7 @@ namespace Content.Server.GameTicking
 
         public void Respawn(IPlayerSession player)
         {
-            _mindSystem.WipeMind(player);
+            _mind.WipeMind(player);
             _adminLogger.Add(LogType.Respawn, LogImpact.Medium, $"Player {player} was respawned.");
 
             if (LobbyEnabled)
@@ -263,32 +260,42 @@ namespace Content.Server.GameTicking
             SpawnPlayer(player, station, jobId);
         }
 
-        public void MakeObserve(IPlayerSession player)
+        /// <summary>
+        /// Causes the given player to join the current game as observer ghost. See also <see cref="SpawnObserver"/>
+        /// </summary>
+        public void JoinAsObserver(IPlayerSession player)
         {
             // Can't spawn players with a dummy ticker!
             if (DummyTicker)
                 return;
 
             PlayerJoinGame(player);
+            SpawnObserver(player);
+            RaiseNetworkEvent(GetStatusSingle(player, PlayerGameStatus.JoinedGame));
+        }
+
+        /// <summary>
+        /// Spawns an observer ghost and attaches the given player to it. If the player does not yet have a mind, the
+        /// player is given a new mind with the observer role. Otherwise, the current mind is transferred to the ghost.
+        /// </summary>
+        public void SpawnObserver(IPlayerSession player)
+        {
+            if (DummyTicker)
+                return;
+
+            var mind = player.GetMind();
+            if (mind == null)
+            {
+                mind = _mind.CreateMind(player.UserId);
+                _mind.SetUserId(mind, player.UserId);
+                _mind.AddRole(mind, new ObserverRole(mind));
+            }
 
             var name = GetPlayerProfile(player).Name;
-
+            var ghost = SpawnObserverMob();
-            var data = player.ContentData();
+            MetaData(ghost).EntityName = name;
-
+            _ghost.SetCanReturnToBody(ghost, false);
-            DebugTools.AssertNotNull(data);
+            _mind.TransferTo(mind, ghost);
-
-            var newMind = _mindSystem.CreateMind(data!.UserId);
-            _mindSystem.SetUserId(newMind, data.UserId);
-            _mindSystem.AddRole(newMind, new ObserverRole(newMind));
-
-            var mob = SpawnObserverMob();
-            EntityManager.GetComponent<MetaDataComponent>(mob).EntityName = name;
-            var ghost = EntityManager.GetComponent<GhostComponent>(mob);
-            EntitySystem.Get<SharedGhostSystem>().SetCanReturnToBody(ghost, false);
-            _mindSystem.TransferTo(newMind, mob);
-
-            _playerGameStatuses[player.UserId] = PlayerGameStatus.JoinedGame;
-            RaiseNetworkEvent(GetStatusSingle(player, PlayerGameStatus.JoinedGame));
         }
 
         #region Mob Spawning Helpers

Content.Server/GameTicking/GameTicker.cs

@@ -5,6 +5,7 @@ using Content.Server.Chat.Systems;
 using Content.Server.Database;
 using Content.Server.Ghost;
 using Content.Server.Maps;
+using Content.Server.Mind;
 using Content.Server.Players.PlayTimeTracking;
 using Content.Server.Preferences.Managers;
 using Content.Server.ServerUpdates;
@@ -13,6 +14,8 @@ using Content.Server.Station.Systems;
 using Content.Shared.Chat;
 using Content.Shared.Damage;
 using Content.Shared.GameTicking;
+using Content.Shared.Ghost;
+using Content.Shared.Mobs.Systems;
 using Content.Shared.Roles;
 using Robust.Server;
 using Robust.Server.GameObjects;
@@ -34,6 +37,9 @@ namespace Content.Server.GameTicking
         [Dependency] private readonly ArrivalsSystem _arrivals = default!;
         [Dependency] private readonly MapLoaderSystem _map = default!;
         [Dependency] private readonly SharedTransformSystem _transform = default!;
+        [Dependency] private readonly GhostSystem _ghost = default!;
+        [Dependency] private readonly MindSystem _mind = default!;
+        [Dependency] private readonly MobStateSystem _mobState = default!;
 
         [ViewVariables] private bool _initialized;
         [ViewVariables] private bool _postInitialized;

Content.Server/Players/PlayerData.cs

@@ -61,5 +61,13 @@ namespace Content.Server.Players
         {
             return session.Data.ContentData();
         }
+
+        /// <summary>
+        ///     Gets the mind that is associated with this player.
+        /// </summary>
+        public static Mind.Mind? GetMind(this IPlayerSession session)
+        {
+            return session.Data.ContentData()?.Mind;
+        }
     }
 }

Content.Shared/Ghost/SharedGhostSystem.cs

@@ -29,6 +29,14 @@ namespace Content.Shared.Ghost
                 args.Cancel();
         }
 
+        public void SetCanReturnToBody(EntityUid uid, bool value, SharedGhostComponent? component = null)
+        {
+            if (!Resolve(uid, ref component))
+                return;
+
+            component.CanReturnToBody = value;
+        }
+
         public void SetCanReturnToBody(SharedGhostComponent component, bool value)
         {
             component.CanReturnToBody = value;