refactor: harden the single-instance pipe server

src/Bravo.csproj

@@ -55,6 +55,7 @@
     <PackageReference Include="Microsoft.ApplicationInsights" Version="2.23.0" />
     <PackageReference Include="Microsoft.Web.WebView2" Version="1.0.2739.15" />
     <PackageReference Include="Microsoft.Identity.Client.Desktop" Version="4.73.1" />
+    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.363" PrivateAssets="all" />
     <PackageReference Include="Nerdbank.GitVersioning" Version="3.10.85" PrivateAssets="all" />
     <PackageReference Include="Swashbuckle.AspNetCore" Version="7.0.0" PrivateAssets="all" />
     <PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" />

src/Host/BravoApplicationInstance.cs

@@ -1,6 +1,6 @@
 using System;
 using System.Diagnostics;
-using System.Text;
+using System.IO;
 using System.Text.Json;
 using Sqlbi.Bravo.Infrastructure.Configuration.Settings;
 using Sqlbi.Bravo.Infrastructure.Helpers;
@@ -13,7 +13,7 @@ namespace Sqlbi.Bravo.Host;
 /// <summary>
 /// Defines the contract for a single-instance application.
 /// </summary>
-internal interface IBravoApplicationInstance : IInstanceActivationEvents, IDisposable
+internal interface IBravoApplicationInstance : IInstanceEvents, IDisposable
 {
     /// <summary>
     /// Gets a value indicating whether this process is the primary instance, the one that runs the application.
@@ -71,45 +71,41 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e)
         var startupMessage = default(AppInstanceStartupMessage?);
         try
         {
-            var json = Encoding.UTF8.GetString(e.Payload);
-            startupMessage = JsonSerializer.Deserialize<AppInstanceStartupMessage>(json);
+            startupMessage = JsonSerializer.Deserialize<AppInstanceStartupMessage>(e.Payload);
         }
         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.
+        // Known limitation: requests arriving while this instance is still starting up are dropped
+        // here. 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. A request that lands
+        // before the window exists is lost, 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.
         // TODO: Consider buffering the requests and replaying them once the UI is ready.
         ActivationRequested?.Invoke(this, new InstanceActivationRequestedEventArgs(startupMessage));
     }
 
-    private void OnError(object? sender, SingleInstanceErrorEventArgs e) => Report(e.Exception);
+    private void OnError(object? sender, ErrorEventArgs e) => Report(e.GetException());
 
     /// <inheritdoc/>
     public void RedirectActivationToPrimary()
     {
         var startupSettings = StartupSettings.CreateFromCommandLineArguments();
         var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings);
-        var json = JsonSerializer.Serialize(startupMessage);
-        var payload = Encoding.UTF8.GetBytes(json);
+        var payload = JsonSerializer.SerializeToUtf8Bytes(startupMessage);
 
         var result = SingleInstanceClient.Send(_options, payload);
-        if (!result.IsDelivered && result.Exception is not null)
+        if (!result.IsDelivered)
         {
-            // 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.RedirectActivationToPrimary), but no user-facing error is shown.
-
+            // The primary instance cannot be reached and activation fails silently. One known cause is
+            // a launch whose elevation differs from the primary's: PipeOptions.CurrentUserOnly builds
+            // and checks the pipe ACL on WindowsIdentity.Owner, which elevation changes to
+            // BUILTIN\Administrators, while the pipe name is keyed on WindowsIdentity.User
+            // (https://github.com/dotnet/runtime/issues/123903, open, milestone .NET 12).
             // TODO: Consider surfacing activation failures to the user.
             Report(result.Exception);
         }

src/Host/BravoServiceCollectionExtensions.cs

