Fix erase verb not removing chat messages in some cases (#21355)

* Fix erase verb not removing chat messages in some cases

* Admin changelog

* Fix deleting messages with entity id 0

Content.Server/Chat/ChatUser.cs

@@ -0,0 +1,34 @@
+using Content.Shared.Chat;
+
+namespace Content.Server.Chat;
+
+public sealed class ChatUser
+{
+    /// <summary>
+    ///     The unique key associated with this chat user, starting from 1 and incremented.
+    ///     Used when the server sends <see cref="MsgChatMessage"/>.
+    ///     Used on the client to delete messages sent by this user when receiving
+    ///     <see cref="MsgDeleteChatMessagesBy"/>.
+    /// </summary>
+    public readonly int Key;
+
+    /// <summary>
+    ///     All entities that this chat user was attached to while sending chat messages.
+    ///     Sent to the client to delete messages sent by those entities when receiving
+    ///     <see cref="MsgDeleteChatMessagesBy"/>.
+    /// </summary>
+    public readonly HashSet<NetEntity> Entities = new();
+
+    public ChatUser(int key)
+    {
+        Key = key;
+    }
+
+    public void AddEntity(NetEntity entity)
+    {
+        if (!entity.Valid)
+            return;
+
+        Entities.Add(entity);
+    }
+}

Content.Server/Chat/Managers/ChatManager.cs

@@ -1,3 +1,4 @@
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Runtime.InteropServices;
 using Content.Server.Administration.Logs;
@@ -49,8 +50,7 @@ namespace Content.Server.Chat.Managers
         private bool _oocEnabled = true;
         private bool _adminOocEnabled = true;
 
-        public Dictionary<ICommonSession, int> SenderKeys { get; } = new();
+        private readonly Dictionary<NetUserId, ChatUser> _players = new();
-        public Dictionary<ICommonSession, HashSet<NetEntity>> SenderEntities { get; } = new();
 
         public void Initialize()
         {
@@ -79,13 +79,26 @@ namespace Content.Server.Chat.Managers
 
         public void DeleteMessagesBy(ICommonSession player)
         {
-            var key = SenderKeys.GetValueOrDefault(player);
+            if (!_players.TryGetValue(player.UserId, out var user))
-            var entities = SenderEntities.GetValueOrDefault(player) ?? new HashSet<NetEntity>();
+                return;
-            var msg = new MsgDeleteChatMessagesBy { Key = key, Entities = entities };
 
+            var msg = new MsgDeleteChatMessagesBy { Key = user.Key, Entities = user.Entities };
             _netManager.ServerSendToAll(msg);
         }
 
+        [return: NotNullIfNotNull(nameof(author))]
+        public ChatUser? EnsurePlayer(NetUserId? author)
+        {
+            if (author == null)
+                return null;
+
+            ref var user = ref CollectionsMarshal.GetValueRefOrAddDefault(_players, author.Value, out var exists);
+            if (!exists || user == null)
+                user = new ChatUser(_players.Count);
+
+            return user;
+        }
+
         #region Server Announcements
 
         public void DispatchServerAnnouncement(string message, Color? colorOverride = null)
@@ -214,12 +227,8 @@ namespace Content.Server.Chat.Managers
                 wrappedMessage = Loc.GetString("chat-manager-send-ooc-patron-wrap-message", ("patronColor", patronColor),("playerName", player.Name), ("message", FormattedMessage.EscapeText(message)));
             }
 
-            ref var key = ref CollectionsMarshal.GetValueRefOrAddDefault(SenderKeys, player, out var exists);
-            if (!exists)
-                key = SenderKeys.Count;
-
             //TODO: player.Name color, this will need to change the structure of the MsgChatMessage
-            ChatMessageToAll(ChatChannel.OOC, message, wrappedMessage, EntityUid.Invalid, hideChat: false, recordReplay: true, colorOverride: colorOverride, senderKey: key);
+            ChatMessageToAll(ChatChannel.OOC, message, wrappedMessage, EntityUid.Invalid, hideChat: false, recordReplay: true, colorOverride: colorOverride, author: player.UserId);
             _mommiLink.SendOOCMessage(player.Name, message);
             _adminLogger.Add(LogType.Chat, LogImpact.Low, $"OOC from {player:Player}: {message}");
         }
