From 67027be4f5c607ab040910b363375c0862d60ff9 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Tue, 20 Oct 2026 15:35:14 +0200 Subject: [PATCH 1/2] refactor: build the activation payload without reading user settings AppInstanceStartupMessage.CreateFrom wrote a verbose diagnostic, which read UserPreferences.Current and therefore usersettings.json. A secondary instance built its payload through it and lost the diagnostic on exit. The diagnostic moves to AppWindow, the only caller in the primary. --- src/Infrastructure/AppWindow.cs | 4 ++++ src/Infrastructure/Messages/AppInstanceStartupMessage.cs | 8 ++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 6a9f4a06..099e7ad6 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -355,6 +355,10 @@ private void SendAppStartupWebMessage() if (!startupSettings.IsEmpty) { var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); + + if (AppEnvironment.IsDiagnosticLevelVerbose) + AppEnvironment.AddDiagnostics(DiagnosticMessageType.Json, name: $"{nameof(AppInstanceStartupMessage)}.{nameof(AppInstanceStartupMessage.CreateFrom)}", content: JsonSerializer.Serialize(startupMessage)); + var startupMessageString = startupMessage.ToWebMessageString(); WebView.CoreWebView2.PostWebMessageAsString(startupMessageString); diff --git a/src/Infrastructure/Messages/AppInstanceStartupMessage.cs b/src/Infrastructure/Messages/AppInstanceStartupMessage.cs index efcf5776..738bd475 100644 --- a/src/Infrastructure/Messages/AppInstanceStartupMessage.cs +++ b/src/Infrastructure/Messages/AppInstanceStartupMessage.cs @@ -35,9 +35,10 @@ internal class AppInstanceStartupMessage [JsonIgnore] public bool IsExternalTool => AppEnvironment.PBIDesktopProcessName.EqualsI(ParentProcessName); + // Reads no user settings: a secondary instance builds its activation payload before the bootstrap. public static AppInstanceStartupMessage CreateFrom(StartupSettings settings) { - var message = new AppInstanceStartupMessage + return new AppInstanceStartupMessage { IsEmpty = settings.IsEmpty, ParentProcessId = settings.ParentProcessId, @@ -47,11 +48,6 @@ public static AppInstanceStartupMessage CreateFrom(StartupSettings settings) ArgumentDatabaseName = settings.ArgumentDatabaseName, CommandLineErrors = settings.CommandLineErrors, }; - - if (AppEnvironment.IsDiagnosticLevelVerbose) - AppEnvironment.AddDiagnostics(DiagnosticMessageType.Json, name: $"{nameof(AppInstanceStartupMessage)}.{nameof(CreateFrom)}", content: JsonSerializer.Serialize(message)); - - return message; } } From 52565142041063eccf2b243043d5e3eed452193f Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Tue, 20 Oct 2026 15:35:27 +0200 Subject: [PATCH 2/2] refactor: arbitrate the single instance before the bootstrap A secondary process now forwards its startup arguments and exits before any directory setup, bootstrap service or host exists. - BravoSingleInstance.ClaimOrRedirect claims the pipe or redirects, right after the exception handler is installed. - Program.Main owns the SingleInstanceServer and releases it last; the host receives it as an existing instance. - BravoApplicationInstance only raises activation events and is registered as IInstanceEvents; IBravoApplicationInstance is removed. - A redirect failure is logged to the Event Log only, through a logger factory with the Event Log provider alone; no telemetry, since consent needs the bootstrap. AddBravoEventLog keeps the Event Log setup shared with the bootstrap logger factory. - InstancePipeName reads the session from the process, not AppEnvironment. --- ...ootstrap-services-outside-the-container.md | 4 + docs/design/architecture.md | 34 +++--- docs/design/diagnostics.md | 10 +- src/Host/BootstrapContextFactory.cs | 4 +- src/Host/BravoApplication.cs | 12 +-- src/Host/BravoApplicationBuilder.cs | 9 +- src/Host/BravoApplicationInstance.Log.cs | 4 - src/Host/BravoApplicationInstance.cs | 85 +++------------ src/Host/BravoLoggingBuilderExtensions.cs | 16 +++ src/Host/BravoServiceCollectionExtensions.cs | 22 +--- src/Host/BravoSingleInstance.Log.cs | 11 ++ src/Host/BravoSingleInstance.cs | 69 ++++++++++++ src/Host/InstancePipeName.cs | 6 +- src/Infrastructure/AppWindow.cs | 2 +- src/Program.cs | 6 +- .../Host/BravoApplicationBuilderTests.cs | 60 ++++------- .../Host/BravoApplicationInstanceTests.cs | 101 +++++++++--------- .../Host/BravoSingleInstanceTests.cs | 78 ++++++++++++++ test/Bravo.Tests/Host/HostFakes.cs | 33 ++---- 19 files changed, 327 insertions(+), 239 deletions(-) create mode 100644 src/Host/BravoLoggingBuilderExtensions.cs create mode 100644 src/Host/BravoSingleInstance.Log.cs create mode 100644 src/Host/BravoSingleInstance.cs create mode 100644 test/Bravo.Tests/Host/BravoSingleInstanceTests.cs diff --git a/docs/adr/0004-bootstrap-services-outside-the-container.md b/docs/adr/0004-bootstrap-services-outside-the-container.md index e90d5306..107af97b 100644 --- a/docs/adr/0004-bootstrap-services-outside-the-container.md +++ b/docs/adr/0004-bootstrap-services-outside-the-container.md @@ -36,3 +36,7 @@ The host has its own dependency injection container. - The bootstrap order is explicit code, and each step can degrade independently; see [ADR 0005](0005-policy-failures-stop-startup.md). - Tests build a `BootstrapContext` from fakes without starting the host. +- Single-instance arbitration runs before the bootstrap, so a secondary process creates none of these services. The + pipe server it claims is not a bootstrap service: `Program.Main` owns it and the host container receives it as an + existing instance, like the bootstrap services; see + [architecture](../design/architecture.md#single-instance-contract). diff --git a/docs/design/architecture.md b/docs/design/architecture.md index fe7fa6dc..b062c2c2 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -2,21 +2,22 @@ ## Composition and resource ownership -[BravoApplication.Run](../../src/Host/BravoApplication.cs) resolves the single-instance role before starting -Kestrel: a secondary process redirects its activation and exits without starting a server or a window. It does so -after the bootstrap services and the host are built, so a secondary process still pays for both. +[Program.Main](../../src/Program.cs) resolves the single-instance role right after installing the exception handler, +before every other startup step; see the [single-instance contract](#single-instance-contract). | Resource | Lifetime owner | Constraint for changes | | --- | --- | --- | +| Pipe server that holds the application instance | `Program.Main`, which claims it through [BravoSingleInstance](../../src/Host/BravoSingleInstance.cs) and disposes it last; the container receives it as an existing instance | Releasing the pipe ends ownership of the application instance and lets the next process become the primary. Releasing it before the bootstrap services are disposed lets two processes write the same user data | +| Activation events | The host container owns [BravoApplicationInstance](../../src/Host/BravoApplicationInstance.cs), registered as `IInstanceEvents` | Its disposal only unsubscribes from the pipe server; it must not release the pipe | | Bootstrap telemetry and logger factory | [BootstrapContext](../../src/Host/BootstrapContext.cs) | Keep them available until host disposal completes; the exception handler is detached first, then telemetry is disposed before the logger factory | | Bootstrap policy service and user settings | Bootstrap creates or obtains them; the container receives those instances | Preserve the shared instances so consumers do not silently use different configuration snapshots | -| Application instance and its pipe server | The host container owns `IBravoApplicationInstance`; that instance owns the pipe server | Releasing the pipe ends ownership of the application instance | | Main window | `BravoApplication.Run` | The window is created through DI but is not registered as a container-owned service | [BravoServiceCollectionExtensions](../../src/Host/BravoServiceCollectionExtensions.cs) registers the bootstrap -services as existing instances, which the container does not dispose. Replacing them with container-owned factories -moves disposal to the container and must keep the order above. Global exception handling uses the bootstrap -services, not the container; its coverage is in [diagnostics](diagnostics.md#startup-coverage-and-fatal-reporting). +services and the pipe server as existing instances, which the container does not dispose. Replacing them with +container-owned factories moves disposal to the container and must keep the order above. Global exception handling +uses the bootstrap services, not the container; its coverage is in +[diagnostics](diagnostics.md#startup-coverage-and-fatal-reporting). Why the bootstrap services live outside the container: [ADR 0004](../adr/0004-bootstrap-services-outside-the-container.md). @@ -72,13 +73,22 @@ stable across releases: changing them allows versions that share user data to ru through a named pipe with one server instance. It disconnects between clients without releasing the pipe name. Do not replace that with disposal and recreation between requests: another process could claim ownership in the gap. -Current activation limits in [BravoApplicationInstance](../../src/Host/BravoApplicationInstance.cs): +`BravoSingleInstance.ClaimOrRedirect` claims the pipe before the bootstrap: a secondary process forwards its startup +arguments and exits without creating directories, bootstrap services, a host or a window, and without reading the +user settings file. Keep the arbitration and the activation payload free of those steps: a secondary that builds +bootstrap services can transmit stored telemetry and reads files that the primary writes. -- A delivered payload does not guarantee that the requested document opened. Requests arriving before the window - or WebView subscriptions are ready are not buffered. +Current activation limits: + +- A delivered payload does not guarantee that the requested document opened. Requests are not buffered: the pipe + server drops those that arrive before the host creates the `BravoApplicationInstance` that subscribes to it, and + the window drops those that arrive before its subscriptions are ready. A request that arrives while the primary + shuts down is lost as well, because the pipe is held until the bootstrap services are disposed. - Activation callbacks can run concurrently and in a different order from arrival. -- Different elevation levels still share an instance name, but pipe access can prevent activation redirection. A - redirect failure is logged and tracked without a user-facing dialog. +- Different elevation levels still share an instance name, but pipe access can prevent activation redirection. The + secondary logs a redirect failure to the Event Log through a logger factory that has only that provider, and exits + without a user-facing dialog. It tracks no telemetry: consent needs the policies and settings that the secondary + does not create. ## Configuration boundaries diff --git a/docs/design/diagnostics.md b/docs/design/diagnostics.md index 227c1cee..6f563762 100644 --- a/docs/design/diagnostics.md +++ b/docs/design/diagnostics.md @@ -22,10 +22,12 @@ ends the process: [ADR 0003](../adr/0003-fatal-exceptions-end-the-process.md). enables the visual styles the task dialog needs, and before every other startup step. Logging and telemetry do not exist yet: until `BootstrapContextFactory` attaches them to the handler, a fatal exception is saved to the report and shown in the dialog, without a log entry from Bravo, without telemetry and without the Send -button. The runtime still writes its own Event Log entry when it ends the process. This covers directory and proxy -setup, the WebView2 runtime check and bootstrap creation. `BootstrapContext` detaches the handler before it disposes -the services, so an exception raised during that disposal is reported the same way and never through disposed -services. +button. The runtime still writes its own Event Log entry when it ends the process. This covers single-instance +arbitration, directory and proxy setup, the WebView2 runtime check and bootstrap creation. A secondary instance exits +after the arbitration, so the whole process is covered this way +([single-instance contract](architecture.md#single-instance-contract)). `BootstrapContext` detaches the handler +before it disposes the services, so an exception raised during that disposal is reported the same way and never +through disposed services. [BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs) degrades to default settings and disabled telemetry when they cannot be created, but a policy read failure propagates on purpose, so that administrative restrictions are never bypassed: a policy registry key that cannot be read ends the process through this path diff --git a/src/Host/BootstrapContextFactory.cs b/src/Host/BootstrapContextFactory.cs index 97ec0096..0fe3c29e 100644 --- a/src/Host/BootstrapContextFactory.cs +++ b/src/Host/BootstrapContextFactory.cs @@ -1,6 +1,5 @@ using System; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.EventLog; using Sqlbi.Bravo.Infrastructure.Configuration; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Diagnostics; @@ -64,8 +63,7 @@ private static ILoggerFactory CreateLoggerFactory() return LoggerFactory.Create((builder) => { builder.AddEventSourceLogger(); - builder.AddEventLog(); - builder.AddFilter((level) => level >= LogLevel.Warning); + builder.AddBravoEventLog(); builder.AddDiagnosticMessages(); builder.AddFilter((level) => level >= LogLevel.Warning); #if DEBUG diff --git a/src/Host/BravoApplication.cs b/src/Host/BravoApplication.cs index 8e99c75d..e984e6eb 100644 --- a/src/Host/BravoApplication.cs +++ b/src/Host/BravoApplication.cs @@ -4,6 +4,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Sqlbi.Bravo.Infrastructure; +using Sqlbi.Bravo.Infrastructure.SingleInstance; namespace Sqlbi.Bravo.Host; @@ -14,9 +15,9 @@ internal sealed class BravoApplication : IDisposable { private readonly WebApplication _innerApplication; - public static BravoApplicationBuilder CreateBuilder(BootstrapContext bootstrap) + public static BravoApplicationBuilder CreateBuilder(BootstrapContext bootstrap, SingleInstanceServer instanceServer) { - return new BravoApplicationBuilder(bootstrap); + return new BravoApplicationBuilder(bootstrap, instanceServer); } internal BravoApplication(WebApplication innerApplication) @@ -31,13 +32,6 @@ internal BravoApplication(WebApplication innerApplication) /// public void Run() { - var instance = Services.GetRequiredService(); - if (!instance.IsPrimary) - { - instance.RedirectActivationToPrimary(); - return; - } - _innerApplication.Start(); try { diff --git a/src/Host/BravoApplicationBuilder.cs b/src/Host/BravoApplicationBuilder.cs index f283d9d4..5b57774d 100644 --- a/src/Host/BravoApplicationBuilder.cs +++ b/src/Host/BravoApplicationBuilder.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Hosting; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Services; +using Sqlbi.Bravo.Infrastructure.SingleInstance; namespace Sqlbi.Bravo.Host; @@ -17,7 +18,7 @@ internal sealed class BravoApplicationBuilder { private readonly WebApplicationBuilder _innerBuilder; - internal BravoApplicationBuilder(BootstrapContext bootstrap) + internal BravoApplicationBuilder(BootstrapContext bootstrap, SingleInstanceServer instanceServer) { // CreateEmptyBuilder, not CreateBuilder: no configuration sources, no environment variables and // no implicit logging providers. The logging pipeline comes from the bootstrap context, everything @@ -29,7 +30,7 @@ internal BravoApplicationBuilder(BootstrapContext bootstrap) }); ConfigureHosting(_innerBuilder); - ConfigureServices(_innerBuilder, bootstrap); + ConfigureServices(_innerBuilder, bootstrap, instanceServer); } internal IServiceCollection Services => _innerBuilder.Services; @@ -69,14 +70,14 @@ private static void ConfigureHosting(WebApplicationBuilder builder) #endif } - private static void ConfigureServices(WebApplicationBuilder builder, BootstrapContext bootstrap) + private static void ConfigureServices(WebApplicationBuilder builder, BootstrapContext bootstrap, SingleInstanceServer instanceServer) { builder.Services.AddOptions() .Configure((settings) => settings.FromCommandLineArguments()); //.ValidateDataAnnotations(); builder.Services.AddSingleton(); builder.Services.AddBravoRestApi(); - builder.Services.AddBravoServices(bootstrap); + builder.Services.AddBravoServices(bootstrap, instanceServer); } private static void ConfigurePipeline(WebApplication application) diff --git a/src/Host/BravoApplicationInstance.Log.cs b/src/Host/BravoApplicationInstance.Log.cs index be9eafbe..37d82a9c 100644 --- a/src/Host/BravoApplicationInstance.Log.cs +++ b/src/Host/BravoApplicationInstance.Log.cs @@ -1,6 +1,5 @@ using System; using Microsoft.Extensions.Logging; -using Sqlbi.Bravo.Infrastructure.SingleInstance; namespace Sqlbi.Bravo.Host; @@ -9,9 +8,6 @@ internal sealed partial class BravoApplicationInstance [LoggerMessage(EventId = 1, Level = LogLevel.Warning, Message = "The single-instance listener reported a failure and keeps listening.")] private partial void LogListenerFailed(Exception exception); - [LoggerMessage(EventId = 2, Level = LogLevel.Error, Message = "The activation could not be redirected to the primary instance ({Status}).")] - private partial void LogActivationRedirectFailed(Exception exception, SingleInstanceSendStatus status); - [LoggerMessage(EventId = 3, Level = LogLevel.Warning, Message = "The activation payload could not be read; the window is activated without a document.")] private partial void LogActivationPayloadUnreadable(Exception exception); } diff --git a/src/Host/BravoApplicationInstance.cs b/src/Host/BravoApplicationInstance.cs index 57974382..ebe41afd 100644 --- a/src/Host/BravoApplicationInstance.cs +++ b/src/Host/BravoApplicationInstance.cs @@ -2,7 +2,6 @@ using System.IO; using System.Text.Json; using Microsoft.Extensions.Logging; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Messages; using Sqlbi.Bravo.Infrastructure.SingleInstance; using Sqlbi.Bravo.Infrastructure.Telemetry; @@ -10,51 +9,31 @@ namespace Sqlbi.Bravo.Host; /// -/// Defines the contract for a single-instance application. +/// Turns the payloads received by the primary instance into activation requests. /// -internal interface IBravoApplicationInstance : IInstanceEvents, IDisposable +/// +/// The server is owned by Program.Main, which claims it through : disposing +/// this instance stops the events but keeps the pipe. +/// +internal sealed partial class BravoApplicationInstance : IInstanceEvents, IDisposable { - /// - /// Gets a value indicating whether this process is the primary instance, the one that runs the application. - /// - bool IsPrimary { get; } - - /// - /// Redirects this process's activation to the primary instance, forwarding its startup arguments. - /// - void RedirectActivationToPrimary(); -} - -internal sealed partial class BravoApplicationInstance : IBravoApplicationInstance -{ - private readonly SingleInstanceOptions _options; - private readonly SingleInstanceServer? _server; + private readonly SingleInstanceServer _server; private readonly ITelemetryService _telemetry; private readonly ILogger _logger; - private bool _disposed; - public BravoApplicationInstance( - SingleInstanceOptions options, - SingleInstanceServer? server, + SingleInstanceServer server, ITelemetryService telemetry, ILoggerFactory loggerFactory) { - _options = options; _server = server; _telemetry = telemetry; _logger = loggerFactory.CreateLogger(); - if (_server is not null) - { - _server.Activated += OnActivated; - _server.Error += OnError; - } + _server.Activated += OnActivated; + _server.Error += OnError; } - /// - public bool IsPrimary => _server is not null; - /// public event EventHandler? ActivationRequested; @@ -72,13 +51,8 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) _telemetry.TrackException(ex); } - // 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. + // Requests are not buffered: those that arrive before the subscribers exist are lost; see the activation + // limits in docs/design/architecture.md. // TODO: Consider buffering the requests and replaying them once the UI is ready. ActivationRequested?.Invoke(this, new InstanceActivationRequestedEventArgs(startupMessage)); } @@ -91,40 +65,9 @@ private void OnError(object? sender, ErrorEventArgs e) _telemetry.TrackException(exception); } - /// - public void RedirectActivationToPrimary() - { - var startupSettings = StartupSettings.CreateFromCommandLineArguments(); - var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); - var payload = JsonSerializer.SerializeToUtf8Bytes(startupMessage); - - var result = SingleInstanceClient.Send(_options, payload); - if (!result.IsDelivered) - { - // 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. - LogActivationRedirectFailed(result.Exception, result.Status); - _telemetry.TrackException(result.Exception); - } - } - - /// public void Dispose() { - if (_disposed) - return; - - _disposed = true; - - if (_server is not null) - { - _server.Activated -= OnActivated; - _server.Error -= OnError; - _server.Dispose(); - } + _server.Activated -= OnActivated; + _server.Error -= OnError; } } diff --git a/src/Host/BravoLoggingBuilderExtensions.cs b/src/Host/BravoLoggingBuilderExtensions.cs new file mode 100644 index 00000000..36f421a5 --- /dev/null +++ b/src/Host/BravoLoggingBuilderExtensions.cs @@ -0,0 +1,16 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.EventLog; + +namespace Sqlbi.Bravo.Host; + +internal static class BravoLoggingBuilderExtensions +{ + // Shared by the primary and the secondary instance, so that both write to the Event Log with the same source and threshold. + public static ILoggingBuilder AddBravoEventLog(this ILoggingBuilder builder) + { + builder.AddEventLog(); + builder.AddFilter((level) => level >= LogLevel.Warning); + + return builder; + } +} diff --git a/src/Host/BravoServiceCollectionExtensions.cs b/src/Host/BravoServiceCollectionExtensions.cs index a5b9d4be..18db3175 100644 --- a/src/Host/BravoServiceCollectionExtensions.cs +++ b/src/Host/BravoServiceCollectionExtensions.cs @@ -1,12 +1,10 @@ using Dax.Formatter; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; -using Microsoft.Extensions.Logging; using Sqlbi.Bravo.Infrastructure.Extensions; using Sqlbi.Bravo.Infrastructure.PowerBI; using Sqlbi.Bravo.Infrastructure.Services.PowerBI; using Sqlbi.Bravo.Infrastructure.SingleInstance; -using Sqlbi.Bravo.Infrastructure.Telemetry; using Sqlbi.Bravo.Services; namespace Sqlbi.Bravo.Host; @@ -35,10 +33,10 @@ public static IServiceCollection AddBravoRestApi(this IServiceCollection service /// /// Registers and configures the services required for the Bravo application. /// - public static IServiceCollection AddBravoServices(this IServiceCollection services, BootstrapContext bootstrap) + public static IServiceCollection AddBravoServices(this IServiceCollection services, BootstrapContext bootstrap, SingleInstanceServer instanceServer) { services.AddBootstrapServices(bootstrap); - services.AddBravoApplicationInstance(); + services.AddBravoApplicationInstance(instanceServer); services.AddHttpClient(); services.AddPowerBI(); services.AddSingleton(); @@ -55,20 +53,10 @@ public static IServiceCollection AddBravoServices(this IServiceCollection servic return services; } - private static IServiceCollection AddBravoApplicationInstance(this IServiceCollection services) + private static IServiceCollection AddBravoApplicationInstance(this IServiceCollection services, SingleInstanceServer instanceServer) { - services.AddSingleton((provider) => - { - var telemetry = provider.GetRequiredService(); - var loggerFactory = provider.GetRequiredService(); - var options = new SingleInstanceOptions - { - PipeName = InstancePipeName.Create(), - }; - _ = SingleInstanceServer.TryStart(options, out var server); - - return new BravoApplicationInstance(options, server, telemetry, loggerFactory); - }); + services.AddSingleton(instanceServer); + services.AddSingleton(); return services; } diff --git a/src/Host/BravoSingleInstance.Log.cs b/src/Host/BravoSingleInstance.Log.cs new file mode 100644 index 00000000..1137ca39 --- /dev/null +++ b/src/Host/BravoSingleInstance.Log.cs @@ -0,0 +1,11 @@ +using System; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure.SingleInstance; + +namespace Sqlbi.Bravo.Host; + +internal static partial class BravoSingleInstance +{ + [LoggerMessage(EventId = 1, Level = LogLevel.Error, Message = "The activation could not be redirected to the primary instance ({Status}).")] + private static partial void LogActivationRedirectFailed(ILogger logger, Exception exception, SingleInstanceSendStatus status); +} diff --git a/src/Host/BravoSingleInstance.cs b/src/Host/BravoSingleInstance.cs new file mode 100644 index 00000000..8b2a47c0 --- /dev/null +++ b/src/Host/BravoSingleInstance.cs @@ -0,0 +1,69 @@ +using System; +using System.Text.Json; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Messages; +using Sqlbi.Bravo.Infrastructure.SingleInstance; + +namespace Sqlbi.Bravo.Host; + +internal static partial class BravoSingleInstance +{ + /// + /// Claims the application instance or, when another process holds it, forwards the startup arguments to it. + /// + /// + /// The server that holds the application instance, owned by the caller; when this process + /// is a secondary instance and must exit. + /// + public static SingleInstanceServer? ClaimOrRedirect() + { + var options = new SingleInstanceOptions + { + PipeName = InstancePipeName.Create(), + }; + + return ClaimOrRedirect(options, CreateActivationPayload, CreateFailureLoggerFactory); + } + + internal static SingleInstanceServer? ClaimOrRedirect( + SingleInstanceOptions options, + Func createActivationPayload, + Func createFailureLoggerFactory) + { + if (SingleInstanceServer.TryStart(options, out var server)) + return server; + + var result = SingleInstanceClient.Send(options, createActivationPayload()); + if (!result.IsDelivered) + { + // One known cause is a launch whose elevation differs from the primary's: PipeOptions.CurrentUserOnly checks + // the ACL on WindowsIdentity.Owner, which elevation changes (https://github.com/dotnet/runtime/issues/123903). + // No telemetry: consent needs the bootstrap. + // TODO: Consider surfacing activation failures to the user. + using var loggerFactory = createFailureLoggerFactory(); + LogActivationRedirectFailed(loggerFactory.CreateLogger(typeof(BravoSingleInstance)), result.Exception, result.Status); + } + + return null; + } + + private static byte[] CreateActivationPayload() + { + var startupSettings = StartupSettings.CreateFromCommandLineArguments(); + var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); + + return JsonSerializer.SerializeToUtf8Bytes(startupMessage); + } + + private static ILoggerFactory CreateFailureLoggerFactory() + { + return LoggerFactory.Create((builder) => + { + builder.AddBravoEventLog(); +#if DEBUG + builder.AddDebug(); +#endif + }); + } +} diff --git a/src/Host/InstancePipeName.cs b/src/Host/InstancePipeName.cs index 0f6dcd9c..88f56d65 100644 --- a/src/Host/InstancePipeName.cs +++ b/src/Host/InstancePipeName.cs @@ -1,6 +1,6 @@ using System; +using System.Diagnostics; using System.Security.Principal; -using Sqlbi.Bravo.Infrastructure; namespace Sqlbi.Bravo.Host; @@ -35,7 +35,9 @@ public static string Create() var userSid = identity.User?.Value ?? throw new InvalidOperationException("The current Windows identity has no user SID."); - return Create(AppEnvironment.SessionId, userSid); + using var process = Process.GetCurrentProcess(); + + return Create(process.SessionId, userSid); } /// diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 099e7ad6..d05b825d 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -49,7 +49,7 @@ internal partial class AppWindow : Form private readonly IPolicyService _policyService; public AppWindow( - IBravoApplicationInstance applicationInstance, + IInstanceEvents applicationInstance, IServerAddressProvider serverAddressProvider, IOptions startupSettingsOptionsAccessor, IPolicyService policyService) diff --git a/src/Program.cs b/src/Program.cs index fc5b3c0e..f91a5cd2 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -12,12 +12,16 @@ private static void Main() var exceptionHandler = BravoGlobalExceptionHandling.Configure(); + using var instanceServer = BravoSingleInstance.ClaimOrRedirect(); + if (instanceServer is null) + return; + BravoApplicationConfiguration.Initialize(); using var context = BootstrapContextFactory.CreateDefault(exceptionHandler); using var app = BravoApplication - .CreateBuilder(context) + .CreateBuilder(context, instanceServer) .Build(); app.Run(); diff --git a/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs b/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs index 9527fb31..a4ed3d0f 100644 --- a/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs +++ b/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs @@ -1,11 +1,11 @@ using Bravo.Tests.Infrastructure.Policies; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; using Sqlbi.Bravo.Host; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.SingleInstance; using Sqlbi.Bravo.Infrastructure.Telemetry; using Xunit; @@ -14,50 +14,36 @@ namespace Bravo.Tests.Host; public class BravoApplicationBuilderTests { [Fact] - public void Build_ResolvesTheBootstrapInstancesWithoutOwningTheirLifetime() + public void Build_ResolvesTheExistingInstancesWithoutOwningTheirLifetime() { var loggerFactory = new FakeLoggerFactory(); var telemetry = new FakeTelemetry(); var policies = new PolicyService(new FakePolicyReader()); var settings = new UserSettings(); - using var bootstrap = new BootstrapContext(loggerFactory, policies, settings, telemetry); - var instance = new FakeInstance(); - var builder = BravoApplication.CreateBuilder(bootstrap); - builder.Services.Replace(ServiceDescriptor.Singleton((_) => instance)); + var options = TestInstanceOptions.Create(); - using (var application = builder.Build()) - { - Assert.Same(loggerFactory, application.Services.GetRequiredService()); - Assert.Same(telemetry, application.Services.GetRequiredService()); - Assert.Same(policies, application.Services.GetRequiredService()); - Assert.Same(settings, application.Services.GetRequiredService()); - Assert.Same(instance, application.Services.GetRequiredService()); - Assert.False(application.Services.GetRequiredService().ApplicationStarted.IsCancellationRequested); - } + Assert.True(SingleInstanceServer.TryStart(options, out var instanceServer)); - // The container disposes the services it creates, not the registered bootstrap instances. - Assert.Equal(1, instance.DisposeCount); - Assert.Equal(0, telemetry.DisposeCount); - Assert.Equal(0, loggerFactory.DisposeCount); - } - - [Fact] - public void Build_WithoutRunningDoesNotClaimAnInstance() - { - var instanceCreations = 0; - using var bootstrap = new BootstrapContext(new FakeLoggerFactory(), new PolicyService(new FakePolicyReader()), new UserSettings(), new FakeTelemetry()); - var builder = BravoApplication.CreateBuilder(bootstrap); - builder.Services.Replace(ServiceDescriptor.Singleton((_) => + using (instanceServer) + using (var bootstrap = new BootstrapContext(loggerFactory, policies, settings, telemetry)) { - instanceCreations++; - return new FakeInstance(); - })); + using (var application = BravoApplication.CreateBuilder(bootstrap, instanceServer).Build()) + { + Assert.Same(loggerFactory, application.Services.GetRequiredService()); + Assert.Same(telemetry, application.Services.GetRequiredService()); + Assert.Same(policies, application.Services.GetRequiredService()); + Assert.Same(settings, application.Services.GetRequiredService()); + Assert.Same(instanceServer, application.Services.GetRequiredService()); + Assert.IsType(application.Services.GetRequiredService()); + Assert.False(application.Services.GetRequiredService().ApplicationStarted.IsCancellationRequested); + } - using (var application = builder.Build()) - { - Assert.Equal(0, instanceCreations); - } + // The container disposes the services it creates, not the registered instances. + Assert.Equal(0, telemetry.DisposeCount); + Assert.Equal(0, loggerFactory.DisposeCount); - Assert.Equal(0, instanceCreations); + // The pipe stays claimed until Main releases the instance server. + Assert.False(SingleInstanceServer.TryStart(options, out _)); + } } -} \ No newline at end of file +} diff --git a/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs b/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs index 5bf96259..dcee94ef 100644 --- a/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs +++ b/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs @@ -12,59 +12,32 @@ namespace Bravo.Tests.Host; /// -/// Exercises the reporting paths against a real pipe. Each test uses a unique pipe name so they -/// can run in parallel and never claim the production one. +/// Exercises the reporting paths against a real pipe. /// public class BravoApplicationInstanceTests { private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(5); - private static SingleInstanceOptions CreateOptions() => new() - { - PipeName = $"Bravo.Tests.{Guid.NewGuid():N}", - ConnectTimeout = TimeSpan.FromSeconds(1), - }; - - [Fact] - public void RedirectActivationToPrimary_NoPrimary_LogsAnErrorAndTracksTheException() - { - var options = CreateOptions(); - var telemetry = new FakeTelemetry(); - var loggerFactory = new FakeLoggerFactory(); - - using var instance = new BravoApplicationInstance(options, server: null, telemetry, loggerFactory); - - Assert.False(instance.IsPrimary); - - instance.RedirectActivationToPrimary(); - - var exception = Assert.Single(telemetry.Exceptions); - var entry = Assert.Single(loggerFactory.Entries); - Assert.Equal(LogLevel.Error, entry.Level); - Assert.Equal(2, entry.EventId.Id); - Assert.Same(exception, entry.Exception); - Assert.Contains(nameof(SingleInstanceSendStatus.OwnerUnavailable), entry.Message); - } - [Fact] public void Activated_ListenerReportsAFailure_LogsAWarningAndTracksTheException() { - var options = CreateOptions(); + var options = TestInstanceOptions.Create(); var telemetry = new FakeTelemetry(); var loggerFactory = new FakeLoggerFactory(); Assert.True(SingleInstanceServer.TryStart(options, out var server)); - using var instance = new BravoApplicationInstance(options, server, telemetry, loggerFactory); - - Assert.True(instance.IsPrimary); - // Connects and closes without writing: the listener reports an empty payload. - using (var silentClient = new NamedPipeClientStream(".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + using (server) + using (new BravoApplicationInstance(server, telemetry, loggerFactory)) { - silentClient.Connect((int)s_timeout.TotalMilliseconds); - } + // Connects and closes without writing: the listener reports an empty payload. + using (var silentClient = new NamedPipeClientStream(".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + { + silentClient.Connect((int)s_timeout.TotalMilliseconds); + } - Assert.True(SpinWait.SpinUntil(() => telemetry.Exceptions.Count > 0, s_timeout)); + Assert.True(SpinWait.SpinUntil(() => telemetry.Exceptions.Count > 0, s_timeout)); + } // Empty payload when the owner accepted before the close; otherwise the accept itself // fails with ERROR_NO_DATA. @@ -79,23 +52,26 @@ public void Activated_ListenerReportsAFailure_LogsAWarningAndTracksTheException( [Fact] public void Activated_UnreadablePayload_ActivatesWithoutADocumentAndReports() { - var options = CreateOptions(); + var options = TestInstanceOptions.Create(); var telemetry = new FakeTelemetry(); var loggerFactory = new FakeLoggerFactory(); + InstanceActivationRequestedEventArgs? activation = null; Assert.True(SingleInstanceServer.TryStart(options, out var server)); - using var instance = new BravoApplicationInstance(options, server, telemetry, loggerFactory); - using var activated = new ManualResetEventSlim(); - InstanceActivationRequestedEventArgs? activation = null; - instance.ActivationRequested += (_, e) => + using (server) + using (var instance = new BravoApplicationInstance(server, telemetry, loggerFactory)) + using (var activated = new ManualResetEventSlim()) { - activation = e; - activated.Set(); - }; - - Assert.True(SingleInstanceClient.Send(options, Encoding.UTF8.GetBytes("not json")).IsDelivered); - Assert.True(activated.Wait(s_timeout)); + instance.ActivationRequested += (sender, e) => + { + activation = e; + activated.Set(); + }; + + Assert.True(SingleInstanceClient.Send(options, Encoding.UTF8.GetBytes("not json")).IsDelivered); + Assert.True(activated.Wait(s_timeout)); + } Assert.NotNull(activation); Assert.Null(activation.StartupMessage); @@ -106,4 +82,31 @@ public void Activated_UnreadablePayload_ActivatesWithoutADocumentAndReports() Assert.Equal(3, entry.EventId.Id); Assert.Same(exception, entry.Exception); } + + [Fact] + public void Dispose_StopsTheActivationsAndKeepsThePipe() + { + var options = TestInstanceOptions.Create(); + var activations = 0; + + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + using (var dispatched = new ManualResetEventSlim()) + { + var instance = new BravoApplicationInstance(server, new FakeTelemetry(), new FakeLoggerFactory()); + instance.ActivationRequested += (sender, e) => Interlocked.Increment(ref activations); + instance.Dispose(); + + Assert.False(SingleInstanceServer.TryStart(options, out _)); + + // The server invokes all its handlers in one call: when the probe runs, a handler left by the instance would have run. + server.Activated += (sender, e) => dispatched.Set(); + + Assert.True(SingleInstanceClient.Send(options, Encoding.UTF8.GetBytes("{}")).IsDelivered); + Assert.True(dispatched.Wait(s_timeout)); + } + + Assert.Equal(0, Volatile.Read(ref activations)); + } } diff --git a/test/Bravo.Tests/Host/BravoSingleInstanceTests.cs b/test/Bravo.Tests/Host/BravoSingleInstanceTests.cs new file mode 100644 index 00000000..98d908f5 --- /dev/null +++ b/test/Bravo.Tests/Host/BravoSingleInstanceTests.cs @@ -0,0 +1,78 @@ +using System; +using System.IO.Pipes; +using System.Text; +using System.Threading; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Host; +using Sqlbi.Bravo.Infrastructure.SingleInstance; +using Xunit; + +namespace Bravo.Tests.Host; + +public class BravoSingleInstanceTests +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(5); + + private static T NotCalled() => throw new InvalidOperationException("The arbitration must not call this factory."); + + [Fact] + public void ClaimOrRedirect_NoPrimary_ClaimsTheInstanceWithoutSendingAPayload() + { + var options = TestInstanceOptions.Create(); + + using (var server = BravoSingleInstance.ClaimOrRedirect(options, NotCalled, NotCalled)) + { + Assert.NotNull(server); + Assert.False(SingleInstanceServer.TryStart(options, out _)); + } + } + + [Fact] + public void ClaimOrRedirect_PrimaryRunning_DeliversThePayloadAndReturnsNull() + { + var options = TestInstanceOptions.Create(); + byte[]? received = null; + + Assert.True(SingleInstanceServer.TryStart(options, out var primary)); + + using (primary) + using (var activated = new ManualResetEventSlim()) + { + primary.Activated += (sender, e) => + { + received = e.Payload; + activated.Set(); + }; + + Assert.Null(BravoSingleInstance.ClaimOrRedirect(options, () => Encoding.UTF8.GetBytes("hello"), NotCalled)); + Assert.True(activated.Wait(s_timeout)); + } + + Assert.Equal("hello", Encoding.UTF8.GetString(received!)); + } + + [Fact] + public void ClaimOrRedirect_PrimaryUnreachable_LogsAnErrorAndReturnsNull() + { + var options = TestInstanceOptions.Create() with { ConnectTimeout = TimeSpan.FromMilliseconds(200) }; + var loggerFactory = new FakeLoggerFactory(); + + Assert.True(SingleInstanceServer.TryStart(options, out var primary)); + + using (primary) + using (var holder = new NamedPipeClientStream(".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + { + // A client that connects and never writes holds the primary's only pipe instance until the read timeout. + holder.Connect((int)s_timeout.TotalMilliseconds); + + Assert.Null(BravoSingleInstance.ClaimOrRedirect(options, () => [1], () => loggerFactory)); + } + + var entry = Assert.Single(loggerFactory.Entries); + Assert.Equal(LogLevel.Error, entry.Level); + Assert.Equal(1, entry.EventId.Id); + Assert.NotNull(entry.Exception); + Assert.Contains(nameof(SingleInstanceSendStatus.OwnerUnavailable), entry.Message); + Assert.Equal(1, loggerFactory.DisposeCount); + } +} diff --git a/test/Bravo.Tests/Host/HostFakes.cs b/test/Bravo.Tests/Host/HostFakes.cs index 9f1bedbd..00c95b12 100644 --- a/test/Bravo.Tests/Host/HostFakes.cs +++ b/test/Bravo.Tests/Host/HostFakes.cs @@ -4,31 +4,19 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; -using Sqlbi.Bravo.Host; +using Sqlbi.Bravo.Infrastructure.SingleInstance; using Sqlbi.Bravo.Infrastructure.Telemetry; namespace Bravo.Tests.Host; -// Avoid claiming the production named pipe in tests. -internal sealed class FakeInstance : IBravoApplicationInstance +internal static class TestInstanceOptions { - public bool IsPrimary { get; init; } - public int DisposeCount { get; private set; } - public Action? OnDispose { get; init; } - - public event EventHandler? ActivationRequested - { - add { } - remove { } - } - - public void RedirectActivationToPrimary() { } - - public void Dispose() + // A unique pipe name per test: tests run in parallel and never claim the production one. + public static SingleInstanceOptions Create() => new() { - DisposeCount++; - OnDispose?.Invoke(); - } + PipeName = $"Bravo.Tests.{Guid.NewGuid():N}", + ConnectTimeout = TimeSpan.FromSeconds(1), + }; } internal sealed class FakeTelemetry : ITelemetryService @@ -36,7 +24,6 @@ internal sealed class FakeTelemetry : ITelemetryService private readonly List _exceptions = []; public int DisposeCount { get; private set; } - public Action? OnDispose { get; init; } public bool TelemetryEnabled { get; set; } public IReadOnlyList Exceptions @@ -53,11 +40,7 @@ public void TrackException(Exception exception, IDictionary? pro public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => false; - public void Dispose() - { - DisposeCount++; - OnDispose?.Invoke(); - } + public void Dispose() => DisposeCount++; } internal sealed record FakeLogEntry(LogLevel Level, EventId EventId, Exception? Exception, string Message);