@@ -23,7 +23,7 @@ public static IServiceCollection AddBravoInitializationServices(this IServiceCol
     {
         // The single instance goes in as its activation events only: IsPrimary and the activation
         // redirection belong to the entry point and stay out of the container.
-        services.AddSingleton<IInstanceActivationEvents>(context.Instance);
+        services.AddSingleton<IInstanceEvents>(context.Instance);
 
         services.AddSingleton<IPolicyService>(new PolicyServiceFactory().Create());
         services.AddSingleton<ITelemetryService>(TelemetryService.Instance);

src/Host/IInstanceActivationEvents.cs → src/Host/IInstanceEvents.cs

@@ -2,10 +2,7 @@
 
 namespace Sqlbi.Bravo.Host;
 
-/// <summary>
-/// Defines the activation events this process instance raises for interested subscribers.
-/// </summary>
-internal interface IInstanceActivationEvents
+internal interface IInstanceEvents
 {
     /// <summary>
     /// Occurs on the primary instance when another instance asks it to come forward.

src/Host/InstanceActivationRequestedEventArgs.cs

@@ -4,7 +4,7 @@
 namespace Sqlbi.Bravo.Host;
 
 /// <summary>
-/// Provides data for the <see cref="IInstanceActivationEvents.ActivationRequested"/> event.
+/// Provides data for the <see cref="IInstanceEvents.ActivationRequested"/> event.
 /// </summary>
 internal class InstanceActivationRequestedEventArgs(AppInstanceStartupMessage? startupMessage) : EventArgs
 {

src/Host/InstancePipeName.cs

@@ -9,53 +9,44 @@ namespace Sqlbi.Bravo.Host;
 /// </summary>
 /// <remarks>
 /// <para>
-/// 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.
-/// </para>
-/// <para>
-/// 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.
+/// 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.
 /// </para>
 /// </remarks>
 internal static class InstancePipeName
 {
+    // Hardcoded constants (stable across releases)
+    private const string ApplicationName = "Bravo";
+    private const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198";
+
     /// <summary>
     /// Builds the name for the current user and session.
     /// </summary>
     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.
+        // User, not the token 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. A "run as
+        // administrator" must not: UAC elevation keeps the same account, so usersettings.json, the
+        // WebView2 user data folder and .msalcache are the same files and need a single writer.
+        // Elevation changes the owner to BUILTIN\Administrators, while User stays the same.
         var userSid = identity.User?.Value
             ?? throw new InvalidOperationException("The current Windows identity has no user SID.");
 
         return Create(AppEnvironment.SessionId, userSid);
     }
 
+    /// <summary>
+    /// Builds the name for the given session and account.
+    /// </summary>
     /// <param name="sessionId">
-    /// 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.
+    /// The Windows session. Every Terminal Services session has its own window, so it is a different instance.
     /// </param>
     /// <param name="userSid">
-    /// Identifies the Windows account and must remain stable across UAC elevation — see
-    /// <see cref="Create()"/> for why this is <c>User</c> rather than the token owner.
+    /// The Windows account. Must be stable across UAC elevation; see <see cref="Create()"/>.
     /// </param>
     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}";
-    }
+        => $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}";
 }

src/Infrastructure/AppWindow.cs

@@ -41,20 +41,20 @@ internal partial class AppWindow : Form
 };";
     public static SynchronizationContext? UISynchronizationContext { get; set; }
 
-    private readonly IInstanceActivationEvents _instanceActivationEvents;
+    private readonly IInstanceEvents _instanceEvents;
     private readonly IServerAddressProvider _serverAddressProvider;
     private readonly IOptions<StartupSettings> _startupSettingsOptionsAccessor;
     private readonly WebView2ProxyAuthHandler _proxyAuthHandler;
     private readonly Color _startupThemeColor;
     private readonly IPolicyService _policyService;
 
     public AppWindow(
-        IInstanceActivationEvents instanceActivationEvents,
+        IInstanceEvents instanceEvents,
         IServerAddressProvider serverAddressProvider,
         IOptions<StartupSettings> startupSettingsOptionsAccessor,
         IPolicyService policyService)
     {
-        _instanceActivationEvents = instanceActivationEvents;
+        _instanceEvents = instanceEvents;
         _serverAddressProvider = serverAddressProvider;
         _startupSettingsOptionsAccessor = startupSettingsOptionsAccessor;
         _policyService = policyService;
@@ -183,7 +183,7 @@ private void OnFormLoad(object? sender, EventArgs e)
 
         CenterToScreen();
 
-        _instanceActivationEvents.ActivationRequested += OnActivationRequestedRestoreWindowToForeground;
+        _instanceEvents.ActivationRequested += OnActivationRequestedRestoreWindowToForeground;
 
         if (AppEnvironment.IsDiagnosticLevelVerbose)
         {
@@ -194,8 +194,8 @@ private void OnFormLoad(object? sender, EventArgs e)
 
     private void OnFormClosed(object? sender, FormClosedEventArgs e)
     {
-        _instanceActivationEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground;
-        _instanceActivationEvents.ActivationRequested -= OnActivationRequestedSendStartupWebMessage;
+        _instanceEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground;
+        _instanceEvents.ActivationRequested -= OnActivationRequestedSendStartupWebMessage;
     }
 
     private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoadedEventArgs e)
@@ -208,7 +208,7 @@ private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoa
             BackgroundImage = null;
             SendAppStartupWebMessage();
 
-            _instanceActivationEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage;
+            _instanceEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage;
         }
     }
 

