From 7507f06c7033ebb016ae4a7bd79c9cbb7fbc6ae9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:14:35 -0400 Subject: [PATCH 1/3] Make a bare second launch surface the running instance instead of clobbering settings (#489) Two full instances each hold a whole-file AppSettings snapshot and Save writes the whole file, so the instance that exits second silently overwrites the other's open_tabs and every other setting - AtomicFile prevents torn writes, not lost updates. Launches with a file argument already forwarded to the running instance over the SQLPerformanceStudio_OpenFile pipe; a bare second launch ran a full instance and was exactly the clobber case. Program.Main now claims a named mutex (SQLPerformanceStudio_SingleInstance, default per-user-session scope, held for the process lifetime - the previous mutex attempt died because its handle was disposed on return). A launch that finds the slot taken hands its work to the owner over the existing pipe - the file path it was given, or a reserved ::activate:: sentinel meaning "surface your main window" - and exits. The sentinel is deliberately unrepresentable as a Windows file name: a pre-#489 receiver File.Exists-guards every pipe line, so an old running build reads it, finds no such file, and drops it without harm. The with-file pipe probe stays first and unchanged, which also keeps version skew safe in the other direction (an old running build answers its pipe but holds no mutex). If the owner never answers after ~2s of retries (wedged, or still booting before its pipe server is up), the launch proceeds as a full instance: losing the user's double-clicked file, or the app simply appearing, is worse than a rare second instance. That is also the honest residue of the startup race - two simultaneous bare launches can both proceed if the loser's retries run out before the winner's pipe exists; the remainder falls back to the old last-write-wins behavior as the accepted floor. --new-instance skips the single-instance check for users who run two on purpose. It is scrubbed from argv both in Program.Main and inside OpenFromStartupArgs, because MainWindow consumes the raw Environment.GetCommandLineArgs() itself - so "PerformanceStudio.exe --new-instance file.sqlplan" still opens the file. The receiver's line dispatch is extracted into a testable seam (SingleInstance.Classify + MainWindow.DispatchPipeMessage): sentinel surfaces (UI-thread marshal, restore from minimized, Activate), an existing path opens exactly as the SSMS extension has always relied on - and now also restores a minimized window instead of loading into the taskbar - and garbage is ignored as before. Headless tests pin the grammar, the old-receiver degradation property, and the argv scrubbing; the mutex/exit flow is process-level, out of the harness's reach (it boots App directly, never Main), and is verified by inspection and commented in Program.cs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n --- src/PlanViewer.App/MainWindow.axaml.cs | 73 +++++- src/PlanViewer.App/Program.cs | 148 ++++++++++-- src/PlanViewer.App/SingleInstance.cs | 124 ++++++++++ .../SingleInstanceTests.cs | 216 ++++++++++++++++++ 4 files changed, 527 insertions(+), 34 deletions(-) create mode 100644 src/PlanViewer.App/SingleInstance.cs create mode 100644 tests/PlanViewer.Core.Tests/SingleInstanceTests.cs diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 62b5cdb..96d6671 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -30,8 +30,6 @@ namespace PlanViewer.App; public partial class MainWindow : Window { - private const string PipeName = "SQLPerformanceStudio_OpenFile"; - private readonly ICredentialService _credentialService; private readonly ConnectionStore _connectionStore; private readonly CancellationTokenSource _pipeCts = new(); @@ -67,7 +65,9 @@ public MainWindow() if (Enum.TryParse(_appSettings.QueryStoreDefaultTimeDisplay, true, out var tdm)) TimeDisplayHelper.Current = tdm; - // Listen for file paths from other instances (e.g. SSMS extension). + // Listen for file paths from other launches (SSMS extension, a second Studio + // launch handing over its file) and for the #489 surface-yourself sentinel a bare + // second launch sends instead of running a full instance. // Not in the test host (#451): every test window would grab the machine's single // SQLPerformanceStudio_OpenFile pipe slot and never release it — OnClosed never // runs there — racing any real Studio instance on the same box. @@ -189,6 +189,15 @@ so the tab that gains a plan is one this window already had. */ /// internal void OpenFromStartupArgs(string[] args) { + /* #489: --new-instance is a launcher directive, not a file, and it is scrubbed + HERE as well as in Program.Main because this method consumes the raw + Environment.GetCommandLineArgs() — Program's scrubbed copy never reaches it. + Without this, "PerformanceStudio.exe --new-instance file.sqlplan" would find the + flag at args[1]: it happens to fail the File.Exists guard below, but the user's + file behind it would still be skipped and the launch would silently + session-restore instead. */ + args = SingleInstance.StripNewInstanceFlag(args); + if (args.Length > 1 && File.Exists(args[1])) { OpenFileByExtension(args[1]); @@ -212,21 +221,22 @@ private void StartPipeServer() try { using var server = new NamedPipeServerStream( - PipeName, PipeDirection.In, 1, + SingleInstance.PipeName, PipeDirection.In, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); await server.WaitForConnectionAsync(token); using var reader = new StreamReader(server); - var filePath = await reader.ReadLineAsync(); + var line = await reader.ReadLineAsync(); - if (!string.IsNullOrWhiteSpace(filePath) && File.Exists(filePath)) + /* Classified out here rather than inside the UI dispatch on purpose: + Classify calls File.Exists, and a dead UNC path can block for + seconds — that wait belongs on this pipe task, not the dispatcher. */ + var kind = SingleInstance.Classify(line); + if (kind != SingleInstance.PipeMessage.Ignore) { await Dispatcher.UIThread.InvokeAsync(() => - { - OpenFileByExtension(filePath); - Activate(); - }); + DispatchPipeMessage(kind, line!)); } } catch (OperationCanceledException) @@ -245,6 +255,49 @@ await Dispatcher.UIThread.InvokeAsync(() => }, token); } + /// + /// Acts on one classified pipe line, on the UI thread. Split from the pipe loop so the + /// dispatch can be pinned by tests without a pipe (#489). + /// + /// The classification is the compatibility contract with every sender version: + /// an existing file path opens — what the SSMS extension and any Studio build have + /// always sent, unchanged — the activation sentinel surfaces the window (what a bare + /// second launch sends since #489), and anything else, including a path that stopped + /// existing between send and receive, was already dropped silently before the + /// classifier existed and still is. + /// + internal void DispatchPipeMessage(SingleInstance.PipeMessage kind, string line) + { + switch (kind) + { + case SingleInstance.PipeMessage.OpenFile: + OpenFileByExtension(line); + SurfaceWindow(); + break; + + case SingleInstance.PipeMessage.Activate: + SurfaceWindow(); + break; + } + } + + /// + /// Brings the main window back to the user after a second launch handed its work to + /// this one (#489): restore from minimized, then activate. Deliberately minimal next + /// to Lite's surface path — Studio never hides to a tray, so there is nothing to + /// re-show. + /// + /// The file-open message uses it too, where the old handler only called + /// Activate(): a plan sent from SSMS to a minimized window used to load into a window + /// that stayed in the taskbar. + /// + internal void SurfaceWindow() + { + if (WindowState == WindowState.Minimized) + WindowState = WindowState.Normal; + Activate(); + } + private void StartMcpServer() { // Set before the settings read: reading the user's real ~/.planview file is itself diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 3d750fc..9db8eb4 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -2,6 +2,7 @@ using System; using System.IO; using System.IO.Pipes; +using System.Threading; using System.Threading.Tasks; using PlanViewer.App.Services; using Velopack; @@ -10,7 +11,14 @@ namespace PlanViewer.App; class Program { - private const string PipeName = "SQLPerformanceStudio_OpenFile"; + /// + /// Held — never released — by the instance that owns the single-instance slot (#489). + /// The OS tears the mutex down when the process exits, crash included, so there is no + /// release path to get wrong; a static field keeps the handle rooted for the whole run + /// (the previous mutex attempt died precisely because its handle was disposed the + /// moment the acquiring method returned, so no instance ever actually held it). + /// + private static Mutex? _singleInstanceMutex; [STAThread] public static void Main(string[] args) @@ -36,12 +44,62 @@ public static void Main(string[] args) } velopack.Run(); - // If another instance is running, send the file path to it and exit - if (args.Length > 0 && TrySendToRunningInstance(args[0])) - return; + /* #489: every instance holds a whole-file AppSettings snapshot and Save writes the + whole file, so a second instance makes the settings file last-write-wins — the + instance that exits second silently clobbers the other's open_tabs and every + other setting (AtomicFile prevents torn writes, not lost updates). File-argument + launches already forwarded to the running instance over the pipe; a BARE second + launch ran a full instance and was exactly the clobber case. So unless the user + explicitly asks for a second instance, a launch that finds one running hands it + its work — a file path, or a bare "surface yourself" — and exits. */ + var newInstanceRequested = SingleInstance.NewInstanceRequested(args); + + // The flag is a launcher directive, not a file: strip it so nothing downstream can + // mistake it for a path ("PerformanceStudio.exe --new-instance file.sqlplan" must + // still open the file). MainWindow re-reads the raw argv and scrubs it again itself. + var effectiveArgs = SingleInstance.StripNewInstanceFlag(args); + + if (!newInstanceRequested) + { + // The pre-#489 forwarding, kept first and unchanged: a with-file launch tries + // the pipe before anything else. Beyond being the common case, probing before + // the mutex is version-skew-proof — an already-running build that predates the + // mutex answers its pipe but holds no mutex, and a mutex-first flow would run a + // second full window beside it instead of handing the file over. + if (effectiveArgs.Length > 0 && TrySendToRunningInstance(effectiveArgs[0], maxAttempts: 1)) + return; + + if (!TryBecomeSingleInstanceOwner()) + { + /* Another instance owns the slot but hasn't answered its pipe yet — bare + launches never probed above, and a with-file probe may have raced the + owner's boot (the pipe server starts in the MainWindow constructor, + which on a cold start is seconds after its Main). Retry for ~2s before + giving up on delivery. */ + var message = effectiveArgs.Length > 0 + ? effectiveArgs[0] + : SingleInstance.ActivateSentinel; + if (TrySendToRunningInstance(message, maxAttempts: 4)) + return; + + /* Delivery failed after retries: the owner is wedged, exiting, or still + booting slowly. Losing the user's action — their double-clicked file, or + the app simply appearing at all — is worse than a rare second instance, + so fall through and run fully. This is also the honest residue of the + startup race: two simultaneous bare launches can BOTH end up proceeding + when the loser's retries run out before the winner's pipe exists. The + mutex closes most of that window; what remains falls back to the + pre-#489 last-write-wins behavior, which is the accepted floor. */ + } + } BuildAvaloniaApp() - .StartWithClassicDesktopLifetime(args); + .StartWithClassicDesktopLifetime(effectiveArgs); + + // Reached only at app shutdown. Statics are GC roots, so the field alone keeps the + // owner's mutex handle alive; this read exists to say out loud that the handle's + // LIFETIME is the point (and to keep the field from reading as write-only). + GC.KeepAlive(_singleInstanceMutex); } // Avalonia configuration, don't remove; also used by visual designer. @@ -52,32 +110,74 @@ public static AppBuilder BuildAvaloniaApp() .LogToTrace(); /// - /// Tries to hand the file path to an already-running instance over its named pipe. - /// A failed/timed-out connect means no instance is listening, so the caller should - /// launch normally. Returns true only if the path was actually delivered. + /// Tries to claim the single-instance slot (#489). True means this process is the + /// owner and should run; false means another instance holds the slot and this launch + /// should hand its work over instead. /// - /// - /// Detection is via the pipe itself rather than a named mutex: the previous mutex - /// was disposed as soon as this method returned, so no instance ever held it and - /// the forwarding path was never taken. - /// - private static bool TrySendToRunningInstance(string filePath) + private static bool TryBecomeSingleInstanceOwner() { try { - using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out); - // Short timeout: a running instance's listener is idle and connects - // immediately; when none is running this is the only added launch delay. - client.Connect(500); - using var writer = new StreamWriter(client); - writer.WriteLine(filePath); - writer.Flush(); - return true; + var mutex = new Mutex(initiallyOwned: true, SingleInstance.MutexName, out var createdNew); + if (createdNew) + { + _singleInstanceMutex = mutex; + return true; + } + + /* Another process owns it. Close our handle right away: if this launch ends up + running anyway (the pipe fallback above), a lingering handle would keep the + kernel object alive after the real owner exits, and a THIRD launch would then + see the name taken with nobody serving the pipe behind it. */ + mutex.Dispose(); + return false; } catch { - // No instance listening (or pipe busy) — fall through to launch normally. - return false; + /* Mutex machinery unavailable — an ACL mismatch on the name, a restrictive + sandbox. Claiming ownership is the conservative answer: this instance runs + fully, which is exactly the pre-#489 behavior for every launch. */ + return true; } } + + /// + /// Tries to hand one line — a file path, or the activation sentinel — to an + /// already-running instance over its named pipe. Returns true only if the line was + /// actually delivered; the caller decides what a failed delivery costs. + /// + private static bool TrySendToRunningInstance(string message, int maxAttempts) + { + for (var attempt = 1; attempt <= maxAttempts; attempt++) + { + if (attempt > 1) + { + // Between attempts only: the owner is presumably mid-boot, so give its + // pipe server a beat to come up rather than burning connects back-to-back. + Thread.Sleep(100); + } + + try + { + using var client = new NamedPipeClientStream(".", SingleInstance.PipeName, PipeDirection.Out); + // 500ms per attempt: a running instance's listener is idle and connects + // immediately, while Connect burns the full timeout when nothing is + // listening — so the single-attempt probe on a with-file launch adds at + // most the same half second it always has, and the 4-attempt retry path + // totals roughly the ~2s boot grace it exists for. + client.Connect(500); + using var writer = new StreamWriter(client); + writer.WriteLine(message); + writer.Flush(); + return true; + } + catch + { + // Not listening yet, or the single server slot was mid-conversation with + // another client — retry if the budget allows, otherwise report undelivered. + } + } + + return false; + } } diff --git a/src/PlanViewer.App/SingleInstance.cs b/src/PlanViewer.App/SingleInstance.cs new file mode 100644 index 0000000..625fe7a --- /dev/null +++ b/src/PlanViewer.App/SingleInstance.cs @@ -0,0 +1,124 @@ +using System; +using System.IO; +using System.Linq; + +namespace PlanViewer.App; + +/// +/// The names and message grammar shared by both halves of single-instance startup (#489): +/// the launcher side in that decides whether to run or to hand its +/// work to an already-running instance, and the receiver side in 's +/// pipe server that acts on what arrives. +/// +/// Why single-instance at all. Every instance holds a whole-file +/// AppSettings snapshot and Save writes the whole file, so two instances make the +/// settings file last-write-wins: whichever exits second silently clobbers the other's +/// open_tabs and every other setting. AtomicFile prevents torn files, not lost updates. +/// A launch with a file argument already forwarded to the running instance over the named +/// pipe; a bare launch ran a full second instance and was exactly the clobber case. +/// +/// Why the grammar is this shape. The pipe protocol is one line per +/// connection, historically always a file path, and two other sender/receiver pairs speak +/// it: the SSMS extension's AppLauncher (sends plain paths, cannot be updated in lockstep +/// with the app) and any older Studio build still running across an upgrade. So the +/// activation message is not a version field or a framed header — it is a single reserved +/// line that an OLD receiver safely ignores: the pre-#489 handler's only guard is +/// File.Exists(line), and contains characters that +/// are illegal in Windows file names, so it can never name an existing file there. An old +/// receiver reads it, finds no such file, drops it, and keeps serving — the worst-case +/// skew is a bare second launch that exits without surfacing anything, not a crash or a +/// junk tab. +/// +internal static class SingleInstance +{ + /// + /// The pipe every Studio sender and receiver has always shared — also written by the + /// SSMS extension's AppLauncher, which is why the name can never change casually. + /// + internal const string PipeName = "SQLPerformanceStudio_OpenFile"; + + /// + /// Unprefixed, so it lands in the default per-user-session Local\ namespace on + /// Windows — two users (or two RDP sessions) each get their own instance, which is the + /// scope the settings file conflict actually has. Distinct from Lite's + /// PerformanceMonitorLite_SingleInstance; the two apps must never see each other. + /// + internal const string MutexName = "SQLPerformanceStudio_SingleInstance"; + + /// + /// Escape hatch (#489): skip the single-instance check and run a full second instance. + /// A user who runs two on purpose accepts settings last-write-wins as their informed + /// choice. Stripped from argv before any file-open logic sees it. + /// + internal const string NewInstanceFlag = "--new-instance"; + + /// + /// The line a bare second launch sends to mean "surface your main window". The + /// double-colons make it unrepresentable as a Windows file name on purpose — see the + /// class comment for why that property is the entire backward-compatibility story. + /// + internal const string ActivateSentinel = "::activate::"; + + /// What one received pipe line means. See . + internal enum PipeMessage + { + /// Blank, or a path that doesn't exist — dropped silently, exactly as the pre-#489 receiver did. + Ignore, + + /// The — a bare second launch asking this window to surface. + Activate, + + /// An existing file — the SSMS extension or a second launch handing over a path to open. + OpenFile, + } + + /// + /// The receiver's dispatch decision for one pipe line, extracted from the pipe loop so + /// it can be pinned by tests without a pipe. + /// + /// The sentinel is checked before File.Exists, not after: on Windows the + /// order can't matter (the sentinel is an illegal file name), but on Linux a file named + /// ::activate:: is representable, and the reserved meaning must win over any + /// such file. File.Exists on a garbage line returns false rather than throwing, + /// which is what made the old receiver's guard safe and keeps this one safe too. + /// + internal static PipeMessage Classify(string? line) + { + if (string.IsNullOrWhiteSpace(line)) + return PipeMessage.Ignore; + + if (string.Equals(line, ActivateSentinel, StringComparison.Ordinal)) + return PipeMessage.Activate; + + if (File.Exists(line)) + return PipeMessage.OpenFile; + + return PipeMessage.Ignore; + } + + /// True when argv carries anywhere. + internal static bool NewInstanceRequested(string[] args) => + args.Any(IsNewInstanceFlag); + + /// + /// Argv minus every , order otherwise preserved. Applied + /// in BOTH places argv is consumed: (so the forwarded path + /// and the args handed to Avalonia are clean) and + /// (which reads the raw + /// Environment.GetCommandLineArgs() itself, so Program's scrubbed copy never + /// reaches it). Without the second scrub, "PerformanceStudio.exe --new-instance + /// file.sqlplan" would see the flag at args[1] instead of the file. The flag happens to + /// fail that path's File.Exists guard today, but the file arg behind it would + /// still be skipped — being explicit here is what makes the flag invisible rather than + /// merely unlucky. + /// + internal static string[] StripNewInstanceFlag(string[] args) => + args.Where(a => !IsNewInstanceFlag(a)).ToArray(); + + /// + /// Case-insensitive, because Windows users type flags in whatever case survived their + /// muscle memory and there is no second flag for this one to collide with. + /// + private static bool IsNewInstanceFlag(string arg) => + string.Equals(arg, NewInstanceFlag, StringComparison.OrdinalIgnoreCase); +} diff --git a/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs new file mode 100644 index 0000000..78fd52d --- /dev/null +++ b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs @@ -0,0 +1,216 @@ +using System.IO; +using System.Linq; +using Avalonia.Controls; +using PlanViewer.App; +using PlanViewer.App.Controls; + +namespace PlanViewer.Core.Tests; + +/// +/// #489: two Studio instances each hold a whole-file AppSettings snapshot and Save writes +/// the whole file, so the instance that exits second silently clobbers the other's +/// open_tabs and every other setting. The fix makes a bare second launch hand a +/// "surface yourself" sentinel to the running instance over the existing OpenFile pipe +/// (with-file launches already forwarded), with --new-instance as the deliberate +/// escape hatch. +/// +/// The process-level halves — the named mutex in Program.Main and the exit of the +/// non-owning launch — cannot run under this harness, which boots App directly and never +/// enters Main; they are verified by inspection and commented in Program.cs. What CAN be +/// pinned, and is here, are the two seams everything else leans on: the receiver's message +/// grammar (sentinel vs file path vs garbage, including the property that makes the +/// sentinel safe to send to a pre-#489 receiver) and the argv scrubbing that keeps the +/// flag from shadowing a real file argument. +/// +public class SingleInstanceTests +{ + /* ---- Classify: the receiver's message grammar ---------------------------------- */ + + [Fact] + public void TheActivationSentinelClassifiesAsActivate() + { + Assert.Equal( + SingleInstance.PipeMessage.Activate, + SingleInstance.Classify(SingleInstance.ActivateSentinel)); + } + + [Fact] + public void AnExistingFileClassifiesAsOpenFile() + { + var path = TempSql("SELECT 1;"); + try + { + Assert.Equal(SingleInstance.PipeMessage.OpenFile, SingleInstance.Classify(path)); + } + finally + { + File.Delete(path); + } + } + + /// + /// A path that stopped existing between send and receive was dropped silently before + /// #489 and must still be — the receiver has no way to open it and no business + /// guessing. + /// + [Fact] + public void AMissingPathClassifiesAsIgnore() + { + var missing = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName(), "never_written.sqlplan"); + Assert.Equal(SingleInstance.PipeMessage.Ignore, SingleInstance.Classify(missing)); + } + + [Fact] + public void BlankLinesClassifyAsIgnore() + { + Assert.Equal(SingleInstance.PipeMessage.Ignore, SingleInstance.Classify(null)); + Assert.Equal(SingleInstance.PipeMessage.Ignore, SingleInstance.Classify(string.Empty)); + Assert.Equal(SingleInstance.PipeMessage.Ignore, SingleInstance.Classify(" ")); + } + + /// + /// The entire backward-compatibility story of the sentinel, pinned: a pre-#489 + /// receiver's only guard is File.Exists(line), so the sentinel is safe to send + /// to an old running build exactly as long as it can never name an existing file. The + /// double-colons are illegal in Windows file names by design; if someone ever + /// "tidies" the token into something path-representable, this fails and points here. + /// + [Fact] + public void TheSentinelCanNeverBeMistakenForAFileByAnOldReceiver() + { + Assert.False( + File.Exists(SingleInstance.ActivateSentinel), + "an old receiver File.Exists-guards every pipe line, so the sentinel must never resolve to a real file"); + } + + /* ---- StripNewInstanceFlag: argv scrubbing -------------------------------------- */ + + /// + /// "PerformanceStudio.exe --new-instance file.sqlplan" must still open the file: the + /// flag disappears and the path keeps its place as the first real argument, because + /// OpenFromStartupArgs only ever looks at args[1]. + /// + [Fact] + public void TheNewInstanceFlagIsRemovedAndTheFileArgSurvives() + { + var raw = new[] { "PerformanceStudio.exe", "--new-instance", @"C:\plans\slow.sqlplan" }; + + var scrubbed = SingleInstance.StripNewInstanceFlag(raw); + + Assert.Equal(new[] { "PerformanceStudio.exe", @"C:\plans\slow.sqlplan" }, scrubbed); + Assert.True(SingleInstance.NewInstanceRequested(raw)); + } + + [Fact] + public void TheFlagIsRecognizedInAnyCase() + { + var raw = new[] { "PerformanceStudio.exe", "--NEW-INSTANCE" }; + + Assert.True(SingleInstance.NewInstanceRequested(raw)); + Assert.Equal(new[] { "PerformanceStudio.exe" }, SingleInstance.StripNewInstanceFlag(raw)); + } + + /// + /// The overwhelmingly common argv has no flag in it, and scrubbing must be invisible + /// there — same contents, same order, nothing else filtered. + /// + [Fact] + public void ArgvWithoutTheFlagPassesThroughUntouched() + { + var raw = new[] { "PerformanceStudio.exe", @"C:\plans\slow.sqlplan" }; + + Assert.Equal(raw, SingleInstance.StripNewInstanceFlag(raw)); + Assert.False(SingleInstance.NewInstanceRequested(raw)); + } + + /* ---- The window-side dispatch -------------------------------------------------- */ + + /// + /// What a bare second launch's sentinel actually buys the user: the running window + /// comes back from minimized. Activate() is also called but has nothing observable + /// headlessly; the state restore is the part that can silently regress. + /// + [Fact] + public void ASentinelDispatchRestoresAMinimizedWindow() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.WindowState = WindowState.Minimized; + + window.DispatchPipeMessage( + SingleInstance.PipeMessage.Activate, SingleInstance.ActivateSentinel); + + Assert.Equal(WindowState.Normal, window.WindowState); + }); + } + + /// + /// The path every existing sender relies on — the SSMS extension and a second launch + /// handing over its file — still lands the file in a tab, and now also surfaces a + /// minimized window instead of loading into one that stays in the taskbar. + /// + [Fact] + public void AFileDispatchOpensTheFileAndRestoresAMinimizedWindow() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS from_pipe;"); + try + { + var window = new MainWindow(); + window.WindowState = WindowState.Minimized; + var queryTabsBefore = QueryTabs(window).Count(); + + window.DispatchPipeMessage(SingleInstance.PipeMessage.OpenFile, path); + + Assert.Equal(queryTabsBefore + 1, QueryTabs(window).Count()); + Assert.Equal(path, ((QuerySessionControl)QueryTabs(window).Last().Content!).SourceFilePath); + Assert.Equal(WindowState.Normal, window.WindowState); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// The end-to-end argv property, through the real startup router: the constructor + /// consumes raw Environment.GetCommandLineArgs(), so OpenFromStartupArgs must + /// scrub the flag itself — Program.Main's scrubbed copy never reaches it. With the + /// flag sitting where the file used to be, the file behind it still opens. + /// + [Fact] + public void OpenFromStartupArgsOpensTheFileBehindTheNewInstanceFlag() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS behind_the_flag;"); + try + { + var window = new MainWindow(); + var queryTabsBefore = QueryTabs(window).Count(); + + window.OpenFromStartupArgs(new[] { "PerformanceStudio.exe", "--new-instance", path }); + + Assert.Equal(queryTabsBefore + 1, QueryTabs(window).Count()); + Assert.Equal(path, ((QuerySessionControl)QueryTabs(window).Last().Content!).SourceFilePath); + } + finally + { + File.Delete(path); + } + }); + } + + private static string TempSql(string text) + { + var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sql"); + File.WriteAllText(path, text); + return path; + } + + private static System.Collections.Generic.IEnumerable QueryTabs(MainWindow window) => + window.MainTabControl.Items.OfType().Where(t => t.Content is QuerySessionControl); +} From beacd5dc30f179aacd0cf79519cb2867ab1cf365 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:25:18 -0400 Subject: [PATCH 2/3] Address review: scope the skew claim honestly, pin the sentinel as a string property Two gate findings. The version-skew-proof comment overclaimed: it holds for with-file launches only, and a bare launch beside a pre-mutex build deliberately runs fully rather than probing first - an old receiver drops the sentinel while delivery reports success, so probe-first would exit having surfaced nothing, worse than one transient window of the pre-#489 status quo. Documented beside the same-version race instead of solved; a real fix needs an acknowledged surfacing protocol. The sentinel test was vacuous on the ubuntu CI runner, where colons are legal in file names and File.Exists only proved the test CWD was clean. The property that carries the old-Windows-receiver guarantee is the colon itself, so it is now pinned as a string property (plus the actual invalid-chars check where the guarantee lives), which catches a tidied path-representable token on every platform. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n --- src/PlanViewer.App/Program.cs | 20 +++++++++--- .../SingleInstanceTests.cs | 31 ++++++++++++++----- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 9db8eb4..582945b 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -61,11 +61,21 @@ launch ran a full instance and was exactly the clobber case. So unless the user if (!newInstanceRequested) { - // The pre-#489 forwarding, kept first and unchanged: a with-file launch tries - // the pipe before anything else. Beyond being the common case, probing before - // the mutex is version-skew-proof — an already-running build that predates the - // mutex answers its pipe but holds no mutex, and a mutex-first flow would run a - // second full window beside it instead of handing the file over. + /* The pre-#489 forwarding, kept first and unchanged: a with-file launch tries + the pipe before anything else. Beyond being the common case, probing before + the mutex makes the WITH-FILE path version-skew-proof — an already-running + build that predates the mutex answers its pipe but holds no mutex, and a + mutex-first flow would run a second full window beside it instead of handing + the file over. + + A BARE launch during that same skew is the one #489 case deliberately left + open: it cannot probe first, because an old receiver silently drops the + sentinel (File.Exists guard) while delivery still reports success — the + launch would exit having surfaced nothing, which is worse than a second + instance. So a bare launch beside a pre-mutex build claims the free mutex + and runs fully: the pre-#489 status quo, for one transient upgrade window + that ends when the old instance exits. Documented rather than solved; a + real fix needs an acknowledged (duplex) surfacing protocol. */ if (effectiveArgs.Length > 0 && TrySendToRunningInstance(effectiveArgs[0], maxAttempts: 1)) return; diff --git a/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs index 78fd52d..ca70cf3 100644 --- a/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs +++ b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs @@ -71,16 +71,33 @@ public void BlankLinesClassifyAsIgnore() /// /// The entire backward-compatibility story of the sentinel, pinned: a pre-#489 /// receiver's only guard is File.Exists(line), so the sentinel is safe to send - /// to an old running build exactly as long as it can never name an existing file. The - /// double-colons are illegal in Windows file names by design; if someone ever - /// "tidies" the token into something path-representable, this fails and points here. + /// to an old running build exactly as long as it can never name an existing file. + /// + /// Pinned as a STRING property, not with File.Exists — the gate review caught + /// that a File.Exists assertion is vacuous on the ubuntu CI runner, where ':' is a + /// legal filename character and the check only proves no such file sits in the test + /// CWD. What actually guarantees old-Windows-receiver safety is the colon, which + /// Windows rejects in file names; asserting the characters directly means "tidying" + /// the token into a path-representable word fails this test on every platform. An old + /// LINUX receiver next to an adversarially created "::activate::" file remains the + /// accepted floor (new receivers classify the sentinel before File.Exists, so only + /// pre-#489 builds are exposed, and only until they restart). /// [Fact] - public void TheSentinelCanNeverBeMistakenForAFileByAnOldReceiver() + public void TheSentinelCanNeverBeMistakenForAFileByAnOldWindowsReceiver() { - Assert.False( - File.Exists(SingleInstance.ActivateSentinel), - "an old receiver File.Exists-guards every pipe line, so the sentinel must never resolve to a real file"); + Assert.Contains(':', SingleInstance.ActivateSentinel); + Assert.StartsWith("::", SingleInstance.ActivateSentinel, StringComparison.Ordinal); + Assert.EndsWith("::", SingleInstance.ActivateSentinel, StringComparison.Ordinal); + + /* Belt and braces for the platform where the guarantee lives: on Windows the + token must actually be rejected by the file-name rules, not just assumed to be. */ + if (OperatingSystem.IsWindows()) + { + Assert.Contains( + SingleInstance.ActivateSentinel, + c => Path.GetInvalidFileNameChars().Contains(c)); + } } /* ---- StripNewInstanceFlag: argv scrubbing -------------------------------------- */ From 46292fe288b4671b0db08c330f30d634f9042dc5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:33:35 -0400 Subject: [PATCH 3/3] Address review: make the mutex degradation observable, exercise the shim in CI The gate's third finding: if the named mutex throws wholesale on some platform, the catch-all's run-fully degradation would silently no-op single-instancing and #489's clobber would be back with no symptom pointing here. Two signals now exist. A unit test drives the named create and second-open paths on whatever platform the suite runs on - the ubuntu runner on every push - so a shim that throws fails CI loudly before it degrades invisibly in the field. And the catch-all says so on stderr, which a Windows GUI launch loses harmlessly while the Unix platforms most likely to hit it are exactly where terminal launches are common. macOS has no CI leg; the bare-double-launch smoke there is a release-checklist item. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n --- src/PlanViewer.App/Program.cs | 19 ++++++++++-- .../SingleInstanceTests.cs | 29 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 582945b..82bb239 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -142,11 +142,24 @@ see the name taken with nobody serving the pipe behind it. */ mutex.Dispose(); return false; } - catch + catch (Exception ex) { /* Mutex machinery unavailable — an ACL mismatch on the name, a restrictive - sandbox. Claiming ownership is the conservative answer: this instance runs - fully, which is exactly the pre-#489 behavior for every launch. */ + sandbox, or a platform where the named-mutex shim misbehaves (on Unix these + are file-backed under /tmp; PlatformNotSupportedException would land here + too). Claiming ownership is the conservative answer: this instance runs + fully, which is exactly the pre-#489 behavior for every launch. + + Said out loud rather than swallowed, because the degradation is otherwise + invisible: if this fires on every launch, single-instancing has quietly + no-oped and #489's settings clobber is back with no symptom pointing here. + stderr is the right channel — a Windows GUI launch has no console and loses + it harmlessly, while the Unix platforms this is most likely to fire on are + exactly where launching from a terminal is common. A unit test exercises + the named-mutex machinery per platform in CI so a shim that throws fails + loudly there first. */ + Console.Error.WriteLine( + $"PerformanceStudio: single-instance detection unavailable ({ex.GetType().Name}); running as a full instance."); return true; } } diff --git a/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs index ca70cf3..89ddfee 100644 --- a/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs +++ b/tests/PlanViewer.Core.Tests/SingleInstanceTests.cs @@ -1,3 +1,4 @@ +using System.Threading; using System.IO; using System.Linq; using Avalonia.Controls; @@ -221,6 +222,34 @@ public void OpenFromStartupArgsOpensTheFileBehindTheNewInstanceFlag() }); } + /// + /// The named-mutex machinery itself, exercised on whatever platform the suite runs on. + /// + /// The gate review's point: Program.Main's catch-all degrades a throwing mutex to + /// "run fully" — the right call, but if named mutexes ever break wholesale on a platform + /// (the Unix shim backs them with files under /tmp, and PlatformNotSupportedException is + /// the classic wholesale failure), single-instancing would silently no-op there and the + /// #489 clobber would be back with no symptom. This runs on the ubuntu CI runner on every + /// push, so a platform where creating or re-opening a named mutex throws fails HERE, + /// loudly, instead of degrading invisibly in the field. In-process rather than + /// cross-process (the harness cannot spawn app instances), which still traverses the + /// named create and second-open paths the shim has to serve; macOS has no CI leg, so the + /// one-time bare-double-launch smoke there is a release-checklist item, not a test. + /// + [Fact] + public void NamedMutexMachineryWorksOnThisPlatform() + { + // A unique name per run: colliding with a real Studio instance on a dev machine + // (or a parallel test run) would turn this into a flake about unrelated state. + var name = $"{SingleInstance.MutexName}_selftest_{Guid.NewGuid():N}"; + + using var first = new Mutex(initiallyOwned: true, name, out var createdFirst); + Assert.True(createdFirst, "a fresh name must be created, not found"); + + using var second = new Mutex(initiallyOwned: true, name, out var createdSecond); + Assert.False(createdSecond, "a second open of the same name must see the existing mutex"); + } + private static string TempSql(string text) { var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sql");