Surface the running instance on a bare second launch instead of clobbering settings - #494
Conversation
…bbering 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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)) |
There was a problem hiding this comment.
The "probe-before-mutex is version-skew-proof" claim (comment above, lines 64-68) only holds for launches that carry a file argument. A bare launch skips this TrySendToRunningInstance probe entirely (effectiveArgs.Length > 0 is false) and goes straight to TryBecomeSingleInstanceOwner().
If the currently-running instance predates this change (or is otherwise a process that never claimed SingleInstance.MutexName), TryBecomeSingleInstanceOwner() succeeds (createdNew == true) because nobody holds the mutex yet. The if (!TryBecomeSingleInstanceOwner()) block — the only place that retries the pipe with the sentinel — is then skipped, and this launch falls straight through to StartWithClassicDesktopLifetime, running a full second instance alongside the old one.
That's exactly the #489 clobber scenario (two full instances, each with its own AppSettings snapshot, last exit wins) reproduced for the one combination the PR doesn't cover: a bare second launch during version skew. Every other case (with-file launch during skew, any launch once both sides are on this build) is handled correctly.
Given this is meant to close #489 completely, worth either probing the pipe unconditionally before claiming the mutex (send the sentinel when there's no file, exactly like the fallback retry does), or explicitly documenting this as an accepted gap the way the same-version race is documented at lines 88-92.
There was a problem hiding this comment.
Resolved in beacd5d as documentation, taking your second option: probing first on a bare launch can't work against an old receiver — it drops the sentinel while delivery reports success, so the launch would exit having surfaced nothing, which is worse than one transient window of the pre-#489 status quo. The comment now scopes the skew-proof claim to with-file launches and documents the bare-launch residue beside the same-version race, with the duplex-ack protocol named as what a real fix requires.
| public void TheSentinelCanNeverBeMistakenForAFileByAnOldReceiver() | ||
| { | ||
| Assert.False( | ||
| File.Exists(SingleInstance.ActivateSentinel), |
There was a problem hiding this comment.
This test doesn't actually pin the invariant its own comment (and the class doc, lines ~340-345) claims: "the double-colons are illegal in Windows file names by design." CI (.github/workflows/ci.yml) runs dotnet test on ubuntu-latest, where : is a perfectly legal filename character — File.Exists("::activate::") here is only checking that no such file happens to exist in the test process's current working directory, not that the string is unrepresentable as a path.
Concretely: if someone later "tidies" ActivateSentinel into an ordinary legal token (the exact regression this test's XML doc says it guards against, e.g. "activate" instead of "::activate::"), this assertion still passes on Linux CI as long as no coincidental file named activate exists in the CWD — it gives no signal at all. The property that actually matters (illegal on Windows) is never exercised anywhere in the suite.
There was a problem hiding this comment.
Fixed in beacd5d — good catch, the File.Exists assertion proved only that CI's CWD was clean. The test now pins the string property directly (contains ':', the ::-delimited shape) so a tidied path-representable token fails on every platform, plus the actual Path.GetInvalidFileNameChars membership check on Windows where the guarantee lives. The old-Linux-receiver-with-adversarial-file edge is documented as the accepted floor: only pre-#489 builds are exposed, and only until they restart.
|
Reviewed the single-instance fix ( The design is sound overall — mutex-for-ownership + pipe-for-handoff, sentinel checked before
Everything else — the argv scrubbing, the |
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
| writer.WriteLine(filePath); | ||
| writer.Flush(); | ||
| return true; | ||
| var mutex = new Mutex(initiallyOwned: true, SingleInstance.MutexName, out var createdNew); |
There was a problem hiding this comment.
Named Mutex cross-process detection is well-tested on Windows, but I don't see this exercised on Linux/macOS anywhere (the test suite explicitly says the process-level half is "verified by inspection" and can't run under the harness). If a named Mutex throws something other than the ACL/sandbox cases this comment anticipates (e.g. PlatformNotSupportedException, or any quirk in the Unix pthread-shared-memory shim under /tmp) on a given Linux/macOS build, the catch-all below returns true and every launch silently becomes its own "owner" — the single-instance protection this whole PR adds no-ops there, and the underlying #489 settings-clobber bug it's meant to fix would still reproduce on those platforms with no visible symptom pointing back to this code path. Worth a manual smoke test (.sqlplan double-click / bare launch, twice, on both Linux and macOS builds) before calling this closed, since CI can't catch it.
There was a problem hiding this comment.
Taken in the latest commit, converted from unobservable to two standing signals: NamedMutexMachineryWorksOnThisPlatform drives the named create and second-open paths on whatever platform the suite runs on — including the ubuntu CI runner on every push — so a shim that throws (PlatformNotSupportedException included) fails CI loudly instead of degrading invisibly; and the catch-all now reports the degradation on stderr, which is lost harmlessly by a Windows GUI launch and visible exactly on the terminal-launched Unix platforms the concern targets. The in-process test can't cover cross-process handoff, and macOS has no CI leg — the bare-double-launch smoke on a Mac is flagged as the remaining manual item for the release checklist.
|
Reviewed the single-instance startup change ( The state machine itself is sound: the with-file pipe probe staying first for version-skew safety, the sentinel being unrepresentable as a Windows file name so old receivers drop it harmlessly, closing the losing mutex handle immediately so it doesn't wedge a third launch, and the documented residual race (two simultaneous bare launches) are all correctly reasoned through and the tradeoffs are honest in the comments rather than hand-waved. One thing worth verifying before calling this done, left as an inline comment: the whole fix rests on named No T-SQL, versioning, or PlanViewer.Web linkage concerns — this PR doesn't touch any of those surfaces. |
…him 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
|
Reviewed the single-instance/mutex logic in Traced the full decision tree in Cross-checked No correctness bugs found. No inline comments to add — didn't find anything worth flagging. |
1 participant
What does this PR do?
Fixes #489.
A bare second launch ran a full second instance; both held whole-file AppSettings snapshots and Save writes the whole file, so whichever instance exited last silently clobbered the other's open_tabs and every other setting (AtomicFile prevents torn files, not lost updates). File-argument launches already forwarded over the pipe; the bare launch was exactly the clobber case.
Local\scope, distinct from Lite's), acquired in Program.Main before Avalonia. The owner roots the handle in a static for the process lifetime — the commit documents the historical bug where a prior mutex attempt disposed its handle immediately and never actually held it. Mutex machinery failure degrades to pre-A plain second launch runs a second instance, and the two clobber each other's settings #489 full-launch behavior.::activate::on the existing pipe. Compatibility verified in both directions by reading the shipped handler: an old receiver'sFile.Existsguard drops the sentinel harmlessly (double-colons are illegal in Windows file names — pinned by a test), and the new receiver classifies the sentinel BEFOREFile.Existsand handles plain paths exactly as today, so the SSMS extension is untouched.--new-instanceescape hatch for deliberate second instances (informed last-write-wins), scrubbed both in Program.Main and insideOpenFromStartupArgs— MainWindow reads raw argv, so--new-instance file.sqlplanstill opens the file.File.Existsfor seconds.How was this tested?
Eleven new tests in
SingleInstanceTestspinning the dispatch seam (sentinel activates, path opens, nonexistent path ignored), the sentinel's cannot-be-a-file property, and argv scrubbing driven through the realOpenFromStartupArgsrouter. The mutex/exit flow in Program.Main is process-level and verified by inspection (the harness boots App directly), with the reasoning in comments. Full suite: 456 tests, 455 passed, 1 platform skip, 0 failed; App builds clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n