src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs

@@ -3,13 +3,12 @@
 namespace Sqlbi.Bravo.Infrastructure.SingleInstance;
 
 /// <summary>
-/// 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.
+/// Carries the payload sent by a secondary instance.
 /// </summary>
+/// <remarks>
+/// The payload is not interpreted by the server. It is never empty.
+/// </remarks>
 internal sealed class SingleInstanceActivatedEventArgs(byte[] payload) : EventArgs
 {
-    /// <summary>
-    /// The bytes received from the secondary instance. Never empty.
-    /// </summary>
     public byte[] Payload { get; } = payload;
 }

src/Infrastructure/SingleInstance/SingleInstanceClient.cs

@@ -14,11 +14,14 @@ internal static class SingleInstanceClient
     /// Sends <paramref name="payload"/> to the owning instance.
     /// </summary>
     /// <remarks>
-    /// The connection is opened with <see cref="PipeOptions.CurrentUserOnly"/>, which makes the
-    /// client verify that the pipe is owned by the current user before writing to it.
+    /// <see cref="PipeOptions.CurrentUserOnly"/> makes the client verify that the pipe is owned by
+    /// the current user before writing to it. The write completes when the owner has read the
+    /// payload, so a delivered payload has reached the owner; an owner that is alive but not
+    /// reading blocks the caller, with no timeout.
     /// </remarks>
     public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[] payload)
     {
+        ArgumentOutOfRangeException.ThrowIfZero(payload.Length);
         ArgumentOutOfRangeException.ThrowIfGreaterThan(payload.Length, options.MaxPayloadBytes);
 
         using var pipeClient = new NamedPipeClientStream(
@@ -39,9 +42,8 @@ public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[
         try
         {
             pipeClient.Write(payload, offset: 0, payload.Length);
-            pipeClient.Flush();
         }
-        catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException)
+        catch (IOException ex)
         {
             return SingleInstanceSendResult.Failed(ex);
         }

src/Infrastructure/SingleInstance/SingleInstanceOptions.cs

@@ -3,38 +3,41 @@
 namespace Sqlbi.Bravo.Infrastructure.SingleInstance;
 
 /// <summary>
-/// Configuration shared by the two sides of the single-instance protocol: the owner
-/// (<see cref="SingleInstanceServer"/>) and any secondary instance (<see cref="SingleInstanceClient"/>).
-/// Both must be configured with the same <see cref="PipeName"/>.
+/// Configuration shared by <see cref="SingleInstanceServer"/> and <see cref="SingleInstanceClient"/>.
 /// </summary>
+/// <remarks>
+/// Both sides must use the same <see cref="PipeName"/>.
+/// </remarks>
 internal sealed record SingleInstanceOptions
 {
     /// <summary>
-    /// 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.
+    /// Name of the named pipe that identifies the application instance.
     /// </summary>
+    /// <remarks>
+    /// Everything sharing this name is the same application instance, so it must be stable across
+    /// releases and unique per user and per session.
+    /// </remarks>
     public required string PipeName { get; init; }
 
     /// <summary>
-    /// How long a secondary instance waits for the owner to accept its connection before giving up.
+    /// How long a secondary instance waits for the owner to accept its connection.
     /// </summary>
     public TimeSpan ConnectTimeout { get; init; } = TimeSpan.FromSeconds(5);
 
     /// <summary>
-    /// How long the owner waits, once a client has connected, for that client to finish sending its
-    /// payload. The counterpart of <see cref="ConnectTimeout"/> 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.
+    /// How long the owner waits for a connected client to finish sending its payload.
     /// </summary>
+    /// <remarks>
+    /// A client that connects and never finishes would otherwise hold the pipe's only server
+    /// instance, leaving the owner unreachable while it stays alive.
+    /// </remarks>
     public TimeSpan ReadTimeout { get; init; } = TimeSpan.FromSeconds(5);
 
     /// <summary>
-    /// 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 <see cref="SingleInstanceServer.Error"/>.
+    /// Upper bound, in bytes, for a single activation payload.
     /// </summary>
+    /// <remarks>
+    /// A larger payload is rejected and reported through <see cref="SingleInstanceServer.Error"/>.
+    /// </remarks>
     public int MaxPayloadBytes { get; init; } = 64 * 1024;
 }