diff --git a/HellionChat/AutoTellTabsService.cs b/HellionChat/AutoTellTabsService.cs index 8e3e8e3..13d8d13 100644 --- a/HellionChat/AutoTellTabsService.cs +++ b/HellionChat/AutoTellTabsService.cs @@ -26,6 +26,11 @@ internal sealed class AutoTellTabsService : IDisposable // MessageManager refilter can share it. See Plugin.TabsListLock / B3. private object TabsListLock => _plugin.TabsListLock; + // Bumped whenever something wipes unpinned temp tabs wholesale (logout). + // HandleTell reads it before releasing the lock and re-checks after, so a + // tab built in between is discarded instead of outliving the wipe. + private int _tabGeneration; + // Hard cap on pinned TempTabs so the sidebar doesn't inflate over years // of usage. Separate pool from AutoTellTabsLimit (15) — pinned tabs live // in their own bucket. A configurable cap is a vault-backlog anchor for @@ -150,15 +155,19 @@ internal sealed class AutoTellTabsService : IDisposable return; } + // Three steps, because building the tab pulls history out of the store and + // that must not happen under TabsListLock (B3 rule; the query sorts the whole + // receiver history). Step 1 and 3 are locked, step 2 is not. + int generation; lock (TabsListLock) { var existing = FindTempTab(partner.Value.Name, partner.Value.World); if (existing != null) { - // Already routed via MessageManager pipeline. Repair the - // tell-target if the fallback hit a pinned tab whose - // TellTarget didn't survive a previous round-trip — keeps - // FindTempTab fast on the next message. + // Already routed via MessageManager pipeline — no AddMessage here, + // HandleTell runs after the delivery loop. Repair the tell-target if + // the fallback hit a pinned tab whose TellTarget didn't survive a + // previous round-trip — keeps FindTempTab fast on the next message. if ( existing.IsPinned && (existing.TellTarget is null || !existing.TellTarget.IsSet()) @@ -175,12 +184,29 @@ internal sealed class AutoTellTabsService : IDisposable return; } - if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit) + generation = _tabGeneration; + } + + var tab = BuildTempTabWithHistory(partner.Value, message); + + lock (TabsListLock) + { + // A logout in between wiped the unpinned pool; committing now would + // resurrect a tab for a character we already left. + if (generation != _tabGeneration) + return; + + // Someone else (self-test, UI) may have created the tab while we built + // ours. Hand the message to theirs and drop what we built — unlike the + // early return above, this tab appeared after the delivery loop ran. + var raced = FindTempTab(partner.Value.Name, partner.Value.World); + if (raced != null) { - DropOldestTempTab(); + raced.AddMessage(message, unread: true); + return; } - SpawnTempTab(partner.Value, message); + CommitTempTab(tab); } } @@ -274,7 +300,9 @@ internal sealed class AutoTellTabsService : IDisposable } var dropped = victim.Tab; - Plugin.Config.Tabs.RemoveAt(victim.Index); + // By reference, not by index: the index came from a Select() earlier in + // this block and would point at the wrong tab if anything shifted the list. + Plugin.Config.Tabs.Remove(dropped); // 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 @@ -290,7 +318,10 @@ internal sealed class AutoTellTabsService : IDisposable } } - private void SpawnTempTab((string Name, uint World) partner, Message currentMessage) + // Runs WITHOUT TabsListLock: PreloadHistory hits the store, which used to hold + // the lock across a query that sorted the whole receiver history. The tab is not + // public until CommitTempTab adds it, so building it unlocked is safe. + private Tab BuildTempTabWithHistory((string Name, uint World) partner, Message currentMessage) { var tab = BuildTempTab(partner.Name, partner.World); @@ -306,10 +337,21 @@ internal sealed class AutoTellTabsService : IDisposable tab.PopOut = true; } + return tab; + } + + // Caller MUST hold TabsListLock. + private void CommitTempTab(Tab tab) + { + if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit) + { + DropOldestTempTab(); + } + Plugin.Config.Tabs.Add(tab); // 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 + // flag was dead (a PopOut tab with no window). CommitTempTab runs on the // 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 @@ -472,6 +514,11 @@ internal sealed class AutoTellTabsService : IDisposable Plugin.Config.Tabs.RemoveAll(TabLifecycleHelpers.IsInUnpinnedPool); + // HandleTell builds a tab outside the lock; bumping here lets it detect + // that the world moved on and drop what it built. Read and compared under + // the same lock, so no volatile needed. + _tabGeneration++; + // Re-anchor the UI selection if the active tab was one of the stripped // unpinned temp tabs (reference predicate, not an index). Logout is a // framework-thread event, so this is already serialized with Draw — no @@ -493,16 +540,23 @@ internal sealed class AutoTellTabsService : IDisposable return false; } - if (PinnedTempTabCount >= MaxPinnedTempTabs) + // Count and flag under one lock so the cap can't be raced. SaveConfig stays + // OUTSIDE -- holding TabsListLock across a save would put an fsync on the + // click path, which is what B6 just removed elsewhere. + lock (TabsListLock) { - WrapperUtil.AddNotification( - string.Format(HellionStrings.PinTab_LimitReached, MaxPinnedTempTabs), - NotificationType.Warning - ); - return false; + if (PinnedTempTabCount >= MaxPinnedTempTabs) + { + WrapperUtil.AddNotification( + string.Format(HellionStrings.PinTab_LimitReached, MaxPinnedTempTabs), + NotificationType.Warning + ); + return false; + } + + tab.IsPinned = true; } - tab.IsPinned = true; _logger.LogDebug( $"[Pin] Pinned tab '{tab.Name}' target={tab.TellTarget?.Name}@{tab.TellTarget?.World}" ); @@ -519,13 +573,18 @@ internal sealed class AutoTellTabsService : IDisposable // If the unpinned pool is already full, dropping the oldest before // flipping the flag avoids counting the just-unpinned tab as a drop - // candidate. - if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit) + // candidate. Under lock, since DropOldestTempTab mutates the list. + // SaveConfig stays outside, see TryPin. + lock (TabsListLock) { - DropOldestTempTab(); + if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit) + { + DropOldestTempTab(); + } + + tab.IsPinned = false; } - tab.IsPinned = false; _logger.LogDebug("[Pin] Unpinned tab '{TabName}'", tab.Name); _plugin.SaveConfig(); } @@ -542,7 +601,12 @@ internal sealed class AutoTellTabsService : IDisposable // see StripTellBindingOnPromote; clearing Tab.TellTarget alone would leave // CurrentChannel.Channel == Tell + a stale target and route a typed line // silently as /tell to the old partner. - TabLifecycleHelpers.StripTellBindingOnPromote(tab); + // Flips IsTempTab/IsPinned, which decide pool membership and whether a save + // strips the tab. Under lock so a concurrent save sees one or the other, never + // half. SaveConfig stays outside, see TryPin. + lock (TabsListLock) + TabLifecycleHelpers.StripTellBindingOnPromote(tab); + _logger.LogDebug($"[Pin] Promoted tab '{tab.Name}' to permanent (tell-binding dropped)"); _plugin.SaveConfig(); }