A second window from --new-instance no longer duplicates or overwrites the first window's session (F9)

[erikdarlingdata]

Summary

A window started with --new-instance beside a running Studio restored the running window's saved tab list. It opened a copy of every tab. Its scratch tabs used the same buffer ids as the first window's tabs, so both windows wrote, dropped, and swept the same buffer files. The window that closed last overwrote the other window's saved list.

--new-instance now claims the single-instance slot first. If no other instance holds the slot, the launch is an ordinary one. If another instance holds it, this process runs as a secondary and leaves the saved session alone.

This is review finding F9. There is no issue to close.

Behavior by launch kind

Launch	Before	After
Bare launch	Claims the slot, or asks the running instance to surface itself	No change
Launch with a file	Sends the file to the running instance over the pipe	No change
--new-instance, slot free	Never touched the slot. The window ran as a full instance that did not own the slot. A later bare launch then found the slot free, claimed it, and ran as a second full window.	Claims the slot and runs as an ordinary launch. It restores, saves, and owns the slot, so later launches hand their work to it.
--new-instance, slot held	Ran as a full instance. It restored the running instance's list, shared its buffer ids, and the last window to close overwrote the other's list.	Runs as a secondary, described below.

The --new-instance launch never hands its work to a running instance, because the user asked for a window of their own. That is unchanged.

What the secondary skips

• It restores nothing. It still opens the file from its command line. Without a file, it opens the usual new tab.
• It never writes the saved open-tab list. This covers the debounced write, the write on close, and the write before an update restart.
• It never writes, drops, or sweeps a scratch buffer file. It does not subscribe to scratch edits, so no buffer id is ever created.
• It never writes a stale open-tab list back through a settings save. The next section explains how.
• It does not start the pipe server. The listener retries until it gets the pipe, so a secondary used to take it once the owner exited. Later launches then handed their files to a window that does not save its tabs. Now such a launch finds no pipe, claims the slot, and runs as the new owner. The test host never starts the pipe server, so no test covers this guard. It was added after review round 1, in d019ef4.

The secondary loses one thing. Its own tabs are not remembered for the next start, after a clean close or after a crash. Nothing else changes for it. The unsaved-changes prompts do not depend on persistence. A test checks that closing a secondary with a dirty scratch tab still asks, and it does, so the prompt code is unchanged.

The settings save

Every whole-file settings save writes the open_tabs list that the process read at startup. In a secondary, that list is stale. Recent plans, the Settings window, the server-filter toggle, and the format-settings migration all save this way.

The smallest safe change is one guard in AppSettingsService.Save, so it covers every one of those paths. In a secondary, Save first reads the settings file the way Load does, with SettingsFileStore.Read. It puts the open_tabs from disk into the settings object, and then it writes the file.

• If the file is missing or does not parse, Save writes an empty list. The read moves a bad file aside first. An empty list is not a stale one.
• If the file exists but cannot be read, Save skips the write.
• Every other setting stays last-write-wins, as Program.cs already accepts.

How it works

Program.Main calls the new Program.ClaimSlotForNewInstance for a --new-instance launch. The method sets SingleInstance.IsSecondaryInstance when another instance holds the slot. It takes a mutex name, so tests can use their own name and never meet the real slot.

These places check the flag:

• MainWindow.OpenFromStartupArgs skips RestoreOpenPlans as a whole. That method also clears the saved list and sweeps the buffer folder, so a flag inside it is not enough.
• MainWindow.SaveOpenPlans returns first. The debounce, the final write on close, and the update restart all come through it.
• MainWindow.RequestSessionPersist schedules nothing.
• MainWindow.HookScratchPersistence subscribes to nothing. With no subscription, no buffer id exists, and every place that drops a buffer finds no id and does nothing.
• AppSettingsService.Save keeps the list on disk, as described above.

Tests

The new class SecondaryInstanceTests has 9 tests. Each one stages what a running owner leaves on disk. That is a list with a file entry and a scratch entry. It also holds the scratch tab's buffer and an old orphan buffer that an ordinary start sweeps.

The tests compare the settings file and the buffer folder before and after. They compare content and last-write time, so a rewrite of identical bytes also fails. The test host arms no timers. So the tests call the flush methods themselves and close the window through the real close path.

The secondary tests build their scenario the way production does. A mutex stands for the running instance. The test calls ClaimSlotForNewInstance, and then it builds a window.

1. A secondary restores none of the saved list and leaves the files as they were, including at start-up.
2. A secondary still opens its file argument, and nothing else.
3. Opening, editing, and closing scratch tabs in a secondary leaves the list and the buffer folder unchanged, byte for byte. The test includes a forced flush and the update-restart write.
4. A secondary still asks about a dirty scratch tab when it closes. Cancel keeps the window and the text.
5. A secondary's settings save keeps the list that is on disk, and the save itself still happens.
6. A secondary that cannot read the list does not save. This test runs on Windows only.
7. A --new-instance launch beside a held slot runs as a secondary.
8. A --new-instance launch with no other owner claims the slot and restores normally.
9. A normal launch restores, sweeps, and persists as before.

