perf(tabs): share Plugin.TabsListLock across AutoTellTabsService + MessageManager refilter + SaveConfig (B3)

HellionChat/AutoTellTabsService.cs

@@ -21,7 +21,10 @@ internal sealed class AutoTellTabsService : IDisposable
     private readonly MessageManager _messageManager;
     private readonly MessageStore _store;
     private readonly ILogger<AutoTellTabsService> _logger;
-    private readonly object _tempTabsLock = new();
+
+    // Tabs-list structure lock now lives on Plugin (neutral owner) so the
+    // MessageManager refilter can share it. See Plugin.TabsListLock / B3.
+    private object TabsListLock => _plugin.TabsListLock;
 
     // Hard cap on pinned TempTabs so the sidebar doesn't inflate over years
     // of usage. Separate pool from AutoTellTabsLimit (15) — pinned tabs live
@@ -147,7 +150,7 @@ internal sealed class AutoTellTabsService : IDisposable
             return;
         }
 
-        lock (_tempTabsLock)
+        lock (TabsListLock)
         {
             var existing = FindTempTab(partner.Value.Name, partner.Value.World);
             if (existing != null)
@@ -240,46 +243,51 @@ internal sealed class AutoTellTabsService : IDisposable
     }
 
     // Lock-protected lookup for the framework-thread caller (TellRouterService).
-    // Config.Tabs is mutated under _tempTabsLock on the PendingMessage worker thread,
+    // Config.Tabs is mutated under the shared Plugin.TabsListLock on the worker thread,
     // so a framework-tick reader must take the same lock to avoid enumerating the list
     // mid-mutation.
     internal Tab? FindTempTabSafe(string name, uint world)
     {
-        lock (_tempTabsLock)
+        lock (TabsListLock)
             return FindTempTab(name, world);
     }
 
     internal void DropOldestTempTab()
     {
-        // Pinned tabs live in their own bucket (MaxPinnedTempTabs) and are
-        // never drop candidates. They leave the bucket only via Unpin or
-        // PromoteToPermanent.
-        var victim = Plugin
-            .Config.Tabs.Select((tab, idx) => (Tab: tab, Index: idx))
-            .Where(t => TabLifecycleHelpers.IsInUnpinnedPool(t.Tab))
-            .OrderByDescending(t => t.Tab.IsGreeted)
-            .ThenBy(t => t.Tab.LastActivity)
-            .FirstOrDefault();
-
-        if (victim.Tab == null)
+        // B3: lock the list-structure ops so the (currently caller-less) Unpin path
+        // can't race the worker; re-entrant when HandleTell already holds the lock.
+        lock (TabsListLock)
         {
-            return;
+            // Pinned tabs live in their own bucket (MaxPinnedTempTabs) and are
+            // never drop candidates. They leave the bucket only via Unpin or
+            // PromoteToPermanent.
+            var victim = Plugin
+                .Config.Tabs.Select((tab, idx) => (Tab: tab, Index: idx))
+                .Where(t => TabLifecycleHelpers.IsInUnpinnedPool(t.Tab))
+                .OrderByDescending(t => t.Tab.IsGreeted)
+                .ThenBy(t => t.Tab.LastActivity)
+                .FirstOrDefault();
+
+            if (victim.Tab == null)
+            {
+                return;
+            }
+
+            var dropped = victim.Tab;
+            Plugin.Config.Tabs.RemoveAt(victim.Index);
+
+            // Re-anchor the UI selection if it pointed at the dropped tab, and close any
+            // pop-out window the dropped tab owned. Both run on the PendingMessage worker
+            // thread and touch window state the Draw path reads (OnTabActivated re-seed +
+            // the pool's Unbind), so marshal onto the framework thread to serialize with
+            // Draw (reference_dalamud_framework_thread). TryClose is idempotent: a tab that
+            // was never popped is a silent no-op.
+            Plugin.Framework.RunOnFrameworkThread(() =>
+            {
+                _plugin.ChannelPopoutPool.TryClose(dropped.Identifier);
+                _plugin.MainWindow?.ResetActiveTabIfRemoved(dropped);
+            });
         }
-
-        var dropped = victim.Tab;
-        Plugin.Config.Tabs.RemoveAt(victim.Index);
-
-        // Re-anchor the UI selection if it pointed at the dropped tab, and close any
-        // pop-out window the dropped tab owned. Both run on the PendingMessage worker
-        // thread and touch window state the Draw path reads (OnTabActivated re-seed +
-        // the pool's Unbind), so marshal onto the framework thread to serialize with
-        // Draw (reference_dalamud_framework_thread). TryClose is idempotent: a tab that
-        // was never popped is a silent no-op.
-        Plugin.Framework.RunOnFrameworkThread(() =>
-        {
-            _plugin.ChannelPopoutPool.TryClose(dropped.Identifier);
-            _plugin.MainWindow?.ResetActiveTabIfRemoved(dropped);
-        });
     }
 
     private void SpawnTempTab((string Name, uint World) partner, Message currentMessage)