@@ -237,10 +246,6 @@ namespace Content.Server.Chat.Managers
                                             ("adminChannelName", Loc.GetString("chat-manager-admin-channel-name")),
                                             ("playerName", player.Name), ("message", FormattedMessage.EscapeText(message)));
 
-            ref var key = ref CollectionsMarshal.GetValueRefOrAddDefault(SenderKeys, player, out var exists);
-            if (!exists)
-                key = SenderKeys.Count;
-
             foreach (var client in clients)
             {
                 var isSource = client != player.ConnectedClient;
@@ -251,7 +256,8 @@ namespace Content.Server.Chat.Managers
                     false,
                     client,
                     audioPath: isSource ? _netConfigManager.GetClientCVar(client, CCVars.AdminChatSoundPath) : default,
-                    audioVolume: isSource ? _netConfigManager.GetClientCVar(client, CCVars.AdminChatSoundVolume) : default, senderKey: key);
+                    audioVolume: isSource ? _netConfigManager.GetClientCVar(client, CCVars.AdminChatSoundVolume) : default,
+                    author: player.UserId);
             }
 
             _adminLogger.Add(LogType.Chat, $"Admin chat from {player:Player}: {message}");
@@ -261,9 +267,13 @@ namespace Content.Server.Chat.Managers
 
         #region Utility
 
-        public void ChatMessageToOne(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, INetChannel client, Color? colorOverride = null, bool recordReplay = false, string? audioPath = null, float audioVolume = 0, int? senderKey = null)
+        public void ChatMessageToOne(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, INetChannel client, Color? colorOverride = null, bool recordReplay = false, string? audioPath = null, float audioVolume = 0, NetUserId? author = null)
         {
-            var msg = new ChatMessage(channel, message, wrappedMessage, _entityManager.GetNetEntity(source), senderKey, hideChat, colorOverride, audioPath, audioVolume);
+            var user = author == null ? null : EnsurePlayer(author);
+            var netSource = _entityManager.GetNetEntity(source);
+            user?.AddEntity(netSource);
+
+            var msg = new ChatMessage(channel, message, wrappedMessage, netSource, user?.Key, hideChat, colorOverride, audioPath, audioVolume);
             _netManager.ServerSendMessage(new MsgChatMessage() { Message = msg }, client);
 
             if (!recordReplay)
@@ -276,12 +286,16 @@ namespace Content.Server.Chat.Managers
             }
         }
 
-        public void ChatMessageToMany(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, IEnumerable<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0)
+        public void ChatMessageToMany(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, IEnumerable<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, NetUserId? author = null)
-            => ChatMessageToMany(channel, message, wrappedMessage, source, hideChat, recordReplay, clients.ToList(), colorOverride, audioPath, audioVolume);
+            => ChatMessageToMany(channel, message, wrappedMessage, source, hideChat, recordReplay, clients.ToList(), colorOverride, audioPath, audioVolume, author);
 
-        public void ChatMessageToMany(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, List<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0)
+        public void ChatMessageToMany(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, List<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, NetUserId? author = null)
         {
-            var msg = new ChatMessage(channel, message, wrappedMessage, _entityManager.GetNetEntity(source), null, hideChat, colorOverride, audioPath, audioVolume);
+            var user = author == null ? null : EnsurePlayer(author);
+            var netSource = _entityManager.GetNetEntity(source);
+            user?.AddEntity(netSource);
+
+            var msg = new ChatMessage(channel, message, wrappedMessage, netSource, user?.Key, hideChat, colorOverride, audioPath, audioVolume);
             _netManager.ServerSendToMany(new MsgChatMessage() { Message = msg }, clients);
 
             if (!recordReplay)
@@ -309,9 +323,13 @@ namespace Content.Server.Chat.Managers
             ChatMessageToMany(channel, message, wrappedMessage, source, hideChat, recordReplay, clients, colorOverride, audioPath, audioVolume);
         }
 