Fail without the fix: I reverted the four files that hold the guards, MainWindow.axaml.cs, MainWindow.FileOps.cs, MainWindow.ScratchPersist.cs, and AppSettingsService.cs. I kept SingleInstance.cs and Program.cs, because the tests compile against them. Six of the nine tests failed and three passed. The three that passed are tests 7, 8, and 9. They check the slot claim and the unchanged ordinary start, so they must pass either way. I then restored the files.

Full suite on this branch: total 1278, succeeded 1248, failed 0, skipped 30. The 30 skips are existing platform skips, and none are in the new class. dotnet build PlanViewer.sln -c Debug reports 0 warnings and 0 errors.

The new class is in the SettingsFileStore serial collection. The flag is process-wide and it changes what Save writes, so no other test class can run beside it.

Not done

• The launch path in Program.Main where a non-owner runs fully after the pipe hand-off failed is unchanged. That launch is not a secondary. It still restores and saves, and it is still last-write-wins.
• A secondary reads the settings file and then writes it. The owner can write in the gap between the two, so its newest list can still be lost in that gap. The gap is a few milliseconds. A lock across processes is a larger change.
• Settings other than open_tabs stay last-write-wins in a secondary. The saved connections file and the integrations file also stay last-write-wins.
• Where named mutexes do not work at all, a --new-instance launch acts as an owner. It restores the saved session, as every such launch did before. A comment in Program.ClaimSlotForNewInstance says so.
• The MCP startup is unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

[claude]

Reviewed the diff. I found no correctness, untrusted-input or security problems, and no version, csproj or T-SQL convention issues. I did not build or run the tests.

Two edge cases to consider. Neither blocks the merge.

1. Secondary outliving the owner. IsSecondaryInstance is set once at startup and never cleared. If the owner exits first, the secondary keeps running with no persistence and no restore, and it holds no mutex. MainWindow.StartPipeServer still runs in the secondary. Once the owner's pipe is gone, the secondary can take the single SQLPerformanceStudio_OpenFile pipe slot. A later bare launch would then get the mutex, find the pipe taken by a non-owner, and forward files to the secondary. That is harmless, but the secondary's tabs would never be saved, so a crash there loses them. Consider skipping StartPipeServer when IsSecondaryInstance is true. I confirmed that method has no such guard. I did not check how it handles CreatePipeServer failing while the owner holds the pipe.

2. TryBecomeSingleInstanceOwner catch path. For the mutex-unavailable case (ACL mismatch or a sandbox), its catch block claims ownership. In --new-instance mode that means such a launch will restore the owner's tabs, which is the bug this PR fixes. It only applies where the mutex is broken, so it is Low severity. A comment noting it would be enough.

Test coverage looks thorough for the changed paths. The read-failure test only runs on Windows and is skipped elsewhere, which is fine.

[claude]

Reviewed the diff. The design holds together: every writer of the open-tab list goes through SaveOpenPlans, so the one guard covers the debounce, close and update-restart writes. Restore, the scratch hooks and the pipe server are all gated on IsSecondaryInstance. The tests compare the settings file and buffer folder by content and write time. I didn't build or run the tests.

Two minor points, neither blocking:

1. Lost-update window in AppSettingsService.Save. The secondary reads the on-disk OpenTabs in TryAdoptSavedOpenTabs and then writes the whole file. If the owner saves a tab change between that read and the write, the owner's newer list is overwritten with the older one. The window is small and needs a secondary settings save at the same moment as an owner tab change, so it's low impact. The comment says the list is 'handed back unchanged', which overstates it slightly; a sentence noting the race would be more accurate.
2. No test for the failure path. I saw no test for TryAdoptSavedOpenTabs returning false, meaning an unreadable settings file makes a secondary skip the save entirely. The docs promise that behavior, so it's worth covering. This is from reading the diff; I didn't search the test file for it.

I found no problems on the repo conventions: no new warnings or NoWarn, no version bump, no new Core files for Blazor, and no T-SQL.

[erikdarlingdata]

Round 2 notes: the comment in AppSettingsService.Save now says an owner save between the read and the write is still lost (the PR body already listed it under Not done). The unreadable-file path has a test, ASecondaryThatCannotReadTheSavedListDoesNotSave, which runs on Windows only.

[claude]

Reviewed the diff. I found no correctness, untrusted-input, or convention problems (no new warnings, and no version, T-SQL or Blazor-link changes).

One low-severity behavior to be aware of: IsSecondaryInstance is fixed at launch. If the owner exits while the secondary is still running, the secondary never becomes the owner. It keeps skipping the pipe server and session persistence for the rest of its life, and a later plain launch claims the mutex and starts a second owner. The docs describe most of this, but not the hand-over case, so a one-line note or a follow-up issue would be enough.

Test coverage of the changed paths looks thorough. ASecondaryThatCannotReadTheSavedListDoesNotSave is skipped on non-Windows.