From cd42ec2c3016968463d89dad158798841fc9de44 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Fri, 4 Sep 2026 21:39:26 +0200 Subject: [PATCH] Refactor host init, exception handling and bootstrap service ownership Major refactor: introduced BootstrapContext/Factory for service lifetimes; added PolicyService, PolicySnapshot, and RegistryPolicyReader for robust, testable policy access; telemetry pipeline now DI-driven with NullTelemetryService and explicit consent handling. Overhauled exception handling with GlobalExceptionHandling, UnhandledExceptionHandler, and user-facing crash dialogs. Logging pipeline is explicit and testable with DiagnosticMessageLoggerProvider. Single-instance logic clarified and hardened. Rewrote docs for clarity and LLM use; added documentation guidelines. Updated/added tests for new behaviors and edge cases. Minor: added ApplicationInsights dependency, improved error messages, clarified comments. --- AGENTS.md | 4 + CLAUDE.md | 2 +- docs/design/architecture.md | 59 +++++ docs/design/code-conventions.md | 46 ++-- docs/design/diagnostics.md | 85 +++++++ docs/design/versioning.md | 26 +- docs/documentation-guidelines.md | 58 +++-- src/Bravo.csproj | 1 + 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 | 35 +-- ...er.cs => BravoApplicationConfiguration.cs} | 20 +- .../BravoApplicationInitializationContext.cs | 28 --- src/Host/BravoApplicationInstance.Log.cs | 17 ++ src/Host/BravoApplicationInstance.cs | 89 +++---- src/Host/BravoGlobalExceptionHandling.cs | 20 ++ src/Host/BravoServiceCollectionExtensions.cs | 61 +++-- src/Host/CrashTest.cs | 72 ++++++ ...ActivationEvents.cs => IInstanceEvents.cs} | 5 +- .../InstanceActivationRequestedEventArgs.cs | 2 +- src/Host/InstancePipeName.cs | 45 ++-- src/Infrastructure/AppEnvironment.cs | 5 + src/Infrastructure/AppWindow.cs | 26 +- .../UserSettingsServiceFactory.cs | 13 + .../Diagnostics/DiagnosticMessageLogger.cs | 53 ++++ .../DiagnosticMessageLoggerProvider.cs | 16 ++ .../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 +- src/Infrastructure/Policies/IPolicyReader.cs | 6 + src/Infrastructure/Policies/IPolicySource.cs | 22 +- .../Policies/IPolicySourceFactory.cs | 6 + src/Infrastructure/Policies/Policies.cs | 28 --- src/Infrastructure/Policies/PolicyScope.cs | 7 + src/Infrastructure/Policies/PolicyService.cs | 25 ++ .../Policies/PolicyServiceFactory.cs | 12 + src/Infrastructure/Policies/PolicySnapshot.cs | 21 ++ ...ciesFactory.cs => RegistryPolicyReader.cs} | 27 +-- .../Policies/RegistryPolicySource.cs | 5 +- .../Policies/RegistryPolicySourceFactory.cs | 21 ++ .../CloudAuthenticationClient.cs | 8 +- .../DaxTemplate/DaxTemplateManager.cs | 8 +- .../SingleInstanceActivatedEventArgs.cs | 9 +- .../SingleInstance/SingleInstanceClient.cs | 10 +- .../SingleInstanceErrorEventArgs.cs | 13 - .../SingleInstance/SingleInstanceOptions.cs | 35 +-- .../SingleInstanceSendResult.cs | 26 +- .../SingleInstance/SingleInstanceServer.cs | 227 +++++++++--------- .../Telemetry/ITelemetryServiceFactory.cs | 12 + .../Telemetry/NullTelemetryService.cs | 18 ++ .../Telemetry/TelemetryConsent.cs | 26 ++ .../Telemetry/TelemetryService.cs | 132 ++++++---- .../Windows/Dialogs/TaskDialogBuilder.cs | 17 +- src/Program.cs | 30 +-- src/Properties/launchSettings.json | 7 +- src/Services/ManageDatesService.cs | 4 +- src/Services/TemplateDevelopmentService.cs | 10 +- src/packages.lock.json | 20 ++ .../Bravo.Tests/Host/BootstrapContextTests.cs | 177 ++++++++++++++ .../Host/BravoApplicationBuilderTests.cs | 63 ++++- ...voApplicationInitializationContextTests.cs | 51 ---- .../Host/BravoApplicationInstanceTests.cs | 109 +++++++++ test/Bravo.Tests/Host/HostFakes.cs | 89 ++++--- .../DiagnosticMessageLoggerProviderTests.cs | 56 +++++ .../GlobalExceptionHandlingTests.cs | 139 +++++++++++ .../Infrastructure/Policies/PolicyFakes.cs | 45 ++++ .../Policies/PolicyServiceTests.cs | 54 +++++ .../RegistryPolicyReaderLifetimeTests.cs | 83 +++++++ ...yTests.cs => RegistryPolicyReaderTests.cs} | 86 ++----- .../Policies/RegistryPolicySourceTests.cs | 29 ++- .../SingleInstanceServerTests.cs | 74 +++++- .../Telemetry/TelemetryConsentTests.cs | 39 +++ .../Telemetry/TelemetryServiceTests.cs | 100 ++++++++ 80 files changed, 2744 insertions(+), 966 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} (65%) 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 rename src/Host/{IInstanceActivationEvents.cs => IInstanceEvents.cs} (69%) create mode 100644 src/Infrastructure/Configuration/UserSettingsServiceFactory.cs create mode 100644 src/Infrastructure/Diagnostics/DiagnosticMessageLogger.cs create mode 100644 src/Infrastructure/Diagnostics/DiagnosticMessageLoggerProvider.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/Policies/IPolicyReader.cs create mode 100644 src/Infrastructure/Policies/IPolicySourceFactory.cs delete mode 100644 src/Infrastructure/Policies/Policies.cs create mode 100644 src/Infrastructure/Policies/PolicyScope.cs create mode 100644 src/Infrastructure/Policies/PolicyService.cs create mode 100644 src/Infrastructure/Policies/PolicyServiceFactory.cs create mode 100644 src/Infrastructure/Policies/PolicySnapshot.cs rename src/Infrastructure/Policies/{PoliciesFactory.cs => RegistryPolicyReader.cs} (62%) create mode 100644 src/Infrastructure/Policies/RegistryPolicySourceFactory.cs delete mode 100644 src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs create mode 100644 src/Infrastructure/Telemetry/ITelemetryServiceFactory.cs create mode 100644 src/Infrastructure/Telemetry/NullTelemetryService.cs create mode 100644 src/Infrastructure/Telemetry/TelemetryConsent.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/DiagnosticMessageLoggerProviderTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Diagnostics/GlobalExceptionHandlingTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs rename test/Bravo.Tests/Infrastructure/Policies/{PoliciesFactoryTests.cs => RegistryPolicyReaderTests.cs} (65%) create mode 100644 test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs diff --git a/AGENTS.md b/AGENTS.md index 25a89627..4dbc6865 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,6 +10,8 @@ TypeScript frontend in `src/Scripts` and is built into `src/wwwroot`. ## Build and test +Run these commands from the repository root on Windows. A full build also requires Node.js and npm. + | Task | Command | | --- | --- | | Build and test | `build.cmd` | @@ -30,6 +32,8 @@ or a CI checkout must be complete: with a shallow clone the build fails or produ | 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/code-conventions.md](docs/design/code-conventions.md) | creating or editing any `.cs` file. Encoding and design rules apply from its first line, and a wrong encoding rewrites the whole file. | | [docs/design/versioning.md](docs/design/versioning.md) | touching `version.json`, the build number, or any code that compares versions. | +| [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/documentation-guidelines.md](docs/documentation-guidelines.md) | writing or updating anything under `docs/`. | diff --git a/CLAUDE.md b/CLAUDE.md index 43c994c2..eef4bd20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1 +1 @@ -@AGENTS.md +@AGENTS.md \ No newline at end of file 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/code-conventions.md b/docs/design/code-conventions.md index cd178285..7b195b2c 100644 --- a/docs/design/code-conventions.md +++ b/docs/design/code-conventions.md @@ -4,33 +4,41 @@ Rules that apply to code in this repository, beyond what `.editorconfig` enforce ## Files -New or rewritten `.cs` files are UTF-8 **with** BOM and use CRLF. This is not configured anywhere: it is what -every existing file under `src/` is, and `.editorconfig` sets `charset` only for project and resource files. -A tool that defaults to UTF-8 without BOM, or to LF, turns a small edit into a whole-file diff. +New or rewritten `.cs` files are UTF-8 **with** BOM and use CRLF. A tool that defaults to UTF-8 without BOM, +or to LF, turns a small edit into a whole-file diff. ## Naming and design -Follow the Microsoft .NET naming and design guidelines. Where a pattern already in the repository diverges -from them, the guidelines win: a local precedent is not an argument for repeating a mistake. - -## Types - -Assign primary constructor parameters to explicit `private readonly` fields. Do not reference a parameter -directly from the body of the type: an explicit field declares the dependency, its lifetime and its -mutability at the top of the type, where they can be read. +Follow the Microsoft [.NET naming guidelines](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/naming-guidelines) for names and [framework design guidelines](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/) for type and member contracts. Read the relevant section when introducing or changing that kind of API. Where existing code diverges from the applicable guideline, follow the guideline rather than copying the divergence. Apply explicit formatting and field-naming rules from [.editorconfig](../../.editorconfig). ## Comments Comment only what the code cannot express: origin, constraints, or rationale. Never restate the code. Document -the member, not its callers. Usage rules belong to the caller or to docs/. Keep summaries short and semantic -and exclude implementation details; put them in inline comments when they matter. +the member, not its callers. Usage rules belong to the caller or to docs/. + +A `` is one sentence, third person, stating what the member is or does. Constraints and rationale go in +``. Implementation details go in inline comments, next to the code they explain. + +Plain prose only, in comments and XML documentation alike: state the fact, do not perform it. A comment written +as a voice is copied in the same voice by the next reader, and the tone spreads faster than the content. + +| Do not write | Write | +| --- | --- | +| "We safely allow the runtime to continue." | "The runtime continues." | +| "Catastrophic error while handling the exception." | "The handler failed while handling the exception." | +| "This ensures accurate crash dumps." | "The stack is intact at the throw site." | +| "There is nobody left to tell." | "The dialog is closed; the outcome is not reported." | -## Testability +Banned: first person; narration of the next statement; intensifiers and reassurance such as "safely", "simply", +"properly", "gracefully", "robust", "catastrophic"; metaphors and personification; rhetorical questions; +exclamation marks. -Design for testability from the start: +## Logging -- keep classes small and focused on one responsibility; -- inject dependencies through DI instead of relying on static state; -- pass external state — process, registry, filesystem, clock, network — through dependencies that tests can control. +A type that logs through `[LoggerMessage]` is `partial`, and the generated log methods live in a companion file +named `.Log.cs` next to the main file; the main file contains only the calls. Log methods inline in +the main file bury the flow of the type under attributes and message strings. -A type that can only be tested by running the application is not finished. +Each log method has its own `EventId`, unique within the type, so that an event can be filtered in the Event +Log without parsing the message. The level follows the outcome: `Warning` when the application continues unaffected, +`Error` when the operation visible to the user failed. Example: [BravoApplicationInstance.Log.cs](../../src/Host/BravoApplicationInstance.Log.cs). diff --git a/docs/design/diagnostics.md b/docs/design/diagnostics.md new file mode 100644 index 00000000..756def1f --- /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 and the [logging conventions](code-conventions.md#logging). 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). Consent is applied differently at each entry point: + +| 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) assigns the raw user preference; the service setter does not reapply policy precedence | +| Crash dialog Send | Temporarily enables host telemetry, tracks the exception and awaits one flush, then restores the previous value | + +Do not assume startup policy precedence also holds after saving preferences. 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/docs/design/versioning.md b/docs/design/versioning.md index 595ddb33..425f7f95 100644 --- a/docs/design/versioning.md +++ b/docs/design/versioning.md @@ -1,6 +1,6 @@ # Versioning -Bravo has a single Semantic Version, declared in `version.json` and computed by Nerdbank.GitVersioning (NBGV). +Bravo has a single Semantic Version, declared in [version.json](../../version.json) and computed by Nerdbank.GitVersioning (NBGV). The application exposes generated values through [AppVersion](../../src/Infrastructure/AppVersion.cs). ## Source of truth @@ -12,32 +12,32 @@ Bravo has a single Semantic Version, declared in `version.json` and computed by One release is one bump: minor for a feature, patch for a fix. The prerelease tag is chosen by hand and incremented per preview, so that a preview never consumes a patch number of the stable line. -Do not set `Version`, `FileVersion` or `InformationalVersion` in `Bravo.csproj`: NBGV stamps them from +Do not set `Version`, `FileVersion` or `InformationalVersion` in [Bravo.csproj](../../src/Bravo.csproj): NBGV stamps them from `version.json` at build time. ## Derived values -| Value | Shape | Authoritative for | +| Value | Shape and condition | Authoritative for | | --- | --- | --- | | `AssemblyFileVersion` | `X.Y.Z.{height}` | ordering two builds; MSI `ProductVersion` | | `AssemblyInformationalVersion` | `X.Y.Z.{height}[-tag]+{commit}` | diagnostics | | `AssemblyVersion` | `X.Y.0.0` | assembly identity (`assemblyVersion.precision: minor`) | -| `NBGV_SemVer2` | `X.Y.Z[-tag]` | artifact names, git tag, `AppVersion.SemanticVersion`, WiX `-dVersion` | +| `NBGV_SemVer2`, public build | `X.Y.Z[-tag]`, when the ref matches `publicReleaseRefSpec` | artifact names, `AppVersion.SemanticVersion`, WiX `-dVersion` | +| `NBGV_SemVer2`, internal build | `X.Y.Z-tag.g{commit}` with a prerelease tag; `X.Y.Z-g{commit}` without one | same consumers, with commit identity | -`{height}` is the number of commits since the numeric `X.Y.Z` last changed. It is a build counter: it makes -every build uniquely identifiable and orders builds that share the same `X.Y.Z`. +`{height}` is derived from Git history since the numeric `X.Y.Z` last changed. It orders builds on a release line; it is not a unique identity across branches or repeated builds of one commit. Use the commit identifier in `AssemblyInformationalVersion` to distinguish source revisions. ## Rules - **The prerelease tag never reaches a numeric field.** `AssemblyFileVersion` stays numeric in every state, so Windows Installer and `System.Version` keep working unchanged. WiX `-dVersion` carries the tag but feeds the -installer telemetry only: the MSI `ProductVersion` is bound to the file version of `Bravo.exe`. +installer telemetry only: the MSI `ProductVersion` is bound to the file version of `Bravo.exe` in [Bravo.wxs](../../installer/wix/src/Bravo/Bravo.wxs) and [Bravo-perUser.wxs](../../installer/wix/src/Bravo/Bravo-perUser.wxs). - **`SemVer2` is the version that the application, the installer and the artifacts report.** The application reads it as `ThisAssembly.NuGetPackageVersion`, which equals `SemVer2` only while `nuGetPackageVersion.semVer` is `2` in `version.json` and `NBGV_ThisAssemblyIncludesPackageVersion` is set in `Bravo.csproj`. Without the first, the application reports the SemVer1 form `X.Y.Z-tag-0001-g{commit}`; without -the second, it does not compile. A test in `AppVersionTests` checks the shape. +the second, it does not compile. [AppVersionTests](../../test/Bravo.Tests/Infrastructure/AppVersionTests.cs) checks the generated shapes, including public and internal build differences. - **The height resets only when the numeric `X.Y.Z` changes.** Adding, changing or removing the prerelease tag does not reset it. `AssemblyFileVersion` is therefore monotonic across `1.1.0-beta.1 → 1.1.0-beta.2 → 1.1.0`, @@ -53,13 +53,9 @@ to its final release changes `version.json` alone: such a filter would give the tag; numeric fields and `AssemblyInformationalVersion` are unaffected. A tag checkout runs in detached HEAD and matches no branch pattern, so building from a tag requires adding the tag pattern. -- **Artifact names come from `SemVer2`, not from `SimpleVersion`.** The two are identical for a release and -differ only for a preview, where `SimpleVersion` drops the tag: naming artifacts from it would give a preview -and the release that follows it the same file names. +- **Artifact names come from `SemVer2`, not from `SimpleVersion`.** The [packaging pipeline](../../.azure/pipelines/build-bravo.yaml) uses `NBGV_SemVer2`. On public builds, `SimpleVersion` drops the prerelease tag: using it would give a preview and the release that follows it the same file names. Internal `SemVer2` values also carry the commit identifier. -- **NBGV reads `version.json` from `HEAD`, not from the working copy.** An edit takes effect from the commit -that contains it: a build or a test run on a dirty `version.json` still reflects the committed values, and only -the height resets. +- **Verify release versions from the intended committed ref.** The generated version depends on configuration, Git history and whether the ref is a public release. A local build with uncommitted version changes is not evidence of the version that the release pipeline will produce. - **The height needs full history.** Shallow clones make NBGV fail or compute a wrong number, so both -pipelines check out with unlimited depth. +[CI](../../.github/workflows/ci.yml) and [packaging](../../.azure/pipelines/build-bravo.yaml) check out with unlimited depth. diff --git a/docs/documentation-guidelines.md b/docs/documentation-guidelines.md index daa84f92..934f5a43 100644 --- a/docs/documentation-guidelines.md +++ b/docs/documentation-guidelines.md @@ -1,42 +1,48 @@ # Documentation guidelines -Rules for writing and updating the documents under `docs/`. The reader is an LLM working on this repository, -not an end user. +Rules for documents under `docs/`. The reader is an LLM working on this repository. End-user documentation has a separate audience. -## What a document contains +## Content and scope -Write what the code cannot state: +Document constraints, invariants, verified reasons for decisions and facts about external integrations or published artifacts. Include interactions across components when they explain ownership, ordering, thread requirements or failure boundaries that would otherwise require reading several files. -- constraints and invariants, each with the consequence of violating it; -- the reason behind a choice, when the result alone does not explain it; -- facts that live outside the repository: external services, deployed clients, published artifacts. +Do not translate individual methods into prose or maintain property lists, file walkthroughs or class catalogs. Link to the implementation instead. A useful runtime description explains the contract between components and the consequence of breaking it. -Do not restate the code: no property or method lists, no file walkthroughs, no description of a control flow -that can be read in the source. Anything copied from the code drifts as soon as the code changes. Reference a -file by link when the reader needs it; do not reproduce its content. +Keep one coherent topic per document. Update an existing document before creating another; create a new document only for a distinct task or concept. Keep each rule in one authoritative location and link to it elsewhere. -## Scope +## Facts, rules and exceptions -One document, one topic. A document named after a topic covers that topic only. Adjacent material belongs to -the adjacent document, or does not exist yet. +| Statement | Required form | +| --- | --- | +| Current behavior | Describe what the checked-in implementation does; link to the relevant source, configuration or test | +| Required rule | State the action or invariant, its scope and the consequence of violating it | +| Recommendation | Identify it as a recommendation and state when it applies | +| Exception or limitation | Place it beside the affected rule or behavior; name the condition explicitly | -## Form +Do not present a desired behavior as an implemented guarantee. A rule for new changes can differ from existing behavior; label that distinction. When evidence is insufficient, omit the claim or identify the uncertainty instead of inferring a guarantee from a name or comment. + +## Retrieval and navigation -- Schematic: short sections, tables for enumerable facts, one idea per paragraph. -- Direct, professional language. Short sentences. No filler, no narration. -- English. -- A rule states what holds and what breaks when it is violated. -- Motivation is a clause attached to the rule it justifies, never a section of its own. +- Keep the selective-reading index in [AGENTS.md](../AGENTS.md). Add a document there with an explicit reading condition; do not duplicate the index under `docs/`. +- Use descriptive file names and headings based on repository concepts or tasks. +- Make each section understandable independently: name the component and keep conditions such as Debug/Release, host/frontend or public/internal build beside the claim, including in tables. +- Link to repository-relative paths and stable heading anchors. Use symbols to identify relevant members; avoid source line numbers that drift. +- Link external claims to their authoritative source. Do not copy external reference material into the repository. -## What a document is not +## Form -- Not a changelog: no dates, no "decision taken on", no "supersedes", no history of what changed. -- Not a plan: no open items, no TODO, no status. Planned work belongs to a plan document. -- Not a tutorial: no step-by-step walkthrough of ordinary tasks. +- English, short sections and plain declarative prose. Use direct instructions for required actions. +- Tables for mappings; numbered steps only when order matters across components or in a repository-specific procedure. +- Include prerequisites, working directory and success criteria when a procedure needs them to be executable. +- No filler, first person, intensifiers, reassurance, metaphors or rhetorical questions. Code-comment examples are in [code conventions](design/code-conventions.md#comments). +- Attach rationale to the rule it explains. Avoid unrelated background or generic programming advice. -A document describes the current state as if it had always been that way. +## Current-state documents + +The design documents describe current behavior and applicable rules. Do not use them as changelogs, plans or tutorials for ordinary tasks. Keep dates, change histories and future work out of these documents; current limitations belong beside the behavior they constrain. ## Maintenance -Update a document in the same change that alters the behaviour it describes. When a rule stops holding, delete -it; do not annotate it as obsolete. +Update documentation in the same change that alters the behavior it describes. Remove claims that no longer hold rather than marking them obsolete. Preserve still-applicable constraints when implementation details change. + +Before finishing, verify claims against the affected sources, check relative links and heading anchors, and confirm that the index routes the relevant tasks to the document. Distinguish commands executed from commands verified only by inspection. A documentation-only change does not require running unrelated application tests. diff --git a/src/Bravo.csproj b/src/Bravo.csproj index 58229c26..0e5fdcac 100644 --- a/src/Bravo.csproj +++ b/src/Bravo.csproj @@ -53,6 +53,7 @@ + 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 3aba7e39..f283d9d4 100644 --- a/src/Host/BravoApplicationBuilder.cs +++ b/src/Host/BravoApplicationBuilder.cs @@ -5,8 +5,6 @@ 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.Services; @@ -15,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, @@ -37,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 . /// @@ -76,26 +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); -#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 65% rename from src/Host/BravoApplicationInitializer.cs rename to src/Host/BravoApplicationConfiguration.cs index aa2e1e17..84c8340c 100644 --- a/src/Host/BravoApplicationInitializer.cs +++ b/src/Host/BravoApplicationConfiguration.cs @@ -5,29 +5,22 @@ using Sqlbi.Bravo.Infrastructure; using Sqlbi.Bravo.Infrastructure.Helpers; using Sqlbi.Bravo.Infrastructure.Services; +using Sqlbi.Bravo.Infrastructure.Telemetry; 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() @@ -40,11 +33,12 @@ private static void ConfigureDirectories() Directory.SetCurrentDirectory(AppContext.BaseDirectory); Directory.CreateDirectory(AppEnvironment.ApplicationDataPath); Directory.CreateDirectory(AppEnvironment.ApplicationTempPath); + Directory.CreateDirectory(TelemetryService.DefaultStorageFolder); } 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 f9b1bb18..57974382 100644 --- a/src/Host/BravoApplicationInstance.cs +++ b/src/Host/BravoApplicationInstance.cs @@ -1,9 +1,8 @@ using System; -using System.Diagnostics; -using System.Text; +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; @@ -13,7 +12,7 @@ namespace Sqlbi.Bravo.Host; /// /// Defines the contract for a single-instance application. /// -internal interface IBravoApplicationInstance : IInstanceActivationEvents, IDisposable +internal interface IBravoApplicationInstance : IInstanceEvents, IDisposable { /// /// Gets a value indicating whether this process is the primary instance, the one that runs the application. @@ -26,32 +25,25 @@ internal interface IBravoApplicationInstance : IInstanceActivationEvents, IDispo 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) { @@ -71,56 +63,55 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) var startupMessage = default(AppInstanceStartupMessage?); try { - var json = Encoding.UTF8.GetString(e.Payload); - startupMessage = JsonSerializer.Deserialize(json); + 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); } - // ACCEPTED LIMITATION — requests arriving while this instance is still starting up are - // dropped here, on purpose. The pipe answers from the moment the process starts, but the - // subscribers appear later: AppWindow attaches the bring-to-front handler in OnLoad, and the - // one that forwards the startup message only in OnWebViewDOMContentLoaded, seconds later. - // So a request that lands before the window exists is lost entirely, and one that lands - // between the window and a loaded WebView brings Bravo to the front but does not open what - // was asked for. The secondary instance is told the payload was delivered either way, so - // nothing records the loss. + // Known limitation: requests arriving while this instance is still starting up are dropped + // here. The pipe answers from the moment the process starts, but the subscribers appear + // later: AppWindow attaches the bring-to-front handler in OnLoad, and the one that forwards + // the startup message only in OnWebViewDOMContentLoaded, seconds later. A request that lands + // before the window exists is lost, and one that lands between the window and a loaded + // WebView brings Bravo to the front but does not open what was asked for. The secondary + // instance is told the payload was delivered either way. // TODO: Consider buffering the requests and replaying them once the UI is ready. ActivationRequested?.Invoke(this, new InstanceActivationRequestedEventArgs(startupMessage)); } - private void OnError(object? sender, SingleInstanceErrorEventArgs e) => Report(e.Exception); + private void OnError(object? sender, ErrorEventArgs e) + { + var exception = e.GetException(); + + LogListenerFailed(exception); + _telemetry.TrackException(exception); + } /// public void RedirectActivationToPrimary() { var startupSettings = StartupSettings.CreateFromCommandLineArguments(); var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); - var json = JsonSerializer.Serialize(startupMessage); - var payload = Encoding.UTF8.GetBytes(json); + var payload = JsonSerializer.SerializeToUtf8Bytes(startupMessage); var result = SingleInstanceClient.Send(_options, payload); - if (!result.IsDelivered && result.Exception is not null) + if (!result.IsDelivered) { - // The primary instance cannot be reached (for example, an elevated "Run as administrator" - // launch against a non-elevated instance), activation fails silently. This can happen because - // PipeOptions.CurrentUserOnly compares WindowsIdentity.Owner, which differs when elevation changes - // the identity to BUILTIN\Administrators. The failure is recorded in telemetry and the event log - // (BravoApplicationInstance.RedirectActivationToPrimary), but no user-facing error is shown. - + // The primary instance cannot be reached and activation fails silently. One known cause is + // a launch whose elevation differs from the primary's: PipeOptions.CurrentUserOnly builds + // and checks the pipe ACL on WindowsIdentity.Owner, which elevation changes to + // BUILTIN\Administrators, while the pipe name is keyed on WindowsIdentity.User + // (https://github.com/dotnet/runtime/issues/123903, open, milestone .NET 12). // TODO: Consider surfacing activation failures to the user. - Report(result.Exception); + 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 6f2b2f53..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(PoliciesFactory.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..22f9242f --- /dev/null +++ b/src/Host/CrashTest.cs @@ -0,0 +1,72 @@ +#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. +/// +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/Host/IInstanceActivationEvents.cs b/src/Host/IInstanceEvents.cs similarity index 69% rename from src/Host/IInstanceActivationEvents.cs rename to src/Host/IInstanceEvents.cs index 8cb6504e..b9768dec 100644 --- a/src/Host/IInstanceActivationEvents.cs +++ b/src/Host/IInstanceEvents.cs @@ -2,10 +2,7 @@ namespace Sqlbi.Bravo.Host; -/// -/// Defines the activation events this process instance raises for interested subscribers. -/// -internal interface IInstanceActivationEvents +internal interface IInstanceEvents { /// /// Occurs on the primary instance when another instance asks it to come forward. diff --git a/src/Host/InstanceActivationRequestedEventArgs.cs b/src/Host/InstanceActivationRequestedEventArgs.cs index cfbff5e1..4d5b6829 100644 --- a/src/Host/InstanceActivationRequestedEventArgs.cs +++ b/src/Host/InstanceActivationRequestedEventArgs.cs @@ -4,7 +4,7 @@ namespace Sqlbi.Bravo.Host; /// -/// Provides data for the event. +/// Provides data for the event. /// internal class InstanceActivationRequestedEventArgs(AppInstanceStartupMessage? startupMessage) : EventArgs { diff --git a/src/Host/InstancePipeName.cs b/src/Host/InstancePipeName.cs index 7c7db824..0f6dcd9c 100644 --- a/src/Host/InstancePipeName.cs +++ b/src/Host/InstancePipeName.cs @@ -9,19 +9,17 @@ namespace Sqlbi.Bravo.Host; /// /// /// -/// THE RULE: one instance per Windows session and per account, across elevation levels, and -/// regardless of how Bravo was installed — Portable, both MSI variants and the MSIX/Store package -/// are the same application to the user, and must not run side by side just because one of them -/// happens to come from a different install mechanism. -/// -/// -/// The format is a frozen contract: changing it makes a new build unable to see an instance started -/// by an older one. It is kept as a pure function so that it can be pinned by tests without touching -/// the registry, the process or the current Windows identity. +/// One instance per Windows session and per account, across elevation levels and regardless of how +/// Bravo was installed: Portable, both MSI variants and the MSIX/Store package are the same +/// application to the user and must not run side by side. /// /// internal static class InstancePipeName { + // Hardcoded constants (stable across releases) + private const string ApplicationName = "Bravo"; + private const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198"; + /// /// Builds the name for the current user and session. /// @@ -29,33 +27,26 @@ public static string Create() { using var identity = WindowsIdentity.GetCurrent(); - // User, not the token's default object owner. A "run as" with a different user gets its own - // instance — that account has its own %LOCALAPPDATA%, MSAL cache and HKCU, so nothing is - // shared with it. A "run as administrator" must not get its own instance: UAC elevation keeps - // the same Windows account, so usersettings.json, the WebView2 user data folder and - // .msalcache are the same files, and they need a single writer. Owner would get this backwards - // — elevation changes it to BUILTIN\Administrators, while User stays stable across elevation. + // User, not the token owner. A "run as" with a different user gets its own instance: that + // account has its own %LOCALAPPDATA%, MSAL cache and HKCU, so nothing is shared. A "run as + // administrator" must not: UAC elevation keeps the same account, so usersettings.json, the + // WebView2 user data folder and .msalcache are the same files and need a single writer. + // Elevation changes the owner to BUILTIN\Administrators, while User stays the same. var userSid = identity.User?.Value ?? throw new InvalidOperationException("The current Windows identity has no user SID."); return Create(AppEnvironment.SessionId, userSid); } + /// + /// Builds the name for the given session and account. + /// /// - /// Identifies the Windows session. Every Terminal Services session needs its own window and its - /// own Bravo instance, so a different session is always a different instance. + /// The Windows session. Every Terminal Services session has its own window, so it is a different instance. /// /// - /// Identifies the Windows account and must remain stable across UAC elevation — see - /// for why this is User rather than the token owner. + /// The Windows account. Must be stable across UAC elevation; see . /// public static string Create(int sessionId, string userSid) - { - // Both constants below are hardcoded; they are part of the frozen - // contract that makes the instance name stable across releases. - const string ApplicationName = "Bravo"; - const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198"; - - return $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}"; - } + => $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}"; } diff --git a/src/Infrastructure/AppEnvironment.cs b/src/Infrastructure/AppEnvironment.cs index ce638ed6..b9333c1a 100644 --- a/src/Infrastructure/AppEnvironment.cs +++ b/src/Infrastructure/AppEnvironment.cs @@ -82,6 +82,7 @@ static AppEnvironment() ApplicationDataPath = Path.Combine(Environment.GetFolderPath(DeploymentMode == AppDeploymentMode.Packaged ? Environment.SpecialFolder.UserProfile : Environment.SpecialFolder.LocalApplicationData, Environment.SpecialFolderOption.DoNotVerify), ApplicationName); ApplicationTempPath = Path.Combine(ApplicationDataPath, ".temp"); + ApplicationCachePath = Path.Combine(ApplicationDataPath, ".cache"); UserSettingsFilePath = Path.Combine(ApplicationDataPath, "usersettings.json"); MsalTokenCacheFilePath = Path.Combine(ApplicationDataPath, ".msalcache"); WebView2VersionInfo = WebView2Helper.GetRuntimeVersionInfo(); @@ -136,6 +137,10 @@ public static RegistryKey? ApplicationInstallerRegistryHKey public static string ApplicationTempPath { get; } + public static string ApplicationCachePath { get; } + + public static string ApplicationDiagnosticsPath { get; } + public static string UserSettingsFilePath { get; } public static string MsalTokenCacheFilePath { get; } diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 133fc2bd..6a9f4a06 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -41,23 +41,23 @@ internal partial class AppWindow : Form };"; public static SynchronizationContext? UISynchronizationContext { get; set; } - private readonly IInstanceActivationEvents _instanceActivationEvents; + private readonly IInstanceEvents _applicationInstance; private readonly IServerAddressProvider _serverAddressProvider; private readonly IOptions _startupSettingsOptionsAccessor; private readonly WebView2ProxyAuthHandler _proxyAuthHandler; private readonly Color _startupThemeColor; - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; public AppWindow( - IInstanceActivationEvents instanceActivationEvents, + IBravoApplicationInstance applicationInstance, IServerAddressProvider serverAddressProvider, IOptions startupSettingsOptionsAccessor, - IPolicies policies) + IPolicyService policyService) { - _instanceActivationEvents = instanceActivationEvents; + _applicationInstance = applicationInstance; _serverAddressProvider = serverAddressProvider; _startupSettingsOptionsAccessor = startupSettingsOptionsAccessor; - _policies = policies; + _policyService = policyService; _proxyAuthHandler = new WebView2ProxyAuthHandler(WebProxyWrapper.Current); _startupThemeColor = ThemeHelper.ShouldUseDarkMode(UserPreferences.Current.Theme) ? AppEnvironment.ThemeColorDark : AppEnvironment.ThemeColorLight; @@ -183,19 +183,19 @@ private void OnFormLoad(object? sender, EventArgs e) CenterToScreen(); - _instanceActivationEvents.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) { - _instanceActivationEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; - _instanceActivationEvents.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(); - _instanceActivationEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage; + _applicationInstance.ActivationRequested += OnActivationRequestedSendStartupWebMessage; } } @@ -328,7 +328,7 @@ private MemoryStream GetConfigJs() token = AppEnvironment.ApiAuthenticationToken, version = AppVersion.SemanticVersion, options = BravoOptions.CreateFromUserPreferences(), - policies = _policies, + policies = _policyService.Current, culture = new { ietfLanguageTag = CultureInfo.CurrentCulture.IetfLanguageTag, 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/DiagnosticMessageLogger.cs b/src/Infrastructure/Diagnostics/DiagnosticMessageLogger.cs new file mode 100644 index 00000000..563521bd --- /dev/null +++ b/src/Infrastructure/Diagnostics/DiagnosticMessageLogger.cs @@ -0,0 +1,53 @@ +using System; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Models; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +internal sealed class DiagnosticMessageLogger(string categoryName) : ILogger +{ + private readonly string _categoryName = categoryName; + + 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 content = formatter(state, exception); + + if (exception is not null) + content = $"{content}{Environment.NewLine}{exception}"; + + try + { + AppEnvironment.AddDiagnostics(DiagnosticMessageType.Text, _categoryName, content, GetSeverity(logLevel)); + } + catch (Exception) + { + // A sink failure does not propagate to the caller of ILogger.Log. + } + } + + private static DiagnosticMessageSeverity GetSeverity(LogLevel logLevel) + { + return logLevel switch + { + LogLevel.Critical or LogLevel.Error => DiagnosticMessageSeverity.Error, + LogLevel.Warning => DiagnosticMessageSeverity.Warning, + _ => DiagnosticMessageSeverity.None, + }; + } +} + +internal static class DiagnosticMessageLoggerExtensions +{ + public static ILoggingBuilder AddDiagnosticMessages(this ILoggingBuilder builder) + { + builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton()); + + return builder; + } +} diff --git a/src/Infrastructure/Diagnostics/DiagnosticMessageLoggerProvider.cs b/src/Infrastructure/Diagnostics/DiagnosticMessageLoggerProvider.cs new file mode 100644 index 00000000..1cdae225 --- /dev/null +++ b/src/Infrastructure/Diagnostics/DiagnosticMessageLoggerProvider.cs @@ -0,0 +1,16 @@ +using Microsoft.Extensions.Logging; + +namespace Sqlbi.Bravo.Infrastructure.Diagnostics; + +/// +/// Writes log records to , the channel the user interface polls. +/// +[ProviderAlias("Diagnostics")] +internal sealed class DiagnosticMessageLoggerProvider : ILoggerProvider +{ + public ILogger CreateLogger(string categoryName) => new DiagnosticMessageLogger(categoryName); + + public void Dispose() + { + } +} 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/Policies/IPolicyReader.cs b/src/Infrastructure/Policies/IPolicyReader.cs new file mode 100644 index 00000000..c4be6520 --- /dev/null +++ b/src/Infrastructure/Policies/IPolicyReader.cs @@ -0,0 +1,6 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicyReader +{ + PolicySnapshot Read(); +} diff --git a/src/Infrastructure/Policies/IPolicySource.cs b/src/Infrastructure/Policies/IPolicySource.cs index c3241754..3418c5b8 100644 --- a/src/Infrastructure/Policies/IPolicySource.cs +++ b/src/Infrastructure/Policies/IPolicySource.cs @@ -1,24 +1,13 @@ using System; -using Microsoft.Win32; namespace Sqlbi.Bravo.Infrastructure.Policies; -/// -/// Abstraction over a single raw policy value store (e.g. a registry key), so that -/// ' parsing/precedence logic does not depend on -/// directly and can be unit tested against a fake, without touching the real registry. -/// -internal interface IPolicySource +internal interface IPolicySource : IDisposable { int? GetInt(string name); string? GetString(string name); } -/// -/// Typed reading conventions shared by every : a policy is a -/// DWORD (0/1 -> bool, or a defined enum member) or a string. Kept as extensions rather than -/// interface members so itself stays minimal (raw int/string only). -/// internal static class PolicySourceExtensions { private const int PolicyDisabledValue = 0; @@ -37,10 +26,13 @@ internal static class PolicySourceExtensions }; } - public T? GetEnum(string name) where T : struct, Enum + /// + /// TEnum must have int as its underlying type. + /// + public TEnum? GetEnum(string name) where TEnum : struct, Enum { - if (source.GetInt(name) is { } value && Enum.IsDefined(typeof(T), value)) - return (T)(object)value; + if (source.GetInt(name) is { } value && Enum.IsDefined(typeof(TEnum), value)) + return (TEnum)(object)value; return null; // Policy not set or invalid } diff --git a/src/Infrastructure/Policies/IPolicySourceFactory.cs b/src/Infrastructure/Policies/IPolicySourceFactory.cs new file mode 100644 index 00000000..9a9c8459 --- /dev/null +++ b/src/Infrastructure/Policies/IPolicySourceFactory.cs @@ -0,0 +1,6 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicySourceFactory +{ + IPolicySource Open(PolicyScope scope); +} diff --git a/src/Infrastructure/Policies/Policies.cs b/src/Infrastructure/Policies/Policies.cs deleted file mode 100644 index c0dc94aa..00000000 --- a/src/Infrastructure/Policies/Policies.cs +++ /dev/null @@ -1,28 +0,0 @@ -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; - -namespace Sqlbi.Bravo.Infrastructure.Policies; - -internal interface IPolicies -{ - bool? TelemetryEnabled { get; } - UpdateChannelType? UpdateChannel { get; } - bool? UpdateCheckEnabled { get; } - bool? UseSystemBrowserForAuthentication { get; } - bool? BuiltInTemplatesEnabled { get; } - bool? CustomTemplatesEnabled { get; } - string? CustomTemplatesOrganizationRepositoryPath { get; } -} - -/// -/// Immutable snapshot of Bravo's effective policy values. Pure data - see -/// for how instances are read from the registry, parsed, -/// and merged with LocalMachine/CurrentUser precedence. -/// -internal sealed record Policies( - bool? TelemetryEnabled, - UpdateChannelType? UpdateChannel, - bool? UpdateCheckEnabled, - bool? UseSystemBrowserForAuthentication, - bool? BuiltInTemplatesEnabled, - bool? CustomTemplatesEnabled, - string? CustomTemplatesOrganizationRepositoryPath) : IPolicies; diff --git a/src/Infrastructure/Policies/PolicyScope.cs b/src/Infrastructure/Policies/PolicyScope.cs new file mode 100644 index 00000000..62abdec5 --- /dev/null +++ b/src/Infrastructure/Policies/PolicyScope.cs @@ -0,0 +1,7 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal enum PolicyScope +{ + Computer, + User, +} diff --git a/src/Infrastructure/Policies/PolicyService.cs b/src/Infrastructure/Policies/PolicyService.cs new file mode 100644 index 00000000..3129bea0 --- /dev/null +++ b/src/Infrastructure/Policies/PolicyService.cs @@ -0,0 +1,25 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +public interface IPolicyService +{ + /// + /// Gets the policy snapshot initialized once for this service instance. + /// + PolicySnapshot Current { get; } +} + +internal sealed class PolicyService : IPolicyService +{ + private readonly IPolicyReader _reader; + private readonly Lazy _current; + + public PolicyService(IPolicyReader reader) + { + _reader = reader; + _current = new Lazy(_reader.Read); + } + + public PolicySnapshot Current => _current.Value; +} diff --git a/src/Infrastructure/Policies/PolicyServiceFactory.cs b/src/Infrastructure/Policies/PolicyServiceFactory.cs new file mode 100644 index 00000000..577be97e --- /dev/null +++ b/src/Infrastructure/Policies/PolicyServiceFactory.cs @@ -0,0 +1,12 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicyServiceFactory +{ + IPolicyService Create(); +} + +internal sealed class PolicyServiceFactory : IPolicyServiceFactory +{ + public IPolicyService Create() + => new PolicyService(new RegistryPolicyReader(new RegistryPolicySourceFactory())); +} diff --git a/src/Infrastructure/Policies/PolicySnapshot.cs b/src/Infrastructure/Policies/PolicySnapshot.cs new file mode 100644 index 00000000..923d8f7c --- /dev/null +++ b/src/Infrastructure/Policies/PolicySnapshot.cs @@ -0,0 +1,21 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +/// +/// Contains the effective policy values from one read of the configured sources. +/// +/// +/// Null means not configured; false and zero are configured values. +/// +public sealed record PolicySnapshot( + bool? TelemetryEnabled, + UpdateChannelType? UpdateChannel, + bool? UpdateCheckEnabled, + bool? UseSystemBrowserForAuthentication, + bool? BuiltInTemplatesEnabled, + bool? CustomTemplatesEnabled, + string? CustomTemplatesOrganizationRepositoryPath) +{ + public static PolicySnapshot NotConfigured { get; } = new(null, null, null, null, null, null, null); +} diff --git a/src/Infrastructure/Policies/PoliciesFactory.cs b/src/Infrastructure/Policies/RegistryPolicyReader.cs similarity index 62% rename from src/Infrastructure/Policies/PoliciesFactory.cs rename to src/Infrastructure/Policies/RegistryPolicyReader.cs index 0daffa76..5a97375a 100644 --- a/src/Infrastructure/Policies/PoliciesFactory.cs +++ b/src/Infrastructure/Policies/RegistryPolicyReader.cs @@ -1,28 +1,23 @@ -using Microsoft.Win32; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; namespace Sqlbi.Bravo.Infrastructure.Policies; -/// -/// Builds instances: parses a single , -/// and composes the effective policy set from LocalMachine + CurrentUser with precedence. -/// -internal static class PoliciesFactory +internal sealed class RegistryPolicyReader(IPolicySourceFactory sourceFactory) : IPolicyReader { - private const string OptionSettingsSubKeyName = @"SOFTWARE\Policies\SQLBI\Bravo\OptionSettings"; + private readonly IPolicySourceFactory _sourceFactory = sourceFactory; - public static Policies Create() + public PolicySnapshot Read() { - using var machineKey = Registry.LocalMachine.OpenSubKey(OptionSettingsSubKeyName); - var machinePolicies = FromSource(new RegistryPolicySource(machineKey)); + using var machineSource = _sourceFactory.Open(PolicyScope.Computer); + var machinePolicies = FromSource(machineSource); - using var userKey = Registry.CurrentUser.OpenSubKey(OptionSettingsSubKeyName); - var userPolicies = FromSource(new RegistryPolicySource(userKey)); + using var userSource = _sourceFactory.Open(PolicyScope.User); + var userPolicies = FromSource(userSource); return Merge(machinePolicies, userPolicies); } - internal static Policies FromSource(IPolicySource source) => new( + internal static PolicySnapshot FromSource(IPolicySource source) => new( TelemetryEnabled: source.GetBool("TelemetryEnabled"), UpdateChannel: source.GetEnum("UpdateChannel"), UpdateCheckEnabled: source.GetBool("UpdateCheckEnabled"), @@ -31,10 +26,10 @@ public static Policies Create() CustomTemplatesEnabled: source.GetBool("CustomTemplatesEnabled"), CustomTemplatesOrganizationRepositoryPath: source.GetString("CustomTemplatesOrganizationRepositoryPath")); - internal static Policies Merge(Policies machinePolicies, Policies userPolicies) + internal static PolicySnapshot Merge(PolicySnapshot machinePolicies, PolicySnapshot userPolicies) { // LocalMachine takes precedence over CurrentUser when both are configured - return new Policies( + return new PolicySnapshot( TelemetryEnabled: machinePolicies.TelemetryEnabled ?? userPolicies.TelemetryEnabled, UpdateChannel: machinePolicies.UpdateChannel ?? userPolicies.UpdateChannel, UpdateCheckEnabled: machinePolicies.UpdateCheckEnabled ?? userPolicies.UpdateCheckEnabled, diff --git a/src/Infrastructure/Policies/RegistryPolicySource.cs b/src/Infrastructure/Policies/RegistryPolicySource.cs index bf598cd0..ce966475 100644 --- a/src/Infrastructure/Policies/RegistryPolicySource.cs +++ b/src/Infrastructure/Policies/RegistryPolicySource.cs @@ -3,8 +3,7 @@ namespace Sqlbi.Bravo.Infrastructure.Policies; /// -/// Adapter that bridges to a real . -/// Intentionally a thin pass-through with no logic of its own. +/// Owns a read-only registry key containing raw policy values. /// internal sealed class RegistryPolicySource(RegistryKey? key) : IPolicySource { @@ -15,4 +14,6 @@ internal sealed class RegistryPolicySource(RegistryKey? key) : IPolicySource public string? GetString(string name) => _key?.GetValue(name) as string; + + public void Dispose() => _key?.Dispose(); } diff --git a/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs b/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs new file mode 100644 index 00000000..b1d9864a --- /dev/null +++ b/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs @@ -0,0 +1,21 @@ +using System; +using Microsoft.Win32; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal sealed class RegistryPolicySourceFactory : IPolicySourceFactory +{ + private const string OptionSettingsSubKeyName = @"SOFTWARE\Policies\SQLBI\Bravo\OptionSettings"; + + public IPolicySource Open(PolicyScope scope) + { + var hive = scope switch + { + PolicyScope.Computer => Registry.LocalMachine, + PolicyScope.User => Registry.CurrentUser, + _ => throw new ArgumentOutOfRangeException(nameof(scope)), + }; + + return new RegistryPolicySource(hive.OpenSubKey(OptionSettingsSubKeyName)); + } +} diff --git a/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs b/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs index c14d8d9c..d0ffd560 100644 --- a/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs +++ b/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs @@ -22,12 +22,12 @@ public interface ICloudAuthenticationClient /// /// Handles authentication with Microsoft Entra ID (Azure AD) using MSAL.NET, including token acquisition and cache management. /// -internal sealed class CloudAuthenticationClient(IPolicies policies) : ICloudAuthenticationClient +internal sealed class CloudAuthenticationClient(IPolicyService policyService) : ICloudAuthenticationClient { private const string SystemBrowserRedirectUri = "http://localhost"; private const string OrganizationalAccountsOnlyQueryParameter = "msafed=0"; // no Microsoft accounts (MSA) allowed - private readonly IPolicies _policies = policies; + private readonly IPolicyService _policyService = policyService; public async Task AcquireTokenAsync( CloudEnvironment environment, string email, CancellationToken cancellationToken) @@ -77,7 +77,7 @@ public async Task ClearTokenCacheAsync(CloudEnvironment environment) private async Task AcquireTokenInteractiveAsync( IPublicClientApplication client, string[] scopes, string email, string claims, CancellationToken cancellationToken) { - var useSystemBrowser = _policies.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; + var useSystemBrowser = _policyService.Current.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; var useEmbeddedBrowser = !useSystemBrowser; var extraQueryParameters = OrganizationalAccountsOnlyQueryParameter; var prompt = Prompt.SelectAccount; @@ -125,7 +125,7 @@ public async Task ClearTokenCacheAsync(CloudEnvironment environment) private IPublicClientApplication CreatePublicClient(CloudEnvironment environment) { - var useSystemBrowser = _policies.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; + var useSystemBrowser = _policyService.Current.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; var useEmbeddedBrowser = !useSystemBrowser; var redirectUri = (useEmbeddedBrowser ? environment.RedirectUri : SystemBrowserRedirectUri); diff --git a/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs b/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs index 3aa470bd..f7c1c5fb 100644 --- a/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs +++ b/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs @@ -31,11 +31,11 @@ internal class DaxTemplateManager internal static readonly string UserPath = Path.Combine(AppEnvironment.ApplicationDataPath, @"ManageDates\Templates"); private readonly object _cacheSyncLock = new(); - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; - public DaxTemplateManager(IPolicies policies) + public DaxTemplateManager(IPolicyService policyService) { - _policies = policies; + _policyService = policyService; InitializeCache(); } @@ -46,7 +46,7 @@ public Package GetPackage(string path) public IEnumerable GetPackages() { - if (_policies.BuiltInTemplatesEnabled is false) + if (_policyService.Current.BuiltInTemplatesEnabled is false) { return Array.Empty(); } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs index 288ac5ea..ff5b581c 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs @@ -3,13 +3,12 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// -/// Carries the raw payload sent by a secondary instance. The component is deliberately unaware of -/// how the payload is encoded: interpreting it is the composition layer's responsibility. +/// Carries the payload sent by a secondary instance. /// +/// +/// The payload is not interpreted by the server. It is never empty. +/// internal sealed class SingleInstanceActivatedEventArgs(byte[] payload) : EventArgs { - /// - /// The bytes received from the secondary instance. Never empty. - /// public byte[] Payload { get; } = payload; } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceClient.cs b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs index d515b4e8..646a2a99 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceClient.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs @@ -14,11 +14,14 @@ internal static class SingleInstanceClient /// Sends to the owning instance. /// /// - /// The connection is opened with , which makes the - /// client verify that the pipe is owned by the current user before writing to it. + /// makes the client verify that the pipe is owned by + /// the current user before writing to it. The write completes when the owner has read the + /// payload, so a delivered payload has reached the owner; an owner that is alive but not + /// reading blocks the caller, with no timeout. /// public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[] payload) { + ArgumentOutOfRangeException.ThrowIfZero(payload.Length); ArgumentOutOfRangeException.ThrowIfGreaterThan(payload.Length, options.MaxPayloadBytes); using var pipeClient = new NamedPipeClientStream( @@ -39,9 +42,8 @@ public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[ try { pipeClient.Write(payload, offset: 0, payload.Length); - pipeClient.Flush(); } - catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) + catch (IOException ex) { return SingleInstanceSendResult.Failed(ex); } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs deleted file mode 100644 index e8e3dac8..00000000 --- a/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace Sqlbi.Bravo.Infrastructure.SingleInstance; - -/// -/// Reports a failure that happened on the listener loop, where there is no caller to return it to. -/// Modelled on : the component owns no logging -/// dependency, so the host decides what to do with it. -/// -internal sealed class SingleInstanceErrorEventArgs(Exception exception) : EventArgs -{ - public Exception Exception { get; } = exception; -} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs index dc33f6db..b927f755 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs @@ -3,38 +3,41 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// -/// Configuration shared by the two sides of the single-instance protocol: the owner -/// () and any secondary instance (). -/// Both must be configured with the same . +/// Configuration shared by and . /// +/// +/// Both sides must use the same . +/// internal sealed record SingleInstanceOptions { /// - /// Name of the named pipe that both identifies the application and arbitrates ownership of the - /// running instance. It must be stable across releases, and unique per user and per session: - /// everything sharing this name is considered the same application instance. + /// Name of the named pipe that identifies the application instance. /// + /// + /// Everything sharing this name is the same application instance, so it must be stable across + /// releases and unique per user and per session. + /// public required string PipeName { get; init; } /// - /// How long a secondary instance waits for the owner to accept its connection before giving up. + /// How long a secondary instance waits for the owner to accept its connection. /// public TimeSpan ConnectTimeout { get; init; } = TimeSpan.FromSeconds(5); /// - /// How long the owner waits, once a client has connected, for that client to finish sending its - /// payload. The counterpart of on the owner's side: a legitimate - /// client writes its whole payload in one call right after connecting, so this bounds how long a - /// stalled or malfunctioning client can hold the pipe's only server instance, which would - /// otherwise leave the owner unreachable to every later instance while it stays alive. + /// How long the owner waits for a connected client to finish sending its payload. /// + /// + /// A client that connects and never finishes would otherwise hold the pipe's only server + /// instance, leaving the owner unreachable while it stays alive. + /// public TimeSpan ReadTimeout { get; init; } = TimeSpan.FromSeconds(5); /// - /// Upper bound, in bytes, for a single activation payload. The limit is checked after each read, - /// so a rejected payload is buffered up to one read block past it and never more, keeping a - /// malfunctioning or hostile client from growing the owner's memory. The rejection is reported - /// through . + /// Upper bound, in bytes, for a single activation payload. /// + /// + /// A larger payload is rejected and reported through . + /// public int MaxPayloadBytes { get; init; } = 64 * 1024; } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs index 69e66b67..e6be8e3d 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics.CodeAnalysis; namespace Sqlbi.Bravo.Infrastructure.SingleInstance; @@ -13,8 +14,7 @@ internal enum SingleInstanceSendStatus Delivered = 0, /// - /// No owner accepted the connection within the configured timeout. Either the owner is gone, or - /// it is alive but not listening — which the caller may want to surface rather than ignore. + /// No owner accepted the connection within the configured timeout, or the owner refused it. /// OwnerUnavailable = 1, @@ -25,15 +25,29 @@ internal enum SingleInstanceSendStatus } /// -/// Result of a send. Failing to reach the owner is an expected -/// runtime condition, not an exceptional one, so it is returned rather than thrown. +/// Result of a send. /// -internal readonly record struct SingleInstanceSendResult(SingleInstanceSendStatus Status, Exception? Exception) +/// +/// Failing to reach the owner is an expected runtime condition, so it is returned rather than +/// thrown. A failed result always carries the exception that caused it. +/// +internal readonly record struct SingleInstanceSendResult { + private SingleInstanceSendResult(SingleInstanceSendStatus status, Exception? exception) + { + Status = status; + Exception = exception; + } + + public SingleInstanceSendStatus Status { get; } + + public Exception? Exception { get; } + + [MemberNotNullWhen(false, nameof(Exception))] public bool IsDelivered => Status == SingleInstanceSendStatus.Delivered; public static SingleInstanceSendResult Delivered() - => new(SingleInstanceSendStatus.Delivered, Exception: null); + => new(SingleInstanceSendStatus.Delivered, exception: null); public static SingleInstanceSendResult OwnerUnavailable(Exception exception) => new(SingleInstanceSendStatus.OwnerUnavailable, exception); diff --git a/src/Infrastructure/SingleInstance/SingleInstanceServer.cs b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs index d1e9c4e2..65ea0c35 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceServer.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs @@ -1,4 +1,5 @@ using System; +using System.Buffers; using System.Diagnostics.CodeAnalysis; using System.IO; using System.IO.Pipes; @@ -14,26 +15,27 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// /// /// Ownership is arbitrated by the named pipe itself, which is created allowing a single server -/// instance: only one process at a time can hold the name. There is deliberately no separate mutex, -/// so there is no second piece of state that can disagree with the listener. If this process stops -/// listening for any reason, the name is released and the next process to start becomes the owner, -/// instead of every later instance failing to reach an owner that no longer answers. +/// instance: only one process at a time can hold the name. There is no separate mutex, so there is +/// no second piece of state that can disagree with the listener. If this process stops listening +/// for any reason, the name is released and the next process to start becomes the owner. /// /// -/// The pipe is disconnected — not recreated — between connections: recreating it would release the +/// The pipe is disconnected, not recreated, between connections: recreating it would release the /// name for an instant, during which another process could claim ownership. /// /// internal sealed class SingleInstanceServer : IDisposable { - private static readonly TimeSpan s_errorBackoff = TimeSpan.FromMilliseconds(250); + private const int ErrorPipeBusy = unchecked((int)0x800700E7); + + private static readonly TimeSpan s_acceptFailureBackoff = TimeSpan.FromMilliseconds(250); private static readonly TimeSpan s_shutdownTimeout = TimeSpan.FromSeconds(1); private readonly SingleInstanceOptions _options; private readonly NamedPipeServerStream _pipeServer; private readonly CancellationTokenSource _cancellation; + private readonly Task _listener; - private Task? _listener; private bool _disposed; private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStream pipeServer) @@ -41,21 +43,29 @@ private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStrea _options = options; _pipeServer = pipeServer; _cancellation = new CancellationTokenSource(); + // Started on the thread pool so that the loop runs with no SynchronizationContext, whatever + // thread called TryStart: every continuation stays on the pool, and the blocking wait in + // Dispose cannot deadlock against the caller's context. + _listener = Task.Run(() => ListenAsync(_cancellation.Token)); } /// - /// Raised when a secondary instance sends an activation payload. Raised on a thread pool thread, - /// never on the listener loop, so a subscriber is free to block: subscribers that need a specific - /// thread — a UI thread, typically — marshal by themselves. + /// Raised when a secondary instance sends an activation payload. /// + /// + /// Raised on a thread pool thread, not on the listener loop: a blocking handler does not stop + /// the listener. Handlers for different payloads may run concurrently and in any order. + /// public event EventHandler? Activated; /// - /// Raised when a connection fails, a client does not finish sending within - /// , or a subscriber of - /// throws. The loop keeps running: ownership is not given up because of a single bad connection. + /// Raised when a connection fails, a client sends no valid payload within + /// , or an handler throws. /// - public event EventHandler? Error; + /// + /// The listener continues after the event. Handlers may be invoked concurrently. + /// + public event EventHandler? Error; /// /// Attempts to take ownership of the application instance identified by @@ -63,14 +73,17 @@ private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStrea /// /// /// if ownership was acquired by this process; if - /// another process already owns it, in which case the caller should notify the owner through - /// and exit. + /// another process already owns it. /// public static bool TryStart(SingleInstanceOptions options, [NotNullWhen(true)] out SingleInstanceServer? server) { NamedPipeServerStream pipeServer; try { + // The default zero-sized buffer is deliberate: a client's Write then blocks until this + // process reads, which can only happen after the connection is accepted. With a buffer, + // a client that writes and closes before ConnectNamedPipe completes makes the accept fail + // with ERROR_NO_DATA, and the payload in the buffer is lost. pipeServer = new NamedPipeServerStream( options.PipeName, PipeDirection.In, @@ -78,122 +91,118 @@ public static bool TryStart(SingleInstanceOptions options, [NotNullWhen(true)] o PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + catch (Exception ex) when (ex is UnauthorizedAccessException || (ex is IOException && ex.HResult == ErrorPipeBusy)) { - // The name is already taken: another process owns this application instance. + // ERROR_PIPE_BUSY: the single allowed instance is held by another process. Access denied: + // it is held by a process whose pipe ACL excludes this one. Any other failure propagates. server = null; return false; } - var instance = new SingleInstanceServer(options, pipeServer); - instance._listener = Task.Run(() => instance.ListenAsync(instance._cancellation.Token)); - - server = instance; + server = new SingleInstanceServer(options, pipeServer); return true; } /// - /// Serves one connection at a time until shutdown. A bad connection degrades this loop, it never - /// ends it: giving up would release the pipe name and hand ownership to the next process. + /// Serves one connection at a time until shutdown. /// + /// + /// A failed connection is reported and the loop continues: ending the loop would release the + /// pipe name. Only a failure to accept is paced, since a repeatedly failing pipe would otherwise + /// spin. A client that fails after connecting is not paced: the pipe is intact and the next + /// client may already be waiting for the instance. + /// private async Task ListenAsync(CancellationToken cancellationToken) { - while (!cancellationToken.IsCancellationRequested) + try { - try - { - var payload = await AcceptAndReadAsync(cancellationToken).ConfigureAwait(false); - if (payload is not null) - Dispatch(payload); - } - catch (Exception ex) when (IsShutdown(ex, cancellationToken)) + while (true) { - break; - } - catch (Exception ex) - { - RaiseError(ex); + try + { + await _pipeServer.WaitForConnectionAsync(cancellationToken); + } + catch (Exception ex) when (!IsShutdown(ex, cancellationToken)) + { + Disconnect(); + RaiseError(ex); + await Task.Delay(s_acceptFailureBackoff, cancellationToken); + continue; + } - if (!await TryBackOffAsync(cancellationToken).ConfigureAwait(false)) - break; + try + { + Dispatch(await ReadPayloadAsync(cancellationToken)); + } + catch (Exception ex) when (!IsShutdown(ex, cancellationToken)) + { + RaiseError(ex); + } + finally + { + Disconnect(); + } } } + catch (Exception ex) when (IsShutdown(ex, cancellationToken)) + { + } } /// - /// Tells a shutdown apart from a connection that went wrong. The two arrive as the same exception - /// types, and only the caller's token says which happened: a cancelled read is the ReadTimeout - /// firing, unless shutdown was requested. + /// Returns whether was caused by shutdown rather than by a failed + /// connection. /// - private static bool IsShutdown(Exception exception, CancellationToken cancellationToken) => exception switch - { - // The pipe was disposed underneath the loop, which only Dispose does. - ObjectDisposedException => true, - OperationCanceledException => cancellationToken.IsCancellationRequested, - _ => false, - }; + /// + /// Disposing the pipe under a pending operation surfaces as an , an + /// or an depending + /// on timing, so the shutdown token decides, not the exception type. + /// + private static bool IsShutdown(Exception exception, CancellationToken cancellationToken) + => cancellationToken.IsCancellationRequested || exception is ObjectDisposedException; /// - /// Accepts one connection and reads its payload, always releasing the pipe afterwards. Returns - /// when the client sent nothing usable. + /// Reads the payload of the connected client, up to the client closing its end of the pipe. /// - private async Task AcceptAndReadAsync(CancellationToken cancellationToken) + /// The client did not finish within . + /// The payload is empty or exceeds . + private async Task ReadPayloadAsync(CancellationToken cancellationToken) { + // Waiting for a connection has no deadline; once connected a deadline applies, otherwise a + // client that never finishes sending would hold the pipe's only server instance. + using var readCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + readCancellation.CancelAfter(_options.ReadTimeout); + + var limit = _options.MaxPayloadBytes; + var buffer = ArrayPool.Shared.Rent(limit + 1); try { - // Waiting for a connection has no deadline: an idle pipe with nobody connected is the - // normal state, not a problem. - await _pipeServer.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false); + var length = 0; + while (length <= limit) + { + var count = await _pipeServer.ReadAsync(buffer.AsMemory(length, buffer.Length - length), readCancellation.Token); + if (count == 0) + break; - // Once connected a deadline applies, otherwise a client that connects and never finishes - // sending would hold the pipe's only server instance forever — see ReadTimeout. - using var readCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - readCancellation.CancelAfter(_options.ReadTimeout); + length += count; + } - return await ReadPayloadAsync(readCancellation.Token).ConfigureAwait(false); - } - finally - { - Disconnect(); - } - } + if (length > limit) + throw new InvalidDataException($"The payload exceeds {limit} bytes."); - /// - /// Paces the loop after a failure, so that a pipe or a client failing immediately and repeatedly - /// leaves the listener degraded rather than spinning at full speed. - /// - /// if shutdown was requested while waiting. - private static async Task TryBackOffAsync(CancellationToken cancellationToken) - { - try - { - await Task.Delay(s_errorBackoff, cancellationToken).ConfigureAwait(false); - return true; + if (length == 0) + throw new InvalidDataException("The payload is empty."); + + return buffer.AsSpan(0, length).ToArray(); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { - return false; + throw new TimeoutException($"The client did not finish sending within {_options.ReadTimeout}."); } - } - - private async Task ReadPayloadAsync(CancellationToken cancellationToken) - { - using var buffer = new MemoryStream(); - var chunk = new byte[4096]; - - int count; - while ((count = await _pipeServer.ReadAsync(chunk, cancellationToken).ConfigureAwait(false)) > 0) + finally { - buffer.Write(chunk, 0, count); - - // Thrown rather than returned as null so that it travels the same path as any other bad - // connection: reported through Error, then paced. Returning null would have made an - // oversized payload indistinguishable from an empty one and dropped it silently. - if (buffer.Length > _options.MaxPayloadBytes) - throw new InvalidDataException($"Payload exceeds {_options.MaxPayloadBytes} bytes."); + ArrayPool.Shared.Return(buffer); } - - return buffer.Length == 0 ? null : buffer.ToArray(); } private void Dispatch(byte[] payload) @@ -219,11 +228,11 @@ private void RaiseError(Exception exception) { try { - Error?.Invoke(this, new SingleInstanceErrorEventArgs(exception)); + Error?.Invoke(this, new ErrorEventArgs(exception)); } catch { - // A failing error handler must not take down the listener loop. + // An exception from an Error handler does not end the listener loop. } } @@ -231,15 +240,14 @@ private void Disconnect() { try { - // Unconditionally, and never guarded by IsConnected: when the client closes first the - // property is already false while the pipe instance is still in a connected state, and - // skipping the call makes every later WaitForConnectionAsync fail with - // InvalidOperationException — the owner stops answering while still holding the name. + // Not guarded by IsConnected: when the client closes first, IsConnected is already false + // while the instance is still connected, and a skipped Disconnect makes every later + // WaitForConnectionAsync throw InvalidOperationException. _pipeServer.Disconnect(); } catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) { - // Not connected, or already gone: nothing to release. + // Not connected, or already disposed: nothing to release. } } @@ -251,17 +259,10 @@ public void Dispose() _disposed = true; _cancellation.Cancel(); - // Unblocks a WaitForConnectionAsync that is not observing cancellation yet. _pipeServer.Dispose(); - try - { - _listener?.Wait(s_shutdownTimeout); - } - catch (AggregateException) - { - // The loop faulted on its way out; there is nothing left to report at this point. - } + // ListenAsync catches every exception, so this only returns or times out. + _listener.Wait(s_shutdownTimeout); _cancellation.Dispose(); } 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/TelemetryConsent.cs b/src/Infrastructure/Telemetry/TelemetryConsent.cs new file mode 100644 index 00000000..acd4ad75 --- /dev/null +++ b/src/Infrastructure/Telemetry/TelemetryConsent.cs @@ -0,0 +1,26 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; + +namespace Sqlbi.Bravo.Infrastructure.Telemetry; + +internal enum TelemetryConsentSource +{ + UserSettings, + Policy, +} + +/// +/// The effective decision on whether telemetry is collected, and where that decision comes from. +/// +internal readonly record struct TelemetryConsent(bool IsEnabled, TelemetryConsentSource Source) +{ + /// + /// A configured policy overrides the user preference: the source tells the user which one applies. + /// + public static TelemetryConsent Resolve(PolicySnapshot policies, IUserSettings settings) + { + return policies.TelemetryEnabled is bool policyValue + ? new TelemetryConsent(policyValue, TelemetryConsentSource.Policy) + : new TelemetryConsent(settings.TelemetryEnabled, TelemetryConsentSource.UserSettings); + } +} \ No newline at end of file diff --git a/src/Infrastructure/Telemetry/TelemetryService.cs b/src/Infrastructure/Telemetry/TelemetryService.cs index 23d5031f..fc78e721 100644 --- a/src/Infrastructure/Telemetry/TelemetryService.cs +++ b/src/Infrastructure/Telemetry/TelemetryService.cs @@ -1,81 +1,125 @@ using System; +using System.Collections.Generic; using System.Diagnostics; +using System.IO; +using System.Threading; +using System.Threading.Tasks; using Microsoft.ApplicationInsights; +using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.Extensibility; -using Sqlbi.Bravo.Infrastructure.Configuration; -using Sqlbi.Bravo.Infrastructure.Policies; +using Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel; namespace Sqlbi.Bravo.Infrastructure.Telemetry; public interface ITelemetryService : IDisposable { bool TelemetryEnabled { get; set; } - void TrackException(Exception exception); + void TrackException(Exception exception, IDictionary? properties = null); + Task FlushAsync(CancellationToken cancellationToken = default); + bool TryFlushBeforeShutdown(int timeoutMilliseconds); } internal sealed class TelemetryService : ITelemetryService { + public static readonly string DefaultStorageFolder = + Path.Combine(AppEnvironment.ApplicationCachePath, ".telemetry"); + private readonly TelemetryConfiguration _configuration; + private readonly ITelemetryChannel _channel; private readonly TelemetryClient _client; - private readonly IPolicies _policies; - - private static readonly Lazy _instance = new(CreateInstance, isThreadSafe: true); - - public static TelemetryService Instance => _instance.Value; + private bool _disposed; - private static TelemetryService CreateInstance() + private TelemetryService(TelemetryConfiguration configuration) { - // Use parameterless constructor to avoid default processors (e.g. adaptive sampling) - // that TelemetryConfiguration.CreateDefault() would register. - var configuration = new TelemetryConfiguration(); - - // Remark: ensure no sampling is applied — every exception must be recorded - var processorChain = configuration.DefaultTelemetrySink.TelemetryProcessorChainBuilder; - processorChain.Use((next) => new DefaultTelemetryProcessor(next)); - processorChain.Build(); - - configuration.TelemetryInitializers.Add(new DefaultTelemetryInitializer()); - configuration.ConnectionString = TelemetrySessionInfo.ConnectionString; + _configuration = configuration; + _channel = configuration.TelemetryChannel; + _client = new TelemetryClient(configuration); + } - var policies = PoliciesFactory.Create(); + public bool TelemetryEnabled + { + get => _configuration.DisableTelemetry == false; + set => _configuration.DisableTelemetry = !value; + } - // Determine whether telemetry is enabled based on policies and user settings. - var telemetryEnabled = policies.TelemetryEnabled ?? UserPreferences.Current.TelemetryEnabled; - configuration.DisableTelemetry = !telemetryEnabled; -#if DEBUG - configuration.TelemetryChannel.DeveloperMode = Debugger.IsAttached; -#endif - return new TelemetryService(configuration, policies); + public void TrackException(Exception exception, IDictionary? properties = null) + { + _client.TrackException(exception, properties); + // With ServerTelemetryChannel, Flush moves telemetry out of the in-memory telemetry + // buffer and into the transmission pipeline without waiting for delivery. Calling it + // after TrackException improves the chances of preserving the exception if the process + // terminates before the next scheduled flush, while keeping this path non-blocking. + // It does not guarantee that the telemetry has been sent or persisted to local storage. + _client.Flush(); } - private TelemetryService(TelemetryConfiguration configuration, IPolicies policies) + public Task FlushAsync(CancellationToken cancellationToken = default) { - _configuration = configuration; - _client = new TelemetryClient(configuration); - _policies = policies; + return _client.FlushAsync(cancellationToken); } - public bool TelemetryEnabled + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) { - get => !_configuration.DisableTelemetry; - set + // At shutdown, wait for ServerTelemetryChannel to flush telemetry that could + // otherwise be lost when the process exits. FlushAsync completes once pending + // telemetry has been sent or persisted to local storage. The wait is bounded + // because telemetry must never prevent the application from terminating. + using var cts = new CancellationTokenSource(timeoutMilliseconds); + try + { + return _client.FlushAsync(cts.Token).GetAwaiter().GetResult(); + } + catch (OperationCanceledException) { - var telemetryEnabled = _policies.TelemetryEnabled ?? value; - _configuration.DisableTelemetry = !telemetryEnabled; + return false; } } - public void TrackException(Exception exception) + public void Dispose() { - if (exception is AggregateException aex) - exception = aex.GetBaseException(); + if (_disposed) + return; + + _disposed = true; + + _ = TryFlushBeforeShutdown(5_000); + + _configuration.Dispose(); - _client.TrackException(exception); - _client.Flush(); + // A channel assigned to the sink is not owned by it. + _channel.Dispose(); } - public void Dispose() + public static TelemetryService Create(string connectionString, bool telemetryEnabled, string storageFolder) { - _configuration.Dispose(); + var channel = new ServerTelemetryChannel + { + // Remark: folder must exist beforehand as the channel does not auto-create it. + // If missing, local storage remains disabled and unsent telemetry is lost. + StorageFolder = storageFolder + }; + +#if DEBUG + channel.DeveloperMode = Debugger.IsAttached; +#endif + + // Avoid CreateDefault() to prevent registering default processors (e.g., adaptive sampling) + var configuration = new TelemetryConfiguration + { + ConnectionString = connectionString, + DisableTelemetry = !telemetryEnabled, + TelemetryChannel = channel + }; + + configuration.TelemetryInitializers.Add(new DefaultTelemetryInitializer()); + + // Build clean processor chain with DefaultTelemetryProcessor (disables sampling/filtering). + var processorChain = configuration.DefaultTelemetrySink.TelemetryProcessorChainBuilder; + processorChain.Use(next => new DefaultTelemetryProcessor(next)); + processorChain.Build(); + + channel.Initialize(configuration); + + return new TelemetryService(configuration); } } 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 cdb81d60..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,31 +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(); - throw; - } + app.Run(); } } diff --git a/src/Properties/launchSettings.json b/src/Properties/launchSettings.json index 28a50d37..708b4fb6 100644 --- a/src/Properties/launchSettings.json +++ b/src/Properties/launchSettings.json @@ -5,7 +5,10 @@ "commandLineArgs": "", "launchBrowser": false, "launchUrl": "swagger", - "applicationUrl": "http://localhost:5000" + "applicationUrl": "http://localhost:5000", + "environmentVariables": { + "BRAVO_CrashTest": "startup|ui|thread|unobserved" + } } } -} \ No newline at end of file +} diff --git a/src/Services/ManageDatesService.cs b/src/Services/ManageDatesService.cs index 8d62ba96..24260431 100644 --- a/src/Services/ManageDatesService.cs +++ b/src/Services/ManageDatesService.cs @@ -32,9 +32,9 @@ internal class ManageDatesService : IManageDatesService { private readonly DaxTemplateManager _templateManager; - public ManageDatesService(IPolicies policies) + public ManageDatesService(IPolicyService policyService) { - _templateManager = new DaxTemplateManager(policies); + _templateManager = new DaxTemplateManager(policyService); } public IEnumerable GetConfigurations(PBIDesktopReport report, CancellationToken cancellationToken) diff --git a/src/Services/TemplateDevelopmentService.cs b/src/Services/TemplateDevelopmentService.cs index 8c3f5da8..03680df5 100644 --- a/src/Services/TemplateDevelopmentService.cs +++ b/src/Services/TemplateDevelopmentService.cs @@ -52,13 +52,13 @@ internal class TemplateDevelopmentService : ITemplateDevelopmentService private readonly JsonSerializerOptions _serializerOptions; private readonly DaxTemplateManager _templateManager; private readonly IServerAddressProvider _serverAddressProvider; - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; - public TemplateDevelopmentService(IServerAddressProvider serverAddressProvider, IPolicies policies) + public TemplateDevelopmentService(IServerAddressProvider serverAddressProvider, IPolicyService policyService) { _serverAddressProvider = serverAddressProvider; - _policies = policies; - _templateManager = new DaxTemplateManager(policies); + _policyService = policyService; + _templateManager = new DaxTemplateManager(policyService); _serializerOptions = new JsonSerializerOptions(AppEnvironment.DefaultJsonOptions) { WriteIndented = true }; } @@ -163,7 +163,7 @@ public IEnumerable GetOrganizationCustomPackages() { var customPackages = new List(); - var repositoryPath = _policies.CustomTemplatesOrganizationRepositoryPath; + var repositoryPath = _policyService.Current.CustomTemplatesOrganizationRepositoryPath; if (repositoryPath is not null && Directory.Exists(repositoryPath)) { var packagePaths = Directory.EnumerateFiles(repositoryPath, searchPattern: $"*{CustomPackageFileExtension}", new EnumerationOptions diff --git a/src/packages.lock.json b/src/packages.lock.json index 7204d4ba..ac230315 100644 --- a/src/packages.lock.json +++ b/src/packages.lock.json @@ -85,6 +85,16 @@ "resolved": "2.23.0", "contentHash": "nWArUZTdU7iqZLycLKWe0TDms48KKGE6pONH2terYNa8REXiqixrMOkf1sk5DHGMaUTqONU2YkS4SAXBhLStgw==" }, + "Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel": { + "type": "Direct", + "requested": "[2.23.0, )", + "resolved": "2.23.0", + "contentHash": "798Dudr4tkujslk1w+XcXOcCErmVsk+nhp+QCHLa3lcgi25vkAxBmzPUeQlRJVCNL/1f4x/YF+vQZ8RSuTXWCw==", + "dependencies": { + "Microsoft.ApplicationInsights": "2.23.0", + "System.IO.FileSystem.AccessControl": "4.7.0" + } + }, "Microsoft.Identity.Client.Desktop": { "type": "Direct", "requested": "[4.73.1, )", @@ -242,6 +252,11 @@ "type": "Transitive", "resolved": "7.0.0", "contentHash": "rJJony+jsxvpfJM9ZGVxjp0DVpalZv8cAhiMSLW6L2hgUWb7k5qPVuzQHWXtkT8lrG1hQ8vWeR+HUwgCQm9J3A==" + }, + "System.IO.FileSystem.AccessControl": { + "type": "Transitive", + "resolved": "4.7.0", + "contentHash": "vMToiarpU81LR1/KZtnT7VDPvqAZfw9oOS5nY6pPP78nGYz3COLsQH3OfzbR+SjTgltd31R6KmKklz/zDpTmzw==" } }, "net10.0-windows7.0/win-x64": { @@ -270,6 +285,11 @@ "type": "Transitive", "resolved": "0.19.2", "contentHash": "qXNtYJxZ15yKV5pnI+V0sRzIH++vNzmMOU3K8kyNGsMD8/q7+ZejQjm7gJI9MMEGDPQkGg3QaiMvrIgbH2ym2g==" + }, + "System.IO.FileSystem.AccessControl": { + "type": "Transitive", + "resolved": "4.7.0", + "contentHash": "vMToiarpU81LR1/KZtnT7VDPvqAZfw9oOS5nY6pPP78nGYz3COLsQH3OfzbR+SjTgltd31R6KmKklz/zDpTmzw==" } } } 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 9cb61a5d..9f1bedbd 100644 --- a/test/Bravo.Tests/Host/HostFakes.cs +++ b/test/Bravo.Tests/Host/HostFakes.cs @@ -1,23 +1,20 @@ using System; -using System.Net; +using System.Collections.Generic; +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 { @@ -25,43 +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 void TrackException(Exception exception) + public IReadOnlyList Exceptions { + get { lock (_exceptions) return [.. _exceptions]; } } - public void Dispose() => Disposed = true; -} + public void TrackException(Exception exception, IDictionary? properties = null) + { + lock (_exceptions) _exceptions.Add(exception); + } -internal sealed class FakeWebProxy : IWebProxy -{ - public ICredentials? Credentials { get; set; } + public Task FlushAsync(CancellationToken cancellationToken = default) => Task.FromResult(false); - public Uri? GetProxy(Uri destination) => null; + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => false; - public bool IsBypassed(Uri host) => true; + public void Dispose() + { + DisposeCount++; + OnDispose?.Invoke(); + } } -internal static class FakeInitializationContext +internal sealed record FakeLogEntry(LogLevel Level, EventId EventId, Exception? Exception, string Message); + +internal sealed class FakeLoggerFactory : ILoggerFactory { - /// - /// Creates a whose entries are all fakes. - /// - public static BravoApplicationInitializationContext Create(FakeInstance? instance = null) + private readonly List _entries = []; + + public int DisposeCount { get; private set; } + + public IReadOnlyList Entries + { + get { lock (_entries) return [.. _entries]; } + } + + 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/DiagnosticMessageLoggerProviderTests.cs b/test/Bravo.Tests/Infrastructure/Diagnostics/DiagnosticMessageLoggerProviderTests.cs new file mode 100644 index 00000000..a339ef5d --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Diagnostics/DiagnosticMessageLoggerProviderTests.cs @@ -0,0 +1,56 @@ +using System; +using System.Linq; +using Microsoft.Extensions.Logging; +using Sqlbi.Bravo.Infrastructure; +using Sqlbi.Bravo.Infrastructure.Diagnostics; +using Sqlbi.Bravo.Models; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Diagnostics; + +public class DiagnosticMessageLoggerProviderTests +{ + [Theory] + [InlineData(LogLevel.Critical, DiagnosticMessageSeverity.Error)] + [InlineData(LogLevel.Error, DiagnosticMessageSeverity.Error)] + [InlineData(LogLevel.Warning, DiagnosticMessageSeverity.Warning)] + [InlineData(LogLevel.Information, DiagnosticMessageSeverity.None)] + public void Log_MapsTheLevelToTheMessageSeverity(LogLevel logLevel, DiagnosticMessageSeverity expected) + { + var category = $"{nameof(Log_MapsTheLevelToTheMessageSeverity)}.{logLevel}"; + using var provider = new DiagnosticMessageLoggerProvider(); + var logger = provider.CreateLogger(category); + + logger.Log(logLevel, default, "the message", null, (state, _) => state); + + var message = FindMessage(category); + Assert.Equal(expected, message.Severity); + Assert.Equal(DiagnosticMessageType.Text, message.Type); + Assert.Equal("the message", message.Content); + } + + [Fact] + public void Log_AppendsTheExceptionToTheContent() + { + var category = nameof(Log_AppendsTheExceptionToTheContent); + var exception = new InvalidOperationException("policies unavailable"); + using var provider = new DiagnosticMessageLoggerProvider(); + var logger = provider.CreateLogger(category); + + logger.LogError(exception, "the message"); + + var message = FindMessage(category); + Assert.StartsWith("the message", message.Content); + Assert.Contains("policies unavailable", message.Content); + } + + private static DiagnosticMessage FindMessage(string category) + { + var message = AppEnvironment.Diagnostics.Values.SingleOrDefault((m) => m.Name == $"[HOST] {category}"); + + Assert.NotNull(message); + Assert.NotNull(message.Content); + + return message; + } +} 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++; + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs b/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs new file mode 100644 index 00000000..344a9d8b --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs @@ -0,0 +1,45 @@ +using System; +using System.Collections.Generic; +using Sqlbi.Bravo.Infrastructure.Policies; + +namespace Bravo.Tests.Infrastructure.Policies; + +internal sealed class FakePolicySource : IPolicySource +{ + private readonly Dictionary _values = new(); + public int DisposeCount { get; private set; } + public Exception? ReadFailure { get; set; } + + public void Set(string name, object value) => _values[name] = value; + + public int? GetInt(string name) + { + if (ReadFailure is not null) + throw ReadFailure; + + return _values.TryGetValue(name, out var value) && value is int intValue ? intValue : null; + } + + public string? GetString(string name) + => _values.TryGetValue(name, out var value) ? value as string : null; + + public void Dispose() => DisposeCount++; +} + +internal sealed class FakePolicySourceFactory(Func open) : IPolicySourceFactory +{ + public IPolicySource Open(PolicyScope scope) => open(scope); +} + +internal sealed class FakePolicyReader : IPolicyReader +{ + public int ReadCount { get; private set; } + public PolicySnapshot Snapshot { get; set; } = PolicySnapshot.NotConfigured; + public Func? OnRead { get; set; } + + public PolicySnapshot Read() + { + ReadCount++; + return OnRead?.Invoke() ?? Snapshot; + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs b/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs new file mode 100644 index 00000000..b81f9345 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs @@ -0,0 +1,54 @@ +using System; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Sqlbi.Bravo.Infrastructure.Policies; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Policies; + +public class PolicyServiceTests +{ + [Fact] + public void Current_ReadsOnceAndKeepsTheSameSnapshot() + { + var initial = PolicySnapshot.NotConfigured with { TelemetryEnabled = false }; + var reader = new FakePolicyReader { Snapshot = initial }; + var service = new PolicyService(reader); + + Assert.Equal(0, reader.ReadCount); + Assert.Same(initial, service.Current); + + reader.Snapshot = PolicySnapshot.NotConfigured with { TelemetryEnabled = true }; + + Assert.Same(initial, service.Current); + Assert.False(service.Current.TelemetryEnabled); + Assert.Equal(1, reader.ReadCount); + } + + [Fact] + public async Task Current_ConcurrentAccessInitializesOnce() + { + var reader = new FakePolicyReader(); + var service = new PolicyService(reader); + + var snapshots = await Task.WhenAll( + Enumerable.Range(0, 16).Select(_ => Task.Run(() => service.Current))) + .WaitAsync(TimeSpan.FromSeconds(10)); + + Assert.All(snapshots, snapshot => Assert.Same(service.Current, snapshot)); + Assert.Equal(1, reader.ReadCount); + } + + [Fact] + public void Current_ReadFailurePropagatesWithoutRetryingOrReturningDefaults() + { + var failure = new IOException("Registry unavailable"); + var reader = new FakePolicyReader { OnRead = () => throw failure }; + var service = new PolicyService(reader); + + Assert.Same(failure, Assert.Throws(() => service.Current)); + Assert.Same(failure, Assert.Throws(() => service.Current)); + Assert.Equal(1, reader.ReadCount); + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs new file mode 100644 index 00000000..cf7e8cfd --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs @@ -0,0 +1,83 @@ +using System; +using System.Collections.Generic; +using System.IO; +using Sqlbi.Bravo.Infrastructure.Policies; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Policies; + +public class RegistryPolicyReaderLifetimeTests +{ + [Fact] + public void Read_CombinesBothScopesAndDisposesTheirSources() + { + var computer = new FakePolicySource(); + computer.Set("TelemetryEnabled", 0); + computer.Set("UpdateCheckEnabled", "invalid"); + var user = new FakePolicySource(); + user.Set("TelemetryEnabled", 1); + user.Set("UpdateCheckEnabled", 1); + var scopes = new List(); + var factory = new FakePolicySourceFactory(scope => + { + scopes.Add(scope); + return scope == PolicyScope.Computer ? computer : user; + }); + var reader = new RegistryPolicyReader(factory); + + var policies = reader.Read(); + + Assert.False(policies.TelemetryEnabled); + Assert.True(policies.UpdateCheckEnabled); + Assert.Equal([PolicyScope.Computer, PolicyScope.User], scopes); + Assert.All(new[] { computer, user }, source => Assert.Equal(1, source.DisposeCount)); + } + + [Fact] + public void Read_UserKeyOpenFailureDisposesComputerSourceAndPropagates() + { + var computer = new FakePolicySource(); + var failure = new UnauthorizedAccessException(); + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + scope == PolicyScope.Computer ? computer : throw failure)); + + Assert.Same(failure, Assert.Throws(() => reader.Read())); + Assert.Equal(1, computer.DisposeCount); + } + + [Fact] + public void Read_ValueReadFailureDisposesEveryOpenedSourceAndPropagates() + { + var computer = new FakePolicySource(); + var failure = new IOException(); + var user = new FakePolicySource { ReadFailure = failure }; + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + scope == PolicyScope.Computer ? computer : user)); + + Assert.Same(failure, Assert.Throws(() => reader.Read())); + Assert.Equal(1, computer.DisposeCount); + Assert.Equal(1, user.DisposeCount); + } + + [Fact] + public void Read_ComputerOpenFailureDoesNotFallBackToUser() + { + var scopes = new List(); + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + { + scopes.Add(scope); + throw new UnauthorizedAccessException(); + })); + + Assert.Throws(() => reader.Read()); + Assert.Equal([PolicyScope.Computer], scopes); + } + + [Fact] + public void Read_MissingKeysAreAnEmptySnapshot() + { + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(_ => new RegistryPolicySource(null))); + + Assert.Equal(PolicySnapshot.NotConfigured, reader.Read()); + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs similarity index 65% rename from test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs rename to test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs index 507d42c1..b85e9941 100644 --- a/test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs @@ -5,46 +5,24 @@ namespace Bravo.Tests.Infrastructure.Policies; -/// -/// Exercises PoliciesFactory's parsing/precedence logic using an in-memory fake instead of the -/// real registry. Registry-adapter behavior itself is covered separately by -/// . -/// -public class PoliciesFactoryTests +public class RegistryPolicyReaderTests { - private sealed class FakePolicySource : IPolicySource - { - private readonly Dictionary _values = new(); - - public void Set(string name, object value) => _values[name] = value; - - public int? GetInt(string name) - => _values.TryGetValue(name, out var value) && value is int intValue ? intValue : null; - - public string? GetString(string name) - => _values.TryGetValue(name, out var value) ? value as string : null; - } - - // MemberData deliberately carries only the (public) property name rather than a - // Func selector: Policies/IPolicies are internal, and a public test - // method cannot declare a parameter of a less-accessible type (CS0051), even with - // InternalsVisibleTo. The property is read via reflection inside the test body instead. public static IEnumerable BoolPolicyNames() => new[] { - new object[] { nameof(IPolicies.TelemetryEnabled) }, - new object[] { nameof(IPolicies.UpdateCheckEnabled) }, - new object[] { nameof(IPolicies.UseSystemBrowserForAuthentication) }, - new object[] { nameof(IPolicies.BuiltInTemplatesEnabled) }, - new object[] { nameof(IPolicies.CustomTemplatesEnabled) }, + new object[] { nameof(PolicySnapshot.TelemetryEnabled) }, + new object[] { nameof(PolicySnapshot.UpdateCheckEnabled) }, + new object[] { nameof(PolicySnapshot.UseSystemBrowserForAuthentication) }, + new object[] { nameof(PolicySnapshot.BuiltInTemplatesEnabled) }, + new object[] { nameof(PolicySnapshot.CustomTemplatesEnabled) }, }; - private static bool? GetBoolProperty(Sqlbi.Bravo.Infrastructure.Policies.Policies policies, string propertyName) - => (bool?)typeof(Sqlbi.Bravo.Infrastructure.Policies.Policies).GetProperty(propertyName)!.GetValue(policies); + private static bool? GetBoolProperty(PolicySnapshot policies, string propertyName) + => (bool?)typeof(PolicySnapshot).GetProperty(propertyName)!.GetValue(policies); [Fact] public void FromSource_EmptySource_AllPropertiesAreNull() { - var policies = PoliciesFactory.FromSource(new FakePolicySource()); + var policies = RegistryPolicyReader.FromSource(new FakePolicySource()); Assert.Null(policies.TelemetryEnabled); Assert.Null(policies.UpdateChannel); @@ -62,7 +40,7 @@ public void FromSource_BoolPolicyValueOne_ReturnsTrue(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, 1); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.True(GetBoolProperty(policies, propertyName)); } @@ -74,7 +52,7 @@ public void FromSource_BoolPolicyValueZero_ReturnsFalse(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, 0); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.False(GetBoolProperty(policies, propertyName)); } @@ -86,7 +64,7 @@ public void FromSource_BoolPolicyOutOfRangeIntValue_ReturnsNull(string propertyN var source = new FakePolicySource(); source.Set(propertyName, 42); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(GetBoolProperty(policies, propertyName)); } @@ -98,7 +76,7 @@ public void FromSource_BoolPolicyNonIntValue_ReturnsNull(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, "not-a-number"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(GetBoolProperty(policies, propertyName)); } @@ -109,7 +87,7 @@ public void FromSource_UpdateChannelDefinedEnumValue_ReturnsParsedValue() var source = new FakePolicySource(); source.Set("UpdateChannel", (int)UpdateChannelType.Dev); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Equal(UpdateChannelType.Dev, policies.UpdateChannel); } @@ -121,7 +99,7 @@ public void FromSource_UpdateChannelUndefinedEnumValue_ReturnsNull() var source = new FakePolicySource(); source.Set("UpdateChannel", 1); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.UpdateChannel); } @@ -132,7 +110,7 @@ public void FromSource_UpdateChannelNonIntValue_ReturnsNull() var source = new FakePolicySource(); source.Set("UpdateChannel", "Dev"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.UpdateChannel); } @@ -143,7 +121,7 @@ public void FromSource_CustomTemplatesOrganizationRepositoryPathStringValueSet_R var source = new FakePolicySource(); source.Set("CustomTemplatesOrganizationRepositoryPath", @"C:\Templates\Org"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Equal(@"C:\Templates\Org", policies.CustomTemplatesOrganizationRepositoryPath); } @@ -154,24 +132,12 @@ public void FromSource_CustomTemplatesOrganizationRepositoryPathNonStringValue_R var source = new FakePolicySource(); source.Set("CustomTemplatesOrganizationRepositoryPath", 123); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.CustomTemplatesOrganizationRepositoryPath); } - [Fact] - public void Create_DoesNotThrow_AndReturnsAnInstance() - { - // Create() reads from the real HKLM/HKCU policy path, so its output depends on the - // machine's actual group-policy configuration and cannot be asserted deterministically here. - // This is a smoke test for the composition wiring (LocalMachine + CurrentUser -> Merge); - // the precedence rule itself is covered deterministically by the Merge tests below. - var policies = PoliciesFactory.Create(); - - Assert.NotNull(policies); - } - - private static readonly Sqlbi.Bravo.Infrastructure.Policies.Policies AllNull = new( + private static readonly PolicySnapshot AllNull = new( TelemetryEnabled: null, UpdateChannel: null, UpdateCheckEnabled: null, @@ -186,7 +152,7 @@ public void Merge_OnlyMachineValueSet_ReturnsMachineValue() var machine = AllNull with { TelemetryEnabled = true }; var user = AllNull; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); } @@ -197,7 +163,7 @@ public void Merge_OnlyUserValueSet_ReturnsUserValue() var machine = AllNull; var user = AllNull with { TelemetryEnabled = false }; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.False(merged.TelemetryEnabled); } @@ -208,7 +174,7 @@ public void Merge_BothSet_MachineTakesPrecedenceOverUser() var machine = AllNull with { TelemetryEnabled = true, UpdateChannel = UpdateChannelType.Stable, UpdateCheckEnabled = false }; var user = AllNull with { TelemetryEnabled = false, UpdateChannel = UpdateChannelType.Dev, UpdateCheckEnabled = true }; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); Assert.Equal(UpdateChannelType.Stable, merged.UpdateChannel); @@ -218,7 +184,7 @@ public void Merge_BothSet_MachineTakesPrecedenceOverUser() [Fact] public void Merge_NeitherSet_ReturnsNull() { - var merged = PoliciesFactory.Merge(AllNull, AllNull); + var merged = RegistryPolicyReader.Merge(AllNull, AllNull); Assert.Null(merged.TelemetryEnabled); } @@ -229,7 +195,7 @@ public void Merge_EachPropertyResolvedIndependently() // Guards against a copy-paste wiring mistake in Merge() (e.g. reading the wrong // property from machine/user) by exercising all 7 properties in a single assertion, // each with a distinct machine/user combination. - var machine = new Sqlbi.Bravo.Infrastructure.Policies.Policies( + var machine = new PolicySnapshot( TelemetryEnabled: true, UpdateChannel: null, UpdateCheckEnabled: null, @@ -238,7 +204,7 @@ public void Merge_EachPropertyResolvedIndependently() CustomTemplatesEnabled: null, CustomTemplatesOrganizationRepositoryPath: null); - var user = new Sqlbi.Bravo.Infrastructure.Policies.Policies( + var user = new PolicySnapshot( TelemetryEnabled: false, // machine wins UpdateChannel: UpdateChannelType.Dev, // machine unset -> user wins UpdateCheckEnabled: true, // machine unset -> user wins @@ -247,7 +213,7 @@ public void Merge_EachPropertyResolvedIndependently() CustomTemplatesEnabled: null, // neither set CustomTemplatesOrganizationRepositoryPath: @"C:\User\Path"); // machine unset -> user wins - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); Assert.Equal(UpdateChannelType.Dev, merged.UpdateChannel); diff --git a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs index 68261a3e..dee23141 100644 --- a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs @@ -6,10 +6,7 @@ namespace Bravo.Tests.Infrastructure.Policies; /// -/// Covers only the thin RegistryKey-to-IPolicySource bridging contract (missing key, missing -/// value, type mismatches). Uses a real, isolated key under HKEY_CURRENT_USER (writable without -/// admin rights) since RegistryKey is a sealed BCL type that cannot be faked. The -/// parsing/precedence logic that matters is covered by PoliciesTests against a fake source instead. +/// Verifies registry value types and handle ownership with an isolated key under HKEY_CURRENT_USER. /// public class RegistryPolicySourceTests : IDisposable { @@ -30,7 +27,7 @@ public void Dispose() [Fact] public void GetInt_NullKey_ReturnsNull() { - var source = new RegistryPolicySource(key: null); + using var source = new RegistryPolicySource(key: null); Assert.Null(source.GetInt("AnyValue")); } @@ -38,7 +35,7 @@ public void GetInt_NullKey_ReturnsNull() [Fact] public void GetInt_ValueNotSet_ReturnsNull() { - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetInt("Missing")); } @@ -47,7 +44,7 @@ public void GetInt_ValueNotSet_ReturnsNull() public void GetInt_DWordValue_ReturnsInt() { _testKey.SetValue("TelemetryEnabled", 1, RegistryValueKind.DWord); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Equal(1, source.GetInt("TelemetryEnabled")); } @@ -56,7 +53,7 @@ public void GetInt_DWordValue_ReturnsInt() public void GetInt_StringValue_ReturnsNull() { _testKey.SetValue("TelemetryEnabled", "1", RegistryValueKind.String); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetInt("TelemetryEnabled")); } @@ -64,7 +61,7 @@ public void GetInt_StringValue_ReturnsNull() [Fact] public void GetString_NullKey_ReturnsNull() { - var source = new RegistryPolicySource(key: null); + using var source = new RegistryPolicySource(key: null); Assert.Null(source.GetString("AnyValue")); } @@ -73,7 +70,7 @@ public void GetString_NullKey_ReturnsNull() public void GetString_StringValue_ReturnsString() { _testKey.SetValue("CustomTemplatesOrganizationRepositoryPath", @"C:\Templates\Org", RegistryValueKind.String); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Equal(@"C:\Templates\Org", source.GetString("CustomTemplatesOrganizationRepositoryPath")); } @@ -82,8 +79,18 @@ public void GetString_StringValue_ReturnsString() public void GetString_DWordValue_ReturnsNull() { _testKey.SetValue("CustomTemplatesOrganizationRepositoryPath", 123, RegistryValueKind.DWord); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetString("CustomTemplatesOrganizationRepositoryPath")); } + + [Fact] + public void Dispose_ClosesTheOwnedRegistryKey() + { + var source = new RegistryPolicySource(_testKey); + + source.Dispose(); + + Assert.Throws(() => _testKey.GetValue("AnyValue")); + } } diff --git a/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs index 0c85762c..b1f5389a 100644 --- a/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs +++ b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs @@ -1,4 +1,5 @@ using System; +using System.IO; using System.IO.Pipes; using System.Text; using System.Threading; @@ -159,14 +160,20 @@ public void Activated_ClientConnectsButNeverWrites_RecoversAndKeepsListening() using (server) { using var errorRaised = new ManualResetEventSlim(); - server.Error += (_, _) => errorRaised.Set(); + Exception? error = null; + server.Error += (_, e) => + { + error = e.GetException(); + errorRaised.Set(); + }; using (var stalledClient = new NamedPipeClientStream( ".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) { stalledClient.Connect((int)s_timeout.TotalMilliseconds); - // Connected, but deliberately never writes: stands in for a stuck or hostile client. + // Connected, but never writes: stands in for a stuck or hostile client. Assert.True(errorRaised.Wait(s_timeout)); + Assert.IsType(error); } // The stalled connection released the pipe: a real client is served next. @@ -220,18 +227,26 @@ public void Activated_PayloadExceedsTheLimit_IsRejectedAndTheOwnerKeepsListening using var received = new ManualResetEventSlim(); using var rejected = new ManualResetEventSlim(); byte[]? payload = null; + Exception? error = null; server.Activated += (_, e) => { payload = e.Payload; received.Set(); }; - // An oversized payload is a reported rejection, not a silent drop: without this the owner - // would discard it with nothing recorded anywhere. - server.Error += (_, _) => rejected.Set(); + // An oversized payload is a reported rejection, not a silent drop. + server.Error += (_, e) => + { + error = e.GetException(); + rejected.Set(); + }; - Assert.True(SingleInstanceClient.Send(clientOptions, Payload(new string('x', 256))).IsDelivered); + // The owner stops reading past the limit and disconnects, so the client's write of the + // remaining bytes fails: a client with a larger limit learns that its payload was refused. + var result = SingleInstanceClient.Send(clientOptions, Payload(new string('x', 256))); + Assert.Equal(SingleInstanceSendStatus.Failed, result.Status); Assert.True(rejected.Wait(s_timeout)); + Assert.IsType(error); Assert.False(received.Wait(TimeSpan.FromSeconds(1))); Assert.True(SingleInstanceClient.Send(clientOptions, Payload("small")).IsDelivered); @@ -248,6 +263,53 @@ public void Send_PayloadExceedsTheConfiguredLimit_Throws() Assert.Throws(() => SingleInstanceClient.Send(options, Payload("far too long"))); } + [Fact] + public void Send_EmptyPayload_Throws() + { + var options = CreateOptions(); + + Assert.Throws(() => SingleInstanceClient.Send(options, Payload(""))); + } + + /// + /// A client that connects and closes without writing is reported, like an oversized payload, + /// and the owner keeps listening. + /// + [Fact] + public void Activated_ClientClosesWithoutWriting_IsReportedAndTheOwnerKeepsListening() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var errorRaised = new ManualResetEventSlim(); + Exception? error = null; + server.Error += (_, e) => + { + error = e.GetException(); + errorRaised.Set(); + }; + + using (var silentClient = new NamedPipeClientStream( + ".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + { + silentClient.Connect((int)s_timeout.TotalMilliseconds); + } + + Assert.True(errorRaised.Wait(s_timeout)); + // Empty payload when the owner accepted before the close; otherwise the accept itself + // fails with ERROR_NO_DATA. + Assert.True(error is InvalidDataException or IOException, error?.ToString()); + + using var received = new ManualResetEventSlim(); + server.Activated += (_, _) => received.Set(); + + Assert.True(SingleInstanceClient.Send(options, Payload("hello")).IsDelivered); + Assert.True(received.Wait(s_timeout)); + } + } + [Fact] public void Dispose_CalledTwice_DoesNotThrow() { diff --git a/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs new file mode 100644 index 00000000..8a638132 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs @@ -0,0 +1,39 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Telemetry; + +public class TelemetryConsentTests +{ + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Resolve_WithoutPolicy_UsesTheUserSetting(bool settingValue) + { + var policies = PolicySnapshot.NotConfigured; + var settings = new UserSettings { TelemetryEnabled = settingValue }; + + var consent = TelemetryConsent.Resolve(policies, settings); + + Assert.Equal(settingValue, consent.IsEnabled); + Assert.Equal(TelemetryConsentSource.UserSettings, consent.Source); + } + + [Theory] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(true, true)] + [InlineData(false, false)] + public void Resolve_WithPolicy_OverridesTheUserSetting(bool policyValue, bool settingValue) + { + var policies = PolicySnapshot.NotConfigured with { TelemetryEnabled = policyValue }; + var settings = new UserSettings { TelemetryEnabled = settingValue }; + + var consent = TelemetryConsent.Resolve(policies, settings); + + Assert.Equal(policyValue, consent.IsEnabled); + Assert.Equal(TelemetryConsentSource.Policy, consent.Source); + } +} \ No newline at end of file diff --git a/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs new file mode 100644 index 00000000..aa5a59b9 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs @@ -0,0 +1,100 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Net; +using System.Threading; +using System.Threading.Tasks; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Telemetry; + +/// +/// Exercises the real channel against a local ingestion endpoint: the behaviour worth protecting is +/// what reaches the wire before the process exits, and it only exists in the channel itself. +/// +public class TelemetryServiceTests +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(10); + + private static string CreateConnectionString(int port) + => $"InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=http://127.0.0.1:{port}/"; + + private static int GetFreePort() + { + using var socket = new System.Net.Sockets.TcpListener(IPAddress.Loopback, port: 0); + socket.Start(); + return ((IPEndPoint)socket.LocalEndpoint).Port; + } + + /// + /// Regression: Dispose used the synchronous Flush, which hands the buffer to the sender and + /// returns before the request is sent, so the last exceptions of a session never left the process. + /// + [Fact] + public void Dispose_TrackedExceptionsAreSentBeforeReturning() + { + var port = GetFreePort(); + using var listener = new HttpListener(); + listener.Prefixes.Add($"http://127.0.0.1:{port}/"); + listener.Start(); + + using var received = new ManualResetEventSlim(); + string? path = null; + _ = Task.Run(() => + { + var context = listener.GetContext(); + path = context.Request.Url?.AbsolutePath; + // Set before the response is sent: Dispose can only return after the response. + received.Set(); + context.Response.StatusCode = 200; + context.Response.Close(); + }); + + using var storage = new TemporaryFolder(); + var service = TelemetryService.Create(CreateConnectionString(port), telemetryEnabled: true, storage.Path); + service.TrackException(new InvalidOperationException("tracked before exit")); + + service.Dispose(); + + // Checked without waiting: the process exits right after Dispose, so a request that is + // still in flight at this point is lost. + Assert.True(received.IsSet, "The request had not reached the endpoint when Dispose returned."); + Assert.Contains("/track", path); + } + + /// + /// With no endpoint the flush must fail fast and store the transmission for the next start, + /// instead of holding the shutdown for the whole TCP timeout. + /// + [Fact] + public void Dispose_EndpointUnreachable_StoresTheTransmissionAndReturnsWithinTheBound() + { + using var storage = new TemporaryFolder(); + var service = TelemetryService.Create(CreateConnectionString(GetFreePort()), telemetryEnabled: true, storage.Path); + service.TrackException(new InvalidOperationException("tracked while offline")); + + var stopwatch = Stopwatch.StartNew(); + service.Dispose(); + + Assert.True(stopwatch.Elapsed < s_timeout, $"Dispose took {stopwatch.Elapsed}"); + Assert.NotEmpty(Directory.GetFiles(storage.Path, "*.trn")); + } + + private sealed class TemporaryFolder : IDisposable + { + public string Path { get; } = Directory.CreateDirectory(System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"Bravo.Tests.{Guid.NewGuid():N}")).FullName; + + public void Dispose() + { + try + { + Directory.Delete(Path, recursive: true); + } + catch (IOException) + { + // A file still held by the channel is left behind. + } + } + } +}