-        public void ChatMessageToAll(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, int? senderKey = null)
+        public void ChatMessageToAll(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, NetUserId? author = null)
         {
-            var msg = new ChatMessage(channel, message, wrappedMessage, _entityManager.GetNetEntity(source), senderKey, hideChat, colorOverride, audioPath, audioVolume);
+            var user = author == null ? null : EnsurePlayer(author);
+            var netSource = _entityManager.GetNetEntity(source);
+            user?.AddEntity(netSource);
+
+            var msg = new ChatMessage(channel, message, wrappedMessage, netSource, user?.Key, hideChat, colorOverride, audioPath, audioVolume);
             _netManager.ServerSendToAll(new MsgChatMessage() { Message = msg });
 
             if (!recordReplay)

Content.Server/Chat/Managers/IChatManager.cs

@@ -1,3 +1,4 @@
+using System.Diagnostics.CodeAnalysis;
 using Content.Shared.Chat;
 using Robust.Shared.Network;
 using Robust.Shared.Player;
@@ -6,17 +7,6 @@ namespace Content.Server.Chat.Managers
 {
     public interface IChatManager
     {
-        /// <summary>
-        ///     Keys identifying messages sent by a specific player, used when sending
-        ///     <see cref="MsgChatMessage"/>
-        /// </summary>
-        Dictionary<ICommonSession, int> SenderKeys { get; }
-
-        /// <summary>
-        ///     Tracks which entities a player was attached to while sending messages.
-        /// </summary>
-        Dictionary<ICommonSession, HashSet<NetEntity>> SenderEntities { get; }
-
         void Initialize();
 
         /// <summary>
@@ -36,17 +26,20 @@ namespace Content.Server.Chat.Managers
         void SendAdminAlert(EntityUid player, string message);
 
         void ChatMessageToOne(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat,
-            INetChannel client, Color? colorOverride = null, bool recordReplay = false, string? audioPath = null, float audioVolume = 0, int? senderKey = null);
+            INetChannel client, Color? colorOverride = null, bool recordReplay = false, string? audioPath = null, float audioVolume = 0, NetUserId? author = null);
 
         void ChatMessageToMany(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay,
-            IEnumerable<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0);
+            IEnumerable<INetChannel> clients, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, NetUserId? author = null);
 
         void ChatMessageToManyFiltered(Filter filter, ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, Color? colorOverride, string? audioPath = null, float audioVolume = 0);
 
-        void ChatMessageToAll(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, int? senderKey = null);
+        void ChatMessageToAll(ChatChannel channel, string message, string wrappedMessage, EntityUid source, bool hideChat, bool recordReplay, Color? colorOverride = null, string? audioPath = null, float audioVolume = 0, NetUserId? author = null);
 
         bool MessageCharacterLimit(ICommonSession player, string message);
 
         void DeleteMessagesBy(ICommonSession player);
+
+        [return: NotNullIfNotNull(nameof(author))]
+        ChatUser? EnsurePlayer(NetUserId? author);
     }
 }

Content.Server/Chat/Systems/ChatSystem.cs

@@ -1,12 +1,12 @@
 using System.Globalization;
 using System.Linq;
 using System.Text;
-using Content.Server.Speech.EntitySystems;
-using Content.Server.Speech.Components;
 using Content.Server.Administration.Logs;
 using Content.Server.Administration.Managers;
 using Content.Server.Chat.Managers;
 using Content.Server.GameTicking;
+using Content.Server.Speech.Components;
+using Content.Server.Speech.EntitySystems;
 using Content.Server.Station.Components;
 using Content.Server.Station.Systems;
 using Content.Shared.ActionBlocker;
@@ -19,7 +19,6 @@ using Content.Shared.Interaction;
 using Content.Shared.Mobs.Systems;
 using Content.Shared.Players;
 using Content.Shared.Radio;
-using Robust.Server.GameObjects;
 using Robust.Server.Player;
 using Robust.Shared.Audio;
 using Robust.Shared.Configuration;
@@ -193,8 +192,18 @@ public sealed partial class ChatSystem : SharedChatSystem
         if (!CanSendInGame(message, shell, player))
             return;
 
+        // this method is a disaster
+        // every second i have to spend working with this code is fucking agony
+        // scientists have to wonder how any of this was merged
+        // coding any game admin feature that involves chat code is pure torture
+        // changing even 10 lines of code feels like waterboarding myself
+        // and i dont feel like vibe checking 50 code paths
+        // so we set this here
+        // todo free me from chat code
         if (player != null)
