From 84595aebf0782753988b29447312837b8539865c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:33:09 -0400 Subject: [PATCH 1/5] A second window started with --new-instance no longer duplicates or overwrites the first window's session (F9) --new-instance skipped the single-instance slot entirely, so a second window restored the running window's saved open-tab list. It opened a copy of every tab, shared the running window's scratch buffer ids (so both windows wrote, dropped and swept the same files), and whichever window closed last overwrote the other's saved list. --new-instance now claims the slot first. If it is free, the launch is an ordinary one. If another instance holds it, this process is a secondary: it restores nothing (a file argument still opens, otherwise the usual new tab), never writes the saved list or a scratch buffer, never sweeps the buffer folder, and keeps the list already on disk when it saves settings. It loses crash recovery for its own tabs only; the unsaved-changes prompts are unchanged. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza --- src/PlanViewer.App/MainWindow.FileOps.cs | 15 + .../MainWindow.ScratchPersist.cs | 10 + src/PlanViewer.App/MainWindow.axaml.cs | 18 +- src/PlanViewer.App/Program.cs | 33 +- .../Services/AppSettingsService.cs | 39 ++ src/PlanViewer.App/SingleInstance.cs | 35 +- .../SecondaryInstanceTests.cs | 540 ++++++++++++++++++ .../SettingsFileStoreTests.cs | 4 + 8 files changed, 686 insertions(+), 8 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index a206a1f8..f6f1459a 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -583,9 +583,18 @@ internal List CollectOpenTabEntries() /// /// Saves the restore entries of all currently open tabs — file paths, and since #496 /// scratch buffer entries — docked and detached alike (#490). + /// + /// Not in a secondary instance (): + /// the list on disk is the owner's, and this is the one writer that would replace it + /// with the secondary's own tabs. Every write of the list comes through here — the + /// debounce, the final write in , and the update restart — so the + /// one guard covers all three. /// private void SaveOpenPlans() { + if (SingleInstance.IsSecondaryInstance) + return; + _appSettings.OpenTabs.Clear(); _appSettings.OpenTabs.AddRange(CollectOpenTabEntries()); @@ -627,6 +636,12 @@ timer against a window being torn down. */ if (IsShuttingDown) return; + /* A secondary instance writes no session state (see SaveOpenPlans), so there is + nothing to schedule: no pending flag, and no timer that would tick just to find + the write refused. */ + if (SingleInstance.IsSecondaryInstance) + return; + _sessionPersistPending = true; /* No real timer under the test host, in #451's pattern: the suite shares one diff --git a/src/PlanViewer.App/MainWindow.ScratchPersist.cs b/src/PlanViewer.App/MainWindow.ScratchPersist.cs index 3433e82d..10519a86 100644 --- a/src/PlanViewer.App/MainWindow.ScratchPersist.cs +++ b/src/PlanViewer.App/MainWindow.ScratchPersist.cs @@ -99,6 +99,16 @@ different questions (staleness against on-disk changes, most of all). That is #4 /// private void HookScratchPersistence(QuerySessionControl session) { + /* A secondary instance never persists scratch content. The buffer folder and the ids + in it belong to the instance that owns the slot: writing there would put this + window's text under names the owner also writes, drops and sweeps. With no + subscription nothing is ever queued, no buffer id is minted, and every place that + drops a buffer finds no id and does nothing — the drop sites need no guard of + their own. The cost is crash recovery for this window's scratch tabs; the close + prompts do not depend on it. */ + if (SingleInstance.IsSecondaryInstance) + return; + /* Only sessions that are scratch NOW. SourceFilePath moves null→path exactly once (the save) and never back, so a session arriving here file-backed can never need this hook later — the scope fence again, applied at subscription time. It also diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 24b3d237..bb5a9c72 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -224,7 +224,23 @@ this only changes the cold start. The restore's new-tab fallback stays out of th way when a file is about to open — a stray empty scratch tab beside the file the user asked for is nobody's intent. */ var hasFileArg = args.Length > 1 && File.Exists(args[1]); - RestoreOpenPlans(createFallbackTab: !hasFileArg); + + if (SingleInstance.IsSecondaryInstance) + { + /* A secondary instance (--new-instance beside a running one) does not restore. + The saved list belongs to the instance that owns the slot, which rewrites it on + every tab change: restoring it here would open a copy of every one of its tabs + and share its scratch buffer ids. RestoreOpenPlans is skipped whole, not called + with a flag, because it also clears and saves the list and sweeps the buffer + folder — both of which would land on the owner's files. What is left is what a + launch with nothing to restore does: the file it was given, else a new tab. */ + if (!hasFileArg) + NewQuery_Click(this, new RoutedEventArgs()); + } + else + { + RestoreOpenPlans(createFallbackTab: !hasFileArg); + } if (hasFileArg) OpenFileByExtension(args[1]); diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 6e9f9161..696f61cd 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -50,7 +50,10 @@ 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. */ + its work — a file path, or a bare "surface yourself" — and exits. A launch that + does ask (--new-instance) still gets its own window, but it claims the slot first + and, if another instance already holds it, runs as a secondary that leaves the + saved session alone — see ClaimSlotForNewInstance. */ var newInstanceRequested = SingleInstance.NewInstanceRequested(args); // The flag is a launcher directive, not a file: strip it so nothing downstream can @@ -101,6 +104,10 @@ when the loser's retries run out before the winner's pipe exists. The pre-#489 last-write-wins behavior, which is the accepted floor. */ } } + else + { + ClaimSlotForNewInstance(); + } BuildAvaloniaApp() .StartWithClassicDesktopLifetime(effectiveArgs); @@ -118,16 +125,34 @@ public static AppBuilder BuildAvaloniaApp() .WithInterFont() .LogToTrace(); + /// + /// What --new-instance does about the slot. It never hands its launch to the + /// running instance — the user asked for a window of their own — but it no longer skips + /// the slot either. It claims it the same way an ordinary launch does: + /// + /// Slot free: no other instance is running, so this launch is an ordinary one. It + /// restores, persists and owns the slot, and later launches hand their work to it. + /// Slot held: another instance owns the saved session, so this process runs as a + /// secondary () and leaves that session + /// alone. Before this, it restored the owner's list into a second window, and the two + /// windows then wrote, dropped and swept the same scratch buffers. + /// + /// Split from so the decision can be tested with a mutex name of the + /// test's own; the harness cannot start a second app process to hold the real one. + /// + internal static void ClaimSlotForNewInstance(string mutexName = SingleInstance.MutexName) => + SingleInstance.IsSecondaryInstance = !TryBecomeSingleInstanceOwner(mutexName); + /// /// 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. + /// should hand its work over instead (or, for --new-instance, run as a secondary). /// - private static bool TryBecomeSingleInstanceOwner() + private static bool TryBecomeSingleInstanceOwner(string mutexName = SingleInstance.MutexName) { try { - var mutex = new Mutex(initiallyOwned: true, SingleInstance.MutexName, out var createdNew); + var mutex = new Mutex(initiallyOwned: true, mutexName, out var createdNew); if (createdNew) { _singleInstanceMutex = mutex; diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index f1b4d8bd..6b2862dc 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -193,6 +193,16 @@ public static AppSettings Load() /// while found the file unreadable, which would otherwise overwrite /// content this process has never actually seen. The Settings window reports that case /// through . + /// + /// In a secondary instance () the + /// saved open-tab list is kept as the disk has it. The whole file is written, so the + /// settings object's list goes with it — and a secondary's copy is the one it read at its + /// own startup, long out of date next to the list the owner rewrites on every tab change. + /// Recent plans, the Settings dialog and the server-filter toggle all save through here. + /// The list is taken off the disk first, so the write hands it back unchanged. Every other + /// setting stays last-write-wins, which is what two instances on purpose has always meant + /// (see Program.Main). If the file cannot be read there is no list to keep, so the save is + /// skipped rather than written with a guess. /// public static void Save(AppSettings settings) { @@ -201,6 +211,9 @@ public static void Save(AppSettings settings) try { + if (SingleInstance.IsSecondaryInstance && !TryAdoptSavedOpenTabs(settings)) + return; + Directory.CreateDirectory(SettingsDir); var json = JsonSerializer.Serialize(settings, JsonOptions); AtomicFile.WriteAllText(SettingsPath, json); @@ -212,6 +225,32 @@ public static void Save(AppSettings settings) } } + /// + /// Puts the open-tab list that is on disk right now into , for a + /// secondary instance about to write the whole file (see ). Read the way + /// reads, so a file that will not parse is moved aside rather than + /// overwritten. False when the file exists but cannot be read — the caller must not write. + /// + private static bool TryAdoptSavedOpenTabs(AppSettings settings) + { + var outcome = SettingsFileStore.Read( + SettingsPath, + nameof(AppSettingsService), + json => JsonSerializer.Deserialize(json, JsonOptions), + out var onDisk); + + if (outcome == SettingsFileStore.ReadOutcome.Unreadable) + return false; + + if (onDisk != null) + MigrateOpenTabs(onDisk); + + // Missing, or unparseable and just moved aside: there is no list on disk to keep, and + // an empty one is a truer thing to write than a stale one. + settings.OpenTabs = onDisk?.OpenTabs ?? new List(); + return true; + } + /// /// Moves an "open_plans" list written by an older version onto , /// so upgrading does not cost the user the tabs they had open. Only fills an empty OpenTabs: diff --git a/src/PlanViewer.App/SingleInstance.cs b/src/PlanViewer.App/SingleInstance.cs index 2bd53ad2..c51ec93c 100644 --- a/src/PlanViewer.App/SingleInstance.cs +++ b/src/PlanViewer.App/SingleInstance.cs @@ -47,12 +47,41 @@ internal static class SingleInstance 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. + /// Escape hatch (#489): open a window of its own instead of handing the launch to the + /// running instance. A user who runs two on purpose accepts settings last-write-wins as + /// their informed choice — but not a duplicated session: the launch claims the slot + /// first, and when another instance already holds it this process runs as a secondary + /// (see ). Stripped from argv before any file-open + /// logic sees it. /// internal const string NewInstanceFlag = "--new-instance"; + /// + /// True in a process started with while another instance + /// already owned the single-instance slot. Set once by before any + /// window exists; nothing else in the product sets it. Tests that set it belong in the + /// serial collection, because it changes what every settings save writes. + /// + /// Why a secondary must not own the session. The saved open-tab list and the + /// scratch buffer folder have one writer by design: the owner rewrites the list on every + /// tab change, names buffers by ids only it knows, and sweeps any buffer its own list + /// does not name. A second process that restored that list would open a copy of every + /// tab, share the owner's buffer ids so that both windows write, drop and sweep the same + /// files, and whichever window closed last would overwrite the other's list. + /// + /// What a secondary does instead. It restores nothing (a file argument still + /// opens; otherwise the usual new tab), never writes the list or a scratch buffer, never + /// sweeps the buffer folder, and keeps the list already on disk when it saves settings + /// (see AppSettingsService.Save). It loses crash recovery for its own tabs and + /// nothing else: the unsaved-changes prompts do not depend on persistence and work + /// as they always have. + /// + /// Not set on the launch path where a non-owner runs fully after the pipe hand-off + /// failed (see Program.Main): that launch never asked for a second window, so it + /// keeps the pre-#489 behavior. + /// + internal static bool IsSecondaryInstance { get; set; } + /// /// 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 diff --git a/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs b/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs new file mode 100644 index 00000000..44f1939b --- /dev/null +++ b/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs @@ -0,0 +1,540 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using System.Threading; +using Avalonia.Controls; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #489 review: a window started with --new-instance beside a running Studio used to +/// restore the running one's saved open-tab list, so it opened a copy of every tab, shared the +/// running window's scratch buffer ids (both then wrote, dropped and swept the same files), and +/// whichever window closed last overwrote the other's list. --new-instance now claims the +/// single-instance slot first; when another instance holds it, this process is a secondary. A +/// secondary restores nothing, never writes the list or a scratch buffer, never sweeps the +/// buffer folder, and keeps the list already on disk when it saves settings. It loses crash +/// recovery for its own tabs and nothing else. +/// +/// What "leaves it alone" is checked against. Every secondary test stages what a +/// running owner leaves on disk — a list with a file and a scratch entry, that scratch's buffer, +/// and an old orphaned buffer that an ordinary start would sweep — and compares the settings file +/// and the buffer folder before and after, content AND last-write time, so a write of identical +/// bytes still fails. The test host arms no real timers, so each test drives the flushes itself +/// (, +/// ) and closes the window through +/// its real close path: a test that only waited would pass for the wrong reason. +/// +/// Why this joins the serial collection. +/// is process-wide and changes what writes, the same kind of +/// hazard as the save block the collection was made for. A test in another class that saved +/// settings while it was on would have its open-tab list replaced with the one on disk. +/// +[Collection("SettingsFileStore serial")] +public class SecondaryInstanceTests +{ + private const string OwnerScratchText = "SELECT 2 AS owner_scratch;"; + + // ── the secondary ───────────────────────────────────────────────────── + + [Fact] + public void ASecondaryWindowRestoresNoneOfTheSavedListAndLeavesItAlone() + { + AsSecondaryOverOwnerSession((window, owner) => + { + var sessions = Sessions(window).ToList(); + Assert.DoesNotContain(sessions, s => s.SourceFilePath == owner.SqlPath); + Assert.DoesNotContain(sessions, s => s.ScratchBufferId == owner.ScratchId); + Assert.DoesNotContain(sessions, s => s.QueryEditor.Text == OwnerScratchText); + + /* What a launch with nothing to restore does: the usual new tab, empty and clean. */ + var only = Assert.Single(window.MainTabControl.Items.OfType()); + var session = Assert.IsType(only.Content); + Assert.Null(session.SourceFilePath); + Assert.False(session.IsDirty); + + /* Nothing pending anywhere, and a forced flush of both writers stays inert. */ + window.FlushPendingSessionPersistForTests(); + window.FlushPendingScratchPersistForTests(); + }); + } + + [Fact] + public void ASecondaryStillOpensItsFileArgumentAndNothingElse() + { + AsSecondaryOverOwnerSession((window, owner) => + { + var fileArg = TempSql("SELECT 1 AS launched_with_this;"); + try + { + var tabsBefore = window.MainTabControl.Items.Count; // the constructor's usual new tab + + window.OpenFromStartupArgs(new[] { "PerformanceStudio.exe", "--new-instance", fileArg }); + + var sessions = Sessions(window).ToList(); + Assert.Single(sessions, s => s.SourceFilePath == fileArg); + Assert.Equal(tabsBefore + 1, window.MainTabControl.Items.Count); // the file, no fallback tab + Assert.DoesNotContain(sessions, s => s.SourceFilePath == owner.SqlPath); + Assert.DoesNotContain(sessions, s => s.ScratchBufferId == owner.ScratchId); + } + finally + { + File.Delete(fileArg); + } + }); + } + + [Fact] + public void ScratchTabsInASecondaryNeverTouchTheSavedListOrTheBufferFolder() + { + AsSecondaryOverOwnerSession((window, _) => + { + var first = NewScratchTab(window, "SELECT 1 AS typed_in_secondary;"); + window.FlushPendingScratchPersistForTests(); + + first.QueryEditor.Text = "SELECT 1 AS edited_in_secondary;"; + window.FlushPendingScratchPersistForTests(); + + NewScratchTab(window, "SELECT 2 AS second_scratch;"); + window.FlushPendingSessionPersistForTests(); + window.PersistSessionForRestart(); // the update-restart write + + /* Close the first tab through the real button and answer Don't Save: the moment an + ordinary window deletes the buffer it wrote for that tab. */ + CloseButton(TabOf(window, first)).RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + AnswerDontSave(window); + Assert.DoesNotContain(first, Sessions(window)); + window.FlushPendingScratchPersistForTests(); + + /* Then close the window with the second tab still dirty: the walk asks, Don't Save + lets it through, and OnClosed runs its final drain and list write. */ + window.Close(); + Dispatcher.UIThread.RunJobs(); + AnswerDontSave(window); + Assert.False(window.IsVisible); + }); + } + + [Fact] + public void ASecondaryStillAsksAboutADirtyScratchTabWhenItCloses() + { + AsSecondaryOverOwnerSession((window, _) => + { + var session = NewScratchTab(window, "SELECT 1 AS unsaved_in_secondary;"); + window.FlushPendingScratchPersistForTests(); // persistence is off, so no buffer holds this text + + Assert.True(window.CloseNeedsConfirmation()); + + window.Close(); + Dispatcher.UIThread.RunJobs(); + + Assert.True(window.IsVisible, "the close is held while the question is up"); + var prompt = Assert.Single(window.OwnedWindows); + + /* Dismissing the prompt is Cancel: the window stays and the text is still there. */ + prompt.Close(); + Dispatcher.UIThread.RunJobs(); + Assert.True(window.IsVisible); + Assert.Equal("SELECT 1 AS unsaved_in_secondary;", session.QueryEditor.Text); + + window.Close(); + Dispatcher.UIThread.RunJobs(); + AnswerDontSave(window); + Assert.False(window.IsVisible); + }); + } + + [Fact] + public void ASecondarysSettingsSaveKeepsTheListThatIsOnDisk() + { + AsSecondaryOverOwnerSession((window, owner) => + { + /* The owner keeps working after this window started: it opens another tab and + rewrites the list. Written straight to the file, as another process would, so this + process's cached settings still hold the list they read at startup. */ + var opened = Path.Combine(Path.GetTempPath(), "opened_by_the_owner_later.sql"); + var later = new List + { + owner.SqlPath, + ScratchBufferStore.EntryFor(owner.ScratchId), + opened, + }; + var ownerCopy = AppSettingsService.Load().Clone(); + ownerCopy.OpenTabs = later; + File.WriteAllText(AppSettingsService.SettingsFilePath, JsonSerializer.Serialize(ownerCopy)); + + /* A save the secondary makes for its own reasons: opening a plan puts it on the + Recent Plans list, which writes the whole settings file. */ + var plan = Path.Combine(AppContext.BaseDirectory, "Plans", "row_goal_plan.sqlplan"); + window.LoadPlanFile(plan); + + var saved = ReadSettingsFile(); + Assert.Equal(later, saved.OpenTabs); + Assert.Contains(Path.GetFullPath(plan), saved.RecentPlans); // the save itself did happen + }, + filesMustStay: false); + } + + [Fact] + public void ASecondaryThatCannotReadTheSavedListDoesNotSave() + { + Assert.SkipUnless(OperatingSystem.IsWindows(), + "Unix permissions don't reliably block a same-user read the way FileShare.None does on Windows."); + + var owner = StageOwnerSession(); + var path = AppSettingsService.SettingsFilePath; + var settings = AppSettingsService.Load(); + var originalDays = settings.QueryStoreSlicerDays; + try + { + var before = File.ReadAllBytes(path); + settings.QueryStoreSlicerDays = 91; + + SingleInstance.IsSecondaryInstance = true; + using (new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.None)) + { + AppSettingsService.Save(settings); // must not throw + } + SingleInstance.IsSecondaryInstance = false; + + /* Not written, and not even attempted: an attempt would have staged a .tmp + sibling and then failed on the rename. With no way to see the list there is no + list to keep, and a guess is not written. */ + Assert.True(before.AsSpan().SequenceEqual(File.ReadAllBytes(path))); + Assert.False(File.Exists(path + ".tmp")); + } + finally + { + SingleInstance.IsSecondaryInstance = false; + settings.QueryStoreSlicerDays = originalDays; + File.Delete(path + ".tmp"); + ResetState(owner); + } + } + + // ── the slot claim ──────────────────────────────────────────────────── + + [Fact] + public void ANewInstanceLaunchBesideAnOwnerRunsAsASecondary() + { + var name = UniqueMutexName(); + using var runningOwner = new Mutex(initiallyOwned: true, name, out var held); + Assert.True(held); + + try + { + PlanViewer.App.Program.ClaimSlotForNewInstance(name); + + Assert.True(SingleInstance.IsSecondaryInstance); + } + finally + { + SingleInstance.IsSecondaryInstance = false; + } + } + + [Fact] + public void ANewInstanceLaunchWithNoOtherOwnerClaimsTheSlotAndRestoresNormally() + { + var name = UniqueMutexName(); + + HeadlessUi.Run(() => + { + var owner = StageOwnerSession(); + MainWindow? window = null; + try + { + PlanViewer.App.Program.ClaimSlotForNewInstance(name); + + Assert.False(SingleInstance.IsSecondaryInstance, "the slot was free, so this launch owns it"); + + /* And it holds the slot now: the next launch finds the name taken and hands its + work over, which is the whole point of claiming it. */ + using (var probe = new Mutex(initiallyOwned: false, name, out var created)) + Assert.False(created); + + window = new MainWindow(); + window.Show(); + AssertOwnsTheSavedSession(window, owner); + } + finally + { + SingleInstance.IsSecondaryInstance = false; + PutAway(window); + ResetState(owner); + } + }); + } + + [Fact] + public void ANormalLaunchIsUnchanged() + { + HeadlessUi.Run(() => + { + var owner = StageOwnerSession(); + MainWindow? window = null; + try + { + Assert.False(SingleInstance.IsSecondaryInstance, "nothing sets the flag on an ordinary launch"); + + window = new MainWindow(); + window.Show(); + AssertOwnsTheSavedSession(window, owner); + } + finally + { + PutAway(window); + ResetState(owner); + } + }); + } + + // ── plumbing ────────────────────────────────────────────────────────── + + /// What a running owner leaves on disk, as far as these tests care. + private sealed record OwnerSession(string SqlPath, Guid ScratchId, Guid OrphanId); + + /// + /// The whole secondary scenario, through the same door production uses: a slot already held + /// by "the running instance" (a mutex of this test's own name), a --new-instance + /// launch claiming it and finding it taken, and then a window built under the flag that + /// leaves behind. The window is closed while still a secondary, and unless a test says + /// otherwise the owner's files must come out exactly as they went in. + /// + private static void AsSecondaryOverOwnerSession( + Action body, bool filesMustStay = true) + { + HeadlessUi.Run(() => + { + var owner = StageOwnerSession(); + MainWindow? window = null; + try + { + var before = Snapshot(); + + var name = UniqueMutexName(); + using var runningOwner = new Mutex(initiallyOwned: true, name, out var held); + Assert.True(held); + PlanViewer.App.Program.ClaimSlotForNewInstance(name); + Assert.True(SingleInstance.IsSecondaryInstance, "the slot is held, so this launch is a secondary"); + + try + { + window = new MainWindow(); + window.Show(); + body(window, owner); + } + finally + { + PutAway(window); + SingleInstance.IsSecondaryInstance = false; + } + + if (filesMustStay) + AssertUnchanged(before); + } + finally + { + ResetState(owner); + } + }); + } + + /// + /// What ordinary startup did before and still does: restore the list in place, sweep debris, + /// keep the restored scratch's buffer, and keep persisting new scratch content and the list. + /// + private static void AssertOwnsTheSavedSession(MainWindow window, OwnerSession owner) + { + var sessions = Sessions(window).ToList(); + Assert.Equal(2, window.MainTabControl.Items.Count); // the file and the scratch, no extra new tab + Assert.Single(sessions, s => s.SourceFilePath == owner.SqlPath); + + var scratch = Assert.Single(sessions, s => s.ScratchBufferId == owner.ScratchId); + Assert.Equal(OwnerScratchText, scratch.QueryEditor.Text); + Assert.True(scratch.IsDirty); + + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(owner.ScratchId))); + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(owner.OrphanId)), + "an ordinary start sweeps a buffer nothing lists"); + Assert.Equal( + new[] { owner.SqlPath, ScratchBufferStore.EntryFor(owner.ScratchId) }, + ReadSettingsFile().OpenTabs); + + var typed = NewScratchTab(window, "SELECT 9 AS typed_by_the_owner;"); + window.FlushPendingScratchPersistForTests(); + + var id = typed.ScratchBufferId; + Assert.NotNull(id); + Assert.Equal( + "SELECT 9 AS typed_by_the_owner;", + File.ReadAllText(ScratchBufferStore.BufferPathFor(id!.Value))); + Assert.Contains(ScratchBufferStore.EntryFor(id.Value), ReadSettingsFile().OpenTabs); + } + + /// + /// Writes what a running owner leaves behind: the list (a file tab, then a scratch tab), the + /// scratch tab's buffer, and one buffer nothing lists that is old enough for a start-up sweep + /// to delete. Done in ordinary mode, before any secondary exists, and left in the shared + /// cache as well as on disk, which is where the next window reads its list from. + /// + private static OwnerSession StageOwnerSession() + { + var sqlPath = TempSql("SELECT 1 AS owner_file;"); + var scratchId = Guid.NewGuid(); + var orphanId = Guid.NewGuid(); + + ScratchBufferStore.Write(scratchId, OwnerScratchText); + ScratchBufferStore.Write(orphanId, "SELECT 3 AS orphan;"); + File.SetLastWriteTimeUtc( + ScratchBufferStore.BufferPathFor(orphanId), + DateTime.UtcNow - ScratchBufferStore.OrphanGracePeriod - TimeSpan.FromDays(1)); + + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + settings.OpenTabs.Add(sqlPath); + settings.OpenTabs.Add(ScratchBufferStore.EntryFor(scratchId)); + AppSettingsService.Save(settings); + + return new OwnerSession(sqlPath, scratchId, orphanId); + } + + /// Leaves nothing for the next test's window to restore or sweep. + private static void ResetState(OwnerSession? owner) + { + SingleInstance.IsSecondaryInstance = false; // the save below has to be an ordinary one + + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + AppSettingsService.Save(settings); + + foreach (var file in ScratchFiles()) + { + try + { + File.Delete(file); + } + catch + { + // Best-effort — a stuck file is swept by a later window anyway. + } + } + + if (owner != null) + File.Delete(owner.SqlPath); + } + + /// + /// The settings file and every file in the scratch folder, with the time each was last + /// written. Content alone would let a rewrite of identical bytes through. + /// + private static SortedDictionary Snapshot() + { + var snapshot = new SortedDictionary(StringComparer.Ordinal); + + void Add(string key, string path) => + snapshot[key] = (File.ReadAllBytes(path), File.GetLastWriteTimeUtc(path).Ticks); + + Add("appsettings.json", AppSettingsService.SettingsFilePath); + foreach (var file in ScratchFiles()) + Add("scratch/" + Path.GetFileName(file), file); + + return snapshot; + } + + private static void AssertUnchanged(SortedDictionary before) + { + var after = Snapshot(); + + Assert.Equal(before.Keys.ToArray(), after.Keys.ToArray()); + foreach (var (name, was) in before) + { + Assert.True(was.Bytes.AsSpan().SequenceEqual(after[name].Bytes), $"{name} changed"); + Assert.True(was.Written == after[name].Written, $"{name} was written to"); + } + } + + private static AppSettings ReadSettingsFile() => + JsonSerializer.Deserialize(File.ReadAllText(AppSettingsService.SettingsFilePath))!; + + private static string[] ScratchFiles() => + Directory.Exists(AppSettingsService.ScratchDirectory) + ? Directory.GetFiles(AppSettingsService.ScratchDirectory) + : Array.Empty(); + + /// + /// A name of this test's own, so nothing here can meet the real slot on a developer's + /// machine, or another run's. Same reasoning as SingleInstanceTests' mutex self-test. + /// + private static string UniqueMutexName() => + $"{SingleInstance.MutexName}_secondary_{Guid.NewGuid():N}"; + + private static QuerySessionControl NewScratchTab(MainWindow window, string text) + { + window.NewQuery_Click(window, new RoutedEventArgs()); + var session = Sessions(window).Last(); + session.QueryEditor.Text = text; + return session; + } + + /// Clicks Don't Save on the one prompt the window is asking. + private static void AnswerDontSave(MainWindow window) + { + PromptButton(Assert.Single(window.OwnedWindows), "Don't Save") + .RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + } + + /// + /// Prompts closed, every session settled, window shut — from a finally, because the run + /// where it matters is the run where an assertion above it failed (#474). + /// + private static void PutAway(MainWindow? window) + { + if (window == null || !window.IsVisible) + return; + + foreach (var prompt in window.OwnedWindows.ToList()) + prompt.Close(); + + foreach (var session in Sessions(window).ToList()) + session.MarkClean(); + + Dispatcher.UIThread.RunJobs(); + window.Close(); + Dispatcher.UIThread.RunJobs(); + } + + private static TabItem TabOf(MainWindow window, QuerySessionControl session) => + window.MainTabControl.Items.OfType().Single(t => t.Content == session); + + private static Button CloseButton(TabItem tab) => + ((StackPanel)tab.Header!).Children.OfType [CollectionDefinition("SettingsFileStore serial", DisableParallelization = true)] public class SettingsFileStoreSerialCollection From 2a63145a04d023e4576161b9283248399779b25b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:36:06 -0400 Subject: [PATCH 2/5] Check the owner's files right after a secondary starts, and note why RestoreOpenPlans is skipped The secondary tests now keep the settings file and buffer folder as the owner left them and compare them at start-up as well as at the end, so the sweep and the clear-and-save that an ordinary start does are caught on their own. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza --- src/PlanViewer.App/MainWindow.FileOps.cs | 5 ++++ src/PlanViewer.App/Program.cs | 1 + .../SecondaryInstanceTests.cs | 28 +++++++++++++------ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index f6f1459a..933b98f8 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -756,6 +756,11 @@ internal void PersistSessionForRestart() /// the poison defense wholesale — the entry is already off the cleared list before its /// buffer is read, and a buffer that fails to load is skipped, never re-added, and /// deleted (). + /// + /// Never in a secondary instance. Everything above is written for the one + /// instance that owns the saved session: the clear-and-save empties the owner's list on + /// disk, and the sweep deletes buffers the owner still needs. + /// skips this method when is set. /// /// /// Whether an empty restore opens a fresh query tab. False when the caller is about to diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 696f61cd..0a128929 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -106,6 +106,7 @@ when the loser's retries run out before the winner's pipe exists. The } else { + // --new-instance: never handed to a running instance, but it still claims the slot. ClaimSlotForNewInstance(); } diff --git a/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs b/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs index 44f1939b..bfb89fec 100644 --- a/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs +++ b/tests/PlanViewer.Core.Tests/SecondaryInstanceTests.cs @@ -49,6 +49,10 @@ public void ASecondaryWindowRestoresNoneOfTheSavedListAndLeavesItAlone() { AsSecondaryOverOwnerSession((window, owner) => { + /* Starting up alone is enough to have touched them: an ordinary start clears the list + and saves it empty before it opens anything, then sweeps the buffer folder. */ + AssertUnchanged(owner); + var sessions = Sessions(window).ToList(); Assert.DoesNotContain(sessions, s => s.SourceFilePath == owner.SqlPath); Assert.DoesNotContain(sessions, s => s.ScratchBufferId == owner.ScratchId); @@ -299,8 +303,15 @@ public void ANormalLaunchIsUnchanged() // ── plumbing ────────────────────────────────────────────────────────── - /// What a running owner leaves on disk, as far as these tests care. - private sealed record OwnerSession(string SqlPath, Guid ScratchId, Guid OrphanId); + /// + /// What a running owner leaves on disk, as far as these tests care, and the settings file and + /// buffer folder exactly as it left them (see ). + /// + private sealed record OwnerSession( + string SqlPath, + Guid ScratchId, + Guid OrphanId, + SortedDictionary OnDisk); /// /// The whole secondary scenario, through the same door production uses: a slot already held @@ -318,8 +329,6 @@ private static void AsSecondaryOverOwnerSession( MainWindow? window = null; try { - var before = Snapshot(); - var name = UniqueMutexName(); using var runningOwner = new Mutex(initiallyOwned: true, name, out var held); Assert.True(held); @@ -339,7 +348,7 @@ private static void AsSecondaryOverOwnerSession( } if (filesMustStay) - AssertUnchanged(before); + AssertUnchanged(owner); } finally { @@ -404,7 +413,7 @@ private static OwnerSession StageOwnerSession() settings.OpenTabs.Add(ScratchBufferStore.EntryFor(scratchId)); AppSettingsService.Save(settings); - return new OwnerSession(sqlPath, scratchId, orphanId); + return new OwnerSession(sqlPath, scratchId, orphanId, Snapshot()); } /// Leaves nothing for the next test's window to restore or sweep. @@ -450,12 +459,13 @@ void Add(string key, string path) => return snapshot; } - private static void AssertUnchanged(SortedDictionary before) + /// The settings file and buffer folder are exactly as the owner left them. + private static void AssertUnchanged(OwnerSession owner) { var after = Snapshot(); - Assert.Equal(before.Keys.ToArray(), after.Keys.ToArray()); - foreach (var (name, was) in before) + Assert.Equal(owner.OnDisk.Keys.ToArray(), after.Keys.ToArray()); + foreach (var (name, was) in owner.OnDisk) { Assert.True(was.Bytes.AsSpan().SequenceEqual(after[name].Bytes), $"{name} changed"); Assert.True(was.Written == after[name].Written, $"{name} was written to"); From a97961c2cb81d145c57a3542a07a5bbaa0a4a9c4 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:42:13 -0400 Subject: [PATCH 3/5] Say what a secondary loses: session restore for its own tabs The comment said crash recovery only. Its file tabs are not reopened at the next start after a clean close either, as the PR body already says. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza --- src/PlanViewer.App/SingleInstance.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/PlanViewer.App/SingleInstance.cs b/src/PlanViewer.App/SingleInstance.cs index c51ec93c..2eb1ea0f 100644 --- a/src/PlanViewer.App/SingleInstance.cs +++ b/src/PlanViewer.App/SingleInstance.cs @@ -72,9 +72,10 @@ internal static class SingleInstance /// What a secondary does instead. It restores nothing (a file argument still /// opens; otherwise the usual new tab), never writes the list or a scratch buffer, never /// sweeps the buffer folder, and keeps the list already on disk when it saves settings - /// (see AppSettingsService.Save). It loses crash recovery for its own tabs and - /// nothing else: the unsaved-changes prompts do not depend on persistence and work - /// as they always have. + /// (see AppSettingsService.Save). What it loses is session restore for its own + /// tabs: they are not reopened at the next start, whether it closed cleanly or crashed, + /// and its scratch tabs have no crash recovery. The unsaved-changes prompts do not depend + /// on persistence and work as they always have. /// /// Not set on the launch path where a non-owner runs fully after the pipe hand-off /// failed (see Program.Main): that launch never asked for a second window, so it From d019ef4bf08531aab9de2458a01ac10e59ae9807 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:52:52 -0400 Subject: [PATCH 4/5] A secondary does not start the pipe server The listener retries until it gets the pipe, so a secondary took it once the owner exited, and later launches handed their files to a window whose tabs are never saved. Without it, such a launch finds no pipe, claims the slot, and runs as the new owner. Also notes that --new-instance acts as an owner where named mutexes fail. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza --- src/PlanViewer.App/MainWindow.axaml.cs | 7 ++++++- src/PlanViewer.App/Program.cs | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index bb5a9c72..ab7494d8 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -70,7 +70,12 @@ public MainWindow() // 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. - if (!AppRuntimeMode.IsTestHost) + // Not in a secondary instance either: the pipe belongs to the instance that owns the + // slot. The listener retries until it gets the pipe, so a secondary would take it once + // the owner exits, and later launches would hand their files to a window whose tabs are + // never saved. Without it, such a launch finds no pipe, claims the slot, and runs as the + // new owner. + if (!AppRuntimeMode.IsTestHost && !SingleInstance.IsSecondaryInstance) StartPipeServer(); InitializeComponent(); diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 0a128929..f932e35c 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -140,6 +140,12 @@ public static AppBuilder BuildAvaloniaApp() /// /// Split from so the decision can be tested with a mutex name of the /// test's own; the harness cannot start a second app process to hold the real one. + /// + /// Where named mutexes do not work at all, + /// answers true so that the launch still runs, and this launch then acts as an owner: it + /// restores the saved session even if another instance is running. That is how every + /// --new-instance launch behaved before, and it only happens where the mutex + /// machinery itself fails. /// internal static void ClaimSlotForNewInstance(string mutexName = SingleInstance.MutexName) => SingleInstance.IsSecondaryInstance = !TryBecomeSingleInstanceOwner(mutexName); From a6636a8f3442ff26954c5fa0d73369be44622f79 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:57:40 -0400 Subject: [PATCH 5/5] Say that the secondary's settings save can still lose a concurrent owner save The comment said the write hands the list back unchanged. An owner save between the read and the write is still lost, as the PR body already says. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza --- src/PlanViewer.App/Services/AppSettingsService.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index 6b2862dc..d8f8b40c 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -199,7 +199,9 @@ public static AppSettings Load() /// settings object's list goes with it — and a secondary's copy is the one it read at its /// own startup, long out of date next to the list the owner rewrites on every tab change. /// Recent plans, the Settings dialog and the server-filter toggle all save through here. - /// The list is taken off the disk first, so the write hands it back unchanged. Every other + /// The list is read off the disk just before the write, so the write hands back what the + /// owner last saved. An owner save that lands between that read and the write is still + /// lost; nothing locks the file across processes, and the gap is milliseconds. Every other /// setting stays last-write-wins, which is what two instances on purpose has always meant /// (see Program.Main). If the file cannot be read there is no list to keep, so the save is /// skipped rather than written with a guess.