From 61734b7d12e0781bb3f4621aaf1711ec6f33ff43 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Mon, 19 Oct 2026 21:58:33 +0200 Subject: [PATCH 1/3] refactor: compose the host from a bootstrap context BootstrapContext owns the logger factory, policies, user settings and telemetry, created before the host and registered in the container as instances, so the container never disposes them. Settings and telemetry failures degrade to defaults and NullTelemetryService. Global exception handling covers AppDomain unhandled exceptions and unobserved task exceptions: the report is saved to ErrorReport.txt and shown in a task dialog that can send it through telemetry. The single instance is created by the container with its logger and telemetry. --- AGENTS.md | 2 + docs/design/architecture.md | 59 ++++++ docs/design/diagnostics.md | 85 ++++++++ src/Host/BootstrapContext.cs | 47 +++++ src/Host/BootstrapContextFactory.Log.cs | 13 ++ src/Host/BootstrapContextFactory.cs | 109 ++++++++++ src/Host/BravoApplication.cs | 43 ++-- src/Host/BravoApplicationBuilder.cs | 38 +--- ...er.cs => BravoApplicationConfiguration.cs} | 18 +- .../BravoApplicationInitializationContext.cs | 28 --- src/Host/BravoApplicationInstance.Log.cs | 17 ++ src/Host/BravoApplicationInstance.cs | 51 ++--- src/Host/BravoGlobalExceptionHandling.cs | 20 ++ src/Host/BravoServiceCollectionExtensions.cs | 61 ++++-- src/Host/CrashTest.cs | 77 +++++++ src/Infrastructure/AppWindow.cs | 18 +- .../UserSettingsServiceFactory.cs | 13 ++ .../Diagnostics/EnvironmentDiagnostics.cs | 103 +++++++++ .../Diagnostics/EnvironmentInfo.cs | 93 -------- src/Infrastructure/Diagnostics/ErrorReport.cs | 70 ++---- .../Diagnostics/ErrorReportDialog.cs | 85 -------- .../Diagnostics/ErrorReportTaskDialog.cs | 199 ++++++++++++++++++ .../Diagnostics/GlobalExceptionHandling.cs | 96 +++++++++ .../Diagnostics/UnhandledExceptionHandler.cs | 75 +++++++ src/Infrastructure/Helpers/ProcessHelper.cs | 3 +- .../Telemetry/ITelemetryServiceFactory.cs | 12 ++ .../Telemetry/NullTelemetryService.cs | 18 ++ .../Telemetry/TelemetryService.cs | 27 --- .../Windows/Dialogs/TaskDialogBuilder.cs | 17 +- src/Program.cs | 37 +--- .../Bravo.Tests/Host/BootstrapContextTests.cs | 177 ++++++++++++++++ .../Host/BravoApplicationBuilderTests.cs | 63 +++++- ...voApplicationInitializationContextTests.cs | 51 ----- .../Host/BravoApplicationInstanceTests.cs | 109 ++++++++++ test/Bravo.Tests/Host/HostFakes.cs | 84 +++++--- .../GlobalExceptionHandlingTests.cs | 139 ++++++++++++ 36 files changed, 1627 insertions(+), 530 deletions(-) create mode 100644 docs/design/architecture.md create mode 100644 docs/design/diagnostics.md create mode 100644 src/Host/BootstrapContext.cs create mode 100644 src/Host/BootstrapContextFactory.Log.cs create mode 100644 src/Host/BootstrapContextFactory.cs rename src/Host/{BravoApplicationInitializer.cs => BravoApplicationConfiguration.cs} (68%) delete mode 100644 src/Host/BravoApplicationInitializationContext.cs create mode 100644 src/Host/BravoApplicationInstance.Log.cs create mode 100644 src/Host/BravoGlobalExceptionHandling.cs create mode 100644 src/Host/CrashTest.cs create mode 100644 src/Infrastructure/Configuration/UserSettingsServiceFactory.cs create mode 100644 src/Infrastructure/Diagnostics/EnvironmentDiagnostics.cs delete mode 100644 src/Infrastructure/Diagnostics/EnvironmentInfo.cs delete mode 100644 src/Infrastructure/Diagnostics/ErrorReportDialog.cs create mode 100644 src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs create mode 100644 src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs create mode 100644 src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs create mode 100644 src/Infrastructure/Telemetry/ITelemetryServiceFactory.cs create mode 100644 src/Infrastructure/Telemetry/NullTelemetryService.cs create mode 100644 test/Bravo.Tests/Host/BootstrapContextTests.cs delete mode 100644 test/Bravo.Tests/Host/BravoApplicationInitializationContextTests.cs create mode 100644 test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Diagnostics/GlobalExceptionHandlingTests.cs diff --git a/AGENTS.md b/AGENTS.md index 1e029e10..6c409028 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,6 +56,8 @@ asked for it. | Document | Read it before | | --- | --- | +| [docs/design/architecture.md](docs/design/architecture.md) | changing startup, service ownership, UI threading, frontend/host contracts, API authentication, single-instance activation, or policy composition. | +| [docs/design/diagnostics.md](docs/design/diagnostics.md) | catching, logging, or reporting an exception, or touching the crash dialog, the diagnostics pane, or telemetry. | | [docs/design/versioning.md](docs/design/versioning.md) | touching `version.json`, the build number, or any code that compares versions. | | [ADR 0001](docs/adr/0001-releases-without-update-channels.md) | touching the update check, the update feed, or the release of a version. | | [ADR 0002](docs/adr/0002-shared-msi-upgrade-code.md) | changing `UpgradeCode`, `MajorUpgrade` or the product version in `installer/wix`. | diff --git a/docs/design/architecture.md b/docs/design/architecture.md new file mode 100644 index 00000000..512c35c6 --- /dev/null +++ b/docs/design/architecture.md @@ -0,0 +1,59 @@ +# Architecture and runtime boundaries + +Bravo is a Windows desktop application. Its .NET process owns a WinForms window and an ASP.NET Core loopback API. The window embeds the TypeScript UI in WebView2; UI requests reach application services through the API. This document covers the boundaries that constrain changes, not individual feature implementations. + +## Composition and resource ownership + +[Program](../../src/Program.cs) configures the process, creates bootstrap services, installs global exception handling, then builds and runs the host. [BravoApplication](../../src/Host/BravoApplication.cs) resolves the single-instance role before starting Kestrel: a secondary process redirects activation and exits without starting its own server or window. + +| Resource | Lifetime owner | Constraint for changes | +| --- | --- | --- | +| Bootstrap telemetry and logger factory | [BootstrapContext](../../src/Host/BootstrapContext.cs) | Keep them available until host disposal completes; 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 bootstrap services as existing instances. Replacing those registrations with container-owned factories changes disposal responsibility and must preserve the ownership above. Global exception handling uses bootstrap services independently of host construction; its exact coverage is in [diagnostics](diagnostics.md#startup-coverage-and-fatal-reporting). + +## UI and server threads + +[AppWindow](../../src/Infrastructure/AppWindow.cs) is created on the STA entry thread, which runs the WinForms message loop. API work and single-instance activation callbacks do not have that UI-thread guarantee. + +- Marshal operations on the window or WebView2 to the UI thread. Activation handlers use `ProcessHelper.InvokeOnUIThread` with the window as the control. +- Setting `SynchronizationContext.Current` is not thread dispatch. [ProcessHelper.RunWithUISynchronizationContext](../../src/Infrastructure/Helpers/ProcessHelper.cs) changes the ambient context only during a synchronous callback; do not treat it as permission to access controls from a server thread. +- Keep window activation and document delivery distinct: the window can exist before WebView2 is ready to receive a startup message. + +Violating these boundaries can cause cross-thread UI failures or lost activation requests. The [single-instance contract](#single-instance-contract) describes the current startup limitation. + +## Frontend and API contract + +[AppWindow](../../src/Infrastructure/AppWindow.cs) injects `CONFIG` with the API address, token, options, policies and environment information. The frontend uses [Host](../../src/Scripts/controllers/host.ts) for API calls and WebView messages for desktop integration. The C# serializer and TypeScript consumers must agree on field names, nullability and enum values; changing only one side can compile in C# while breaking the UI. Follow the full-build requirement in [AGENTS.md](../../AGENTS.md#build-and-test) for changes to this contract. + +[BravoApplicationBuilder](../../src/Host/BravoApplicationBuilder.cs) configures Kestrel and endpoint authorization: + +| Build | Listening address | Controller authorization | +| --- | --- | --- | +| Debug | IPv4 loopback, port 5000 | No blanket authorization requirement; Swagger enabled | +| Release | IPv4 loopback, dynamically assigned port | Controllers require authorization | + +[AppAuthenticationHandler](../../src/Infrastructure/Authentication/AppAuthenticationHandler.cs) requires both a loopback remote address and the application token in the `Authorization` header. The template-development token is accepted only under the template-development controller path. Loopback binding and token validation are separate checks; neither should be removed on the assumption that the other is sufficient. + +Keep ProblemDetails mapping order and error contracts consistent with the frontend's interpretation of failures. Reporting responsibility at the API boundary is defined in [diagnostics](diagnostics.md#api-boundary). + +## Single-instance contract + +[InstancePipeName](../../src/Host/InstancePipeName.cs) identifies one instance per Windows session and user SID, independent of installation mode and elevation. Keep its application identifier, scope identifier and naming format stable across releases: changing them allows versions that share user data to run as independent owners. + +[SingleInstanceServer](../../src/Infrastructure/SingleInstance/SingleInstanceServer.cs) arbitrates ownership 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): + +- A delivered payload does not guarantee that the requested document opened. Requests arriving before the window or WebView subscriptions are ready are not buffered. +- 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. + +## Configuration boundaries + +[PolicyService](../../src/Infrastructure/Policies/PolicyService.cs) exposes a snapshot initialized once. [RegistryPolicyReader](../../src/Infrastructure/Policies/RegistryPolicyReader.cs) merges machine and user policies, with configured machine values taking precedence. A null policy value means not configured; `false` is an explicit policy, not a request to use a default. + +User preferences remain mutable through [UserPreferences](../../src/Infrastructure/Configuration/UserPreferences.cs). Do not assume that saving preferences reloads policies or that every consumer reapplies policy precedence. Host telemetry has different startup and update paths, documented in [telemetry consent](diagnostics.md#telemetry-consent). diff --git a/docs/design/diagnostics.md b/docs/design/diagnostics.md new file mode 100644 index 00000000..dea92b82 --- /dev/null +++ b/docs/design/diagnostics.md @@ -0,0 +1,85 @@ +# Diagnostics + +Exception reporting in the .NET host. The frontend has a separate [telemetry client](../../src/Scripts/controllers/telemetry.ts); host flush and crash handling do not control that client. + +## Reporting paths + +| Origin | Process outcome | Current reporting | +| --- | --- | --- | +| `AppDomain.UnhandledException`, including WinForms exceptions routed through `ThrowException` | Runtime terminates after the handler returns | Error log; telemetry subject to consent; saved report and crash dialog | +| `TaskScheduler.UnobservedTaskException` | Continues | Error log and telemetry subject to consent; no dialog or shutdown flush | +| Exception caught by an operation that can recover or return an error | Continues | Reporting belongs to the operation or its API boundary; apply the [handled-exception rules](#handled-exception-rules) | + +The two runtime callbacks use [GlobalExceptionHandling](../../src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs). An unobserved task exception is not a fatal crash, even though its telemetry has `ExceptionKind = Unhandled`. + +## Startup coverage and fatal reporting + +[Program](../../src/Program.cs) installs global handling after application configuration and bootstrap creation, before building the host. Exceptions during directory setup, proxy configuration or bootstrap creation occur before these hooks exist. Host construction is covered; earlier initialization is not. + +[UnhandledExceptionHandler](../../src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs) logs and tracks each callback before attempting a fatal report. Logging and tracking failures are isolated so one does not prevent the other. For a fatal exception, a marker in `Exception.Data` suppresses a repeated dialog and report; it does not suppress the earlier log and telemetry calls. + +[ErrorReport](../../src/Infrastructure/Diagnostics/ErrorReport.cs) saves environment diagnostics and the exception to `ErrorReport.txt` under the application data folder. A failed save does not prevent the dialog. [ErrorReportTaskDialog](../../src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs) runs on a dedicated STA thread and offers copying, closing and, when host telemetry is disabled, sending the exception through telemetry. Automatic tracking is an attempt, not confirmation of delivery. + +The fatal-handler guard suppresses another fatal arrival while a fatal handler runs. It does not coordinate simultaneous crashes or guarantee that the first dialog remains open until the user responds. Unobserved task reporting does not acquire that guard, so it cannot suppress a fatal report. + +If the global handler throws, `GlobalExceptionHandling` calls `Environment.FailFast`. Reporting failures must therefore be contained where recovery is possible. Never show UI or wait for a shutdown flush on the unobserved-task callback: it runs on the finalizer thread. + +## Handled-exception rules + +For an operation with initialized logging and telemetry services, report a caught failure through both `ILogger` and `ITelemetryService.TrackException`. Local logging and remote telemetry serve different purposes; consent can disable the remote record. Use the reporting type's logger. Add telemetry properties only when needed by a specific diagnostic query. + +These rules have explicit boundaries: + +| Condition | Reporting responsibility | Reason | +| --- | --- | --- | +| A failure propagates to an API or global handler | Leave reporting to that boundary unless this layer handles a separate failure | Repeated reporting records the same failure more than once | +| Settings or telemetry creation fails during bootstrap | Use the available bootstrap logger; do not depend on telemetry creation succeeding | Reporting must not require the service whose creation failed | +| Logging, tracking or another recovery step itself fails | Use an independent fallback when available; do not recursively call the failing sink | Recursion can replace the original failure or end recovery | +| An exception represents an explicitly supported outcome, such as cancellation or an unavailable optional capability | Follow that operation's contract; document an intentional ignored exception at the catch site | Expected outcomes are not automatically application failures | + +These are requirements for changes, not a claim that every existing catch site already uses both services. [BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs), for example, logs settings and telemetry creation failures and continues with defaults or disabled telemetry; policy initialization failures propagate. + +### API boundary + +[HostingExtensions.AddAndConfigureProblemDetails](../../src/Infrastructure/Extensions/HostingExtensions.cs) owns reporting for exceptions mapped to API responses. Its mappings track exceptions. Specific mappings add host diagnostics at `Verbose`; the catch-all mapping adds diagnostics regardless of that setting. Exception details are enabled for the UI to display and copy. Do not add duplicate tracking in each controller for exceptions that propagate to these mappings. + +## Local logging and the diagnostics pane + +[BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs) configures the host logger factory: + +| Sink | Filter configured here | Destination | +| --- | --- | --- | +| EventSource | No provider-specific minimum | ETW listeners, subject to effective logging filters | +| Windows Event Log | `Warning` and above | `Application` log | +| DiagnosticMessageLoggerProvider | `Warning` and above | In-memory host diagnostics | +| Console and Debug | No provider-specific minimum; Debug builds only | Development output, subject to effective logging filters | + +[AppEnvironment.Diagnostics](../../src/Infrastructure/AppEnvironment.cs) stores messages written by the diagnostics logger and explicit `AddDiagnostics` calls. Collection in this store is separate from visibility in the UI. The [frontend logger](../../src/Scripts/controllers/logger.ts) polls `GetDiagnostics` only at diagnostic level `Verbose`; host messages may be collected without appearing in the pane. The host store is lost at process exit. + +## Telemetry consent + +The host uses Application Insights through [TelemetryService](../../src/Infrastructure/Telemetry/TelemetryService.cs). The `TelemetryEnabled` setter assigns the value it receives, so every caller resolves the consent first: + +| Entry point | Current behavior | +| --- | --- | +| Bootstrap | [TelemetryConsent.Resolve](../../src/Infrastructure/Telemetry/TelemetryConsent.cs) gives a configured policy precedence over the user preference | +| Preferences saved through `UpdateOptions` | [ApplicationController](../../src/Controllers/ApplicationController.cs) resolves the consent the same way before assigning it | +| Crash dialog Send | Temporarily enables host telemetry, tracks the exception and awaits one flush, then restores the previous value | + +A disabled service drops new tracked exceptions. If bootstrap supplied `NullTelemetryService`, enabling it or requesting a flush still cannot send a report. + +Never add personal data to telemetry messages or properties. The single-instance pipe name includes the user SID and must not be recorded. [DefaultTelemetryProcessor](../../src/Infrastructure/Telemetry/DefaultTelemetryProcessor.cs) redacts supported sensitive tags in exception messages; it is not a general sanitizer for arbitrary properties or untagged data. + +### Flush and shutdown + +`TelemetryService.TrackException` calls synchronous `Flush` to move buffered telemetry into the channel's transmission pipeline. That call does not wait for network delivery. The [ServerTelemetryChannel 2.23.0 contract](https://raw.githubusercontent.com/microsoft/ApplicationInsights-dotnet/2.23.0/BASE/src/ServerTelemetryChannel/ServerTelemetryChannel.cs) defines `FlushAsync` success as transfer out of process, which can mean server delivery or local persistence; success is not proof that ingestion has completed. + +| Caller | Operation | Cancellation timeout | +| --- | --- | --- | +| `TelemetryService.Dispose` | `TryFlushBeforeShutdown` | 5 s | +| Fatal `UnhandledExceptionHandler`, after the dialog closes | `TryFlushBeforeShutdown` | 10 s | +| Crash dialog Send | `FlushAsync` | 30 s | + +`TryFlushBeforeShutdown` blocks the caller and returns `false` on `OperationCanceledException`. It does not catch other exception types. Its name must not be treated as a general no-throw contract. + +The server channel stores unsent transmissions under `.cache/.telemetry` in the application data folder when it can persist them. Data still only in memory at termination can be lost. Do not remove the awaited shutdown flush on the assumption that synchronous `Flush` guarantees delivery. The [telemetry tests](../../test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs) cover shutdown delivery to a local endpoint and persistence when the endpoint is unavailable. diff --git a/src/Host/BootstrapContext.cs b/src/Host/BootstrapContext.cs new file mode 100644 index 00000000..1a730d8a --- /dev/null +++ b/src/Host/BootstrapContext.cs @@ -0,0 +1,47 @@ +using System; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.Telemetry; + +namespace Sqlbi.Bravo.Host; + +/// +/// Holds the services used to bootstrap the application. +/// +/// +/// The context owns the lifetime of the services it holds. +/// +internal sealed class BootstrapContext : IDisposable +{ + private bool _disposed; + + internal BootstrapContext( + ILoggerFactory loggerFactory, + IPolicyService policyService, + IUserSettings settings, + ITelemetryService telemetry) + { + LoggerFactory = loggerFactory; + PolicyService = policyService; + Settings = settings; + Telemetry = telemetry; + } + + public ILoggerFactory LoggerFactory { get; } + public IPolicyService PolicyService { get; } + public IUserSettings Settings { get; } + public ITelemetryService Telemetry { get; } + + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + Telemetry.Dispose(); + // LoggerFactory is disposed last so all other services can safely log during their disposal. + LoggerFactory.Dispose(); + } +} diff --git a/src/Host/BootstrapContextFactory.Log.cs b/src/Host/BootstrapContextFactory.Log.cs new file mode 100644 index 00000000..628f64d0 --- /dev/null +++ b/src/Host/BootstrapContextFactory.Log.cs @@ -0,0 +1,13 @@ +using System; +using Microsoft.Extensions.Logging; + +namespace Sqlbi.Bravo.Host; + +internal sealed partial class BootstrapContextFactory +{ + [LoggerMessage(EventId = 1, Level = LogLevel.Error, Message = "Telemetry could not be created. Telemetry is disabled for this session.")] + private static partial void LogTelemetryCreationFailed(ILogger logger, Exception exception); + + [LoggerMessage(EventId = 2, Level = LogLevel.Error, Message = "User settings could not be created. Default settings will be used for this session.")] + private static partial void LogUserSettingsCreationFailed(ILogger logger, Exception exception); +} diff --git a/src/Host/BootstrapContextFactory.cs b/src/Host/BootstrapContextFactory.cs new file mode 100644 index 00000000..748b299d --- /dev/null +++ b/src/Host/BootstrapContextFactory.cs @@ -0,0 +1,109 @@ +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; +using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.Telemetry; + +namespace Sqlbi.Bravo.Host; + +internal sealed partial class BootstrapContextFactory( + ILoggerFactory loggerFactory, + IPolicyServiceFactory policyFactory, + IUserSettingsServiceFactory settingsFactory, + ITelemetryServiceFactory telemetryFactory) +{ + private readonly ILoggerFactory _loggerFactory = loggerFactory; + private readonly IPolicyServiceFactory _policyFactory = policyFactory; + private readonly IUserSettingsServiceFactory _settingsFactory = settingsFactory; + private readonly ITelemetryServiceFactory _telemetryFactory = telemetryFactory; + + public static BootstrapContext CreateDefault() + { + var loggerFactory = CreateLoggerFactory(); + try + { + var factory = new BootstrapContextFactory( + loggerFactory, + new PolicyServiceFactory(), + new UserSettingsServiceFactory(), + new TelemetryServiceFactory()); + + return factory.Create(); + } + catch + { + loggerFactory.Dispose(); + throw; + } + } + + public BootstrapContext Create() + { + var policies = CreatePolicies(); + var settings = CreateSettings(); + + var consent = TelemetryConsent.Resolve(policies.Current, settings); + var telemetry = CreateTelemetry(consent.IsEnabled); + + return new BootstrapContext(_loggerFactory, policies, settings, telemetry); + } + + private static ILoggerFactory CreateLoggerFactory() + { + return LoggerFactory.Create((builder) => + { + builder.AddEventSourceLogger(); + builder.AddEventLog(); + builder.AddFilter((level) => level >= LogLevel.Warning); + builder.AddDiagnosticMessages(); + builder.AddFilter((level) => level >= LogLevel.Warning); +#if DEBUG + builder.AddConsole(); + builder.AddDebug(); +#endif + }); + } + + /// + /// Policy initialization failures must halt the application startup + /// to prevent bypassing administrative restrictions. + /// + private IPolicyService CreatePolicies() + { + var factory = _policyFactory.Create(); + + // Force instantiation to detect any potential exceptions + _ = factory.Current; + + return factory; + } + + private IUserSettings CreateSettings() + { + try + { + return _settingsFactory.Create(); + } + catch (Exception ex) + { + LogUserSettingsCreationFailed(_loggerFactory.CreateLogger(), ex); + return new UserSettings(); + } + } + + private ITelemetryService CreateTelemetry(bool enabled) + { + try + { + return _telemetryFactory.Create(enabled); + } + catch (Exception ex) + { + LogTelemetryCreationFailed(_loggerFactory.CreateLogger(), ex); + return new NullTelemetryService(); + } + } +} diff --git a/src/Host/BravoApplication.cs b/src/Host/BravoApplication.cs index caa2bc4e..8e99c75d 100644 --- a/src/Host/BravoApplication.cs +++ b/src/Host/BravoApplication.cs @@ -14,9 +14,9 @@ internal sealed class BravoApplication : IDisposable { private readonly WebApplication _innerApplication; - public static BravoApplicationBuilder CreateBuilder(BravoApplicationInitializationContext context) + public static BravoApplicationBuilder CreateBuilder(BootstrapContext bootstrap) { - return new BravoApplicationBuilder(context); + return new BravoApplicationBuilder(bootstrap); } internal BravoApplication(WebApplication innerApplication) @@ -24,31 +24,40 @@ internal BravoApplication(WebApplication innerApplication) _innerApplication = innerApplication; } + internal IServiceProvider Services => _innerApplication.Services; + /// /// Runs the application and starts the message loop. /// public void Run() { + var instance = Services.GetRequiredService(); + if (!instance.IsPrimary) + { + instance.RedirectActivationToPrimary(); + return; + } + _innerApplication.Start(); try { // Create the main window through DI without registering it. // Its lifetime is owned by the WinForms message loop rather than the service container. - using var window = ActivatorUtilities.CreateInstance(_innerApplication.Services); -/* - // If the host stops independently of the UI, the message loop must also end. - // Otherwise the window could outlive the backend it depends on. - // ApplicationStopping is raised on a background thread, so closing the window - // is marshaled to the UI thread. - // The registration is disposed before StopAsync() is invoked below, preventing - // the normal window-close shutdown path from re-entering. - var lifetime = _innerApplication.Services.GetRequiredService(); - using var stopRegistration = lifetime.ApplicationStopping.Register(() => - { - if (window.IsHandleCreated) - window.BeginInvoke(window.Close); - }); -*/ + using var window = ActivatorUtilities.CreateInstance(Services); + + //// If the host stops independently of the UI, the message loop must also end. + //// Otherwise the window could outlive the backend it depends on. + //// ApplicationStopping is raised on a background thread, so closing the window + //// is marshaled to the UI thread. + //// The registration is disposed before StopAsync() is invoked below, preventing + //// the normal window-close shutdown path from re-entering. + //var lifetime = _innerApplication.Services.GetRequiredService(); + //using var stopRegistration = lifetime.ApplicationStopping.Register(() => + //{ + // if (window.IsHandleCreated) + // window.BeginInvoke(window.Close); + //}); + Application.Run(window); } finally diff --git a/src/Host/BravoApplicationBuilder.cs b/src/Host/BravoApplicationBuilder.cs index 1ab481d7..f283d9d4 100644 --- a/src/Host/BravoApplicationBuilder.cs +++ b/src/Host/BravoApplicationBuilder.cs @@ -5,10 +5,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.EventLog; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; -using Sqlbi.Bravo.Infrastructure.Diagnostics; using Sqlbi.Bravo.Infrastructure.Services; namespace Sqlbi.Bravo.Host; @@ -16,21 +13,15 @@ namespace Sqlbi.Bravo.Host; /// /// Provides a builder for creating and configuring a instance. /// -/// -/// Everything is configured by the time the builder is returned, and is the only -/// method: the registrations are composed through , not -/// by mutating the builder from outside. Should a caller — a test, typically — ever need to override -/// a registration, expose as a pass-through property -/// then; it is deliberately not exposed while nothing needs it. -/// internal sealed class BravoApplicationBuilder { private readonly WebApplicationBuilder _innerBuilder; - internal BravoApplicationBuilder(BravoApplicationInitializationContext context) + internal BravoApplicationBuilder(BootstrapContext bootstrap) { // CreateEmptyBuilder, not CreateBuilder: no configuration sources, no environment variables and - // no implicit logging providers. Everything the host needs is declared explicitly below. + // no implicit logging providers. The logging pipeline comes from the bootstrap context, everything + // else the host needs is declared explicitly below. _innerBuilder = WebApplication.CreateEmptyBuilder(new WebApplicationOptions { EnvironmentName = Environments.Production, @@ -38,10 +29,11 @@ internal BravoApplicationBuilder(BravoApplicationInitializationContext context) }); ConfigureHosting(_innerBuilder); - ConfigureLogging(_innerBuilder); - ConfigureServices(_innerBuilder, context); + ConfigureServices(_innerBuilder, bootstrap); } + internal IServiceCollection Services => _innerBuilder.Services; + /// /// Builds the host, configures its request pipeline, and returns the . /// @@ -77,28 +69,14 @@ private static void ConfigureHosting(WebApplicationBuilder builder) #endif } - private static void ConfigureLogging(WebApplicationBuilder builder) - { - builder.Logging.AddEventSourceLogger(); - builder.Logging.AddEventLog(); - builder.Logging.AddFilter((level) => level >= LogLevel.Warning); - builder.Logging.AddDiagnosticMessages(); - builder.Logging.AddFilter((level) => level >= LogLevel.Warning); -#if DEBUG - builder.Logging.AddConsole(); - builder.Logging.AddDebug(); -#endif - } - - private static void ConfigureServices(WebApplicationBuilder builder, BravoApplicationInitializationContext context) + private static void ConfigureServices(WebApplicationBuilder builder, BootstrapContext bootstrap) { builder.Services.AddOptions() .Configure((settings) => settings.FromCommandLineArguments()); //.ValidateDataAnnotations(); builder.Services.AddSingleton(); - builder.Services.AddBravoInitializationServices(context); builder.Services.AddBravoRestApi(); - builder.Services.AddBravoServices(); + builder.Services.AddBravoServices(bootstrap); } private static void ConfigurePipeline(WebApplication application) diff --git a/src/Host/BravoApplicationInitializer.cs b/src/Host/BravoApplicationConfiguration.cs similarity index 68% rename from src/Host/BravoApplicationInitializer.cs rename to src/Host/BravoApplicationConfiguration.cs index 6aaf6629..84c8340c 100644 --- a/src/Host/BravoApplicationInitializer.cs +++ b/src/Host/BravoApplicationConfiguration.cs @@ -10,25 +10,17 @@ namespace Sqlbi.Bravo.Host; /// -/// Initializes the application process, applying process-wide settings -/// and composing the necessary components before the host is created. +/// Configures the application environment before the application starts. /// -internal static class BravoApplicationInitializer +internal static class BravoApplicationConfiguration { - /// - /// Initializes the application process, applying process-wide settings - /// - public static BravoApplicationInitializationContext Initialize() + public static void Initialize() { - ConfigureWebProxy(); ConfigureDirectories(); + ConfigureWebProxy(); ConfigureRuntimeOptimization(); WebView2Helper.EnsureRuntimeIsInstalled(); - - var instance = BravoApplicationInstance.Create(); - - return new BravoApplicationInitializationContext(instance); } private static void ConfigureWebProxy() @@ -46,7 +38,7 @@ private static void ConfigureDirectories() private static void ConfigureRuntimeOptimization() { - ProfileOptimization.SetProfileRoot(AppEnvironment.ApplicationDataPath); + ProfileOptimization.SetProfileRoot(AppEnvironment.ApplicationCachePath); ProfileOptimization.StartProfile(".jitprofile"); } } diff --git a/src/Host/BravoApplicationInitializationContext.cs b/src/Host/BravoApplicationInitializationContext.cs deleted file mode 100644 index c2af5373..00000000 --- a/src/Host/BravoApplicationInitializationContext.cs +++ /dev/null @@ -1,28 +0,0 @@ -using System; - -namespace Sqlbi.Bravo.Host; - -/// -/// Carries what produced, as typed entries. -/// -/// -/// Not a container and not a facade: it resolves nothing, forwards nothing, and never leaves the -/// composition root. publishes each entry to the service -/// collection as an instance; ownership of the disposable entries stays here. -/// -internal sealed class BravoApplicationInitializationContext( - IBravoApplicationInstance instance) : IDisposable -{ - /// - /// Gets the single-instance binding; decides - /// whether this process runs the application or redirects its activation. - /// - public IBravoApplicationInstance Instance { get; } = instance; - - /// - /// Disposes the entries the initialization created and owns, in reverse creation order. Entries - /// that are still process-wide statics (telemetry, user settings, web proxy) own themselves and - /// are not disposed here until the initialization creates them. - /// - public void Dispose() => Instance.Dispose(); -} diff --git a/src/Host/BravoApplicationInstance.Log.cs b/src/Host/BravoApplicationInstance.Log.cs new file mode 100644 index 00000000..be9eafbe --- /dev/null +++ b/src/Host/BravoApplicationInstance.Log.cs @@ -0,0 +1,17 @@ +using System; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure.SingleInstance; + +namespace Sqlbi.Bravo.Host; + +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 2d882f25..57974382 100644 --- a/src/Host/BravoApplicationInstance.cs +++ b/src/Host/BravoApplicationInstance.cs @@ -1,9 +1,8 @@ using System; -using System.Diagnostics; using System.IO; using System.Text.Json; +using Microsoft.Extensions.Logging; 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; @@ -26,32 +25,25 @@ internal interface IBravoApplicationInstance : IInstanceEvents, IDisposable void RedirectActivationToPrimary(); } -internal sealed class BravoApplicationInstance : IBravoApplicationInstance +internal sealed partial class BravoApplicationInstance : IBravoApplicationInstance { private readonly SingleInstanceOptions _options; private readonly SingleInstanceServer? _server; + private readonly ITelemetryService _telemetry; + private readonly ILogger _logger; private bool _disposed; - /// - /// Creates a , claiming the primary role if no other process holds it. - /// - public static BravoApplicationInstance Create() - { - var options = new SingleInstanceOptions - { - PipeName = InstancePipeName.Create(), - }; - - _ = SingleInstanceServer.TryStart(options, out var server); - - return new BravoApplicationInstance(options, server); - } - - private BravoApplicationInstance(SingleInstanceOptions options, SingleInstanceServer? server) + public BravoApplicationInstance( + SingleInstanceOptions options, + SingleInstanceServer? server, + ITelemetryService telemetry, + ILoggerFactory loggerFactory) { _options = options; _server = server; + _telemetry = telemetry; + _logger = loggerFactory.CreateLogger(); if (_server is not null) { @@ -73,9 +65,11 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) { startupMessage = JsonSerializer.Deserialize(e.Payload); } - catch (JsonException) + catch (JsonException ex) { // An unreadable payload still activates the window, with no document to open. + LogActivationPayloadUnreadable(ex); + _telemetry.TrackException(ex); } // Known limitation: requests arriving while this instance is still starting up are dropped @@ -89,7 +83,13 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) ActivationRequested?.Invoke(this, new InstanceActivationRequestedEventArgs(startupMessage)); } - private void OnError(object? sender, ErrorEventArgs e) => Report(e.GetException()); + private void OnError(object? sender, ErrorEventArgs e) + { + var exception = e.GetException(); + + LogListenerFailed(exception); + _telemetry.TrackException(exception); + } /// public void RedirectActivationToPrimary() @@ -107,16 +107,11 @@ public void RedirectActivationToPrimary() // 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); + LogActivationRedirectFailed(result.Exception, result.Status); + _telemetry.TrackException(result.Exception); } } - private static void Report(Exception exception) - { - ExceptionHelper.WriteToEventLog(exception, EventLogEntryType.Warning); - TelemetryService.Instance.TrackException(exception); - } - /// public void Dispose() { diff --git a/src/Host/BravoGlobalExceptionHandling.cs b/src/Host/BravoGlobalExceptionHandling.cs new file mode 100644 index 00000000..e776cdb8 --- /dev/null +++ b/src/Host/BravoGlobalExceptionHandling.cs @@ -0,0 +1,20 @@ +using Sqlbi.Bravo.Infrastructure.Diagnostics; + +namespace Sqlbi.Bravo.Host; + +internal static class BravoGlobalExceptionHandling +{ + public static void Configure(BootstrapContext context) + { + var dialog = new ErrorReportTaskDialog(context.Telemetry); + + var handler = new UnhandledExceptionHandler( + dialog, context.Telemetry, context.LoggerFactory); + + GlobalExceptionHandling.Configure(handler.Handle); + +#if DEBUG + CrashTest.RunIfRequested(); +#endif + } +} diff --git a/src/Host/BravoServiceCollectionExtensions.cs b/src/Host/BravoServiceCollectionExtensions.cs index 14913186..a5b9d4be 100644 --- a/src/Host/BravoServiceCollectionExtensions.cs +++ b/src/Host/BravoServiceCollectionExtensions.cs @@ -1,9 +1,11 @@ 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.Policies; 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; @@ -15,24 +17,7 @@ namespace Sqlbi.Bravo.Host; internal static class BravoServiceCollectionExtensions { /// - /// Publishes what produced. These are instance - /// registrations: the container resolves them but never disposes them — their owner is the - /// initialization context. - /// - public static IServiceCollection AddBravoInitializationServices(this IServiceCollection services, BravoApplicationInitializationContext context) - { - // 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(context.Instance); - - services.AddSingleton(new PolicyServiceFactory().Create()); - services.AddSingleton(TelemetryService.Instance); - - return services; - } - - /// - /// Registers the REST API surface. + /// Registers and configures the services required for the Bravo REST API. /// public static IServiceCollection AddBravoRestApi(this IServiceCollection services) { @@ -48,10 +33,12 @@ public static IServiceCollection AddBravoRestApi(this IServiceCollection service } /// - /// Registers the core services that implement the application's functionality. + /// Registers and configures the services required for the Bravo application. /// - public static IServiceCollection AddBravoServices(this IServiceCollection services) + public static IServiceCollection AddBravoServices(this IServiceCollection services, BootstrapContext bootstrap) { + services.AddBootstrapServices(bootstrap); + services.AddBravoApplicationInstance(); services.AddHttpClient(); services.AddPowerBI(); services.AddSingleton(); @@ -67,4 +54,36 @@ public static IServiceCollection AddBravoServices(this IServiceCollection servic return services; } + + private static IServiceCollection AddBravoApplicationInstance(this IServiceCollection services) + { + 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); + }); + + return services; + } + + private static IServiceCollection AddBootstrapServices(this IServiceCollection services, BootstrapContext bootstrap) + { + // Instance registrations: the container resolves them without creating or disposing them. + + // Replace the default ILoggerFactory registered by the WebApplicationBuilder + services.Replace(ServiceDescriptor.Singleton(bootstrap.LoggerFactory)); + + services.AddSingleton(bootstrap.PolicyService); + services.AddSingleton(bootstrap.Settings); + services.AddSingleton(bootstrap.Telemetry); + + return services; + } } diff --git a/src/Host/CrashTest.cs b/src/Host/CrashTest.cs new file mode 100644 index 00000000..5d1b6cf8 --- /dev/null +++ b/src/Host/CrashTest.cs @@ -0,0 +1,77 @@ +#if DEBUG +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Sqlbi.Bravo.Host; + +/// +/// Provides a mechanism to test the application's global exception handling +/// by throwing exceptions in different contexts. +/// +/// +/// Set the environment variable BRAVO_CrashTest before starting a Debug build: startup +/// throws before the message loop, ui on the UI thread, thread on a background thread, and +/// unobserved in a faulted task that is never observed. The last three throw after 3 seconds. +/// +internal static class CrashTest +{ + private static System.Windows.Forms.Timer? s_timer; + + public static void RunIfRequested() + { + switch (Environment.GetEnvironmentVariable("BRAVO_CrashTest")) + { + case "startup": + throw new InvalidOperationException("Crash test: before the message loop."); + + case "ui": + // Ticks on the thread running the message loop, so the exception escapes a window procedure. + s_timer = new System.Windows.Forms.Timer { Interval = 3_000 }; + s_timer.Tick += (_, _) => throw new InvalidOperationException("Crash test: UI thread."); + s_timer.Start(); + break; + + case "thread": + StartDelayed(() => throw new InvalidOperationException("Crash test: background thread.")); + break; + + case "unobserved": + StartDelayed(() => + { + ThrowUnobserved(); + + // The event is raised when the faulted task is finalized, not when it faults. + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + }); + break; + } + } + + private static void StartDelayed(Action action) + { + var thread = new Thread(() => + { + Thread.Sleep(3_000); + action(); + }) + { + IsBackground = true, + }; + + thread.Start(); + } + + // In its own method: a local in the caller would keep the task alive. + private static void ThrowUnobserved() + { + var task = Task.Run(() => throw new InvalidOperationException("Crash test: unobserved task.")); + + // Wait for the task to complete so that it can be finalized + while (!task.IsCompleted) + Thread.Sleep(10); + } +} +#endif diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 93d105b4..6a9f4a06 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -41,7 +41,7 @@ internal partial class AppWindow : Form };"; public static SynchronizationContext? UISynchronizationContext { get; set; } - private readonly IInstanceEvents _instanceEvents; + private readonly IInstanceEvents _applicationInstance; private readonly IServerAddressProvider _serverAddressProvider; private readonly IOptions _startupSettingsOptionsAccessor; private readonly WebView2ProxyAuthHandler _proxyAuthHandler; @@ -49,12 +49,12 @@ internal partial class AppWindow : Form private readonly IPolicyService _policyService; public AppWindow( - IInstanceEvents instanceEvents, + IBravoApplicationInstance applicationInstance, IServerAddressProvider serverAddressProvider, IOptions startupSettingsOptionsAccessor, IPolicyService policyService) { - _instanceEvents = instanceEvents; + _applicationInstance = applicationInstance; _serverAddressProvider = serverAddressProvider; _startupSettingsOptionsAccessor = startupSettingsOptionsAccessor; _policyService = policyService; @@ -183,19 +183,19 @@ private void OnFormLoad(object? sender, EventArgs e) CenterToScreen(); - _instanceEvents.ActivationRequested += OnActivationRequestedRestoreWindowToForeground; + _applicationInstance.ActivationRequested += OnActivationRequestedRestoreWindowToForeground; if (AppEnvironment.IsDiagnosticLevelVerbose) { - var content = EnvironmentInfo.Collect().ToDictionary(); - AppEnvironment.AddDiagnostics(DiagnosticMessageType.Json, name: $"{nameof(AppWindow)}.{nameof(EnvironmentInfo)}", content: JsonSerializer.Serialize(content)); + var content = EnvironmentDiagnosticsCollector.Collect(); + AppEnvironment.AddDiagnostics(DiagnosticMessageType.Json, name: $"{nameof(AppWindow)}.{nameof(EnvironmentDiagnostics)}", content: JsonSerializer.Serialize(content)); } } private void OnFormClosed(object? sender, FormClosedEventArgs e) { - _instanceEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; - _instanceEvents.ActivationRequested -= OnActivationRequestedSendStartupWebMessage; + _applicationInstance.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; + _applicationInstance.ActivationRequested -= OnActivationRequestedSendStartupWebMessage; } private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoadedEventArgs e) @@ -208,7 +208,7 @@ private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoa BackgroundImage = null; SendAppStartupWebMessage(); - _instanceEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage; + _applicationInstance.ActivationRequested += OnActivationRequestedSendStartupWebMessage; } } diff --git a/src/Infrastructure/Configuration/UserSettingsServiceFactory.cs b/src/Infrastructure/Configuration/UserSettingsServiceFactory.cs new file mode 100644 index 00000000..2adc7188 --- /dev/null +++ b/src/Infrastructure/Configuration/UserSettingsServiceFactory.cs @@ -0,0 +1,13 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; + +namespace Sqlbi.Bravo.Infrastructure.Configuration; + +internal interface IUserSettingsServiceFactory +{ + IUserSettings Create(); +} + +internal sealed class UserSettingsServiceFactory : IUserSettingsServiceFactory +{ + public IUserSettings Create() => UserPreferences.Current; +} diff --git a/src/Infrastructure/Diagnostics/EnvironmentDiagnostics.cs b/src/Infrastructure/Diagnostics/EnvironmentDiagnostics.cs new file mode 100644 index 00000000..508c0a73 --- /dev/null +++ b/src/Infrastructure/Diagnostics/EnvironmentDiagnostics.cs @@ -0,0 +1,103 @@ +using System; +using System.Runtime.InteropServices; +using System.Text.Json; +using System.Text.Json.Serialization; +using Sqlbi.Bravo.Infrastructure.Telemetry; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +internal sealed record EnvironmentDiagnostics( + [property: JsonPropertyName("timestampUtc")] string TimestampUtc, + [property: JsonPropertyName("timestampLocal")] string TimestampLocal, + [property: JsonPropertyName("application")] ApplicationDiagnostics Application, + [property: JsonPropertyName("process")] ProcessDiagnostics Process, + [property: JsonPropertyName("os")] OperatingSystemDiagnostics OS, + [property: JsonPropertyName("runtime")] RuntimeDiagnostics Runtime, + [property: JsonPropertyName("components")] ComponentDiagnostics Components) +{ + private static readonly JsonSerializerOptions? s_options = new() + { + WriteIndented = true + }; + + public string ToJsonString() + => JsonSerializer.Serialize(this, s_options); +} + +internal sealed record ApplicationDiagnostics( + [property: JsonPropertyName("sessionId")] string SessionId, + [property: JsonPropertyName("version")] string Version, + [property: JsonPropertyName("publishMode")] string PublishMode, + [property: JsonPropertyName("deploymentMode")] string DeploymentMode, + [property: JsonPropertyName("dataPath")] string DataPath +); + +internal sealed record ProcessDiagnostics( + [property: JsonPropertyName("id")] string Id, + [property: JsonPropertyName("path")] string Path, + [property: JsonPropertyName("architecture")] string Architecture, + [property: JsonPropertyName("processorCount")] string ProcessorCount +); + +public sealed record OperatingSystemDiagnostics( + [property: JsonPropertyName("version")] string Version, + [property: JsonPropertyName("description")] string Description, + [property: JsonPropertyName("architecture")] string Architecture +); + +internal sealed record RuntimeDiagnostics( + [property: JsonPropertyName("identifier")] string Identifier, + [property: JsonPropertyName("description")] string Description +); + +internal sealed record ComponentDiagnostics( + [property: JsonPropertyName("webView2Version")] string WebView2Version +); + +internal static class EnvironmentDiagnosticsCollector +{ + public static EnvironmentDiagnostics Collect() + { + return new EnvironmentDiagnostics( + TimestampUtc: DateTimeOffset.UtcNow.ToString("O"), + TimestampLocal: DateTime.Now.ToString("O"), + Application: new( + SessionId: SafeRead(() => TelemetrySessionInfo.SessionId), + Version: SafeRead(() => AppVersion.InformationalVersion), + PublishMode: SafeRead(() => AppEnvironment.PublishMode.ToString()), + DeploymentMode: SafeRead(() => AppEnvironment.DeploymentMode.ToString()), + DataPath: SafeRead(() => AppEnvironment.ApplicationDataPath) + ), + Process: new( + Id: SafeRead(() => Environment.ProcessId.ToString()), + Path: SafeRead(() => Environment.ProcessPath), + Architecture: SafeRead(() => RuntimeInformation.ProcessArchitecture.ToString()), + ProcessorCount: SafeRead(() => Environment.ProcessorCount.ToString()) + ), + OS: new( + Version: SafeRead(() => Environment.OSVersion.ToString()), + Description: SafeRead(() => RuntimeInformation.OSDescription), + Architecture: SafeRead(() => RuntimeInformation.OSArchitecture.ToString()) + ), + Runtime: new( + Identifier: SafeRead(() => RuntimeInformation.RuntimeIdentifier), + Description: SafeRead(() => RuntimeInformation.FrameworkDescription) + ), + Components: new( + WebView2Version: SafeRead(() => AppEnvironment.WebView2VersionInfo) + ) + ); + } + + private static string SafeRead(Func read, string fallback = "n/a") + { + try + { + return read() ?? fallback; + } + catch (Exception ex) + { + return $"{fallback} ({ex.GetType().Name})"; + } + } +} diff --git a/src/Infrastructure/Diagnostics/EnvironmentInfo.cs b/src/Infrastructure/Diagnostics/EnvironmentInfo.cs deleted file mode 100644 index bba4e43a..00000000 --- a/src/Infrastructure/Diagnostics/EnvironmentInfo.cs +++ /dev/null @@ -1,93 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Globalization; -using System.Linq; -using System.Runtime.InteropServices; -using System.Text; -using System.Text.Json; -using Sqlbi.Bravo.Infrastructure.Telemetry; - -namespace Sqlbi.Bravo.Infrastructure.Diagnostics; - -/// -/// Provides diagnostic information about the environment in which the application is running. -/// -internal sealed class EnvironmentInfo -{ - private readonly IReadOnlyList> _entries; - - /// - /// Collects the environment information and returns an instance of . - /// - public static EnvironmentInfo Collect() - { - var entries = new KeyValuePair[] - { - new("TimestampUtc", DateTime.UtcNow.ToString("O", CultureInfo.InvariantCulture)), - new("TimestampLocal", DateTime.Now.ToString("O", CultureInfo.InvariantCulture)), - // Application - new("ApplicationSessionId", SafeRead(() => TelemetrySessionInfo.SessionId)), - new("ApplicationVersion", SafeRead(() => AppVersion.InformationalVersion)), - new("ApplicationPublishMode", SafeRead(() => AppEnvironment.PublishMode.ToString())), - new("ApplicationDeploymentMode", SafeRead(() => AppEnvironment.DeploymentMode.ToString())), - new("ApplicationDataPath", SafeRead(() => AppEnvironment.ApplicationDataPath)), - // Process - new("ProcessId", SafeRead(() => Environment.ProcessId.ToString(CultureInfo.InvariantCulture))), - new("ProcessPath", SafeRead(() => Environment.ProcessPath)), - new("ProcessSessionId", SafeRead(() => AppEnvironment.SessionId.ToString(CultureInfo.InvariantCulture))), - new("ProcessArchitecture", SafeRead(() => RuntimeInformation.ProcessArchitecture.ToString())), - new("ProcessProcessorCount", SafeRead(() => Environment.ProcessorCount.ToString(CultureInfo.InvariantCulture))), - // Machine - new("OSDescription", SafeRead(() => RuntimeInformation.OSDescription)), - new("OSArchitecture", SafeRead(() => RuntimeInformation.OSArchitecture.ToString())), - // Runtime - new("RuntimeDescription", SafeRead(() => RuntimeInformation.FrameworkDescription)), - new("RuntimeIdentifier", SafeRead(() => RuntimeInformation.RuntimeIdentifier)), - // Components - new("WebView2Version", SafeRead(() => AppEnvironment.WebView2VersionInfo)), - }; - - return new EnvironmentInfo(entries); - } - - private EnvironmentInfo(IReadOnlyList> entries) - { - _entries = entries; - } - - /// - /// Returns the environment information as a text block. - /// - public string ToText() - { - var builder = new StringBuilder(); - - builder.AppendLine("# Environment Information"); - builder.AppendLine(); - - foreach (var (name, value) in _entries) - builder.Append($"- {name}: ").AppendLine(value); - - return builder.ToString(); - } - - /// - /// Returns the environment information as dictionary. - /// - public Dictionary ToDictionary() - { - return _entries.ToDictionary((entry) => entry.Key, (entry) => entry.Value); - } - - internal static string SafeRead(Func read, string fallback = "n/a") - { - try - { - return read() ?? fallback; - } - catch (Exception ex) - { - return $"unavailable ({ex.GetType().Name})"; - } - } -} diff --git a/src/Infrastructure/Diagnostics/ErrorReport.cs b/src/Infrastructure/Diagnostics/ErrorReport.cs index 9980086d..a718ea6f 100644 --- a/src/Infrastructure/Diagnostics/ErrorReport.cs +++ b/src/Infrastructure/Diagnostics/ErrorReport.cs @@ -1,8 +1,6 @@ using System; using System.IO; -using System.Runtime.InteropServices; using System.Text; -using System.Windows.Forms; namespace Sqlbi.Bravo.Infrastructure.Diagnostics; @@ -11,47 +9,33 @@ namespace Sqlbi.Bravo.Infrastructure.Diagnostics; /// internal sealed class ErrorReport { - private const string FileName = "ErrorReport.txt"; - - private readonly EnvironmentInfo _environment; + private readonly EnvironmentDiagnostics _diagnostics; private readonly Lazy _text; - /// - /// Creates an instance for the given exception. - /// public static ErrorReport Create(Exception exception) { - var environment = EnvironmentInfo.Collect(); - return new ErrorReport(exception, environment); + var diagnostics = EnvironmentDiagnosticsCollector.Collect(); + return new ErrorReport(exception, diagnostics); } - internal ErrorReport(Exception exception, EnvironmentInfo environment) + private ErrorReport(Exception exception, EnvironmentDiagnostics diagnostics) { Exception = exception; - _environment = environment; + _diagnostics = diagnostics; _text = new Lazy(GenerateText); } public Exception Exception { get; } - - /// - /// The text representation of the error report. - /// public string Text => _text.Value; - - /// - /// The file path where the error report was saved, or null if saving failed. - /// public string? FilePath { get; private set; } - /// - /// Attempts to save the error report to a file in the application data folder. - /// public bool TrySave() { try { - var filePath = Path.Combine(AppEnvironment.ApplicationDataPath, FileName); + var directory = Directory.CreateDirectory(AppEnvironment.ApplicationDataPath); + var filePath = Path.Combine(directory.FullName, "ErrorReport.txt"); + File.WriteAllText(filePath, Text, Encoding.UTF8); FilePath = filePath; @@ -59,41 +43,29 @@ public bool TrySave() } catch (Exception) { + FilePath = null; return false; } } - /// - /// Attempts to copy the report to the clipboard. - /// - public bool TryCopyToClipboard() - { - try - { - Clipboard.SetDataObject( - data: new DataObject(DataFormats.UnicodeText, Text), - copy: true, - retryTimes: 10, - retryDelay: 100); - - return true; - } - catch (ExternalException) - { - return false; - } - } - - public override string ToString() => Text; - private string GenerateText() { var builder = new StringBuilder(); - builder.AppendLine(_environment.ToText()); + builder.AppendLine("# Bravo for Power BI — Error Report"); - builder.AppendLine("# Error Details"); builder.AppendLine(); + builder.AppendLine("## Environment information"); + builder.AppendLine(); + + builder.AppendLine("```json"); + builder.AppendLine(_diagnostics.ToJsonString()); + builder.AppendLine("```"); + + builder.AppendLine(); + builder.AppendLine("## Exception details"); + builder.AppendLine(); + builder.AppendLine("```"); builder.AppendLine(Exception.ToString()); builder.AppendLine("```"); diff --git a/src/Infrastructure/Diagnostics/ErrorReportDialog.cs b/src/Infrastructure/Diagnostics/ErrorReportDialog.cs deleted file mode 100644 index 55901869..00000000 --- a/src/Infrastructure/Diagnostics/ErrorReportDialog.cs +++ /dev/null @@ -1,85 +0,0 @@ -using System; -using System.Text; -using System.Windows.Forms; -using Sqlbi.Bravo.Infrastructure.Helpers; -using Sqlbi.Bravo.Infrastructure.Windows.Dialogs; - -namespace Sqlbi.Bravo.Infrastructure.Diagnostics; - -/// -/// Provides a dialog to report an error that occurred in the application. -/// -internal static class ErrorReportDialog -{ - //private const string BugReportUrl = "https://github.com/sql-bi/Bravo/issues/new"; - - public static void Show(ErrorReport report) - { - //var reportButton = new TaskDialogCommandLinkButton("&Report this issue", "Copies the report to the clipboard and opens the Bravo issue tracker in your browser, where you can paste it."); - var copyButton = new TaskDialogCommandLinkButton("&Copy to clipboard", "Copies the crash report to the clipboard.") - { - // Keep the dialog open after clicking the button - AllowCloseDialog = false, - }; - - copyButton.Click += (_, _) => _ = report.TryCopyToClipboard(); - - var clickedButton = TaskDialogBuilder.Create() - .WithCaption(AppEnvironment.ApplicationMainWindowTitle) - .WithIcon(TaskDialogIcon.Error) - .WithStartupLocation(TaskDialogStartupLocation.CenterScreen) - .WithAllowCancel() - .WithSizeToContent() - .WithEnableLinks((href) => _ = ProcessHelper.Open(href)) - .WithHeading("Bravo encountered an unexpected error.") - .WithText(GetText(report)) - .AddButtons(/*reportButton,*/ copyButton, TaskDialogButton.Close) - .WithDefaultButton(copyButton) - .Show(); - - //if (clickedButton == reportButton) - //{ - // _ = report.TryCopyToClipboard(); - // _ = ProcessHelper.OpenBrowser(new Uri(BugReportUrl, UriKind.Absolute)); - //} - } - - private static string GetText(ErrorReport report) - { - var text = new StringBuilder(); - - text.AppendLine(report.FilePath is not null - ? "A crash report has been saved and can be used to diagnose the problem." - : "The crash report could not be saved."); - - if (report.FilePath is not null) - { - text.AppendLine(); - text.AppendLine("Report file:"); - text.Append("").Append(report.FilePath).AppendLine(""); - } - - text.AppendLine(); - text.AppendLine("Exception:"); - text.AppendLine($"{report.Exception.GetType().FullName}: {report.Exception.Message}"); - - text.AppendLine(); - text.AppendLine("Location:"); - text.AppendLine(GetExceptionLocation(report.Exception)); - - return text.ToString(); - - static string GetExceptionLocation(Exception exception) - { - if (exception.TargetSite is { } method) - { - if (method.DeclaringType is { } type) - { - return $"{type.FullName}.{method.Name}"; - } - } - - return exception.Source ?? ""; - } - } -} diff --git a/src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs b/src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs new file mode 100644 index 00000000..fd2ef75b --- /dev/null +++ b/src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs @@ -0,0 +1,199 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using System.Windows.Forms; +using Sqlbi.Bravo.Infrastructure.Helpers; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Sqlbi.Bravo.Infrastructure.Windows.Dialogs; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +internal interface IErrorReportDialog +{ + void Show(ErrorReport report); +} + +/// +/// Implements an error report dialog to display error information to the user. +/// +internal sealed class ErrorReportTaskDialog(ITelemetryService telemetry) : IErrorReportDialog +{ + private readonly ITelemetryService _telemetry = telemetry; + + public void Show(ErrorReport report) + { + // Always run on a dedicated STA thread: TaskDialog and Clipboard require STA, + // while the caller could be executing on any arbitrary background thread. + ProcessHelper.RunOnSTAThread(() => ShowImpl(report)); + } + + private void ShowImpl(ErrorReport report) + { + // Ensure a WindowsFormsSynchronizationContext exists on this STA thread so that async continuations + // post back to this thread's message loop, where the modal dialog can safely process UI updates. + if (SynchronizationContext.Current is not WindowsFormsSynchronizationContext) + SynchronizationContext.SetSynchronizationContext(new WindowsFormsSynchronizationContext()); + + var sendButton = new TaskDialogCommandLinkButton("&Send report", "Sends the crash report to help diagnose the problem.") + { + AllowCloseDialog = false, + // When telemetry is enabled, reports are automatically submitted; hide this redundant button. + Visible = !_telemetry.TelemetryEnabled, + }; + var copyButton = new TaskDialogCommandLinkButton("&Copy to clipboard", "Copies the crash report details to the clipboard.") + { + AllowCloseDialog = false, + }; + + var text = GetText(report); + var builder = TaskDialogBuilder.Create() + .WithCaption(AppEnvironment.ApplicationMainWindowTitle) + .WithIcon(TaskDialogIcon.ShieldErrorRedBar) + .WithCurrentProcessMainWindowOwner() + .WithStartupLocation(TaskDialogStartupLocation.CenterOwner) + .WithAllowCancel() + .WithSizeToContent() + .WithEnableLinks((page, linkHref) => OpenFile(page, linkHref, text)) + .WithHeading("Bravo encountered an unexpected error.") + .WithText(text) + .AddButtons(sendButton, copyButton, TaskDialogButton.Close) + .WithDefaultButton(sendButton.Visible ? sendButton : copyButton); + + var page = builder.Build(); + + copyButton.Click += (_, _) => CopyToClipboard(page, text, report); + sendButton.Click += async (_, _) => await SendAsync(page, sendButton, text, report); + + builder.Show(); + } + + private async Task SendAsync(TaskDialogPage page, TaskDialogButton sendButton, string text, ErrorReport report) + { + var previousTelemetryEnabled = _telemetry.TelemetryEnabled; + try + { + sendButton.Enabled = false; + SetStatus(page, text, "Sending the report..."); + + _telemetry.TelemetryEnabled = true; + _telemetry.TrackException(report.Exception, properties: new Dictionary + { + ["ExceptionKind"] = "Unhandled" + }); + + using var timeout = new CancellationTokenSource(30_000); + var submitted = await _telemetry.FlushAsync(timeout.Token); + + SetStatus(page, text, submitted + ? "The report has been submitted. Thank you." + : "The report could not be submitted. Copy the report to keep its details."); + } + catch (Exception ex) + { + SetStatus(page, text, "The report could not be submitted. Copy the report to keep its details.", ex); + } + finally + { + _telemetry.TelemetryEnabled = previousTelemetryEnabled; + } + } + + private static void CopyToClipboard(TaskDialogPage page, string text, ErrorReport report) + { + try + { + Clipboard.SetDataObject( + data: new DataObject(DataFormats.UnicodeText, report.Text), + copy: true, + retryTimes: 10, + retryDelay: 100); + } + catch (Exception ex) + { + SetStatus(page, text, "The report could not be copied to the clipboard.", ex); + } + } + + private static void OpenFile(TaskDialogPage page, string linkHref, string text) + { + try + { + // The href must be a file path and the file must exist. + if (!File.Exists(linkHref)) + { + SetStatus(page, text, "The report file is no longer available."); + return; + } + + using var _ = Process.Start(new ProcessStartInfo + { + FileName = linkHref, + UseShellExecute = true, + }); + } + catch (Exception ex) + { + SetStatus(page, text, "The report file could not be opened.", ex); + } + } + + private static void SetStatus(TaskDialogPage page, string text, string status, Exception? exception = null) + { + if (page.BoundDialog is null) + return; + + if (exception is not null) + status = $"{status} ({exception.GetType().Name}: {exception.Message})"; + + page.Text = $""" + {text} + ---- + + {status} + """; + } + + private static string GetText(ErrorReport report) + { + var text = new StringBuilder(); + + text.AppendLine(report.FilePath is not null + ? "A crash report has been saved and can be used to diagnose the problem." + : "The crash report could not be saved locally."); + + text.AppendLine(); + text.AppendLine("SessionId:"); + text.AppendLine(TelemetrySessionInfo.SessionId); + + if (report.FilePath is not null) + { + text.AppendLine(); + text.AppendLine("Report file:"); + text.Append("").Append(report.FilePath).AppendLine(""); + } + + text.AppendLine(); + text.AppendLine("Exception:"); + text.AppendLine($"{report.Exception.GetType().Name}: {report.Exception.Message}"); + + //text.AppendLine(); + //text.AppendLine("Location:"); + //text.AppendLine(GetExceptionLocation(report.Exception)); + + return text.ToString(); + + //static string GetExceptionLocation(Exception exception) + //{ + // if (exception.TargetSite is { } method && method.DeclaringType is { } type) + // { + // return $"{type.FullName}.{method.Name}"; + // } + + // return exception.Source ?? ""; + //} + } +} diff --git a/src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs b/src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs new file mode 100644 index 00000000..48cd11c5 --- /dev/null +++ b/src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs @@ -0,0 +1,96 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using System.Windows.Forms; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +/// +/// Provides global exception handling for the application. +/// +internal static class GlobalExceptionHandling +{ + private static GlobalExceptionHandler? s_handler; + private static int s_isHandlingFatalException; + + /// + /// Configures global exception handling for the application. + /// + public static void Configure(GlobalExceptionHandler handler) + { + if (s_handler is not null) + throw new InvalidOperationException("Global exception handling has already been configured."); + + s_handler = handler; + + // mode: ThrowException: Prevents WinForms from swallowing UI exceptions or showing its "Continue/Quit" dialog. + // - Preserves the exact stack trace at the throw site for crash dumps and debugger break. + // - Routes UI exceptions to AppDomain.UnhandledException (consistent with background threads). + // threadScope: false: Sets this mode globally across all threads, ensuring additional STA threads are covered. + Application.SetUnhandledExceptionMode(UnhandledExceptionMode.ThrowException, threadScope: false); + + AppDomain.CurrentDomain.UnhandledException += OnUnhandledException; + TaskScheduler.UnobservedTaskException += OnUnobservedTaskException; + } + + private static void OnUnhandledException(object? sender, UnhandledExceptionEventArgs e) + { + // ExceptionObject may contain a non-CLS exception value. + var exception = e.ExceptionObject as Exception + ?? new InvalidOperationException($"Non-CLS-compliant exception object of type '{e.ExceptionObject?.GetType()}': {e.ExceptionObject}."); + + // Prevent fatal exception handling from being reentered, which could lead to stack overflow or deadlock. + if (Interlocked.CompareExchange(ref s_isHandlingFatalException, value: 1, comparand: 0) != 0) + return; + + try + { + InvokeHandler(exception, ExceptionOrigin.AppDomain); + } + finally + { + Interlocked.Exchange(ref s_isHandlingFatalException, 0); + } + } + + private static void OnUnobservedTaskException(object? sender, UnobservedTaskExceptionEventArgs e) + { + // Prevent the task exception from being raised again if handling fails. + e.SetObserved(); + + InvokeHandler(e.Exception, ExceptionOrigin.UnobservedTask); + } + + private static void InvokeHandler(Exception exception, ExceptionOrigin origin) + { + try + { + s_handler?.Invoke(exception, origin); + } + catch (Exception handlerException) + { + Environment.FailFast($"The unhandled exception handler failed while handling: {exception}", handlerException); + } + } +} + +/// +/// Handles an exception captured by . +/// +internal delegate void GlobalExceptionHandler(Exception exception, ExceptionOrigin origin); + +/// +/// Identifies where an exception was captured and the constraints of handling it. +/// +internal enum ExceptionOrigin +{ + /// + /// An unhandled exception from any thread. The runtime terminates the process after the handler returns. + /// + AppDomain, + + /// + /// An unobserved task exception raised on the finalizer thread. Do not block or show UI. + /// + UnobservedTask, +} diff --git a/src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs b/src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs new file mode 100644 index 00000000..3ef2753a --- /dev/null +++ b/src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs @@ -0,0 +1,75 @@ +using System; +using System.Collections.Generic; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure.Telemetry; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +/// +/// Handles unhandled exceptions in the application, providing a mechanism +/// to report and display error information to the user. +/// +internal sealed class UnhandledExceptionHandler(IErrorReportDialog dialog, ITelemetryService telemetry, ILoggerFactory loggerFactory) +{ + private readonly IErrorReportDialog _dialog = dialog; + private readonly ITelemetryService _telemetry = telemetry; + private readonly ILogger _logger = loggerFactory.CreateLogger(); + + public void Handle(Exception exception, ExceptionOrigin origin) + { + Report(exception, origin); + + // Do not show UI for unobserved task exceptions (finalizer threads are runtime-critical). + // The process keeps running, so the channel sends the telemetry on its own flush interval. + if (origin == ExceptionOrigin.UnobservedTask) + return; + + // Prevent multiple handling of the same exception + if (!TryMarkHandled(exception)) + return; + + var report = ErrorReport.Create(exception); + _ = report.TrySave(); + _dialog.Show(report); + + _ = _telemetry.TryFlushBeforeShutdown(10_000); + } + + private void Report(Exception exception, ExceptionOrigin origin) + { + try + { + _logger.LogError(exception, "An unhandled exception occurred. Origin: {Origin}", origin); + } + catch { /* Ignore failures */ } + + try + { + _telemetry.TrackException(exception, properties: new Dictionary + { + ["ExceptionOrigin"] = origin.ToString(), + ["ExceptionKind"] = "Unhandled" + }); + } + catch { /* Ignore failures */ } + } + + private static bool TryMarkHandled(Exception exception) + { + const string HandledMarker = "Bravo.UnhandledExceptionHandler.Handled"; + + try + { + if (exception.Data.Contains(HandledMarker)) + return false; + + exception.Data[HandledMarker] = true; + return true; + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException) + { + // Data is read-only or rejects the key: report the exception anyway + return true; + } + } +} diff --git a/src/Infrastructure/Helpers/ProcessHelper.cs b/src/Infrastructure/Helpers/ProcessHelper.cs index 7a171728..a3a009dc 100644 --- a/src/Infrastructure/Helpers/ProcessHelper.cs +++ b/src/Infrastructure/Helpers/ProcessHelper.cs @@ -179,13 +179,12 @@ public static bool OpenShellExecute(string path, bool waitForStarted, [NotNullWh { if (File.Exists(path)) { - const string Txt = ".txt"; const string Pbix = ".pbix"; const string Xlsx = ".xlsx"; const string CodeWorkspace = ".code-workspace"; var extension = Path.GetExtension(path); - var isAllowed = (new[] { Txt, Pbix, Xlsx, CodeWorkspace }).Any((ext) => ext.EqualsI(extension)); + var isAllowed = (new[] { Pbix, Xlsx, CodeWorkspace }).Any((ext) => ext.EqualsI(extension)); var isPbix = extension.EqualsI(Pbix); if (isAllowed) diff --git a/src/Infrastructure/Telemetry/ITelemetryServiceFactory.cs b/src/Infrastructure/Telemetry/ITelemetryServiceFactory.cs new file mode 100644 index 00000000..0641d2e2 --- /dev/null +++ b/src/Infrastructure/Telemetry/ITelemetryServiceFactory.cs @@ -0,0 +1,12 @@ +namespace Sqlbi.Bravo.Infrastructure.Telemetry; + +internal interface ITelemetryServiceFactory +{ + ITelemetryService Create(bool enabled); +} + +internal sealed class TelemetryServiceFactory : ITelemetryServiceFactory +{ + public ITelemetryService Create(bool enabled) + => TelemetryService.Create(TelemetrySessionInfo.ConnectionString, enabled, TelemetryService.DefaultStorageFolder); +} diff --git a/src/Infrastructure/Telemetry/NullTelemetryService.cs b/src/Infrastructure/Telemetry/NullTelemetryService.cs new file mode 100644 index 00000000..4ad4f578 --- /dev/null +++ b/src/Infrastructure/Telemetry/NullTelemetryService.cs @@ -0,0 +1,18 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +namespace Sqlbi.Bravo.Infrastructure.Telemetry; + +/// +/// An implementation of that does not perform any telemetry operations. +/// +internal sealed class NullTelemetryService : ITelemetryService +{ + public bool TelemetryEnabled { get => false; set { } } + public void TrackException(Exception exception, IDictionary? properties = null) { } + public Task FlushAsync(CancellationToken cancellationToken = default) => Task.FromResult(false); + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => false; + public void Dispose() { } +} diff --git a/src/Infrastructure/Telemetry/TelemetryService.cs b/src/Infrastructure/Telemetry/TelemetryService.cs index a5c39e91..af2fad8d 100644 --- a/src/Infrastructure/Telemetry/TelemetryService.cs +++ b/src/Infrastructure/Telemetry/TelemetryService.cs @@ -8,8 +8,6 @@ using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.Extensibility; using Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel; -using Sqlbi.Bravo.Infrastructure.Configuration; -using Sqlbi.Bravo.Infrastructure.Policies; namespace Sqlbi.Bravo.Infrastructure.Telemetry; @@ -26,8 +24,6 @@ internal sealed class TelemetryService : ITelemetryService public static readonly string DefaultStorageFolder = Path.Combine(AppEnvironment.ApplicationCachePath, ".telemetry"); - private static readonly Lazy s_instance = new(CreateInstance, isThreadSafe: true); - private readonly TelemetryConfiguration _configuration; private readonly ITelemetryChannel _channel; private readonly TelemetryClient _client; @@ -40,21 +36,6 @@ private TelemetryService(TelemetryConfiguration configuration) _client = new TelemetryClient(configuration); } - /// - /// Gets the process-wide instance, created on first use with the consent resolved from policies - /// and user settings. - /// - public static TelemetryService Instance => s_instance.Value; - - /// - /// Disposes the process-wide instance, if it was created. - /// - public static void DisposeInstance() - { - if (s_instance.IsValueCreated) - s_instance.Value.Dispose(); - } - public bool TelemetryEnabled { get => _configuration.DisableTelemetry == false; @@ -144,12 +125,4 @@ public static TelemetryService Create(string connectionString, bool telemetryEna return new TelemetryService(configuration); } - - private static TelemetryService CreateInstance() - { - var policies = new PolicyServiceFactory().Create().Current; - var consent = TelemetryConsent.Resolve(policies, UserPreferences.Current); - - return Create(TelemetrySessionInfo.ConnectionString, consent.IsEnabled, DefaultStorageFolder); - } } diff --git a/src/Infrastructure/Windows/Dialogs/TaskDialogBuilder.cs b/src/Infrastructure/Windows/Dialogs/TaskDialogBuilder.cs index 7ecb6e1f..699edc0f 100644 --- a/src/Infrastructure/Windows/Dialogs/TaskDialogBuilder.cs +++ b/src/Infrastructure/Windows/Dialogs/TaskDialogBuilder.cs @@ -101,6 +101,16 @@ public TaskDialogBuilder WithEnableLinks(Action linkClicked) return this; } + /// + /// Enables clickable links in the dialog text and footnote, and invokes the specified callback, with the page the link belongs to, when a link is clicked. + /// + public TaskDialogBuilder WithEnableLinks(Action linkClicked) + { + _page.EnableLinks = true; + _page.LinkClicked += (_, e) => linkClicked(_page, e.LinkHref); + return this; + } + /// /// Sets the dialog to automatically size to its content. If false, the dialog will have a fixed size. /// @@ -168,12 +178,7 @@ public TaskDialogPage Build() /// public TaskDialogButton Show() { - if (_ownerHandle != IntPtr.Zero) - { - return TaskDialog.ShowDialog(_ownerHandle, _page, _startupLocation); - } - - return TaskDialog.ShowDialog(_page, _startupLocation); + return TaskDialog.ShowDialog(_ownerHandle, _page, _startupLocation); } private static TaskDialogIcon? LoadCurrentProcessIcon() diff --git a/src/Program.cs b/src/Program.cs index 6562c5d0..9f6eb41b 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -1,7 +1,5 @@ using System; using Sqlbi.Bravo.Host; -using Sqlbi.Bravo.Infrastructure.Diagnostics; -using Sqlbi.Bravo.Infrastructure.Telemetry; namespace Sqlbi.Bravo; @@ -11,38 +9,17 @@ internal static class Program private static void Main() { ApplicationConfiguration.Initialize(); - try - { - using var context = BravoApplicationInitializer.Initialize(); - if (!context.Instance.IsPrimary) - { - context.Instance.RedirectActivationToPrimary(); - return; - } + BravoApplicationConfiguration.Initialize(); - using var application = BravoApplication - .CreateBuilder(context) - .Build(); + using var context = BootstrapContextFactory.CreateDefault(); - application.Run(); - } - catch (Exception ex) - { - TelemetryService.Instance.TrackException(ex); + BravoGlobalExceptionHandling.Configure(context); - var report = ErrorReport.Create(ex); - report.TrySave(); - ErrorReportDialog.Show(report); + using var app = BravoApplication + .CreateBuilder(context) + .Build(); - // An unhandled rethrow can end the process before the finally block below runs. - _ = TelemetryService.Instance.TryFlushBeforeShutdown(10_000); - - throw; - } - finally - { - TelemetryService.DisposeInstance(); - } + app.Run(); } } diff --git a/test/Bravo.Tests/Host/BootstrapContextTests.cs b/test/Bravo.Tests/Host/BootstrapContextTests.cs new file mode 100644 index 00000000..7d84d6fa --- /dev/null +++ b/test/Bravo.Tests/Host/BootstrapContextTests.cs @@ -0,0 +1,177 @@ +using System; +using System.Collections.Generic; +using System.IO; +using Bravo.Tests.Infrastructure.Policies; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Host; +using Sqlbi.Bravo.Infrastructure.Configuration; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Host; + +public class BootstrapContextTests +{ + [Fact] + public void Dispose_DisposesTheOwnedServicesOnce() + { + var loggerFactory = new FakeLoggerFactory(); + var telemetry = new FakeTelemetry(); + var context = new BootstrapContext(loggerFactory, new PolicyService(new FakePolicyReader()), new UserSettings(), telemetry); + + context.Dispose(); + context.Dispose(); + + Assert.Equal(1, telemetry.DisposeCount); + Assert.Equal(1, loggerFactory.DisposeCount); + } + + [Theory] + [InlineData(null, true, true)] + [InlineData(null, false, false)] + [InlineData(false, true, false)] + [InlineData(true, false, true)] + public void Create_LoadsPoliciesOnceBeforeCreatingTelemetry(bool? policy, bool preference, bool expected) + { + var loggerFactory = new FakeLoggerFactory(); + var snapshot = PolicySnapshot.NotConfigured with { TelemetryEnabled = policy }; + var reader = new FakePolicyReader { Snapshot = snapshot }; + var policyService = new PolicyService(reader); + var settings = new UserSettings { TelemetryEnabled = preference }; + var policyFactory = new FakePolicyFactory(() => policyService); + var settingsFactory = new FakeSettingsFactory(() => settings); + var telemetryFactory = new FakeTelemetryFactory(); + var factory = new BootstrapContextFactory(loggerFactory, policyFactory, settingsFactory, telemetryFactory); + + Assert.Equal(0, reader.ReadCount); + Assert.Equal(0, policyFactory.CreateCount); + Assert.Equal(0, settingsFactory.CreateCount); + Assert.Empty(telemetryFactory.EnabledValues); + + using var context = factory.Create(); + reader.Snapshot = PolicySnapshot.NotConfigured with { TelemetryEnabled = !expected }; + + Assert.Equal([expected], telemetryFactory.EnabledValues); + Assert.Equal(expected, context.Telemetry.TelemetryEnabled); + Assert.Same(loggerFactory, context.LoggerFactory); + Assert.Same(policyService, context.PolicyService); + Assert.Same(snapshot, context.PolicyService.Current); + Assert.Same(settings, context.Settings); + Assert.Equal(1, reader.ReadCount); + Assert.Equal(1, policyFactory.CreateCount); + Assert.Equal(1, settingsFactory.CreateCount); + } + + [Fact] + public void Create_ReadFailurePropagatesBeforeTelemetryIsCreated() + { + var failure = new IOException("Registry unavailable"); + var reader = new FakePolicyReader { OnRead = () => throw failure }; + using var loggerFactory = new FakeLoggerFactory(); + var policyFactory = new FakePolicyFactory(() => new PolicyService(reader)); + var settingsFactory = new FakeSettingsFactory(() => new UserSettings()); + var telemetryFactory = new FakeTelemetryFactory(); + var factory = new BootstrapContextFactory(loggerFactory, policyFactory, settingsFactory, telemetryFactory); + + Assert.Same(failure, Assert.Throws(() => factory.Create())); + Assert.Equal(0, settingsFactory.CreateCount); + Assert.Empty(telemetryFactory.EnabledValues); + Assert.Equal(0, loggerFactory.DisposeCount); + } + + [Fact] + public void Create_PolicyCreationFailurePropagatesBeforeCreatingOtherServices() + { + var failure = new InvalidOperationException("Policy service unavailable"); + using var loggerFactory = new FakeLoggerFactory(); + var policyFactory = new FakePolicyFactory(() => throw failure); + var settingsFactory = new FakeSettingsFactory(() => new UserSettings()); + var telemetryFactory = new FakeTelemetryFactory(); + var factory = new BootstrapContextFactory(loggerFactory, policyFactory, settingsFactory, telemetryFactory); + + Assert.Same(failure, Assert.Throws(() => factory.Create())); + Assert.Equal(0, settingsFactory.CreateCount); + Assert.Empty(telemetryFactory.EnabledValues); + Assert.Equal(0, loggerFactory.DisposeCount); + } + + [Fact] + public void Create_SettingsCreationFailureUsesDefaultsCreatesTelemetryAndLogsError() + { + var failure = new IOException("Settings unavailable"); + using var loggerFactory = new FakeLoggerFactory(); + var policyFactory = new FakePolicyFactory(() => new PolicyService(new FakePolicyReader())); + var settingsFactory = new FakeSettingsFactory(() => throw failure); + var telemetryFactory = new FakeTelemetryFactory(); + var factory = new BootstrapContextFactory(loggerFactory, policyFactory, settingsFactory, telemetryFactory); + + using var context = factory.Create(); + + Assert.IsType(context.Settings); + Assert.Equal(UserSettings.DefaultTelemetryEnabled, context.Settings.TelemetryEnabled); + Assert.Equal([UserSettings.DefaultTelemetryEnabled], telemetryFactory.EnabledValues); + Assert.Equal(UserSettings.DefaultTelemetryEnabled, context.Telemetry.TelemetryEnabled); + var entry = Assert.Single(loggerFactory.Entries); + Assert.Equal(LogLevel.Error, entry.Level); + Assert.Equal(2, entry.EventId.Id); + Assert.Same(failure, entry.Exception); + } + + [Fact] + public void Create_TelemetryFailureUsesDisabledServiceAndLogsError() + { + var failure = new InvalidOperationException("Telemetry unavailable"); + var loggerFactory = new FakeLoggerFactory(); + var policyFactory = new FakePolicyFactory(() => new PolicyService(new FakePolicyReader())); + var settingsFactory = new FakeSettingsFactory(() => new UserSettings()); + var telemetryFactory = new FakeTelemetryFactory { Failure = failure }; + var factory = new BootstrapContextFactory(loggerFactory, policyFactory, settingsFactory, telemetryFactory); + + using var context = factory.Create(); + + Assert.IsType(context.Telemetry); + Assert.False(context.Telemetry.TelemetryEnabled); + var entry = Assert.Single(loggerFactory.Entries); + Assert.Equal(LogLevel.Error, entry.Level); + Assert.Same(failure, entry.Exception); + } + + private sealed class FakePolicyFactory(Func create) : IPolicyServiceFactory + { + public int CreateCount { get; private set; } + + public IPolicyService Create() + { + CreateCount++; + return create(); + } + } + + private sealed class FakeSettingsFactory(Func create) : IUserSettingsServiceFactory + { + public int CreateCount { get; private set; } + + public IUserSettings Create() + { + CreateCount++; + return create(); + } + } + + private sealed class FakeTelemetryFactory : ITelemetryServiceFactory + { + public List EnabledValues { get; } = []; + public Exception? Failure { get; init; } + + public ITelemetryService Create(bool enabled) + { + EnabledValues.Add(enabled); + if (Failure is not null) + throw Failure; + + return new FakeTelemetry { TelemetryEnabled = enabled }; + } + } +} diff --git a/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs b/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs index da2845d4..9527fb31 100644 --- a/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs +++ b/test/Bravo.Tests/Host/BravoApplicationBuilderTests.cs @@ -1,20 +1,63 @@ -using Sqlbi.Bravo.Host; +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.Telemetry; using Xunit; namespace Bravo.Tests.Host; public class BravoApplicationBuilderTests { - /// - /// Composition smoke test: builds the full service graph through the real builder. In DEBUG the - /// builder enables ValidateOnBuild/ValidateScopes, so this fails on any unresolvable - /// registration. Building does not bind Kestrel or start the host, so no port is claimed. - /// [Fact] - public void Build_ComposesTheFullServiceGraph() + public void Build_ResolvesTheBootstrapInstancesWithoutOwningTheirLifetime() { - using var context = FakeInitializationContext.Create(); + 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)); - using var application = BravoApplication.CreateBuilder(context).Build(); + 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); + } + + // 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((_) => + { + instanceCreations++; + return new FakeInstance(); + })); + + using (var application = builder.Build()) + { + Assert.Equal(0, instanceCreations); + } + + Assert.Equal(0, instanceCreations); } -} +} \ No newline at end of file diff --git a/test/Bravo.Tests/Host/BravoApplicationInitializationContextTests.cs b/test/Bravo.Tests/Host/BravoApplicationInitializationContextTests.cs deleted file mode 100644 index 713b0b93..00000000 --- a/test/Bravo.Tests/Host/BravoApplicationInitializationContextTests.cs +++ /dev/null @@ -1,51 +0,0 @@ -using Sqlbi.Bravo.Host; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; -using Sqlbi.Bravo.Infrastructure.Policies; -using Xunit; - -namespace Bravo.Tests.Host; - -/// -/// Verifies that carries the initialization -/// entries as-is and owns only what the initialization created. -/// -public class BravoApplicationInitializationContextTests -{ - [Fact] - public void Entries_ReturnTheSameObjectsTheInitializationProduced() - { - var instance = new FakeInstance { IsPrimary = true }; - //var policies = Policies.None; - //var userSettings = new UserSettings(); - //var telemetry = new FakeTelemetry(); - //var webProxy = new FakeWebProxy(); - - using var context = new BravoApplicationInitializationContext(instance); - - Assert.Same(instance, context.Instance); - } - - [Fact] - public void Dispose_DisposesTheInstance() - { - var instance = new FakeInstance(); - var context = FakeInitializationContext.Create(instance); - - context.Dispose(); - - Assert.True(instance.Disposed); - } - - //[Fact] - //public void Dispose_DoesNotDisposeEntriesTheInitializationDidNotCreate() - //{ - // // Telemetry is still a process-wide static that owns itself; the context must not dispose it - // // until the initialization creates it. - // var telemetry = new FakeTelemetry(); - // var context = FakeInitializationContext.Create(telemetry: telemetry); - - // context.Dispose(); - - // Assert.False(telemetry.Disposed); - //} -} diff --git a/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs b/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs new file mode 100644 index 00000000..5bf96259 --- /dev/null +++ b/test/Bravo.Tests/Host/BravoApplicationInstanceTests.cs @@ -0,0 +1,109 @@ +using System; +using System.IO; +using System.IO.Pipes; +using System.Text; +using System.Text.Json; +using System.Threading; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Host; +using Sqlbi.Bravo.Infrastructure.SingleInstance; +using Xunit; + +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. +/// +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 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)) + { + silentClient.Connect((int)s_timeout.TotalMilliseconds); + } + + 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. + var exception = Assert.Single(telemetry.Exceptions); + Assert.True(exception is InvalidDataException or IOException, exception.ToString()); + var entry = Assert.Single(loggerFactory.Entries); + Assert.Equal(LogLevel.Warning, entry.Level); + Assert.Equal(1, entry.EventId.Id); + Assert.Same(exception, entry.Exception); + } + + [Fact] + public void Activated_UnreadablePayload_ActivatesWithoutADocumentAndReports() + { + var options = CreateOptions(); + 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); + + using var activated = new ManualResetEventSlim(); + InstanceActivationRequestedEventArgs? activation = null; + instance.ActivationRequested += (_, 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); + + var exception = Assert.IsAssignableFrom(Assert.Single(telemetry.Exceptions)); + var entry = Assert.Single(loggerFactory.Entries); + Assert.Equal(LogLevel.Warning, entry.Level); + Assert.Equal(3, entry.EventId.Id); + Assert.Same(exception, entry.Exception); + } +} diff --git a/test/Bravo.Tests/Host/HostFakes.cs b/test/Bravo.Tests/Host/HostFakes.cs index d2f09e33..9f1bedbd 100644 --- a/test/Bravo.Tests/Host/HostFakes.cs +++ b/test/Bravo.Tests/Host/HostFakes.cs @@ -1,26 +1,20 @@ using System; using System.Collections.Generic; -using System.Net; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Sqlbi.Bravo.Host; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; -using Sqlbi.Bravo.Infrastructure.Policies; using Sqlbi.Bravo.Infrastructure.Telemetry; namespace Bravo.Tests.Host; -/// -/// Fakes for the initialization entries. A fake rather than a real -/// is required: the real type always claims the production -/// named-pipe name, so constructing one in a test would risk colliding with an actually-running -/// Bravo instance. -/// +// Avoid claiming the production named pipe in tests. internal sealed class FakeInstance : IBravoApplicationInstance { public bool IsPrimary { get; init; } - - public bool Disposed { get; private set; } + public int DisposeCount { get; private set; } + public Action? OnDispose { get; init; } public event EventHandler? ActivationRequested { @@ -28,47 +22,75 @@ public event EventHandler? ActivationReque remove { } } - public void RedirectActivationToPrimary() + public void RedirectActivationToPrimary() { } + + public void Dispose() { + DisposeCount++; + OnDispose?.Invoke(); } - - public void Dispose() => Disposed = true; } internal sealed class FakeTelemetry : ITelemetryService { - public bool Disposed { get; private set; } + private readonly List _exceptions = []; + public int DisposeCount { get; private set; } + public Action? OnDispose { get; init; } public bool TelemetryEnabled { get; set; } + public IReadOnlyList Exceptions + { + get { lock (_exceptions) return [.. _exceptions]; } + } + public void TrackException(Exception exception, IDictionary? properties = null) { + lock (_exceptions) _exceptions.Add(exception); } - public Task FlushAsync(CancellationToken cancellationToken = default) => Task.FromResult(true); + public Task FlushAsync(CancellationToken cancellationToken = default) => Task.FromResult(false); - public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => true; + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => false; - public void Dispose() => Disposed = true; + public void Dispose() + { + DisposeCount++; + OnDispose?.Invoke(); + } } -internal sealed class FakeWebProxy : IWebProxy +internal sealed record FakeLogEntry(LogLevel Level, EventId EventId, Exception? Exception, string Message); + +internal sealed class FakeLoggerFactory : ILoggerFactory { - public ICredentials? Credentials { get; set; } + private readonly List _entries = []; - public Uri? GetProxy(Uri destination) => null; + public int DisposeCount { get; private set; } - public bool IsBypassed(Uri host) => true; -} + public IReadOnlyList Entries + { + get { lock (_entries) return [.. _entries]; } + } -internal static class FakeInitializationContext -{ - /// - /// Creates a whose entries are all fakes. - /// - public static BravoApplicationInitializationContext Create(FakeInstance? instance = null) + public void AddProvider(ILoggerProvider provider) { } + + public ILogger CreateLogger(string categoryName) => new FakeLogger(this); + + public void Dispose() => DisposeCount++; + + private sealed class FakeLogger(FakeLoggerFactory owner) : ILogger { - return new BravoApplicationInitializationContext( - instance ?? new FakeInstance()); + private readonly FakeLoggerFactory _owner = owner; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => logLevel != LogLevel.None; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + var entry = new FakeLogEntry(logLevel, eventId, exception, formatter(state, exception)); + lock (_owner._entries) _owner._entries.Add(entry); + } } } diff --git a/test/Bravo.Tests/Infrastructure/Diagnostics/GlobalExceptionHandlingTests.cs b/test/Bravo.Tests/Infrastructure/Diagnostics/GlobalExceptionHandlingTests.cs new file mode 100644 index 00000000..b4e8806a --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Diagnostics/GlobalExceptionHandlingTests.cs @@ -0,0 +1,139 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Sqlbi.Bravo.Infrastructure.Diagnostics; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Diagnostics; + +[CollectionDefinition(nameof(GlobalExceptionHandlingCollection), DisableParallelization = true)] +public sealed class GlobalExceptionHandlingCollection +{ +} + +[Collection(nameof(GlobalExceptionHandlingCollection))] +public class GlobalExceptionHandlingTests : IDisposable +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(10); + private static readonly FieldInfo s_handlerField = typeof(GlobalExceptionHandling) + .GetField("s_handler", BindingFlags.NonPublic | BindingFlags.Static)!; + + private readonly object? _previousHandler = s_handlerField.GetValue(null); + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RuntimeCallbacks_FatalAndUnobservedReportingOverlap_DeliversBoth(bool unobservedFirst) + { + using var releaseFirst = new ManualResetEventSlim(); + var firstEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var reported = new ConcurrentQueue(); + var fatal = new InvalidOperationException("fatal"); + var unobserved = new UnobservedTaskExceptionEventArgs(new AggregateException("unobserved")); + var firstOrigin = unobservedFirst ? ExceptionOrigin.UnobservedTask : ExceptionOrigin.AppDomain; + + SetHandler((exception, origin) => + { + reported.Enqueue(exception); + if (origin == firstOrigin) + { + firstEntered.TrySetResult(); + releaseFirst.Wait(s_timeout); + } + }); + + var first = Task.Run(() => Raise(unobservedFirst)); + var second = Task.CompletedTask; + try + { + await firstEntered.Task.WaitAsync(s_timeout); + + second = Task.Run(() => Raise(!unobservedFirst)); + await second.WaitAsync(s_timeout); + + Assert.False(first.IsCompleted, "The second callback must complete while the first report is still blocked."); + Assert.Contains(fatal, reported); + Assert.Contains(unobserved.Exception, reported); + Assert.Equal(2, reported.Count); + Assert.True(unobserved.Observed); + } + finally + { + releaseFirst.Set(); + await Task.WhenAll(first, second).WaitAsync(s_timeout); + } + + void Raise(bool isUnobserved) + { + if (isUnobserved) + RaiseUnobserved(unobserved); + else + RaiseFatal(fatal); + } + } + + [Fact] + public void RuntimeCallbacks_ReentrantFatalException_ReportsOnlyTheFirst() + { + var reported = new List(); + var first = new InvalidOperationException("first"); + var nested = new InvalidOperationException("nested"); + SetHandler((exception, _) => + { + reported.Add(exception); + if (ReferenceEquals(exception, first)) + RaiseFatal(nested); + }); + + RaiseFatal(first); + + Assert.Same(first, Assert.Single(reported)); + } + + [Fact] + public void RuntimeCallbacks_UnobservedTask_ReportsWithoutDialogOrFlush() + { + var dialog = new RecordingDialog(); + var telemetry = Substitute.For(); + var handler = new UnhandledExceptionHandler(dialog, telemetry, NullLoggerFactory.Instance); + var args = new UnobservedTaskExceptionEventArgs(new AggregateException("unobserved")); + SetHandler(handler.Handle); + + RaiseUnobserved(args); + + Assert.True(args.Observed); + Assert.Equal(0, dialog.ShowCount); + telemetry.Received(1).TrackException(args.Exception, Arg.Is>( + properties => properties["ExceptionOrigin"] == nameof(ExceptionOrigin.UnobservedTask))); + telemetry.DidNotReceive().TryFlushBeforeShutdown(Arg.Any()); + telemetry.DidNotReceive().FlushAsync(Arg.Any()); + } + + public void Dispose() => s_handlerField.SetValue(null, _previousHandler); + + private static void SetHandler(GlobalExceptionHandler handler) => s_handlerField.SetValue(null, handler); + + // Calling the callbacks directly avoids installing fatal exception hooks in the test process. + private static void RaiseFatal(Exception exception) + => typeof(GlobalExceptionHandling) + .GetMethod("OnUnhandledException", BindingFlags.NonPublic | BindingFlags.Static)! + .Invoke(null, [null, new UnhandledExceptionEventArgs(exception, isTerminating: true)]); + + private static void RaiseUnobserved(UnobservedTaskExceptionEventArgs args) + => typeof(GlobalExceptionHandling) + .GetMethod("OnUnobservedTaskException", BindingFlags.NonPublic | BindingFlags.Static)! + .Invoke(null, [null, args]); + + private sealed class RecordingDialog : IErrorReportDialog + { + public int ShowCount { get; private set; } + + public void Show(ErrorReport report) => ShowCount++; + } +} From 1f60b081b6ab101e49598c550eb5d2ff30cafc35 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Tue, 20 Oct 2026 09:07:39 +0200 Subject: [PATCH 2/3] docs: tighten the architecture and diagnostics documents Remove the application overview and the startup walkthrough, which repeat AGENTS.md and Program. Replace the logging-sink and flush-timeout tables, which copy code, with the behavior they cause, and drop generic advice from the handled-exception rules. State the uncovered startup failures, including a policy read failure, and the ProblemDetails mapping order constraint. Align the consent wording with the current behavior and wrap at 120 columns. --- docs/design/architecture.md | 67 ++++++++++++++------ docs/design/diagnostics.md | 121 +++++++++++++++++++++--------------- 2 files changed, 120 insertions(+), 68 deletions(-) diff --git a/docs/design/architecture.md b/docs/design/architecture.md index 512c35c6..3b9c0bd4 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -1,10 +1,10 @@ # Architecture and runtime boundaries -Bravo is a Windows desktop application. Its .NET process owns a WinForms window and an ASP.NET Core loopback API. The window embeds the TypeScript UI in WebView2; UI requests reach application services through the API. This document covers the boundaries that constrain changes, not individual feature implementations. - ## Composition and resource ownership -[Program](../../src/Program.cs) configures the process, creates bootstrap services, installs global exception handling, then builds and runs the host. [BravoApplication](../../src/Host/BravoApplication.cs) resolves the single-instance role before starting Kestrel: a secondary process redirects activation and exits without starting its own server or window. +[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. | Resource | Lifetime owner | Constraint for changes | | --- | --- | --- | @@ -13,21 +13,35 @@ Bravo is a Windows desktop application. Its .NET process owns a WinForms window | 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 bootstrap services as existing instances. Replacing those registrations with container-owned factories changes disposal responsibility and must preserve the ownership above. Global exception handling uses bootstrap services independently of host construction; its exact coverage is in [diagnostics](diagnostics.md#startup-coverage-and-fatal-reporting). +[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). ## UI and server threads -[AppWindow](../../src/Infrastructure/AppWindow.cs) is created on the STA entry thread, which runs the WinForms message loop. API work and single-instance activation callbacks do not have that UI-thread guarantee. +[AppWindow](../../src/Infrastructure/AppWindow.cs) is created on the STA entry thread, which runs the WinForms +message loop. API work and single-instance activation callbacks do not have that UI-thread guarantee. -- Marshal operations on the window or WebView2 to the UI thread. Activation handlers use `ProcessHelper.InvokeOnUIThread` with the window as the control. -- Setting `SynchronizationContext.Current` is not thread dispatch. [ProcessHelper.RunWithUISynchronizationContext](../../src/Infrastructure/Helpers/ProcessHelper.cs) changes the ambient context only during a synchronous callback; do not treat it as permission to access controls from a server thread. -- Keep window activation and document delivery distinct: the window can exist before WebView2 is ready to receive a startup message. +- Marshal operations on the window or WebView2 to the UI thread. Activation handlers use + `ProcessHelper.InvokeOnUIThread` with the window as the control. +- Setting `SynchronizationContext.Current` is not thread dispatch. + [ProcessHelper.RunWithUISynchronizationContext](../../src/Infrastructure/Helpers/ProcessHelper.cs) changes the + ambient context only during a synchronous callback; do not treat it as permission to access controls from a + server thread. +- Keep window activation and document delivery distinct: the window can exist before WebView2 is ready to receive + a startup message. -Violating these boundaries can cause cross-thread UI failures or lost activation requests. The [single-instance contract](#single-instance-contract) describes the current startup limitation. +Violating these boundaries can cause cross-thread UI failures or lost activation requests. The +[single-instance contract](#single-instance-contract) describes the current startup limitation. ## Frontend and API contract -[AppWindow](../../src/Infrastructure/AppWindow.cs) injects `CONFIG` with the API address, token, options, policies and environment information. The frontend uses [Host](../../src/Scripts/controllers/host.ts) for API calls and WebView messages for desktop integration. The C# serializer and TypeScript consumers must agree on field names, nullability and enum values; changing only one side can compile in C# while breaking the UI. Follow the full-build requirement in [AGENTS.md](../../AGENTS.md#build-and-test) for changes to this contract. +[AppWindow](../../src/Infrastructure/AppWindow.cs) injects `CONFIG` with the API address, token, options, policies +and environment information. The frontend uses [Host](../../src/Scripts/controllers/host.ts) for API calls and +WebView messages for desktop integration. The C# serializer and TypeScript consumers must agree on field names, +nullability and enum values; changing only one side can compile in C# while breaking the UI. Follow the full-build +requirement in [AGENTS.md](../../AGENTS.md#build-and-test) for changes to this contract. [BravoApplicationBuilder](../../src/Host/BravoApplicationBuilder.cs) configures Kestrel and endpoint authorization: @@ -36,24 +50,41 @@ Violating these boundaries can cause cross-thread UI failures or lost activation | Debug | IPv4 loopback, port 5000 | No blanket authorization requirement; Swagger enabled | | Release | IPv4 loopback, dynamically assigned port | Controllers require authorization | -[AppAuthenticationHandler](../../src/Infrastructure/Authentication/AppAuthenticationHandler.cs) requires both a loopback remote address and the application token in the `Authorization` header. The template-development token is accepted only under the template-development controller path. Loopback binding and token validation are separate checks; neither should be removed on the assumption that the other is sufficient. +[AppAuthenticationHandler](../../src/Infrastructure/Authentication/AppAuthenticationHandler.cs) requires both a +loopback remote address and the application token in the `Authorization` header. The template-development token is +accepted only under the template-development controller path. Loopback binding and token validation are separate +checks; neither should be removed on the assumption that the other is sufficient. -Keep ProblemDetails mapping order and error contracts consistent with the frontend's interpretation of failures. Reporting responsibility at the API boundary is defined in [diagnostics](diagnostics.md#api-boundary). +[HostingExtensions.AddAndConfigureProblemDetails](../../src/Infrastructure/Extensions/HostingExtensions.cs) maps +exceptions to responses in order, first match wins. The `SocketException` mapping also matches other exception types +that wrap a socket failure, such as `AMO.ConnectionException`, so it stays after the specific mappings and before the +catch-all. Reporting at the API boundary is in [diagnostics](diagnostics.md#api-boundary). ## Single-instance contract -[InstancePipeName](../../src/Host/InstancePipeName.cs) identifies one instance per Windows session and user SID, independent of installation mode and elevation. Keep its application identifier, scope identifier and naming format stable across releases: changing them allows versions that share user data to run as independent owners. +[InstancePipeName](../../src/Host/InstancePipeName.cs) identifies one instance per Windows session and user SID, +independent of installation mode and elevation. Keep its application identifier, scope identifier and naming format +stable across releases: changing them allows versions that share user data to run as independent owners. -[SingleInstanceServer](../../src/Infrastructure/SingleInstance/SingleInstanceServer.cs) arbitrates ownership 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. +[SingleInstanceServer](../../src/Infrastructure/SingleInstance/SingleInstanceServer.cs) arbitrates ownership +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): -- A delivered payload does not guarantee that the requested document opened. Requests arriving before the window or WebView subscriptions are ready are not buffered. +- A delivered payload does not guarantee that the requested document opened. Requests arriving before the window + or WebView subscriptions are ready are not buffered. - 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. A + redirect failure is logged and tracked without a user-facing dialog. ## Configuration boundaries -[PolicyService](../../src/Infrastructure/Policies/PolicyService.cs) exposes a snapshot initialized once. [RegistryPolicyReader](../../src/Infrastructure/Policies/RegistryPolicyReader.cs) merges machine and user policies, with configured machine values taking precedence. A null policy value means not configured; `false` is an explicit policy, not a request to use a default. +[PolicyService](../../src/Infrastructure/Policies/PolicyService.cs) exposes a snapshot initialized once. +[RegistryPolicyReader](../../src/Infrastructure/Policies/RegistryPolicyReader.cs) merges machine and user policies, +with configured machine values taking precedence. A null policy value means not configured; `false` is an explicit +policy, not a request to use a default. -User preferences remain mutable through [UserPreferences](../../src/Infrastructure/Configuration/UserPreferences.cs). Do not assume that saving preferences reloads policies or that every consumer reapplies policy precedence. Host telemetry has different startup and update paths, documented in [telemetry consent](diagnostics.md#telemetry-consent). +User preferences remain mutable through [UserPreferences](../../src/Infrastructure/Configuration/UserPreferences.cs), +and saving them does not reload policies. A consumer that combines a policy with a preference applies the precedence +itself, as [TelemetryConsent](../../src/Infrastructure/Telemetry/TelemetryConsent.cs) does for telemetry. diff --git a/docs/design/diagnostics.md b/docs/design/diagnostics.md index dea92b82..09ce3e19 100644 --- a/docs/design/diagnostics.md +++ b/docs/design/diagnostics.md @@ -1,6 +1,7 @@ # Diagnostics -Exception reporting in the .NET host. The frontend has a separate [telemetry client](../../src/Scripts/controllers/telemetry.ts); host flush and crash handling do not control that client. +Exception reporting in the .NET host. The frontend has a separate +[telemetry client](../../src/Scripts/controllers/telemetry.ts); host flush and crash handling do not control it. ## Reporting paths @@ -8,57 +9,70 @@ Exception reporting in the .NET host. The frontend has a separate [telemetry cli | --- | --- | --- | | `AppDomain.UnhandledException`, including WinForms exceptions routed through `ThrowException` | Runtime terminates after the handler returns | Error log; telemetry subject to consent; saved report and crash dialog | | `TaskScheduler.UnobservedTaskException` | Continues | Error log and telemetry subject to consent; no dialog or shutdown flush | -| Exception caught by an operation that can recover or return an error | Continues | Reporting belongs to the operation or its API boundary; apply the [handled-exception rules](#handled-exception-rules) | +| Exception caught by an operation that can recover or return an error | Continues | Reporting belongs to the operation or its API boundary; see [handled exceptions](#handled-exceptions) | -The two runtime callbacks use [GlobalExceptionHandling](../../src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs). An unobserved task exception is not a fatal crash, even though its telemetry has `ExceptionKind = Unhandled`. +The two runtime callbacks use +[GlobalExceptionHandling](../../src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs). An unobserved task +exception is not a fatal crash, even though its telemetry has `ExceptionKind = Unhandled`. ## Startup coverage and fatal reporting -[Program](../../src/Program.cs) installs global handling after application configuration and bootstrap creation, before building the host. Exceptions during directory setup, proxy configuration or bootstrap creation occur before these hooks exist. Host construction is covered; earlier initialization is not. - -[UnhandledExceptionHandler](../../src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs) logs and tracks each callback before attempting a fatal report. Logging and tracking failures are isolated so one does not prevent the other. For a fatal exception, a marker in `Exception.Data` suppresses a repeated dialog and report; it does not suppress the earlier log and telemetry calls. - -[ErrorReport](../../src/Infrastructure/Diagnostics/ErrorReport.cs) saves environment diagnostics and the exception to `ErrorReport.txt` under the application data folder. A failed save does not prevent the dialog. [ErrorReportTaskDialog](../../src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs) runs on a dedicated STA thread and offers copying, closing and, when host telemetry is disabled, sending the exception through telemetry. Automatic tracking is an attempt, not confirmation of delivery. - -The fatal-handler guard suppresses another fatal arrival while a fatal handler runs. It does not coordinate simultaneous crashes or guarantee that the first dialog remains open until the user responds. Unobserved task reporting does not acquire that guard, so it cannot suppress a fatal report. - -If the global handler throws, `GlobalExceptionHandling` calls `Environment.FailFast`. Reporting failures must therefore be contained where recovery is possible. Never show UI or wait for a shutdown flush on the unobserved-task callback: it runs on the finalizer thread. - -## Handled-exception rules - -For an operation with initialized logging and telemetry services, report a caught failure through both `ILogger` and `ITelemetryService.TrackException`. Local logging and remote telemetry serve different purposes; consent can disable the remote record. Use the reporting type's logger. Add telemetry properties only when needed by a specific diagnostic query. - -These rules have explicit boundaries: - -| Condition | Reporting responsibility | Reason | -| --- | --- | --- | -| A failure propagates to an API or global handler | Leave reporting to that boundary unless this layer handles a separate failure | Repeated reporting records the same failure more than once | -| Settings or telemetry creation fails during bootstrap | Use the available bootstrap logger; do not depend on telemetry creation succeeding | Reporting must not require the service whose creation failed | -| Logging, tracking or another recovery step itself fails | Use an independent fallback when available; do not recursively call the failing sink | Recursion can replace the original failure or end recovery | -| An exception represents an explicitly supported outcome, such as cancellation or an unavailable optional capability | Follow that operation's contract; document an intentional ignored exception at the catch site | Expected outcomes are not automatically application failures | - -These are requirements for changes, not a claim that every existing catch site already uses both services. [BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs), for example, logs settings and telemetry creation failures and continues with defaults or disabled telemetry; policy initialization failures propagate. +[Program](../../src/Program.cs) installs global handling after `BravoApplicationConfiguration.Initialize` and the +bootstrap, before building the host. Host construction and everything after it are covered. Directory and proxy +setup, the WebView2 runtime check and bootstrap creation are not: an exception there ends the process with only the +runtime's own Event Log entry, without a report or a dialog. +[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 this way. + +[UnhandledExceptionHandler](../../src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs) logs and tracks each +callback before attempting a fatal report. Logging and tracking failures are isolated so one does not prevent the +other. For a fatal exception, a marker in `Exception.Data` suppresses a repeated dialog and report; it does not +suppress the earlier log and telemetry calls. + +[ErrorReport](../../src/Infrastructure/Diagnostics/ErrorReport.cs) saves environment diagnostics and the exception to +`ErrorReport.txt` under the application data folder. A failed save does not prevent the dialog. +[ErrorReportTaskDialog](../../src/Infrastructure/Diagnostics/ErrorReportTaskDialog.cs) runs on a dedicated STA thread +and offers copying, closing and, when host telemetry is disabled, sending the exception through telemetry. Automatic +tracking is an attempt, not confirmation of delivery. + +The fatal-handler guard suppresses another fatal arrival while a fatal handler runs. It does not coordinate +simultaneous crashes or guarantee that the first dialog remains open until the user responds. Unobserved task +reporting does not acquire that guard, so it cannot suppress a fatal report. + +If the global handler throws, `GlobalExceptionHandling` calls `Environment.FailFast`. Reporting failures must +therefore be contained where recovery is possible. Never show UI or wait for a shutdown flush on the unobserved-task +callback: it runs on the finalizer thread. + +## Handled exceptions + +For an operation with initialized logging and telemetry, report a caught failure through both `ILogger` and +`ITelemetryService.TrackException`: consent can disable the remote record, while the local log remains. Use the +reporting type's logger and add telemetry properties only for a specific diagnostic query. A failure that propagates +to the API boundary or to the global handler is reported there; reporting it again in a lower layer records it +twice. These are rules for changes; existing catch sites do not all follow them yet. ### API boundary -[HostingExtensions.AddAndConfigureProblemDetails](../../src/Infrastructure/Extensions/HostingExtensions.cs) owns reporting for exceptions mapped to API responses. Its mappings track exceptions. Specific mappings add host diagnostics at `Verbose`; the catch-all mapping adds diagnostics regardless of that setting. Exception details are enabled for the UI to display and copy. Do not add duplicate tracking in each controller for exceptions that propagate to these mappings. +[HostingExtensions.AddAndConfigureProblemDetails](../../src/Infrastructure/Extensions/HostingExtensions.cs) owns +reporting for exceptions mapped to API responses. Its mappings track exceptions. Specific mappings add host +diagnostics at `Verbose`; the catch-all mapping adds diagnostics regardless of that setting. Exception details are +enabled for the UI to display and copy. Do not add duplicate tracking in each controller for exceptions that +propagate to these mappings. ## Local logging and the diagnostics pane -[BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs) configures the host logger factory: - -| Sink | Filter configured here | Destination | -| --- | --- | --- | -| EventSource | No provider-specific minimum | ETW listeners, subject to effective logging filters | -| Windows Event Log | `Warning` and above | `Application` log | -| DiagnosticMessageLoggerProvider | `Warning` and above | In-memory host diagnostics | -| Console and Debug | No provider-specific minimum; Debug builds only | Development output, subject to effective logging filters | - -[AppEnvironment.Diagnostics](../../src/Infrastructure/AppEnvironment.cs) stores messages written by the diagnostics logger and explicit `AddDiagnostics` calls. Collection in this store is separate from visibility in the UI. The [frontend logger](../../src/Scripts/controllers/logger.ts) polls `GetDiagnostics` only at diagnostic level `Verbose`; host messages may be collected without appearing in the pane. The host store is lost at process exit. +The host logger factory, created by [BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs), writes +`Warning` and above of every category, ASP.NET Core and Kestrel included, to the Windows Event Log and to +[AppEnvironment.Diagnostics](../../src/Infrastructure/AppEnvironment.cs). That store also receives explicit +`AddDiagnostics` calls, lives in memory for the whole process and has no eviction. The +[frontend logger](../../src/Scripts/controllers/logger.ts) polls `GetDiagnostics` only at diagnostic level +`Verbose`: a message can be stored without ever appearing in the pane. ## Telemetry consent -The host uses Application Insights through [TelemetryService](../../src/Infrastructure/Telemetry/TelemetryService.cs). The `TelemetryEnabled` setter assigns the value it receives, so every caller resolves the consent first: +The host uses Application Insights through [TelemetryService](../../src/Infrastructure/Telemetry/TelemetryService.cs). +The `TelemetryEnabled` setter assigns the value it receives, so every caller resolves the consent first: | Entry point | Current behavior | | --- | --- | @@ -66,20 +80,27 @@ The host uses Application Insights through [TelemetryService](../../src/Infrastr | Preferences saved through `UpdateOptions` | [ApplicationController](../../src/Controllers/ApplicationController.cs) resolves the consent the same way before assigning it | | Crash dialog Send | Temporarily enables host telemetry, tracks the exception and awaits one flush, then restores the previous value | -A disabled service drops new tracked exceptions. If bootstrap supplied `NullTelemetryService`, enabling it or requesting a flush still cannot send a report. +A disabled service drops new tracked exceptions. If bootstrap supplied `NullTelemetryService`, enabling it or +requesting a flush still cannot send a report. -Never add personal data to telemetry messages or properties. The single-instance pipe name includes the user SID and must not be recorded. [DefaultTelemetryProcessor](../../src/Infrastructure/Telemetry/DefaultTelemetryProcessor.cs) redacts supported sensitive tags in exception messages; it is not a general sanitizer for arbitrary properties or untagged data. +Never add personal data to telemetry messages or properties. The single-instance pipe name includes the user SID and +must not be recorded. [DefaultTelemetryProcessor](../../src/Infrastructure/Telemetry/DefaultTelemetryProcessor.cs) +redacts supported sensitive tags in exception messages; it is not a general sanitizer for arbitrary properties or +untagged data. ### Flush and shutdown -`TelemetryService.TrackException` calls synchronous `Flush` to move buffered telemetry into the channel's transmission pipeline. That call does not wait for network delivery. The [ServerTelemetryChannel 2.23.0 contract](https://raw.githubusercontent.com/microsoft/ApplicationInsights-dotnet/2.23.0/BASE/src/ServerTelemetryChannel/ServerTelemetryChannel.cs) defines `FlushAsync` success as transfer out of process, which can mean server delivery or local persistence; success is not proof that ingestion has completed. - -| Caller | Operation | Cancellation timeout | -| --- | --- | --- | -| `TelemetryService.Dispose` | `TryFlushBeforeShutdown` | 5 s | -| Fatal `UnhandledExceptionHandler`, after the dialog closes | `TryFlushBeforeShutdown` | 10 s | -| Crash dialog Send | `FlushAsync` | 30 s | +`TelemetryService.TrackException` calls the synchronous `Flush`, which moves telemetry into the channel's +transmission pipeline and returns without waiting for delivery. In the +[ServerTelemetryChannel 2.23.0 contract](https://raw.githubusercontent.com/microsoft/ApplicationInsights-dotnet/2.23.0/BASE/src/ServerTelemetryChannel/ServerTelemetryChannel.cs), +`FlushAsync` succeeds once the data has left the process, sent or persisted locally; success does not prove +ingestion. Every path that ends the process or confirms a report awaits `FlushAsync` with a bounded timeout: +`TelemetryService.Dispose`, the fatal handler after the dialog closes, and the crash dialog's Send. Do not replace +that wait with the synchronous `Flush`. -`TryFlushBeforeShutdown` blocks the caller and returns `false` on `OperationCanceledException`. It does not catch other exception types. Its name must not be treated as a general no-throw contract. +`TryFlushBeforeShutdown` blocks the caller and returns `false` only on cancellation; other exceptions propagate. -The server channel stores unsent transmissions under `.cache/.telemetry` in the application data folder when it can persist them. Data still only in memory at termination can be lost. Do not remove the awaited shutdown flush on the assumption that synchronous `Flush` guarantees delivery. The [telemetry tests](../../test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs) cover shutdown delivery to a local endpoint and persistence when the endpoint is unavailable. +The channel stores unsent transmissions under `.cache\.telemetry` in the application data folder when it can persist +them; data still in memory at termination is lost. The +[telemetry tests](../../test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs) cover delivery at +shutdown and persistence when the endpoint is unavailable. From 53ef2d475e2f71fa8298121377c02620edce8748 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Tue, 20 Oct 2026 09:27:20 +0200 Subject: [PATCH 3/3] docs: record the decisions behind bootstrap and crash reporting ADR 0003: fatal exceptions end the process after a saved report and a dialog. ADR 0004: bootstrap services are created outside the container. ADR 0005: a policy read failure stops the startup. ADR 0006: telemetry is sent through a persistent channel. The design documents link them. --- .../0003-fatal-exceptions-end-the-process.md | 39 +++++++++++++++++++ ...ootstrap-services-outside-the-container.md | 38 ++++++++++++++++++ docs/adr/0005-policy-failures-stop-startup.md | 33 ++++++++++++++++ docs/adr/0006-persistent-telemetry-channel.md | 34 ++++++++++++++++ docs/design/architecture.md | 2 + docs/design/diagnostics.md | 7 +++- 6 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 docs/adr/0003-fatal-exceptions-end-the-process.md create mode 100644 docs/adr/0004-bootstrap-services-outside-the-container.md create mode 100644 docs/adr/0005-policy-failures-stop-startup.md create mode 100644 docs/adr/0006-persistent-telemetry-channel.md diff --git a/docs/adr/0003-fatal-exceptions-end-the-process.md b/docs/adr/0003-fatal-exceptions-end-the-process.md new file mode 100644 index 00000000..9ed7dc41 --- /dev/null +++ b/docs/adr/0003-fatal-exceptions-end-the-process.md @@ -0,0 +1,39 @@ +# 0003. Fatal exceptions end the process after a report + +- Status: Accepted +- Date: 2026-10-20 + +## Context + +Bravo is a thin WinForms shell around a WebView2 UI and an in-process API. An exception that escapes a thread, +the UI thread included, leaves that shell in an unknown state. The users are business analysts, who need to know +that the application failed and need something they can pass to support. + +## Decision + +- `GlobalExceptionHandling` sets `UnhandledExceptionMode.ThrowException` for every thread and handles + `AppDomain.UnhandledException` and `TaskScheduler.UnobservedTaskException`. +- A fatal exception is logged, tracked through telemetry when consent allows it, saved to `ErrorReport.txt` in the + application data folder and shown in a task dialog. The runtime then ends the process. +- When telemetry is disabled, the dialog offers to send that one report; nothing is sent without that action. +- An unobserved task exception is logged and tracked, without a dialog, and the process continues. +- If the handler itself fails, `Environment.FailFast` ends the process. + +## Rejected alternatives + +- Continue after a UI-thread exception (`CatchException` with `Application.ThreadException`): the window stays + open in a broken state, and exceptions on other threads are not covered. +- Keep the process alive at any cost, parking faulting threads: the API and the WebView2 bridge cannot be trusted + after an unknown failure. +- Report through Windows Error Reporting: it adds native interop and an approval flow, for a report that a text + file and telemetry already carry. +- Track fatal exceptions only after the user confirms: crashes would be lost whenever the user closes the dialog, + although they consented to telemetry. + +## Consequences + +- No work survives a fatal exception; a crash is visible, never silent. +- The report file is overwritten by the next crash: it serves the dialog that shows it, not as a history. +- Exceptions raised before the handler is installed are not covered; see + [diagnostics](../design/diagnostics.md#startup-coverage-and-fatal-reporting). +- Code on the finalizer thread, where unobserved task exceptions arrive, must never show UI or wait. diff --git a/docs/adr/0004-bootstrap-services-outside-the-container.md b/docs/adr/0004-bootstrap-services-outside-the-container.md new file mode 100644 index 00000000..e90d5306 --- /dev/null +++ b/docs/adr/0004-bootstrap-services-outside-the-container.md @@ -0,0 +1,38 @@ +# 0004. Bootstrap services are created outside the container + +- Status: Accepted +- Date: 2026-10-20 + +## Context + +Logging, policies, user settings and telemetry are needed before the ASP.NET Core host exists: the global +exception handler and the crash dialog use them, and a failure while building the host must still be reported. +The host has its own dependency injection container. + +## Decision + +- `BootstrapContextFactory` creates the logger factory, the policy service, the user settings and the telemetry + service, in that order, before the host. `BootstrapContext` owns them and disposes telemetry before the logger + factory. +- The host container receives them as existing instances, so it resolves them but never disposes them. Its default + `ILoggerFactory` is replaced by the bootstrap one: bootstrap and host share one logging pipeline. +- No other container exists. Services that only the host needs are registered in the host container. + +## Rejected alternatives + +- A second `ServiceProvider` for the bootstrap: two root containers with separate lifetimes, singletons duplicated + when a type rather than an instance is registered twice, and an ambiguous disposal order. It also turns bootstrap + code into a service locator. +- Lazily created static services that construct one another: their creation order depends on first use, which + caused re-entrancy, and their failures surface at an arbitrary caller. +- Registering factories in the host container: the container would dispose services that the crash path still + needs after the host is gone. + +## Consequences + +- A registration in `BravoServiceCollectionExtensions` that turns one of these instances into a container-created + service changes who disposes it and when; see + [architecture](../design/architecture.md#composition-and-resource-ownership). +- 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. diff --git a/docs/adr/0005-policy-failures-stop-startup.md b/docs/adr/0005-policy-failures-stop-startup.md new file mode 100644 index 00000000..672f7cf1 --- /dev/null +++ b/docs/adr/0005-policy-failures-stop-startup.md @@ -0,0 +1,33 @@ +# 0005. A policy read failure stops the startup + +- Status: Accepted +- Date: 2026-10-20 + +## Context + +Administrators configure Bravo through group policies in the registry: they can disable telemetry, update checks, +built-in or custom templates, or force the system browser for sign-in. User settings and telemetry are created in the +same bootstrap step, and each can fail: a corrupt settings file, an unreachable telemetry configuration, a registry +key that cannot be read. + +## Decision + +- `BootstrapContextFactory` reads the policies eagerly. A failure propagates and the application does not start. +- A user settings failure is logged and the defaults are used. +- A telemetry failure is logged and `NullTelemetryService` is used. + +## Rejected alternatives + +- Treat unreadable policies as not configured: Bravo would start with the restrictions an administrator imposed + silently removed, for example sending telemetry where a policy forbids it. +- Stop the startup on any bootstrap failure: a corrupt settings file or a telemetry problem would make Bravo + unusable, although neither puts an administrative restriction at risk. + +## Consequences + +- A policy registry key that cannot be read ends the process before the global exception handler exists: there is + no report and no dialog, only the runtime's Event Log entry. See + [diagnostics](../design/diagnostics.md#startup-coverage-and-fatal-reporting). +- Defaults replace user settings only for the current session; how a corrupt settings file is kept and reported to + the user is outside this decision. +- A new policy that restricts behavior is safe by construction: it cannot be bypassed by a read failure. diff --git a/docs/adr/0006-persistent-telemetry-channel.md b/docs/adr/0006-persistent-telemetry-channel.md new file mode 100644 index 00000000..b4b18d2d --- /dev/null +++ b/docs/adr/0006-persistent-telemetry-channel.md @@ -0,0 +1,34 @@ +# 0006. Telemetry is sent through a persistent channel + +- Status: Accepted +- Date: 2026-10-20 + +## Context + +Host telemetry carries exceptions, and the most valuable ones are tracked right before the process ends. With the +in-memory channel a transmission that fails or is still pending at exit is lost, and `Flush` gives no confirmation +of delivery. Consent comes from a policy or from the user preference and can change while Bravo runs. + +## Decision + +- `TelemetryService` uses `ServerTelemetryChannel` with a local store under `.cache\.telemetry` in the application + data folder. Transmissions that cannot be sent are persisted and retried at a later start. +- Every path that ends the process or confirms a report awaits `FlushAsync` with a timeout, because the + synchronous `Flush` returns before anything is sent. +- `TelemetryConsent.Resolve` decides whether telemetry is enabled: a configured policy wins over the user + preference. Every caller that enables or disables telemetry resolves the consent first. + +## Rejected alternatives + +- Keep the in-memory channel and call `Flush` after each exception: the call blocks on a network request, it + cannot report a failure, and anything still pending at exit is lost. +- Send crash reports through a separate endpoint or file upload: a second transport, with its own consent handling, + for data that the telemetry pipeline already carries. + +## Consequences + +- An exception tracked offline reaches the service at a later start instead of being lost. +- `FlushAsync` reports that data left the process, sent or persisted, not that it was ingested. +- Transmissions in the store are sent even after telemetry is disabled: they were generated while telemetry was + enabled and consent was given. +- The store folder must exist before the channel starts; `BravoApplicationConfiguration` creates it. diff --git a/docs/design/architecture.md b/docs/design/architecture.md index 3b9c0bd4..88945c19 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -17,6 +17,8 @@ after the bootstrap services and the host are built, so a secondary process stil 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). +Why the bootstrap services live outside the container: +[ADR 0004](../adr/0004-bootstrap-services-outside-the-container.md). ## UI and server threads diff --git a/docs/design/diagnostics.md b/docs/design/diagnostics.md index 09ce3e19..f953fcd7 100644 --- a/docs/design/diagnostics.md +++ b/docs/design/diagnostics.md @@ -13,7 +13,8 @@ Exception reporting in the .NET host. The frontend has a separate The two runtime callbacks use [GlobalExceptionHandling](../../src/Infrastructure/Diagnostics/GlobalExceptionHandling.cs). An unobserved task -exception is not a fatal crash, even though its telemetry has `ExceptionKind = Unhandled`. +exception is not a fatal crash, even though its telemetry has `ExceptionKind = Unhandled`. Why a fatal exception +ends the process: [ADR 0003](../adr/0003-fatal-exceptions-end-the-process.md). ## Startup coverage and fatal reporting @@ -23,7 +24,8 @@ setup, the WebView2 runtime check and bootstrap creation are not: an exception t runtime's own Event Log entry, without a report or a dialog. [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 this way. +restrictions are never bypassed: a policy registry key that cannot be read ends the process this way +([ADR 0005](../adr/0005-policy-failures-stop-startup.md)). [UnhandledExceptionHandler](../../src/Infrastructure/Diagnostics/UnhandledExceptionHandler.cs) logs and tracks each callback before attempting a fatal report. Logging and tracking failures are isolated so one does not prevent the @@ -90,6 +92,7 @@ untagged data. ### Flush and shutdown +Why the channel persists telemetry: [ADR 0006](../adr/0006-persistent-telemetry-channel.md). `TelemetryService.TrackException` calls the synchronous `Flush`, which moves telemetry into the channel's transmission pipeline and returns without waiting for delivery. In the [ServerTelemetryChannel 2.23.0 contract](https://raw.githubusercontent.com/microsoft/ApplicationInsights-dotnet/2.23.0/BASE/src/ServerTelemetryChannel/ServerTelemetryChannel.cs),