-            _chatManager.SenderEntities.GetOrNew(player).Add(GetNetEntity(source));
+        {
+            _chatManager.EnsurePlayer(player.UserId).AddEntity(GetNetEntity(source));
+        }
 
         if (desiredType == InGameICChatType.Speak && message.StartsWith(LocalPrefix))
         {
@@ -519,7 +528,8 @@ public sealed partial class ChatSystem : SharedChatSystem
         string? nameOverride,
         bool hideLog = false,
         bool checkEmote = true,
-        bool ignoreActionBlocker = false
+        bool ignoreActionBlocker = false,
+        NetUserId? author = null
         )
     {
         if (!_actionBlocker.CanEmote(source) && !ignoreActionBlocker)
@@ -537,7 +547,7 @@ public sealed partial class ChatSystem : SharedChatSystem
 
         if (checkEmote)
             TryEmoteChatInput(source, action);
-        SendInVoiceRange(ChatChannel.Emotes, action, wrappedMessage, source, range);
+        SendInVoiceRange(ChatChannel.Emotes, action, wrappedMessage, source, range, author);
         if (!hideLog)
             if (name != Name(source))
                 _adminLogger.Add(LogType.Chat, LogImpact.Low, $"Emote from {ToPrettyString(source):user} as {name}: {action}");
@@ -564,9 +574,7 @@ public sealed partial class ChatSystem : SharedChatSystem
             ("entityName", name),
             ("message", FormattedMessage.EscapeText(message)));
 
-        _chatManager.SenderEntities.GetOrNew(player).Add(GetNetEntity(source));
+        SendInVoiceRange(ChatChannel.LOOC, message, wrappedMessage, source, hideChat ? ChatTransmitRange.HideChat : ChatTransmitRange.Normal, player.UserId);
-
-        SendInVoiceRange(ChatChannel.LOOC, message, wrappedMessage, source, hideChat ? ChatTransmitRange.HideChat : ChatTransmitRange.Normal);
         _adminLogger.Add(LogType.Chat, LogImpact.Low, $"LOOC from {player:Player}: {message}");
     }
 
@@ -592,9 +600,7 @@ public sealed partial class ChatSystem : SharedChatSystem
             _adminLogger.Add(LogType.Chat, LogImpact.Low, $"Dead chat from {player:Player}: {message}");
         }
 
-        _chatManager.SenderEntities.GetOrNew(player).Add(GetNetEntity(source));
+        _chatManager.ChatMessageToMany(ChatChannel.Dead, message, wrappedMessage, source, hideChat, true, clients.ToList(), author: player.UserId);
-
-        _chatManager.ChatMessageToMany(ChatChannel.Dead, message, wrappedMessage, source, hideChat, true, clients.ToList());
     }
     #endregion
 
@@ -648,7 +654,7 @@ public sealed partial class ChatSystem : SharedChatSystem
     /// <summary>
     ///     Sends a chat message to the given players in range of the source entity.
     /// </summary>
-    private void SendInVoiceRange(ChatChannel channel, string message, string wrappedMessage, EntityUid source, ChatTransmitRange range)
+    private void SendInVoiceRange(ChatChannel channel, string message, string wrappedMessage, EntityUid source, ChatTransmitRange range, NetUserId? author = null)
     {
         foreach (var (session, data) in GetRecipients(source, VoiceRange))
         {
@@ -656,7 +662,7 @@ public sealed partial class ChatSystem : SharedChatSystem
             if (entRange == MessageRangeCheckResult.Disallowed)
                 continue;
             var entHideChat = entRange == MessageRangeCheckResult.HideChat;
-            _chatManager.ChatMessageToOne(channel, message, wrappedMessage, source, entHideChat, session.ConnectedClient);
+            _chatManager.ChatMessageToOne(channel, message, wrappedMessage, source, entHideChat, session.ConnectedClient, author: author);
         }
 
         _replay.RecordServerMessage(new ChatMessage(channel, message, wrappedMessage, GetNetEntity(source), null, MessageRangeHideChatForReplay(range)));

Resources/Changelog/Admin.yml

@@ -55,3 +55,9 @@ Entries:
       commands.', type: Tweak}
   id: 8
   time: '2023-10-21T09:53:00.0000000+00:00'
+- author: DrSmugleaf
+  changes:
+  - {message: 'Fixed the Erase verb not removing all chat messages from the player
+      in some cases.', type: Fix}
+  id: 9
+  time: '2023-10-30T01:28:00.0000000+00:00'