From ec92773e6548713566d22d38b36b5f17a39289d5 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Mon, 3 Aug 2026 14:43:11 +0200 Subject: [PATCH] Rafaator single-instance handling on a pipe-only design The old singleton-instance mechanism combined a Mutex ownership gate and a named-pipe activation channel. The two could disagree: the mutex could still be held while the pipe listener was dead, so relaunching Bravo would time out against an owner that never answered and exit silently. Ownership is now arbitrated by the pipe itself (single server instance), so it cannot outlive the ability to answer. Split into a generic Infrastructure/SingleInstance component and a thin Host/BravoApplicationInstance that owns Bravo's naming, wire format and error reporting. Identity is WindowsIdentity.User, stable across UAC elevation and deployment channel. Also fixes found while reviewing the rewrite: listener restarts before dispatching (no longer starves later instances), a read timeout bounds each connection (a silent client can't hold the pipe forever), an oversized payload is reported instead of silently dropped, and the payload is UTF-8 instead of UTF-16. --- src/GlobalSuppressions.cs | 1 - src/Host/ActivationRequestedEventArgs.cs | 17 ++ src/Host/BravoApplicationInstance.cs | 149 ++++++++++ src/Host/InstancePipeName.cs | 61 ++++ src/Infrastructure/AppInstance.cs | 165 ----------- src/Infrastructure/AppWindow.cs | 21 +- .../SingleInstanceActivatedEventArgs.cs | 15 + .../SingleInstance/SingleInstanceClient.cs | 51 ++++ .../SingleInstanceErrorEventArgs.cs | 13 + .../SingleInstance/SingleInstanceOptions.cs | 40 +++ .../SingleInstanceSendResult.cs | 43 +++ .../SingleInstance/SingleInstanceServer.cs | 268 ++++++++++++++++++ src/Program.cs | 22 +- .../Bravo.Tests/Host/InstancePipeNameTests.cs | 46 +++ .../SingleInstanceServerTests.cs | 260 +++++++++++++++++ 15 files changed, 985 insertions(+), 187 deletions(-) create mode 100644 src/Host/ActivationRequestedEventArgs.cs create mode 100644 src/Host/BravoApplicationInstance.cs create mode 100644 src/Host/InstancePipeName.cs delete mode 100644 src/Infrastructure/AppInstance.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceClient.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceOptions.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs create mode 100644 src/Infrastructure/SingleInstance/SingleInstanceServer.cs create mode 100644 test/Bravo.Tests/Host/InstancePipeNameTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs diff --git a/src/GlobalSuppressions.cs b/src/GlobalSuppressions.cs index 6dee1885..b025ea54 100644 --- a/src/GlobalSuppressions.cs +++ b/src/GlobalSuppressions.cs @@ -67,7 +67,6 @@ [assembly: SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Startup.ConfigureServices(Microsoft.Extensions.DependencyInjection.IServiceCollection)")] [assembly: SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Startup.Configure(Microsoft.AspNetCore.Builder.IApplicationBuilder,Microsoft.AspNetCore.Hosting.IWebHostEnvironment)")] -[assembly: SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Infrastructure.AppInstance.NotifyOwner")] [assembly: SuppressMessage("Style", "IDE0037:Use inferred member name", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Infrastructure.Messages.UnknownWebMessage.CreateFrom(System.Exception)~Sqlbi.Bravo.Infrastructure.Messages.UnknownWebMessage")] [assembly: SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Infrastructure.Services.ExportData.ExportDataJobMap`1.Remove(`0)")] [assembly: SuppressMessage("Style", "IDE0042:Deconstruct variable declaration", Justification = "", Scope = "member", Target = "~M:Sqlbi.Bravo.Models.ManageDates.DateConfiguration.CreateFrom(Dax.Template.Package)~Sqlbi.Bravo.Models.ManageDates.DateConfiguration")] diff --git a/src/Host/ActivationRequestedEventArgs.cs b/src/Host/ActivationRequestedEventArgs.cs new file mode 100644 index 00000000..2fd33ed7 --- /dev/null +++ b/src/Host/ActivationRequestedEventArgs.cs @@ -0,0 +1,17 @@ +using System; +using Sqlbi.Bravo.Infrastructure.Messages; + +namespace Sqlbi.Bravo.Host; + +/// +/// Carries the startup arguments of the secondary instance that requested the activation. +/// +internal class ActivationRequestedEventArgs(AppInstanceStartupMessage? startupMessage) : EventArgs +{ + /// + /// The decoded startup message, or when the payload could not be read. + /// The event is raised either way: the user asked for Bravo, so the window is brought to the + /// foreground even when there is nothing to open. + /// + public AppInstanceStartupMessage? StartupMessage { get; } = startupMessage; +} diff --git a/src/Host/BravoApplicationInstance.cs b/src/Host/BravoApplicationInstance.cs new file mode 100644 index 00000000..ce4eb680 --- /dev/null +++ b/src/Host/BravoApplicationInstance.cs @@ -0,0 +1,149 @@ +using System; +using System.Diagnostics; +using System.Text; +using System.Text.Json; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Helpers; +using Sqlbi.Bravo.Infrastructure.Messages; +using Sqlbi.Bravo.Infrastructure.SingleInstance; +using Sqlbi.Bravo.Infrastructure.Telemetry; + +namespace Sqlbi.Bravo.Host; + +/// +/// Binds the generic single-instance component to Bravo: it owns the instance name, the wire format +/// of the activation message and the reporting of failures. The component underneath knows none of +/// these things. +/// +internal sealed class BravoApplicationInstance : IDisposable +{ + private readonly SingleInstanceOptions _options; + private readonly SingleInstanceServer? _server; + + private bool _disposed; + + // Not a primary constructor: it has to stay private (Create is the only entry point) and it + // subscribes to the component's events, which a primary constructor cannot do. + private BravoApplicationInstance(SingleInstanceOptions options, SingleInstanceServer? server) + { + _options = options; + _server = server; + + if (_server is not null) + { + _server.Activated += OnActivated; + _server.Error += OnError; + } + } + + /// + /// Determines whether this process is the primary instance — the one that runs the application. + /// When it is not, another instance is already running and + /// should be called before exiting. + /// + public bool IsPrimary => _server is not null; + + /// + /// Occurs on the primary instance when another instance asks it to come forward, carrying that + /// instance's startup arguments. Raised on a thread pool thread: handlers that touch the UI must + /// marshal. Nothing has been activated yet when this fires — honouring the request is up to the + /// subscribers. + /// + /// + /// Not buffered. Requests that arrive before the subscribers exist are dropped. The pipe accepts + /// connections before the UI is ready to process them, so early requests can be lost even though + /// the secondary instance is told the payload was delivered. Buffering and replaying these requests + /// was considered but rejected because the added complexity is not justified by the short startup + /// window. + /// + public event EventHandler? ActivationRequested; + + /// + /// Takes the role of primary instance if no other process holds it. The returned object is + /// valid either way: inspect to know which role this process got. + /// + public static BravoApplicationInstance Create() + { + var options = new SingleInstanceOptions + { + PipeName = InstancePipeName.Create(), + }; + + _ = SingleInstanceServer.TryStart(options, out var server); + + return new BravoApplicationInstance(options, server); + } + + /// + /// Asks the primary instance to come forward, handing it the startup arguments of this process. + /// Called by a secondary instance, which then exits. + /// + public void RequestActivation() + { + var startupSettings = StartupSettings.CreateFromCommandLineArguments(); + var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); + var json = JsonSerializer.Serialize(startupMessage); + var payload = Encoding.UTF8.GetBytes(json); + + var result = SingleInstanceClient.Send(_options, payload); + if (!result.IsDelivered && result.Exception is not null) + { + // The primary instance cannot be reached (for example, an elevated "Run as administrator" + // launch against a non-elevated instance), activation fails silently. This can happen because + // PipeOptions.CurrentUserOnly compares WindowsIdentity.Owner, which differs when elevation changes + // the identity to BUILTIN\Administrators. The failure is recorded in telemetry and the event log + // (BravoApplicationInstance.RequestActivation), but no user-facing error is shown. + + // TODO: Consider surfacing activation failures to the user. + Report(result.Exception); + } + } + + private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) + { + var startupMessage = default(AppInstanceStartupMessage?); + try + { + var json = Encoding.UTF8.GetString(e.Payload); + startupMessage = JsonSerializer.Deserialize(json); + } + catch (JsonException) + { + // An unreadable payload still activates the window, with no document to open. + } + + // ACCEPTED LIMITATION — requests arriving while this instance is still starting up are + // dropped here, on purpose. The pipe answers from the moment the process starts, but the + // subscribers appear later: AppWindow attaches the bring-to-front handler in OnLoad, and the + // one that forwards the startup message only in OnWebViewDOMContentLoaded, seconds later. + // So a request that lands before the window exists is lost entirely, and one that lands + // between the window and a loaded WebView brings Bravo to the front but does not open what + // was asked for. The secondary instance is told the payload was delivered either way, so + // nothing records the loss. + // TODO: Consider buffering the requests and replaying them once the UI is ready. + ActivationRequested?.Invoke(this, new ActivationRequestedEventArgs(startupMessage)); + } + + private void OnError(object? sender, SingleInstanceErrorEventArgs e) => Report(e.Exception); + + private static void Report(Exception exception) + { + ExceptionHelper.WriteToEventLog(exception, EventLogEntryType.Warning); + TelemetryService.Instance.TrackException(exception); + } + + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + if (_server is not null) + { + _server.Activated -= OnActivated; + _server.Error -= OnError; + _server.Dispose(); + } + } +} diff --git a/src/Host/InstancePipeName.cs b/src/Host/InstancePipeName.cs new file mode 100644 index 00000000..7c7db824 --- /dev/null +++ b/src/Host/InstancePipeName.cs @@ -0,0 +1,61 @@ +using System; +using System.Security.Principal; +using Sqlbi.Bravo.Infrastructure; + +namespace Sqlbi.Bravo.Host; + +/// +/// Builds the named-pipe name that identifies a running Bravo instance. +/// +/// +/// +/// THE RULE: one instance per Windows session and per account, across elevation levels, and +/// regardless of how Bravo was installed — Portable, both MSI variants and the MSIX/Store package +/// are the same application to the user, and must not run side by side just because one of them +/// happens to come from a different install mechanism. +/// +/// +/// The format is a frozen contract: changing it makes a new build unable to see an instance started +/// by an older one. It is kept as a pure function so that it can be pinned by tests without touching +/// the registry, the process or the current Windows identity. +/// +/// +internal static class InstancePipeName +{ + /// + /// Builds the name for the current user and session. + /// + public static string Create() + { + using var identity = WindowsIdentity.GetCurrent(); + + // User, not the token's default object owner. A "run as" with a different user gets its own + // instance — that account has its own %LOCALAPPDATA%, MSAL cache and HKCU, so nothing is + // shared with it. A "run as administrator" must not get its own instance: UAC elevation keeps + // the same Windows account, so usersettings.json, the WebView2 user data folder and + // .msalcache are the same files, and they need a single writer. Owner would get this backwards + // — elevation changes it to BUILTIN\Administrators, while User stays stable across elevation. + var userSid = identity.User?.Value + ?? throw new InvalidOperationException("The current Windows identity has no user SID."); + + return Create(AppEnvironment.SessionId, userSid); + } + + /// + /// Identifies the Windows session. Every Terminal Services session needs its own window and its + /// own Bravo instance, so a different session is always a different instance. + /// + /// + /// Identifies the Windows account and must remain stable across UAC elevation — see + /// for why this is User rather than the token owner. + /// + public static string Create(int sessionId, string userSid) + { + // Both constants below are hardcoded; they are part of the frozen + // contract that makes the instance name stable across releases. + const string ApplicationName = "Bravo"; + const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198"; + + return $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}"; + } +} diff --git a/src/Infrastructure/AppInstance.cs b/src/Infrastructure/AppInstance.cs deleted file mode 100644 index 73c6f71c..00000000 --- a/src/Infrastructure/AppInstance.cs +++ /dev/null @@ -1,165 +0,0 @@ -using System; -using System.Diagnostics; -using System.IO; -using System.IO.Pipes; -using System.Security.Principal; -using System.Text; -using System.Text.Json; -using System.Threading; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; -using Sqlbi.Bravo.Infrastructure.Helpers; -using Sqlbi.Bravo.Infrastructure.Messages; -using Sqlbi.Bravo.Infrastructure.Telemetry; - -namespace Sqlbi.Bravo.Infrastructure; - -internal class AppInstance : IDisposable -{ - private readonly bool _owned; - private readonly Mutex _mutex; - private readonly string _pipeName; - private readonly string _mutexName; - - private NamedPipeServerStream? _pipeServer; - private bool _disposed; - - public AppInstance() - { - var appId = "8D4D9F1D39F94C7789D84729480D8198"; // Do not change !! - var appName = AppEnvironment.DeploymentMode == AppDeploymentMode.Packaged ? AppEnvironment.ApplicationStoreAliasName : AppEnvironment.ApplicationName; - // Named pipes in packaged applications must use the syntax \\.\pipe\LOCAL\ for the pipe name, however, for non-windows store applications there is no such directive yet. - // See https://learn.microsoft.com/en-gb/windows/win32/api/winbase/nf-winbase-createnamedpipea - var pipeNamePrefix = AppEnvironment.DeploymentMode == AppDeploymentMode.Packaged ? "LOCAL\\" : string.Empty; - - using var identity = WindowsIdentity.GetCurrent(); - BravoUnexpectedException.ThrowIfNull(identity.Owner); // A non-null Owner is expected since GetCurrent() ifImpersonating/threadOnly argument is false - var userSid = identity.Owner.Value; // Should we use TokenLogonSid instead of TokenInformationClass.TokenOwner ? - var sessionId = AppEnvironment.SessionId; - - // 'sessionId' allows to run multiple instance - one per session - on multi-session environments such as Remote Desktop Services - // 'userSid' allows to run multiple instance under different user accounts (non-elevated) - _pipeName = $"{pipeNamePrefix}{appName}.{appId}.{sessionId}.{userSid}"; - _mutexName = $"{appName}.{appId}.{userSid}"; - _mutex = new Mutex(initiallyOwned: true, name: _mutexName, createdNew: out _owned); - - if (_owned) - { - StartPipeServer(); - GC.KeepAlive(_mutex); - } - } - - /// - /// Determines if the current instance is the only running instance of the application or if another instance is already running - /// - /// true if the current instance is the only running instance of the application; otherwise, false - public bool IsOwned => _owned; - - /// - /// Occurs when a new (secondary) instance of the application is started and the notification is sent to the primary (owner) instance - /// - public event EventHandler? OnNewInstance; - - /// - /// Sends a message to the primary instance owner notifying it of startup arguments for the current instance - /// - public void NotifyOwner() - { - using var pipeClient = new NamedPipeClientStream(serverName: ".", _pipeName, PipeDirection.Out); - try - { - pipeClient.Connect(timeout: 5_000); - } - catch (Exception ex) when (ex is IOException || ex is TimeoutException) - { - ExceptionHelper.WriteToEventLog(ex, EventLogEntryType.Warning); - TelemetryService.Instance.TrackException(ex); - return; - } - - var startupSettings = StartupSettings.CreateFromCommandLineArguments(); - var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); - var json = JsonSerializer.Serialize(startupMessage); - var bytes = Encoding.Unicode.GetBytes(json).AsSpan(); - - try - { - pipeClient.Write(bytes); - pipeClient.Flush(); - } - catch (Exception ex) when (ex is ObjectDisposedException || ex is InvalidOperationException || ex is IOException) - { - ExceptionHelper.WriteToEventLog(ex, EventLogEntryType.Warning); - TelemetryService.Instance.TrackException(ex); - return; - } - } - - private void StartPipeServer() - { - _pipeServer?.Dispose(); - _pipeServer = new NamedPipeServerStream(_pipeName, PipeDirection.In, 1, PipeTransmissionMode.Byte, PipeOptions.CurrentUserOnly); - _pipeServer.BeginWaitForConnection(OnPipeConnection, state: _pipeServer); - } - - private void OnPipeConnection(IAsyncResult asyncResult) - { - BravoUnexpectedException.ThrowIfNull(asyncResult.AsyncState); - - var pipeServer = (NamedPipeServerStream)asyncResult.AsyncState; - pipeServer.EndWaitForConnection(asyncResult); - - using var reader = new StreamReader(pipeServer, Encoding.Unicode); - var json = reader.ReadToEnd(); - - var startupMessage = default(AppInstanceStartupMessage?); - try - { - startupMessage = JsonSerializer.Deserialize(json); - } - catch (JsonException) - { - // TODO: log JsonException ? - } - - OnNewInstance?.Invoke(this, new AppInstanceStartupEventArgs(startupMessage)); - StartPipeServer(); - } - - #region IDisposable - - protected virtual void Dispose(bool disposing) - { - if (!_disposed) - { - if (disposing) - { - if (_owned) - _mutex.ReleaseMutex(); - - _pipeServer?.Dispose(); - _mutex.Dispose(); - } - - _disposed = true; - } - } - - public void Dispose() - { - Dispose(disposing: true); - GC.SuppressFinalize(this); - } - - #endregion -} - -internal class AppInstanceStartupEventArgs : EventArgs -{ - public AppInstanceStartupEventArgs(AppInstanceStartupMessage? message) - { - Message = message; - } - - public AppInstanceStartupMessage? Message { get; } -} diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index e311404c..a2f99b3d 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -12,6 +12,7 @@ using Microsoft.Extensions.Options; using Microsoft.Web.WebView2.Core; using Microsoft.Web.WebView2.WinForms; +using Sqlbi.Bravo.Host; using Sqlbi.Bravo.Infrastructure.Configuration; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Extensions; @@ -40,14 +41,14 @@ internal partial class AppWindow : Form };"; public static SynchronizationContext? UISynchronizationContext { get; set; } - private readonly AppInstance _instance; + private readonly BravoApplicationInstance _instance; private readonly IServerAddressProvider _serverAddressProvider; private readonly IOptions _startupSettingsOptionsAccessor; private readonly WebView2ProxyAuthHandler _proxyAuthHandler; private readonly Color _startupThemeColor; private readonly IPolicies _policies; - public AppWindow(IServiceProvider services, AppInstance instance) + public AppWindow(IServiceProvider services, BravoApplicationInstance instance) { _instance = instance; _serverAddressProvider = services.GetRequiredService(); @@ -178,13 +179,13 @@ private void OnFormLoad(object? sender, EventArgs e) CenterToScreen(); - _instance.OnNewInstance += OnNewInstanceRestoreFormWindowToForeground; + _instance.ActivationRequested += OnActivationRequestedRestoreWindowToForeground; } private void OnFormClosed(object? sender, FormClosedEventArgs e) { - _instance.OnNewInstance -= OnNewInstanceRestoreFormWindowToForeground; - _instance.OnNewInstance -= OnNewInstanceSendStartupWebMessage; + _instance.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; + _instance.ActivationRequested -= OnActivationRequestedSendStartupWebMessage; } private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoadedEventArgs e) @@ -197,7 +198,7 @@ private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoa BackgroundImage = null; SendAppStartupWebMessage(); - _instance.OnNewInstance += OnNewInstanceSendStartupWebMessage; + _instance.ActivationRequested += OnActivationRequestedSendStartupWebMessage; } } @@ -281,7 +282,7 @@ private void OnWebViewWebResourceResponseReceived(object? sender, CoreWebView2We WebViewLog(message: $"::OnWebViewWebResourceResponseReceived({e.Response.StatusCode}{e.Response.ReasonPhrase}|{e.Request.Uri})"); } - private void OnNewInstanceRestoreFormWindowToForeground(object? sender, AppInstanceStartupEventArgs _) + private void OnActivationRequestedRestoreWindowToForeground(object? sender, ActivationRequestedEventArgs _) { ProcessHelper.InvokeOnUIThread(this, () => { @@ -294,13 +295,13 @@ private void OnNewInstanceRestoreFormWindowToForeground(object? sender, AppInsta }); } - private void OnNewInstanceSendStartupWebMessage(object? sender, AppInstanceStartupEventArgs e) + private void OnActivationRequestedSendStartupWebMessage(object? sender, ActivationRequestedEventArgs e) { - if (e.Message?.IsEmpty == false) + if (e.StartupMessage?.IsEmpty == false) { ProcessHelper.InvokeOnUIThread(this, () => { - var webMessageString = e.Message.ToWebMessageString(); + var webMessageString = e.StartupMessage.ToWebMessageString(); WebView.CoreWebView2.PostWebMessageAsString(webMessageString); }); } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs new file mode 100644 index 00000000..288ac5ea --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs @@ -0,0 +1,15 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Carries the raw payload sent by a secondary instance. The component is deliberately unaware of +/// how the payload is encoded: interpreting it is the composition layer's responsibility. +/// +internal sealed class SingleInstanceActivatedEventArgs(byte[] payload) : EventArgs +{ + /// + /// The bytes received from the secondary instance. Never empty. + /// + public byte[] Payload { get; } = payload; +} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceClient.cs b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs new file mode 100644 index 00000000..d515b4e8 --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs @@ -0,0 +1,51 @@ +using System; +using System.IO; +using System.IO.Pipes; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Secondary-instance side of the single-instance protocol: hands the activation payload to the +/// process that owns the application instance. +/// +internal static class SingleInstanceClient +{ + /// + /// Sends to the owning instance. + /// + /// + /// The connection is opened with , which makes the + /// client verify that the pipe is owned by the current user before writing to it. + /// + public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[] payload) + { + ArgumentOutOfRangeException.ThrowIfGreaterThan(payload.Length, options.MaxPayloadBytes); + + using var pipeClient = new NamedPipeClientStream( + serverName: ".", + options.PipeName, + PipeDirection.Out, + PipeOptions.CurrentUserOnly); + + try + { + pipeClient.Connect(options.ConnectTimeout); + } + catch (Exception ex) when (ex is TimeoutException or IOException or UnauthorizedAccessException) + { + return SingleInstanceSendResult.OwnerUnavailable(ex); + } + + try + { + pipeClient.Write(payload, offset: 0, payload.Length); + pipeClient.Flush(); + } + catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) + { + return SingleInstanceSendResult.Failed(ex); + } + + return SingleInstanceSendResult.Delivered(); + } +} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs new file mode 100644 index 00000000..e8e3dac8 --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs @@ -0,0 +1,13 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Reports a failure that happened on the listener loop, where there is no caller to return it to. +/// Modelled on : the component owns no logging +/// dependency, so the host decides what to do with it. +/// +internal sealed class SingleInstanceErrorEventArgs(Exception exception) : EventArgs +{ + public Exception Exception { get; } = exception; +} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs new file mode 100644 index 00000000..dc33f6db --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs @@ -0,0 +1,40 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Configuration shared by the two sides of the single-instance protocol: the owner +/// () and any secondary instance (). +/// Both must be configured with the same . +/// +internal sealed record SingleInstanceOptions +{ + /// + /// Name of the named pipe that both identifies the application and arbitrates ownership of the + /// running instance. It must be stable across releases, and unique per user and per session: + /// everything sharing this name is considered the same application instance. + /// + public required string PipeName { get; init; } + + /// + /// How long a secondary instance waits for the owner to accept its connection before giving up. + /// + public TimeSpan ConnectTimeout { get; init; } = TimeSpan.FromSeconds(5); + + /// + /// How long the owner waits, once a client has connected, for that client to finish sending its + /// payload. The counterpart of on the owner's side: a legitimate + /// client writes its whole payload in one call right after connecting, so this bounds how long a + /// stalled or malfunctioning client can hold the pipe's only server instance, which would + /// otherwise leave the owner unreachable to every later instance while it stays alive. + /// + public TimeSpan ReadTimeout { get; init; } = TimeSpan.FromSeconds(5); + + /// + /// Upper bound, in bytes, for a single activation payload. The limit is checked after each read, + /// so a rejected payload is buffered up to one read block past it and never more, keeping a + /// malfunctioning or hostile client from growing the owner's memory. The rejection is reported + /// through . + /// + public int MaxPayloadBytes { get; init; } = 64 * 1024; +} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs new file mode 100644 index 00000000..69e66b67 --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs @@ -0,0 +1,43 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Outcome of an attempt to notify the owning instance. +/// +internal enum SingleInstanceSendStatus +{ + /// + /// The payload was written to the owner. + /// + Delivered = 0, + + /// + /// No owner accepted the connection within the configured timeout. Either the owner is gone, or + /// it is alive but not listening — which the caller may want to surface rather than ignore. + /// + OwnerUnavailable = 1, + + /// + /// The connection succeeded but the payload could not be written. + /// + Failed = 2, +} + +/// +/// Result of a send. Failing to reach the owner is an expected +/// runtime condition, not an exceptional one, so it is returned rather than thrown. +/// +internal readonly record struct SingleInstanceSendResult(SingleInstanceSendStatus Status, Exception? Exception) +{ + public bool IsDelivered => Status == SingleInstanceSendStatus.Delivered; + + public static SingleInstanceSendResult Delivered() + => new(SingleInstanceSendStatus.Delivered, Exception: null); + + public static SingleInstanceSendResult OwnerUnavailable(Exception exception) + => new(SingleInstanceSendStatus.OwnerUnavailable, exception); + + public static SingleInstanceSendResult Failed(Exception exception) + => new(SingleInstanceSendStatus.Failed, exception); +} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceServer.cs b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs new file mode 100644 index 00000000..d1e9c4e2 --- /dev/null +++ b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs @@ -0,0 +1,268 @@ +using System; +using System.Diagnostics.CodeAnalysis; +using System.IO; +using System.IO.Pipes; +using System.Threading; +using System.Threading.Tasks; + +namespace Sqlbi.Bravo.Infrastructure.SingleInstance; + +/// +/// Owner side of the single-instance protocol: holds ownership of the application instance and +/// receives activation payloads from secondary instances. +/// +/// +/// +/// Ownership is arbitrated by the named pipe itself, which is created allowing a single server +/// instance: only one process at a time can hold the name. There is deliberately no separate mutex, +/// so there is no second piece of state that can disagree with the listener. If this process stops +/// listening for any reason, the name is released and the next process to start becomes the owner, +/// instead of every later instance failing to reach an owner that no longer answers. +/// +/// +/// The pipe is disconnected — not recreated — between connections: recreating it would release the +/// name for an instant, during which another process could claim ownership. +/// +/// +internal sealed class SingleInstanceServer : IDisposable +{ + private static readonly TimeSpan s_errorBackoff = TimeSpan.FromMilliseconds(250); + private static readonly TimeSpan s_shutdownTimeout = TimeSpan.FromSeconds(1); + + private readonly SingleInstanceOptions _options; + private readonly NamedPipeServerStream _pipeServer; + private readonly CancellationTokenSource _cancellation; + + private Task? _listener; + private bool _disposed; + + private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStream pipeServer) + { + _options = options; + _pipeServer = pipeServer; + _cancellation = new CancellationTokenSource(); + } + + /// + /// Raised when a secondary instance sends an activation payload. Raised on a thread pool thread, + /// never on the listener loop, so a subscriber is free to block: subscribers that need a specific + /// thread — a UI thread, typically — marshal by themselves. + /// + public event EventHandler? Activated; + + /// + /// Raised when a connection fails, a client does not finish sending within + /// , or a subscriber of + /// throws. The loop keeps running: ownership is not given up because of a single bad connection. + /// + public event EventHandler? Error; + + /// + /// Attempts to take ownership of the application instance identified by + /// and, on success, starts listening. + /// + /// + /// if ownership was acquired by this process; if + /// another process already owns it, in which case the caller should notify the owner through + /// and exit. + /// + public static bool TryStart(SingleInstanceOptions options, [NotNullWhen(true)] out SingleInstanceServer? server) + { + NamedPipeServerStream pipeServer; + try + { + pipeServer = new NamedPipeServerStream( + options.PipeName, + PipeDirection.In, + maxNumberOfServerInstances: 1, + PipeTransmissionMode.Byte, + PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // The name is already taken: another process owns this application instance. + server = null; + return false; + } + + var instance = new SingleInstanceServer(options, pipeServer); + instance._listener = Task.Run(() => instance.ListenAsync(instance._cancellation.Token)); + + server = instance; + return true; + } + + /// + /// Serves one connection at a time until shutdown. A bad connection degrades this loop, it never + /// ends it: giving up would release the pipe name and hand ownership to the next process. + /// + private async Task ListenAsync(CancellationToken cancellationToken) + { + while (!cancellationToken.IsCancellationRequested) + { + try + { + var payload = await AcceptAndReadAsync(cancellationToken).ConfigureAwait(false); + if (payload is not null) + Dispatch(payload); + } + catch (Exception ex) when (IsShutdown(ex, cancellationToken)) + { + break; + } + catch (Exception ex) + { + RaiseError(ex); + + if (!await TryBackOffAsync(cancellationToken).ConfigureAwait(false)) + break; + } + } + } + + /// + /// Tells a shutdown apart from a connection that went wrong. The two arrive as the same exception + /// types, and only the caller's token says which happened: a cancelled read is the ReadTimeout + /// firing, unless shutdown was requested. + /// + private static bool IsShutdown(Exception exception, CancellationToken cancellationToken) => exception switch + { + // The pipe was disposed underneath the loop, which only Dispose does. + ObjectDisposedException => true, + OperationCanceledException => cancellationToken.IsCancellationRequested, + _ => false, + }; + + /// + /// Accepts one connection and reads its payload, always releasing the pipe afterwards. Returns + /// when the client sent nothing usable. + /// + private async Task AcceptAndReadAsync(CancellationToken cancellationToken) + { + try + { + // Waiting for a connection has no deadline: an idle pipe with nobody connected is the + // normal state, not a problem. + await _pipeServer.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false); + + // Once connected a deadline applies, otherwise a client that connects and never finishes + // sending would hold the pipe's only server instance forever — see ReadTimeout. + using var readCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + readCancellation.CancelAfter(_options.ReadTimeout); + + return await ReadPayloadAsync(readCancellation.Token).ConfigureAwait(false); + } + finally + { + Disconnect(); + } + } + + /// + /// Paces the loop after a failure, so that a pipe or a client failing immediately and repeatedly + /// leaves the listener degraded rather than spinning at full speed. + /// + /// if shutdown was requested while waiting. + private static async Task TryBackOffAsync(CancellationToken cancellationToken) + { + try + { + await Task.Delay(s_errorBackoff, cancellationToken).ConfigureAwait(false); + return true; + } + catch (OperationCanceledException) + { + return false; + } + } + + private async Task ReadPayloadAsync(CancellationToken cancellationToken) + { + using var buffer = new MemoryStream(); + var chunk = new byte[4096]; + + int count; + while ((count = await _pipeServer.ReadAsync(chunk, cancellationToken).ConfigureAwait(false)) > 0) + { + buffer.Write(chunk, 0, count); + + // Thrown rather than returned as null so that it travels the same path as any other bad + // connection: reported through Error, then paced. Returning null would have made an + // oversized payload indistinguishable from an empty one and dropped it silently. + if (buffer.Length > _options.MaxPayloadBytes) + throw new InvalidDataException($"Payload exceeds {_options.MaxPayloadBytes} bytes."); + } + + return buffer.Length == 0 ? null : buffer.ToArray(); + } + + private void Dispatch(byte[] payload) + { + var handler = Activated; + if (handler is null) + return; + + _ = Task.Run(() => + { + try + { + handler(this, new SingleInstanceActivatedEventArgs(payload)); + } + catch (Exception ex) + { + RaiseError(ex); + } + }); + } + + private void RaiseError(Exception exception) + { + try + { + Error?.Invoke(this, new SingleInstanceErrorEventArgs(exception)); + } + catch + { + // A failing error handler must not take down the listener loop. + } + } + + private void Disconnect() + { + try + { + // Unconditionally, and never guarded by IsConnected: when the client closes first the + // property is already false while the pipe instance is still in a connected state, and + // skipping the call makes every later WaitForConnectionAsync fail with + // InvalidOperationException — the owner stops answering while still holding the name. + _pipeServer.Disconnect(); + } + catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) + { + // Not connected, or already gone: nothing to release. + } + } + + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + _cancellation.Cancel(); + // Unblocks a WaitForConnectionAsync that is not observing cancellation yet. + _pipeServer.Dispose(); + + try + { + _listener?.Wait(s_shutdownTimeout); + } + catch (AggregateException) + { + // The loop faulted on its way out; there is nothing left to report at this point. + } + + _cancellation.Dispose(); + } +} diff --git a/src/Program.cs b/src/Program.cs index f2865f54..8927c7aa 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -1,6 +1,7 @@ using System; using System.Windows.Forms; using Microsoft.Extensions.Hosting; +using Sqlbi.Bravo.Host; using Sqlbi.Bravo.Infrastructure; using Sqlbi.Bravo.Infrastructure.Configuration; using Sqlbi.Bravo.Infrastructure.Helpers; @@ -17,21 +18,20 @@ public static void Main() { StartupConfiguration.Configure(); - using var instance = new AppInstance(); - if (instance.IsOwned) + using var instance = BravoApplicationInstance.Create(); + if (!instance.IsPrimary) { - using var host = CreateHost(); - host.Start(); - { - var window = new AppWindow(host.Services, instance); - Application.Run(window); - } - host.StopAsync().GetAwaiter().GetResult(); + instance.RequestActivation(); + return; } - else + + using var host = CreateHost(); + host.Start(); { - instance.NotifyOwner(); + var window = new AppWindow(host.Services, instance); + Application.Run(window); } + host.StopAsync().GetAwaiter().GetResult(); } catch (Exception ex) { diff --git a/test/Bravo.Tests/Host/InstancePipeNameTests.cs b/test/Bravo.Tests/Host/InstancePipeNameTests.cs new file mode 100644 index 00000000..f0ce130c --- /dev/null +++ b/test/Bravo.Tests/Host/InstancePipeNameTests.cs @@ -0,0 +1,46 @@ +using Sqlbi.Bravo.Host; +using Xunit; + +namespace Bravo.Tests.Host; + +/// +/// Pins the instance name format. It is a frozen contract: a build that computes a different name +/// does not see instances started by another build, so single-instance silently stops working +/// across an upgrade. These assertions are meant to fail when the format is changed by accident. +/// +public class InstancePipeNameTests +{ + private const string UserSid = "S-1-5-21-1111111111-2222222222-3333333333-1001"; + + [Fact] + public void Create_UsesPlainNamedPipeNaming() + { + // No "LOCAL\" prefix: that is an AppContainer requirement, and every Bravo distribution + // (Portable, the two MSI variants, the MSIX/Store package) is a full-trust desktop process, + // not an AppContainer. + var pipeName = InstancePipeName.Create(sessionId: 1, UserSid); + + Assert.Equal($"Bravo.8D4D9F1D39F94C7789D84729480D8198.1.{UserSid}", pipeName); + } + + [Fact] + public void Create_DifferentSession_IsADifferentInstance() + { + // One instance per session, so Remote Desktop Services users do not share one. + var first = InstancePipeName.Create(sessionId: 1, UserSid); + var second = InstancePipeName.Create(sessionId: 2, UserSid); + + Assert.NotEqual(first, second); + } + + [Fact] + public void Create_DifferentUser_IsADifferentInstance() + { + // One instance per account: a "run as" with another user gets its own instance, because + // that account has its own settings, MSAL cache and registry. + var first = InstancePipeName.Create(sessionId: 1, UserSid); + var second = InstancePipeName.Create(sessionId: 1, userSid: "S-1-5-18"); + + Assert.NotEqual(first, second); + } +} diff --git a/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs new file mode 100644 index 00000000..0c85762c --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs @@ -0,0 +1,260 @@ +using System; +using System.IO.Pipes; +using System.Text; +using System.Threading; +using Sqlbi.Bravo.Infrastructure.SingleInstance; +using Xunit; + +namespace Bravo.Tests.Infrastructure.SingleInstance; + +/// +/// Exercises the real named pipe: the component is a thin layer over an OS primitive, and the +/// behaviour worth protecting — who wins ownership, and whether the listener survives — only exists +/// against the primitive itself. Each test uses a unique pipe name so they can run in parallel. +/// +public class SingleInstanceServerTests +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(5); + + private static SingleInstanceOptions CreateOptions() => new() + { + PipeName = $"Bravo.Tests.{Guid.NewGuid():N}", + ConnectTimeout = TimeSpan.FromSeconds(2), + }; + + private static byte[] Payload(string value) => Encoding.UTF8.GetBytes(value); + + [Fact] + public void TryStart_NameIsFree_TakesOwnership() + { + var options = CreateOptions(); + + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + Assert.NotNull(server); + } + } + + [Fact] + public void TryStart_NameIsAlreadyOwned_DoesNotTakeOwnership() + { + var options = CreateOptions(); + + Assert.True(SingleInstanceServer.TryStart(options, out var owner)); + + using (owner) + { + Assert.False(SingleInstanceServer.TryStart(options, out var second)); + Assert.Null(second); + } + } + + /// + /// The reason the pipe is the gate instead of a mutex: ownership cannot outlive the ability to + /// answer. When the owner goes away the name is free again, so the next process starts normally + /// rather than timing out against an owner that no longer listens. + /// + [Fact] + public void TryStart_PreviousOwnerIsGone_TakesOwnership() + { + var options = CreateOptions(); + + Assert.True(SingleInstanceServer.TryStart(options, out var owner)); + owner.Dispose(); + + Assert.True(SingleInstanceServer.TryStart(options, out var next)); + next.Dispose(); + } + + [Fact] + public void Send_NoOwner_ReportsOwnerUnavailable() + { + var options = CreateOptions(); + + var result = SingleInstanceClient.Send(options, Payload("ignored")); + + Assert.False(result.IsDelivered); + Assert.Equal(SingleInstanceSendStatus.OwnerUnavailable, result.Status); + Assert.NotNull(result.Exception); + } + + [Fact] + public void Send_OwnerIsListening_DeliversPayloadVerbatim() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var received = new ManualResetEventSlim(); + byte[]? payload = null; + + server.Activated += (_, e) => + { + payload = e.Payload; + received.Set(); + }; + + Assert.True(SingleInstanceClient.Send(options, Payload("hello")).IsDelivered); + + Assert.True(received.Wait(s_timeout)); + Assert.Equal("hello", Encoding.UTF8.GetString(payload!)); + } + } + + /// + /// Regression: the listener used to be restarted only after the subscribers had returned, so a + /// second instance could not be served while the owner was busy — and every later instance was + /// silently lost after its connection timed out. + /// + [Fact] + public void Activated_SubscriberIsBlocked_KeepsServingNewInstances() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var firstReceived = new ManualResetEventSlim(); + using var releaseFirst = new ManualResetEventSlim(); + using var secondReceived = new ManualResetEventSlim(); + var count = 0; + + server.Activated += (_, _) => + { + if (Interlocked.Increment(ref count) == 1) + { + firstReceived.Set(); + releaseFirst.Wait(s_timeout); // stands in for a modal dialog owning the UI thread + } + else + { + secondReceived.Set(); + } + }; + + Assert.True(SingleInstanceClient.Send(options, Payload("first")).IsDelivered); + Assert.True(firstReceived.Wait(s_timeout)); + + Assert.True(SingleInstanceClient.Send(options, Payload("second")).IsDelivered); + Assert.True(secondReceived.Wait(s_timeout)); + + releaseFirst.Set(); + } + } + + /// + /// Regression: a client that connects and never writes used to hold the pipe's only server + /// instance forever — the owner stayed alive but became unreachable to every later instance, + /// which is indistinguishable in the field from Bravo being broken. + /// + [Fact] + public void Activated_ClientConnectsButNeverWrites_RecoversAndKeepsListening() + { + var options = CreateOptions() with { ReadTimeout = TimeSpan.FromMilliseconds(300) }; + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var errorRaised = new ManualResetEventSlim(); + server.Error += (_, _) => errorRaised.Set(); + + using (var stalledClient = new NamedPipeClientStream( + ".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + { + stalledClient.Connect((int)s_timeout.TotalMilliseconds); + // Connected, but deliberately never writes: stands in for a stuck or hostile client. + Assert.True(errorRaised.Wait(s_timeout)); + } + + // The stalled connection released the pipe: a real client is served next. + using var received = new ManualResetEventSlim(); + byte[]? payload = null; + + server.Activated += (_, e) => + { + payload = e.Payload; + received.Set(); + }; + + Assert.True(SingleInstanceClient.Send(options, Payload("hello")).IsDelivered); + Assert.True(received.Wait(s_timeout)); + Assert.Equal("hello", Encoding.UTF8.GetString(payload!)); + } + } + + [Fact] + public void Send_RepeatedNotifications_AreAllDelivered() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var allReceived = new CountdownEvent(initialCount: 3); + server.Activated += (_, _) => allReceived.Signal(); + + for (var i = 0; i < 3; i++) + { + var result = SingleInstanceClient.Send(options, Payload($"message-{i}")); + Assert.True(result.IsDelivered, $"send #{i} -> {result.Status}: {result.Exception}"); + } + + Assert.True(allReceived.Wait(s_timeout)); + } + } + + [Fact] + public void Activated_PayloadExceedsTheLimit_IsRejectedAndTheOwnerKeepsListening() + { + var pipeName = $"Bravo.Tests.{Guid.NewGuid():N}"; + var serverOptions = new SingleInstanceOptions { PipeName = pipeName, MaxPayloadBytes = 32 }; + var clientOptions = new SingleInstanceOptions { PipeName = pipeName, ConnectTimeout = TimeSpan.FromSeconds(2) }; + + Assert.True(SingleInstanceServer.TryStart(serverOptions, out var server)); + + using (server) + { + using var received = new ManualResetEventSlim(); + using var rejected = new ManualResetEventSlim(); + byte[]? payload = null; + + server.Activated += (_, e) => + { + payload = e.Payload; + received.Set(); + }; + // An oversized payload is a reported rejection, not a silent drop: without this the owner + // would discard it with nothing recorded anywhere. + server.Error += (_, _) => rejected.Set(); + + Assert.True(SingleInstanceClient.Send(clientOptions, Payload(new string('x', 256))).IsDelivered); + Assert.True(rejected.Wait(s_timeout)); + Assert.False(received.Wait(TimeSpan.FromSeconds(1))); + + Assert.True(SingleInstanceClient.Send(clientOptions, Payload("small")).IsDelivered); + Assert.True(received.Wait(s_timeout)); + Assert.Equal("small", Encoding.UTF8.GetString(payload!)); + } + } + + [Fact] + public void Send_PayloadExceedsTheConfiguredLimit_Throws() + { + var options = new SingleInstanceOptions { PipeName = $"Bravo.Tests.{Guid.NewGuid():N}", MaxPayloadBytes = 8 }; + + Assert.Throws(() => SingleInstanceClient.Send(options, Payload("far too long"))); + } + + [Fact] + public void Dispose_CalledTwice_DoesNotThrow() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + server.Dispose(); + server.Dispose(); + } +}