From 3583dfc0325d9eac5374adce503479ebd9ba9306 Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Mon, 17 Aug 2026 18:38:08 +0200 Subject: [PATCH] perf(tells): build the tab outside the lock, guard pin transitions HandleTell was one atomic block, and PreloadHistory sat inside it -- so every new tell partner held TabsListLock across a store query that sorted the whole receiver history before returning a row. That is the lock the draw thread and the message worker both wait on. Now three steps: look for an existing tab under the lock, build the new one (including history) without it, then commit under the lock again. Splitting it opens a window where the world can change, so the second block re-checks: - FindTempTab again, in case something else created the tab meanwhile. The message goes to that one instead. Not in the first block's early return -- HandleTell runs after the delivery loop, so an existing tab already has it and adding again would duplicate the line. - A generation counter, bumped by OnLogout under the same lock. A logout in between wipes the unpinned pool, and without this the freshly built tab would outlive it and show up for a character we already left. Not via CurrentContentId: its getter falls back to a cached value, so the comparison can silently pass. - The pool cap moves into CommitTempTab and stays there exactly once. Evaluating it twice would evict a tab on every spawn. Pin, unpin and promote take the lock around the flag change now -- they decide pool membership and whether a save strips the tab. SaveConfig stays outside, so no fsync lands on the click path. DropOldestTempTab removes by reference: the index came from an earlier Select in the same block and would point at the wrong tab if anything shifted the list. --- HellionChat/AutoTellTabsService.cs | 108 +++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 22 deletions(-) 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(); }