@@ -302,7 +310,7 @@ internal sealed class AutoTellTabsService : IDisposable
 
         // Actually open the pop-out window for the flagged tab — without this the
         // flag was dead (a PopOut tab with no window). SpawnTempTab runs on the
-        // PendingMessage worker thread under _tempTabsLock; TryOpen does
+        // PendingMessage worker thread under Plugin.TabsListLock; TryOpen does
         // OnTabActivated + Bind (window state Draw reads), so marshal onto the
         // framework thread. If the pool is full, drop the flag so it never claims a
         // window it didn't get (flag/window parity).
@@ -428,7 +436,7 @@ internal sealed class AutoTellTabsService : IDisposable
             return;
         }
 
-        lock (_tempTabsLock)
+        lock (TabsListLock)
         {
             // Guard against frame-race: sidebar might render a tab already removed by LRU or logout
             if (!Plugin.Config.Tabs.Contains(tab))
@@ -442,7 +450,7 @@ internal sealed class AutoTellTabsService : IDisposable
 
     private void OnLogout(int type, int code)
     {
-        lock (_tempTabsLock)
+        lock (TabsListLock)
         {
             // Pinned TempTabs must survive char-switch — that's the whole point
             // of pinning. Only unpinned ones get stripped.

HellionChat/MessageManager.cs

@@ -163,8 +163,15 @@ internal class MessageManager : IAsyncDisposable
 
     internal void ClearAllTabs()
     {
+        // B3: snapshot the tab LIST under the shared lock so the worker-thread
+        // add/remove can't tear the enumeration; tab.Clear() then runs lock-free
+        // (each tab's Messages has its own SemaphoreSlim — lock order: list outer).
+        List<Tab> tabsSnapshot;
+        lock (Plugin.TabsListLock)
+            tabsSnapshot = Plugin.Config.Tabs.ToList();
+
         // TempTabs are session-only (not persisted); exclude them to preserve Tell history
-        foreach (var tab in Plugin.Config.Tabs.Where(t => !t.IsTempTab))
+        foreach (var tab in tabsSnapshot.Where(t => !t.IsTempTab))
             tab.Clear();
     }
 
@@ -176,18 +183,19 @@ internal class MessageManager : IAsyncDisposable
 
         using var messages = Store.GetMostRecentMessages(CurrentContentId, since);
 
-        // TempTabs are excluded; they maintain live state from AutoTellTabsService
-        var pendingTabs = Plugin
-            .Config.Tabs.Where(t => !t.IsTempTab)
-            .Select(tab => (tab, new List<Message>()))
-            .ToList();
-        foreach (var message in messages)
-        foreach (var (_, pendingMessages) in pendingTabs.Where(ptab => ptab.Item1.Matches(message)))
-            pendingMessages.Add(message);
+        // TempTabs excluded (live state from AutoTellTabsService). Bucket via the
+        // pure MapMessagesToTabs so the assignment stays testable outside Dalamud (B3-1).
+        // B3: snapshot under the shared lock (list copy only — short critical
+        // section). The Store query above and the AddSortPrune writes below stay
+        // OUTSIDE the lock (lock order: list outer, MessageList inner).
+        List<Tab> nonTempTabs;
+        lock (Plugin.TabsListLock)
+            nonTempTabs = Plugin.Config.Tabs.Where(t => !t.IsTempTab).ToList();
+        var buckets = MapMessagesToTabs(nonTempTabs, messages);
 
         // Apply messages to chat log all at once.
-        foreach (var (tab, pendingMessages) in pendingTabs)
-            tab.Messages.AddSortPrune(pendingMessages, MessageDisplayLimit);
+        foreach (var tab in nonTempTabs)
+            tab.Messages.AddSortPrune(buckets[tab], MessageDisplayLimit);
 
         if (!messages.DidError)
             return;
@@ -206,6 +214,26 @@ internal class MessageManager : IAsyncDisposable
         }
     }
 
+    // Pure message->tab bucketing for the refilter. Dalamud-free + static so the
+    // assignment can be unit-pinned in the build suite; the live caller owns the
+    // Store query, the snapshot and the SemaphoreSlim writes.
+    internal static Dictionary<Tab, List<Message>> MapMessagesToTabs(
+        IReadOnlyList<Tab> tabs,
+        IEnumerable<Message> messages
+    )
+    {
+        var buckets = new Dictionary<Tab, List<Message>>(tabs.Count);
+        foreach (var tab in tabs)
+            buckets[tab] = new List<Message>();
+
+        foreach (var message in messages)
+        foreach (var tab in tabs)
+            if (tab.Matches(message))
+                buckets[tab].Add(message);
+
+        return buckets;
+    }
+
     internal void FilterAllTabsAsync()
     {
         Task.Run(() =>

HellionChat/Plugin.cs

@@ -189,6 +189,12 @@ public sealed class Plugin : IAsyncDalamudPlugin
     internal readonly object RetentionSweepLock = new();
     internal volatile bool RetentionSweepRunning;
 
+    // B3: neutral owner of the Config.Tabs LIST-structure lock so both the
+    // worker-thread mutator (AutoTellTabsService) and the framework-thread
+    // refilter (MessageManager) share ONE monitor. Lock order: this outer,
+    // MessageList's SemaphoreSlim inner — never the reverse.
+    internal readonly object TabsListLock = new();
+
     internal DateTime GameStarted { get; }
 
     // Couples "current tab" to the real UI selection. The chat hooks are
@@ -976,11 +982,10 @@ public sealed class Plugin : IAsyncDalamudPlugin
                 {
                     Log.Information($"Retention sweep deleted {deleted} expired messages.");
                     // Schedule on the next framework tick to avoid the ~194ms
-                    // hitch from blocking with .Wait() while the framework
-                    // finishes the current frame. Tabs-list mutation must
-                    // stay on the framework thread because Plugin.Config.Tabs
-                    // (Configuration.cs:222) is not lock-protected and
-                    // AutoTellTabsService can mutate it from background paths.
+                    // hitch from blocking with .Wait() while the frame finishes.
+                    // The Config.Tabs enumeration in ClearAllTabs/FilterAllTabs is
+                    // now guarded by the shared Plugin.TabsListLock (B3), so this
+                    // tick scheduling is purely hitch-avoidance, not safety.
                     // Pattern reference: SimpleTweaks
                     // Tweaks/Chat/CaseInsensitiveCommands.cs:45.
                     Framework.RunOnTick(() =>
@@ -1091,12 +1096,20 @@ public sealed class Plugin : IAsyncDalamudPlugin
         // Config.Tabs across the save so JSON includes them. Cloning only the
         // unpinned subset keeps the allocation proportional to
         // AutoTellTabsLimit (<=15) instead of the full tab list.
-        var unpinnedTempTabs = Config.Tabs.Where(TabLifecycleHelpers.IsInUnpinnedPool).ToList();
-        Config.Tabs.RemoveAll(TabLifecycleHelpers.ShouldStripOnSave);
+        // B3: the strip/restore mutates the tab LIST, so it shares TabsListLock
+        // with the worker add/remove and the refilter snapshot. Re-entrant: the
+        // one worker caller (HandleTell) already holds it; framework callers take
+        // it here. SavePluginConfig runs inside (short, in-memory) — the §8 fallback
+        // (serialize a copy outside the lock) is a tracked pre-beta to-do.
+        lock (TabsListLock)
+        {
+            var unpinnedTempTabs = Config.Tabs.Where(TabLifecycleHelpers.IsInUnpinnedPool).ToList();
+            Config.Tabs.RemoveAll(TabLifecycleHelpers.ShouldStripOnSave);
 
-        Interface.SavePluginConfig(Config);
+            Interface.SavePluginConfig(Config);
 
-        Config.Tabs.AddRange(unpinnedTempTabs);
+            Config.Tabs.AddRange(unpinnedTempTabs);
+        }
     }
 
     internal void LanguageChanged(string langCode)