From 67db04eb0e75f29e3d914255840fd7889130126c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 09:20:38 -0400 Subject: [PATCH 1/5] Pin every STORE connection's session time zone to UTC (#4277) Defence in depth alongside the v9 managed-runtime conf block: a bring-your-own store, or a managed store before its conf takes effect, keeps whatever zone its owner or initdb set, and every store timestamp column is naive UTC. DarlingStoreConnection.PinSessionTimeZoneUtc is the one helper every STORE connection-string resolution now routes through (25 call sites across the worker, MCP/web hosts, CLI verbs, managed-runtime bootstrap/upgrade, store logins, the store connection self-test, and the viewer). Monitored-target connections are untouched. Committed ahead of the dev-as-is diagnosis run so the run can be done against a clean pre-pin tree; the diagnosis run itself reverts these 9 files temporarily. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../DarlingCliCommands.cs | 18 +++---- .../DarlingManagedPostgres.cs | 6 +-- .../DarlingStoreLogins.cs | 4 +- .../DarlingStoreUpgrade.cs | 11 +++-- .../DarlingWorker.cs | 6 ++- .../Mcp/DarlingMcpHostService.cs | 3 +- .../Mcp/DarlingWebHostService.cs | 3 +- .../DarlingStoreConnection.cs | 49 +++++++++++++++++++ .../StoreConnectionSelfTest.cs | 4 +- .../ViewerDataService.cs | 2 +- 10 files changed, 81 insertions(+), 25 deletions(-) create mode 100644 Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index 2efd6b8d3c..51b3697dbf 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -368,7 +368,7 @@ private static async Task> ResolveValidationTarge return config.Servers; } - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); @@ -474,7 +474,7 @@ public static async Task CheckSettingsAsync( return CheckSettingsExitCode.StoreUnreachable; } - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); @@ -3004,7 +3004,7 @@ since a RETURNING that yields no row is the 0-rows-updated case. Note what this object? returnedPort; try { - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); await connection.OpenAsync(cancellationToken); await using var command = new NpgsqlCommand(sql, connection) { CommandTimeout = ServiceCommandDeadlines.CliStoreReadSeconds }; returnedPort = await command.ExecuteScalarAsync(cancellationToken); @@ -4170,7 +4170,7 @@ which sweeps every port and needs no port at all. */ try { - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); await connection.OpenAsync(budget.Token); await using var command = new NpgsqlCommand(ReadEndpointTogglesSql, connection) { @@ -4314,7 +4314,7 @@ public static async Task CollapseLegacySlicesAsync( return 1; } - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); @@ -4699,7 +4699,7 @@ public static async Task RecompressPlanDimAsync( return 1; } - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); @@ -5151,7 +5151,7 @@ call has to sit where Windows is provable rather than where it merely happens to string resultJson; try { - await using var dataSource = NpgsqlDataSource.Create(connectionString); + await using var dataSource = NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); resultJson = await DarlingMcpServerAdminTools.AddServers(dataSource, json); } catch (Exception ex) when (ex is not OperationCanceledException) @@ -5570,7 +5570,7 @@ bool is not something the platform analyzer can correlate with an earlier OS gua output.WriteLine($"PerformanceMonitor Darling — {(enable ? "enable" : "disable")} a collector ({verb})"); output.WriteLine(); - await using var dataSource = NpgsqlDataSource.Create(connectionString); + await using var dataSource = NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); int? serverId = null; var scopeLabel = "fleet-wide"; @@ -5719,7 +5719,7 @@ public static async Task BackfillRollupsAsync( return 1; } - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs index c8837f8272..2a75c8a8e7 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs @@ -3737,7 +3737,7 @@ connector is dead. */ private async Task EnsureDatabaseOnceAsync(string connectionString, CancellationToken cancellationToken) { var builder = new NpgsqlConnectionStringBuilder(connectionString) { Database = "postgres" }; - await using var connection = new NpgsqlConnection(builder.ConnectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(builder.ConnectionString)); await connection.OpenAsync(cancellationToken); using (var exists = new NpgsqlCommand($"SELECT 1 FROM pg_database WHERE datname = '{DatabaseName}'", connection) { CommandTimeout = ServiceCommandDeadlines.BootstrapConnectProbeSeconds }) @@ -4410,7 +4410,7 @@ private async Task VerifyPgHbaAsync(string ownerConnectionString, NetworkPlan pl var exposed = plan.Mode == NetworkMode.Exposed; try { - await using var connection = new NpgsqlConnection(ownerConnectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(ownerConnectionString)); await connection.OpenAsync(cancellationToken); await using (var errors = new NpgsqlCommand( @@ -4502,7 +4502,7 @@ private async Task GuardAdoptedListenAsync(string ownerConnectionString, Network { try { - await using var connection = new NpgsqlConnection(ownerConnectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(ownerConnectionString)); await connection.OpenAsync(cancellationToken); await using var command = new NpgsqlCommand("SHOW listen_addresses", connection) { CommandTimeout = ServiceCommandDeadlines.BootstrapSeconds }; var liveListen = await command.ExecuteScalarAsync(cancellationToken) as string ?? string.Empty; diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreLogins.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreLogins.cs index 208c7a6ab5..aea4284f51 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreLogins.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreLogins.cs @@ -13,6 +13,7 @@ using Microsoft.Extensions.Logging; using Npgsql; using PerformanceMonitor.Darling.Service.Hosting; +using PerformanceMonitor.Darling.Storage; namespace PerformanceMonitor.Darling.Service; @@ -457,7 +458,8 @@ private static async Task AcceptsLoginAsync( try { await using var connection = new NpgsqlConnection( - new NpgsqlConnectionStringBuilder(connectionString) { Pooling = false }.ConnectionString); + DarlingStoreConnection.PinSessionTimeZoneUtc( + new NpgsqlConnectionStringBuilder(connectionString) { Pooling = false }.ConnectionString)); await connection.OpenAsync(cancellationToken); return true; } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs index f4ec8ed2b2..3e9e28cff4 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs @@ -20,6 +20,7 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Npgsql; +using PerformanceMonitor.Darling.Storage; namespace PerformanceMonitor.Darling.Service; @@ -1492,7 +1493,7 @@ internal static async Task UpdateTimescaleAsFirstStatementAsync( SearchPath = null, }.ConnectionString; - var sourceBuilder = new NpgsqlDataSourceBuilder(connectionString); + var sourceBuilder = new NpgsqlDataSourceBuilder(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); sourceBuilder.ConfigureTypeLoading(typeLoading => typeLoading.EnableTypeLoading(false)); await using var source = sourceBuilder.Build(); await using var connection = await WithTransportRetryAsync( @@ -1555,7 +1556,7 @@ private static Task OpenWithTransportRetryAsync(string connect => WithTransportRetryAsync( async () => { - var connection = new NpgsqlConnection(connectionString); + var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); try { await connection.OpenAsync(cancellationToken); @@ -3229,7 +3230,7 @@ private async Task StopClusterAsync(string binDirectory, string dataDirectory, C internal static async Task ReadClusterIdentityAsync(string ownerConnectionString, CancellationToken cancellationToken) { var builder = new NpgsqlConnectionStringBuilder(ownerConnectionString) { Database = "postgres", Pooling = false }; - await using var connection = new NpgsqlConnection(builder.ConnectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(builder.ConnectionString)); await connection.OpenAsync(cancellationToken); await using var command = new NpgsqlCommand( @@ -3283,7 +3284,7 @@ FROM pg_database AS d var databases = new List(); var listBuilder = new NpgsqlConnectionStringBuilder(ownerConnection) { Database = "postgres", Pooling = false }; - await using (var connection = new NpgsqlConnection(listBuilder.ConnectionString)) + await using (var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(listBuilder.ConnectionString))) { await connection.OpenAsync(cancellationToken); await using var command = new NpgsqlCommand( @@ -3479,7 +3480,7 @@ internal async Task CompleteAfterStartAsync( try { var builder = new NpgsqlConnectionStringBuilder(ownerConnectionString) { Pooling = false }; - await using var connection = new NpgsqlConnection(builder.ConnectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(builder.ConnectionString)); await connection.OpenAsync(cancellationToken); await using (var version = new NpgsqlCommand("SHOW server_version_num", connection) { CommandTimeout = ServiceCommandDeadlines.BootstrapSeconds }) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index c0718b43c6..3184f4f328 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -1738,7 +1738,7 @@ and independent of the pool's connection-open timing relative to PgMigrations' store silently collects nothing until the service is restarted; see EnsureStoreSearchPath for the pool-timing root cause. Managed mode already sets it, so this is a no-op there. */ - storeConnectionString = EnsureStoreSearchPath(storeConnectionString); + storeConnectionString = DarlingStoreConnection.PinSessionTimeZoneUtc(EnsureStoreSearchPath(storeConnectionString)); await using var postgres = NpgsqlDataSource.Create(storeConnectionString); _postgres = postgres; /* #2936: a failure here is triaged rather than uniformly terminal. A store that is unreachable for @@ -2222,7 +2222,9 @@ viewer credential (written by EnsureProvisionedAsync above), or the Linux compos } await using var customAlertViewerSource = - customAlertViewerConnString is not null ? NpgsqlDataSource.Create(customAlertViewerConnString) : null; + customAlertViewerConnString is not null + ? NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(customAlertViewerConnString)) + : null; if (customAlertViewerSource is not null) { _customAlertEvaluator = new CustomAlertEvaluator( diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs index 097c9e328c..a7fe692aa8 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs @@ -22,6 +22,7 @@ using PerformanceMonitor.Common; using PerformanceMonitor.Darling.Analysis; using PerformanceMonitor.Darling.Service.Hosting; +using PerformanceMonitor.Darling.Storage; namespace PerformanceMonitor.Darling.Service.Mcp; @@ -403,7 +404,7 @@ await CheckMcpFirewallAsync( /* Lifetime tied to the running app (#1560): disposed by StopServerAsync, not this method's scope — the supervisor may keep the app running across many poll ticks. */ - var postgres = NpgsqlDataSource.Create(storeConnectionString); + var postgres = NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(storeConnectionString)); _appDataSource = postgres; /* serverId → connection string, keyed by the STORE's identity (review catch on #2218). diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs index 1ce3892d93..713a9c9e6b 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs @@ -29,6 +29,7 @@ using PerformanceMonitor.Common; using PerformanceMonitor.Darling.Analysis; using PerformanceMonitor.Darling.Service.Hosting; +using PerformanceMonitor.Darling.Storage; using PerformanceMonitor.Notifications; namespace PerformanceMonitor.Darling.Service.Mcp; @@ -694,7 +695,7 @@ await CheckWebFirewallAsync( } /* Lifetime tied to the running app: disposed by StopServerAsync, not this method's scope. */ - var postgres = NpgsqlDataSource.Create(storeConnectionString); + var postgres = NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(storeConnectionString)); _appDataSource = postgres; /* FOOTGUN (load-bearing): pin BOTH the content root AND the web root to the binary's directory. A diff --git a/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs b/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs new file mode 100644 index 0000000000..cc14c03da0 --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using Npgsql; + +namespace PerformanceMonitor.Darling.Storage; + +/// +/// #4277: pins every STORE session's timezone to UTC, client-side — defence in depth alongside the +/// managed runtime's own v9 postgresql.conf block (DarlingManagedPostgres.BuildTimeZoneConfAppend), +/// which only reaches a MANAGED store. A bring-your-own store keeps whatever zone its owner set, or whatever +/// initdb took from its host, and every timestamp column in both stores is timestamp without time zone +/// holding naive UTC. A naive-to-timestamptz comparison — a bare now() in the SQL text, or a +/// bound DateTime with Kind=Utc — is resolved by PostgreSQL at the SESSION's TimeZone, so a +/// non-UTC store session silently shifts those reads (measured on #4277: the same one-hour predicate returned +/// 66 rows under UTC and 343 under America/New_York — five hours of data under a one-hour window). +/// +/// Every STORE call site — the worker's own store connection, the MCP/web host connections, the CLI +/// verbs, the managed-runtime bootstrap/upgrade, the viewer, the store connection self-test — routes its +/// resolved connection string through this ONE helper, right before the connection or data source is created. +/// A MONITORED target's connection string never does: its own session zone is read on purpose (see +/// PgTargetBaselineProvider.Clock.cs, which resolves a target's local clock from its collected +/// pg_server_config snapshot, not from a live session), and pinning it there would make that resolver's +/// own read wrong. +/// +/// Overwrites a caller-supplied Timezone rather than leaving it — unlike +/// DarlingWorker.EnsureStoreSearchPath's set-if-absent for SearchPath, and unlike the viewer's +/// connect Timeout, where an operator's explicit value wins. The store's columns are naive UTC +/// regardless of what a caller's connection string says, so there is no reading of "the caller meant it"; +/// a store connection string that names another zone is always a mistake this pin corrects. +/// +public static class DarlingStoreConnection +{ + /// The pin itself: Npgsql's Timezone connection-string keyword, forced to UTC. + public static string PinSessionTimeZoneUtc(string connectionString) + { + var builder = new NpgsqlConnectionStringBuilder(connectionString) + { + Timezone = "UTC", + }; + + return builder.ConnectionString; + } +} diff --git a/Darling/PerformanceMonitor.Darling.Storage/StoreConnectionSelfTest.cs b/Darling/PerformanceMonitor.Darling.Storage/StoreConnectionSelfTest.cs index 952f5e2423..4e4705ff47 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/StoreConnectionSelfTest.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/StoreConnectionSelfTest.cs @@ -203,7 +203,7 @@ public static async Task> RunAsync( NpgsqlConnection? connection = null; try { - connection = new NpgsqlConnection(connectionString); + connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); await connection.OpenAsync(cancellationToken); results.Add(TlsPassResult(builder, sslDisabled, stopwatch.Elapsed)); @@ -303,7 +303,7 @@ private static async Task ProbeStoreShapeAsync( { try { - await using var connection = new NpgsqlConnection(connectionString); + await using var connection = new NpgsqlConnection(DarlingStoreConnection.PinSessionTimeZoneUtc(connectionString)); await connection.OpenAsync(cancellationToken); /* Both tables, both historical homes (V8 moved the store into collect; pre-V8 lived in diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 8352432679..3456bc72d3 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -459,7 +459,7 @@ public ViewerDataService(string connectionString, int? connectionTimeoutSeconds from disagreeing. */ ViewerStorePool.Publish(effectiveConnectionString); - _dataSource = NpgsqlDataSource.Create(effectiveConnectionString); + _dataSource = NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(effectiveConnectionString)); StoreIsOnThisMachine = StoreHostIsLoopback(connectionString); } From d5444ff210c0f897bc9398dfe43832e863d10eb5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 09:52:08 -0400 Subject: [PATCH 2/5] #4277: census-pin every STORE connection string, and no MONITORED-target one StoreSessionTimeZonePinCensusTests source-scans the four ways this codebase opens an Npgsql connection (NpgsqlDataSource.Create(, new NpgsqlDataSourceBuilder(, new NpgsqlConnection( and the fully qualified new Npgsql.NpgsqlConnection() across the storage, service, viewer and analysis projects. EveryStoreConnectionStringSourcePinsItsSessionTimeZoneToUtc asserts every call site not on the named MonitoredTargetSites allow-list routes its connection string through DarlingStoreConnection.PinSessionTimeZoneUtc, directly or indirectly (one site reassigns storeConnectionString through the pin two lines above, so the scan follows that identifier to its assignment rather than trusting it blind). Proven once by reverting DarlingCliCommands.cs's line 371 pin and capturing the resulting [FAIL], then restored. NoMonitoredTargetConnectionStringIsPinned asserts the inverse: each of the four named monitored-target sites (DarlingServerConnector's connect-and-probe, PostgresTargetProvider's collector factory, and DarlingWorker's two runtime.ConnectionString reads) is present in the scan and NOT pinned, so pinning one by mistake - which would misread PgTargetBaselineProvider.Clock's target-local clock - fails loudly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../StoreSessionTimeZonePinCensusTests.cs | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs diff --git a/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs b/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs new file mode 100644 index 0000000000..f9d5163a3c --- /dev/null +++ b/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs @@ -0,0 +1,261 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4277: every STORE connection string is pinned to session timezone = 'UTC' +/// (PerformanceMonitor.Darling.Storage.DarlingStoreConnection.PinSessionTimeZoneUtc) at the moment the +/// Npgsql connection or data source is built from it, and no MONITORED-target connection string is — a +/// monitored target's own session zone is read on purpose (see PgTargetBaselineProvider.Clock.cs). A +/// source scan rather than a live probe, because every developer and CI store runs UTC, so a missed call site +/// is invisible there — the same reasoning gives for scanning +/// source instead of trusting a green suite. +/// +/// Found by a full-repo grep for the four ways this codebase opens an Npgsql connection — +/// NpgsqlDataSource.Create(, new NpgsqlDataSourceBuilder(, new NpgsqlConnection( and the +/// fully qualified new Npgsql.NpgsqlConnection( — the same list #4277's own PR body used to find the 25 +/// STORE and 4 MONITORED-target call sites this test holds to a floor. +/// +/// Direct and indirect pins, both verified. Most call sites pass +/// DarlingStoreConnection.PinSessionTimeZoneUtc(...) straight into the constructor argument, which the +/// scan finds textually. One call site (DarlingWorker.RunCollectionLoopAsync) reassigns its +/// storeConnectionString parameter through the pin, layered underneath EnsureStoreSearchPath, +/// two lines above where it opens the data source from the bare identifier — the two cannot be written as one +/// expression because the search path has to sit UNDER the pin (see that method's own remarks). The scan +/// follows that indirection rather than allow-listing it blind: an argument that is a single bare identifier +/// counts as pinned only if THAT identifier is assigned from a PinSessionTimeZoneUtc(...) call +/// somewhere else in the same file, so a future edit that drops the assignment or renames the identifier still +/// fails this test rather than passing on trust. +/// +public sealed class StoreSessionTimeZonePinCensusTests +{ + /* Measured on dev at #4277: 562 .cs files across the four store-side projects, 29 call sites (25 STORE, + 4 MONITORED-target). Floored well below both so ordinary file churn does not trip them, but a broken + glob or a desynchronised regex fails loudly instead of reporting a vacuous clean bill of health. */ + private const int MinimumFilesScanned = 400; + private const int MinimumCallSitesFound = 25; + + /// + /// MONITORED-target call sites, named and reasoned — every one connects to a server this service WATCHES, + /// not to the store, so its own session zone is read on purpose and pinning it would make that read + /// wrong. Keyed file:member, the same key 's own waiver + /// list uses. + /// + private static readonly HashSet MonitoredTargetSites = new(StringComparer.Ordinal) + { + /* MonitoredServer connect-and-probe: onboarding/health-check reads the TARGET's own clock. */ + "DarlingServerConnector.cs:ConnectPostgresAsync", + + /* ITargetProvider's generic collector connection factory — every PostgreSQL collector's connection, + always against a MONITORED target, never the store. */ + "PostgresTargetProvider.cs:CreateConnection", + + /* Both read runtime.ConnectionString — a ServerRuntime is a MONITORED target's own resolved + connection, not the store's. */ + "DarlingWorker.cs:ReadPgStatementTextAsync", + "DarlingWorker.cs:RunTestHypotheticalIndexAsync", + }; + + [Fact] + public void EveryStoreConnectionStringSourcePinsItsSessionTimeZoneToUtc() + { + var scan = Scan(); + + Assert.True( + scan.FilesScanned >= MinimumFilesScanned, + $"scanned only {scan.FilesScanned} store-side source files (floor {MinimumFilesScanned}) — the " + + "scan would pass vacuously. Check the roots in StoreSourceFiles."); + Assert.True( + scan.Sites.Count >= MinimumCallSitesFound, + $"found only {scan.Sites.Count} Npgsql connection call sites (floor {MinimumCallSitesFound}) — " + + "check the four patterns in CallSiteRegex."); + + var offenders = scan.Sites + .Where(s => !s.Pinned && !MonitoredTargetSites.Contains(s.Key)) + .Select(s => $"{s.Key} ({s.File}:{s.Line}) — {s.Argument}") + .ToList(); + + Assert.True( + offenders.Count == 0, + "a STORE connection string is not pinned to session timezone UTC:" + Environment.NewLine + + string.Join(Environment.NewLine, offenders) + Environment.NewLine + + "Route it through DarlingStoreConnection.PinSessionTimeZoneUtc(...) — or, if it truly connects " + + "to a MONITORED target, add its file:member key to MonitoredTargetSites with the reason."); + } + + [Fact] + public void NoMonitoredTargetConnectionStringIsPinned() + { + var scan = Scan(); + var byKey = scan.Sites.ToLookup(s => s.Key); + + var missing = MonitoredTargetSites.Where(key => !byKey.Contains(key)).ToList(); + + Assert.True( + missing.Count == 0, + "a listed MONITORED-target site was not found by the scan (renamed, removed, or moved) — update " + + "MonitoredTargetSites: " + string.Join(", ", missing)); + + var wronglyPinned = scan.Sites + .Where(s => MonitoredTargetSites.Contains(s.Key) && s.Pinned) + .Select(s => $"{s.Key} ({s.File}:{s.Line})") + .ToList(); + + Assert.True( + wronglyPinned.Count == 0, + "a MONITORED-target connection string is pinned to UTC — its own session zone must be read on " + + "purpose (see PgTargetBaselineProvider.Clock.cs), so pinning it makes that read wrong: " + + string.Join(", ", wronglyPinned)); + } + + /* ---------------- scan ---------------- */ + + private readonly record struct CallSite(string Key, string File, int Line, string Argument, bool Pinned); + + private readonly record struct CensusScan(int FilesScanned, IReadOnlyList Sites); + + private static readonly Regex CallSiteRegex = new( + @"NpgsqlDataSource\.Create\(|new\s+NpgsqlDataSourceBuilder\(|new\s+NpgsqlConnection\(|new\s+Npgsql\.NpgsqlConnection\(", + RegexOptions.Compiled); + + private static CensusScan Scan([CallerFilePath] string thisFile = "") + { + var files = 0; + var sites = new List(); + + foreach (var path in StoreSourceFiles(thisFile)) + { + files++; + var text = File.ReadAllText(path); + var name = Path.GetFileName(path); + CSharpMemberMap.MemberMap? members = null; + + foreach (Match match in CallSiteRegex.Matches(text)) + { + var openParen = match.Index + match.Length - 1; + var argument = ArgumentSpan(text, openParen) + ?? throw new InvalidOperationException($"{name}: unterminated call at offset {match.Index}"); + + members ??= CSharpMemberMap.Of(text); + var member = CSharpMemberMap.EnclosingMember(members, match.Index); + var line = CSharpMemberMap.LineOf(text, match.Index); + + sites.Add(new CallSite( + name + ":" + member, + name, + line, + Collapse(argument), + ArgumentIsPinned(text, argument))); + } + } + + return new CensusScan(files, sites); + } + + /// Whether (the balanced-paren span passed to the connection + /// constructor) is pinned — directly, by calling PinSessionTimeZoneUtc inline, or indirectly, by + /// being a bare identifier that assigns from that same call somewhere + /// else in the SAME file. + private static bool ArgumentIsPinned(string fileText, string argument) + { + if (argument.Contains("PinSessionTimeZoneUtc", StringComparison.Ordinal)) + { + return true; + } + + var inner = argument.Trim(); + + if (inner.Length >= 2 && inner[0] == '(' && inner[^1] == ')') + { + inner = inner[1..^1].Trim(); + } + + if (!Regex.IsMatch(inner, @"^[A-Za-z_][A-Za-zA-Z0-9_]*$")) + { + return false; + } + + var indirectPin = new Regex(@"\b" + Regex.Escape(inner) + @"\s*=\s*DarlingStoreConnection\.PinSessionTimeZoneUtc\s*\("); + + return indirectPin.IsMatch(fileText); + } + + /// The balanced-paren span starting at (which must hold (), + /// through its matching close — the connection constructor's whole argument list, however many calls are + /// nested inside it. + private static string? ArgumentSpan(string text, int openParen) + { + var depth = 0; + + for (var i = openParen; i < text.Length; i++) + { + if (text[i] == '(') + { + depth++; + } + else if (text[i] == ')') + { + depth--; + + if (depth == 0) + { + return text[openParen..(i + 1)]; + } + } + } + + return null; + } + + private static string Collapse(string span) + { + var one = Regex.Replace(span.Trim(), @"\s+", " "); + + return one.Length <= 160 ? one : one[..160] + "…"; + } + + /// The store-side projects that can open an Npgsql connection: the storage helpers, the service + /// (worker, MCP/web hosts, CLI verbs, managed-runtime bootstrap), the analysis project, and the viewer. + /// Lite is excluded — it has no Npgsql dependency at all (DuckDB), see the Lite check in #4277's PR + /// body. + private static IEnumerable StoreSourceFiles(string thisFile) + { + var darlingDir = Path.GetFullPath(Path.Combine(Path.GetDirectoryName(thisFile)!, "..")); + + var roots = new[] + { + Path.Combine(darlingDir, "PerformanceMonitor.Darling.Storage"), + Path.Combine(darlingDir, "PerformanceMonitor.Darling.Service"), + Path.Combine(darlingDir, "PerformanceMonitor.Darling.Viewer"), + Path.Combine(darlingDir, "PerformanceMonitor.Darling.Analysis"), + }; + + foreach (var root in roots) + { + foreach (var path in Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) + { + if (path.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", StringComparison.Ordinal) + || path.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) + { + continue; + } + + yield return path; + } + } + } +} From f9ed975c705c4cc23592e40d39b731cb7c9fa75a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:03:08 -0400 Subject: [PATCH 3/5] #4277: live test proving a store connection pins to UTC on a New York database StoreSessionTimeZoneLiveTests, gated on DARLING_TEST_PG and tagged [Collection("live-postgres")], boots a #1776 own-store scratch database, ALTER DATABASEs its default timezone to America/New_York (not the shared rig conf, so it means something on a UTC CI runner too), and asserts an unpinned connection sees that default while a connection opened the way the product opens one (DarlingStoreConnection.PinSessionTimeZoneUtc first) reports SHOW timezone = UTC regardless. Second assertion exercises DarlingModuleMap.RefreshSql's own bare-now() predicate (the one StoreSqlClockDisciplineTests waives on arithmetic margin, not zone-correctness), pinned to its exact text by an Assert.Contains. Two rows straddle its 48h boundary with a 2h margin (46h old, 50h old) rather than one row safely inside it: at America/New_York a naive column reads younger than its true UTC meaning, so the window only ever widens and a single safely-inside row cannot discriminate pinned from unpinned - only the near-boundary row's wrong admission does. Proven once against the New York rig: passes with the pin in place, and fails (SHOW timezone: expected UTC, got America/New_York) with PinSessionTimeZoneUtc temporarily neutered to return its input unchanged, then restored. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../StoreSessionTimeZoneLiveTests.cs | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 Darling/Darling.Tests/StoreSessionTimeZoneLiveTests.cs diff --git a/Darling/Darling.Tests/StoreSessionTimeZoneLiveTests.cs b/Darling/Darling.Tests/StoreSessionTimeZoneLiveTests.cs new file mode 100644 index 0000000000..72e8194d4c --- /dev/null +++ b/Darling/Darling.Tests/StoreSessionTimeZoneLiveTests.cs @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4277 live proof: a STORE connection opened the way the product opens one +/// (DarlingStoreConnection.PinSessionTimeZoneUtc) reports session timezone UTC even when the +/// DATABASE's own default is America/New_York — proven on a #1776 own-store scratch database +/// (ALTER DATABASE ... SET timezone, not the shared rig conf, so it means something on a UTC CI +/// runner too, not only a rig deliberately set to New York). +/// +/// The second assertion exercises the one bare-now() store SQL predicate +/// waives on ARITHMETIC margin, not on zone-correctness — +/// 's collection_time >= now() - interval '2 days'. Pinned to +/// that exact text by an Assert.Contains so a future change to the interval cannot silently stale this +/// proof. Two rows straddle the 48h boundary with a 2h margin either side (inside: 46h old, outside: 50h +/// old) rather than one row safely in the middle, because a naive column read under a WEST-of-UTC session +/// (America/New_York is UTC-4 in September) is interpreted as YOUNGER than its true UTC meaning — the window +/// only ever WIDENS, so it only ever ADMITS more rows. A single "inside" row stays admitted under either +/// session and cannot discriminate; only the "outside" row's wrong admission does — which is also why this +/// predicate is safely waived rather than a bug: RefreshSql's own remarks show its worst case (34-60h) still +/// covers what it needs regardless of offset, but that tolerance is a fact about ITS margin, not a licence +/// for a naive-to-now() comparison to ignore the session zone in general. +/// +[Collection("live-postgres")] +public sealed class StoreSessionTimeZoneLiveTests +{ + private static string? BaseConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + [Fact] + public async Task AStoreConnection_PinsToUtc_EvenWhenTheDatabaseDefaultIsAmericaNewYork() + { + var baseCs = BaseConnectionString; + Assert.SkipWhen( + string.IsNullOrEmpty(baseCs), + "Set DARLING_TEST_PG to a Postgres connection string to run the live #4277 store-timezone-pin test."); + + var ct = TestContext.Current.CancellationToken; + + /* #1776 own-store: a database-level default this test sets must not leak into any other test that + shares DARLING_TEST_PG's own database. */ + await using var scratch = await ScratchPostgres.CreateAsync(baseCs!, ct); + + await using (var admin = new NpgsqlConnection(baseCs)) + { + await admin.OpenAsync(ct); + await using var alter = new NpgsqlCommand( + $"ALTER DATABASE \"{scratch.DatabaseName}\" SET timezone = 'America/New_York'", admin); + await alter.ExecuteNonQueryAsync(ct); + } + + /* ALTER DATABASE ... SET only takes effect for sessions opened AFTER it commits, so migrate through a + fresh connection rather than the admin one above. */ + await using (var migrate = new NpgsqlConnection(scratch.ConnectionString)) + { + await migrate.OpenAsync(ct); + await PgMigrations.MigrateAsync(migrate, ct); + } + + /* The database's own default, UNPINNED — confirms the fixture set up a genuinely non-UTC store + before asserting that the product's pin overrides it. */ + await using (var unpinned = new NpgsqlConnection(scratch.ConnectionString)) + { + await unpinned.OpenAsync(ct); + Assert.Equal("America/New_York", await ShowTimeZoneAsync(unpinned, ct)); + } + + /* The way the product opens a STORE connection: PinSessionTimeZoneUtc first, then the connection. */ + var pinnedConnectionString = DarlingStoreConnection.PinSessionTimeZoneUtc(scratch.ConnectionString); + await using var connection = new NpgsqlConnection(pinnedConnectionString); + await connection.OpenAsync(ct); + + Assert.Equal("UTC", await ShowTimeZoneAsync(connection, ct)); + + Assert.Contains( + "collection_time >= now() - interval '2 days'", DarlingModuleMap.RefreshSql, StringComparison.Ordinal); + + await using (var create = new NpgsqlCommand( + "CREATE TEMP TABLE probe_4277 (collection_time timestamp NOT NULL)", connection)) + { + await create.ExecuteNonQueryAsync(ct); + } + + /* Kind=Unspecified so Npgsql binds `timestamp` (naive), never `timestamptz` — the same naive-UTC + digits every collector row carries, not a value that shifts on its own before the comparison even + runs. */ + var insideWindow = DateTime.SpecifyKind(DateTime.UtcNow.AddHours(-46), DateTimeKind.Unspecified); + var outsideWindow = DateTime.SpecifyKind(DateTime.UtcNow.AddHours(-50), DateTimeKind.Unspecified); + + await using (var insert = new NpgsqlCommand( + "INSERT INTO probe_4277 (collection_time) VALUES ($1), ($2)", connection)) + { + insert.Parameters.AddWithValue(insideWindow); + insert.Parameters.AddWithValue(outsideWindow); + await insert.ExecuteNonQueryAsync(ct); + } + + await using var read = new NpgsqlCommand( + "SELECT count(*) FILTER (WHERE collection_time = $1), count(*) FILTER (WHERE collection_time = $2) " + + "FROM probe_4277 WHERE collection_time >= now() - interval '2 days'", connection); + read.Parameters.AddWithValue(insideWindow); + read.Parameters.AddWithValue(outsideWindow); + await using var reader = await read.ExecuteReaderAsync(ct); + await reader.ReadAsync(ct); + + Assert.Equal(1L, reader.GetInt64(0)); // 46h old, inside the 48h window: returned, as #4277 asks. + Assert.Equal(0L, reader.GetInt64(1)); // 50h old, past the 48h window: NOT returned — only true under UTC. + } + + private static async Task ShowTimeZoneAsync(NpgsqlConnection connection, CancellationToken ct) + { + await using var command = new NpgsqlCommand("SHOW timezone", connection); + + return (string)(await command.ExecuteScalarAsync(ct))!; + } +} From 36bde40a0c9b59f3e2ea431ecfd48e8ad75bcec9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:14:47 -0400 Subject: [PATCH 4/5] #4277: re-anchor two stale source pins on the new PinSessionTimeZoneUtc-wrapped shape Found running the full suite with the #4277 pin in place for the first time (the diagnosis run tested dev pre-pin): ViewerCommandTimeoutTests.TheDataService_PublishesTheEffectivePoolSizeItBuildsThePoolOn and DarlingStoreLoginsTests.EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBefore both grep source for the literal pre-pin call shape (NpgsqlDataSource.Create(effectiveConnectionString) / (storeConnectionString)), which the pin's own commit rewrote to wrap the argument in DarlingStoreConnection.PinSessionTimeZoneUtc(...). Re-anchored both on the new text; neither pin's actual intent (pool size published before the data source is built; the unmanaged pool comes from the resolver, not config.Postgres.ConnectionString) changed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- Darling/Darling.Tests/DarlingStoreLoginsTests.cs | 8 +++++++- Darling/Darling.Tests/ViewerCommandTimeoutTests.cs | 9 ++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/Darling/Darling.Tests/DarlingStoreLoginsTests.cs b/Darling/Darling.Tests/DarlingStoreLoginsTests.cs index ef50590f56..ec1d2d9d27 100644 --- a/Darling/Darling.Tests/DarlingStoreLoginsTests.cs +++ b/Darling/Darling.Tests/DarlingStoreLoginsTests.cs @@ -847,7 +847,13 @@ public void EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBe var start = Body(code, "private async Task TryStartServerAsync("); Assert.DoesNotContain("config.Postgres.ConnectionString", start, StringComparison.Ordinal); - Assert.Contains("NpgsqlDataSource.Create(storeConnectionString)", start, StringComparison.Ordinal); + + /* #4277: every STORE data source pins its session timezone to UTC, so the resolved + storeConnectionString is wrapped before it reaches Create rather than passed bare. */ + Assert.Contains( + "NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(storeConnectionString))", + start, + StringComparison.Ordinal); var assignments = Regex.Matches(start, @"storeConnectionString\s*=(?!=)").Count; Assert.Equal(2, assignments); diff --git a/Darling/Darling.Tests/ViewerCommandTimeoutTests.cs b/Darling/Darling.Tests/ViewerCommandTimeoutTests.cs index c3ba959553..5103987817 100644 --- a/Darling/Darling.Tests/ViewerCommandTimeoutTests.cs +++ b/Darling/Darling.Tests/ViewerCommandTimeoutTests.cs @@ -1440,7 +1440,14 @@ public void TheDataService_PublishesTheEffectivePoolSizeItBuildsThePoolOn() var code = CSharpSourceWalker.StripCommentsAndStrings(File.ReadAllText(path)); var publish = code.IndexOf("ViewerStorePool.Publish(effectiveConnectionString)", System.StringComparison.Ordinal); - var create = code.IndexOf("NpgsqlDataSource.Create(effectiveConnectionString)", System.StringComparison.Ordinal); + + /* #4277 wraps the argument in DarlingStoreConnection.PinSessionTimeZoneUtc(...) so every STORE data + source pins its session timezone to UTC; Publish still runs on the pre-pin string above (the pin + only touches the Timezone keyword, never MaxPoolSize, so ViewerStorePool.MaxPoolSizeOf reads the + same value either way). */ + var create = code.IndexOf( + "NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(effectiveConnectionString))", + System.StringComparison.Ordinal); Assert.True( create >= 0, From ee4de7d7559bc2085e19fe91ac0eddfc4c091958 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:31:13 -0400 Subject: [PATCH 5/5] #4285 round-1: append the pin instead of rewriting the connection string The round-1 security review on #4285 found PinSessionTimeZoneUtc's builder round trip erases any keyword the caller set to an explicit empty value (Password='', Root Certificate="", ...), because the builder's writer emits an empty value as a bare Key=, which its own reader treats as "not set". Npgsql then falls back to a PG* environment variable or a default file where the caller's empty value meant "never fall back". Fix all three helpers that round-trip a store connection string through a builder the same way: parse once (to keep the malformed-string exception), then append the one keyword to the caller's own string instead of returning the builder's re-serialized string. DarlingWorker.EnsureStoreSearchPath and ViewerDataService.ApplyConnectionTimeout had the same bug already on dev; their set-if-absent detection is unchanged. Also close the census test's Low 2: StoreSessionTimeZonePinCensusTests used to accept an indirect pin (a bare identifier assigned from PinSessionTimeZoneUtc anywhere else in the file), which DarlingWorker.cs's two storeConnectionString variables could fool. The pin now sits inline on the NpgsqlDataSource.Create( call in RunCollectionLoopAsync, and the indirect-pin allowance is deleted, so every store site must show the pin on its own line. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ --- .../StoreConnectionStringAppendPinTests.cs | 140 ++++++++++++++++++ .../StoreSessionTimeZonePinCensusTests.cs | 49 ++---- .../Darling.Tests/ViewerDataServiceTests.cs | 6 +- .../DarlingWorker.cs | 19 ++- .../DarlingStoreConnection.cs | 23 ++- .../ViewerDataService.cs | 10 +- 6 files changed, 195 insertions(+), 52 deletions(-) create mode 100644 Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs diff --git a/Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs b/Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs new file mode 100644 index 0000000000..58249e91be --- /dev/null +++ b/Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Linq; +using Npgsql; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// Round-1 security review on #4285 (the #4277 PR): , +/// and +/// used to set their one keyword by rewriting the caller's connection string through a builder. A builder's +/// writer emits an explicit empty value (Password='') as a bare Key=, and its own reader treats a +/// bare Key= as "not set" — silently dropping the keyword and letting Npgsql fall back to a PG* +/// environment variable or a default file where the caller's empty value meant "never fall back". All three now +/// APPEND their one keyword to the caller's own string instead. This is the harness the review's fix +/// recommendation described running against 24 inputs, committed: every OTHER keyword in the caller's string, +/// including an explicit empty one, must survive byte for byte, and the output must literally start with the +/// caller's own string. +/// +public sealed class StoreConnectionStringAppendPinTests +{ + /// Each row is independently valid Npgsql connection-string content and exercises one of the + /// round-1 review's risk shapes. None of the five sets Timezone, Search Path or Timeout, so all three + /// helpers see their own keyword as ABSENT on every row (the "already present" branch of each helper is + /// covered separately, below and in the pre-existing helper tests). + public static TheoryData RiskyConnectionStrings() => new() + { + { "quoted-empty Password", "Host=store;Database=darling;Password=''" }, + { "double-quoted-empty Root Certificate", "Host=store;Database=darling;Root Certificate=\"\"" }, + { "password holding ; ' \" and =", "Host=store;Database=darling;Password='ab;cd''ef\"gh=ij'" }, + { "trailing semicolon", "Host=store;Database=darling;" }, + { "trailing spaces", "Host=store;Database=darling " }, + }; + + [Theory] + [MemberData(nameof(RiskyConnectionStrings))] + public void PinSessionTimeZoneUtc_AppendsWithoutErasingAnyOtherKeyword(string label, string input) + { + var output = DarlingStoreConnection.PinSessionTimeZoneUtc(input); + + AssertAppendedAndEveryOtherKeywordSurvives(input, output, label); + Assert.Equal("UTC", new NpgsqlConnectionStringBuilder(output).Timezone); + } + + [Theory] + [MemberData(nameof(RiskyConnectionStrings))] + public void EnsureStoreSearchPath_AppendsWithoutErasingAnyOtherKeyword(string label, string input) + { + var output = DarlingWorker.EnsureStoreSearchPath(input); + + AssertAppendedAndEveryOtherKeywordSurvives(input, output, label); + Assert.Equal(DarlingManagedPostgres.SearchPath, new NpgsqlConnectionStringBuilder(output).SearchPath); + } + + [Theory] + [MemberData(nameof(RiskyConnectionStrings))] + public void ApplyConnectionTimeout_AppendsWithoutErasingAnyOtherKeyword(string label, string input) + { + var output = ViewerDataService.ApplyConnectionTimeout(input, 42); + + AssertAppendedAndEveryOtherKeywordSurvives(input, output, label); + Assert.Equal(42, new NpgsqlConnectionStringBuilder(output).Timeout); + } + + /// #4285 round-1 Low 1, generalized to all three helpers: the parsed value of every keyword the + /// CALLER's string sets — including an explicit empty one, which a builder round trip erases — must be + /// identical before and after, and the output must literally start with the caller's own string (an + /// append, never a rewrite). + private static void AssertAppendedAndEveryOtherKeywordSurvives(string input, string output, string label) + { + Assert.True( + output.StartsWith(input, StringComparison.Ordinal), + $"{label}: output does not start with the caller's own string — input=[{input}] output=[{output}]"); + + var before = new NpgsqlConnectionStringBuilder(input); + var after = new NpgsqlConnectionStringBuilder(output); + + foreach (var key in before.Keys.Cast()) + { + Assert.True(after.ContainsKey(key), $"{label}: keyword '{key}' is missing from the output"); + Assert.Equal(before[key], after[key]); + } + } + + /// The pin always wins, even over an existing Timezone spelled in odd case — Npgsql keeps only the + /// LAST value for a repeated keyword, so the appended one, being last, wins regardless of the existing + /// key's case. + [Fact] + public void PinSessionTimeZoneUtc_OverwritesAnExistingTimezoneInOddCase() + { + const string input = "Host=store;Database=darling;TIMEZONE=America/New_York"; + + var output = DarlingStoreConnection.PinSessionTimeZoneUtc(input); + + var parsed = new NpgsqlConnectionStringBuilder(output); + Assert.Equal("UTC", parsed.Timezone); + Assert.Equal("darling", parsed.Database); + } + + /// Detection () is alias-aware and + /// case-insensitive, so an odd-case existing Search Path still counts as present, and the string is + /// returned unchanged rather than double-appended. + [Fact] + public void EnsureStoreSearchPath_DetectsAnExistingSearchPathInOddCase_AndLeavesItUnchanged() + { + const string input = "Host=store;Database=darling;SEARCHPATH=reporting,public"; + + var output = DarlingWorker.EnsureStoreSearchPath(input); + + Assert.Equal(input, output); + Assert.Equal("reporting,public", new NpgsqlConnectionStringBuilder(output).SearchPath); + } + + /// + /// and + /// both still parse the caller's string once, through , before + /// deciding what to append — PinSessionTimeZoneUtc to keep today's malformed-string exception at + /// this exact statement (round-1 review's Q2), EnsureStoreSearchPath as its unchanged presence + /// check. Pinned so neither parse is later mistaken for dead code and deleted. + /// + [Fact] + public void PinSessionTimeZoneUtcAndEnsureStoreSearchPath_StillThrowOnAMalformedString() + { + const string malformed = "Host=store;sslmode=NotARealSslMode"; + + Assert.Throws(() => DarlingStoreConnection.PinSessionTimeZoneUtc(malformed)); + Assert.Throws(() => DarlingWorker.EnsureStoreSearchPath(malformed)); + } +} diff --git a/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs b/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs index f9d5163a3c..5fd2f4eddd 100644 --- a/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs +++ b/Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs @@ -30,16 +30,15 @@ namespace Darling.Tests; /// fully qualified new Npgsql.NpgsqlConnection( — the same list #4277's own PR body used to find the 25 /// STORE and 4 MONITORED-target call sites this test holds to a floor. /// -/// Direct and indirect pins, both verified. Most call sites pass +/// Every pin is direct, on the call line itself. Every call site passes /// DarlingStoreConnection.PinSessionTimeZoneUtc(...) straight into the constructor argument, which the -/// scan finds textually. One call site (DarlingWorker.RunCollectionLoopAsync) reassigns its -/// storeConnectionString parameter through the pin, layered underneath EnsureStoreSearchPath, -/// two lines above where it opens the data source from the bare identifier — the two cannot be written as one -/// expression because the search path has to sit UNDER the pin (see that method's own remarks). The scan -/// follows that indirection rather than allow-listing it blind: an argument that is a single bare identifier -/// counts as pinned only if THAT identifier is assigned from a PinSessionTimeZoneUtc(...) call -/// somewhere else in the same file, so a future edit that drops the assignment or renames the identifier still -/// fails this test rather than passing on trust. +/// scan finds textually. Round-1 review on #4285's PR found that an earlier version of this scan also +/// accepted an INDIRECT pin — a bare identifier argument counted as pinned if it was assigned from a +/// PinSessionTimeZoneUtc(...) call anywhere else in the same file, which let an unrelated unpinned +/// reassignment or a second same-named variable pass on trust (DarlingWorker.cs has two +/// storeConnectionString variables). DarlingWorker.RunCollectionLoopAsync now writes the pin +/// inline on the NpgsqlDataSource.Create(...) line itself, so every STORE site the scan finds must show +/// the pin on its own line, with no indirection rule left to fool. /// public sealed class StoreSessionTimeZonePinCensusTests { @@ -159,7 +158,7 @@ private static CensusScan Scan([CallerFilePath] string thisFile = "") name, line, Collapse(argument), - ArgumentIsPinned(text, argument))); + ArgumentIsPinned(argument))); } } @@ -167,31 +166,13 @@ private static CensusScan Scan([CallerFilePath] string thisFile = "") } /// Whether (the balanced-paren span passed to the connection - /// constructor) is pinned — directly, by calling PinSessionTimeZoneUtc inline, or indirectly, by - /// being a bare identifier that assigns from that same call somewhere - /// else in the SAME file. - private static bool ArgumentIsPinned(string fileText, string argument) + /// constructor) is pinned — DIRECTLY, by calling PinSessionTimeZoneUtc inline on the same call. No + /// indirect form counts (round-1 review on #4285's PR): a bare identifier used to pass if it was assigned + /// from a pin call anywhere else in the file, which couldn't tell that assignment apart from an unrelated + /// same-named variable or a later unpinned reassignment of the same one. + private static bool ArgumentIsPinned(string argument) { - if (argument.Contains("PinSessionTimeZoneUtc", StringComparison.Ordinal)) - { - return true; - } - - var inner = argument.Trim(); - - if (inner.Length >= 2 && inner[0] == '(' && inner[^1] == ')') - { - inner = inner[1..^1].Trim(); - } - - if (!Regex.IsMatch(inner, @"^[A-Za-z_][A-Za-zA-Z0-9_]*$")) - { - return false; - } - - var indirectPin = new Regex(@"\b" + Regex.Escape(inner) + @"\s*=\s*DarlingStoreConnection\.PinSessionTimeZoneUtc\s*\("); - - return indirectPin.IsMatch(fileText); + return argument.Contains("PinSessionTimeZoneUtc", StringComparison.Ordinal); } /// The balanced-paren span starting at (which must hold (), diff --git a/Darling/Darling.Tests/ViewerDataServiceTests.cs b/Darling/Darling.Tests/ViewerDataServiceTests.cs index ae30153880..865a6eab9e 100644 --- a/Darling/Darling.Tests/ViewerDataServiceTests.cs +++ b/Darling/Darling.Tests/ViewerDataServiceTests.cs @@ -897,9 +897,9 @@ public void ApplyConnectionTimeout_ManagedStyleStringWithoutTimeout_SetsThePrefe Assert.Equal(5641, builder.Port); Assert.Equal("darling", builder.Database); /* Search Path is the load-bearing one: it resolves the bare table names to the collect/config schemas - on every connection, so if the base DbConnectionStringBuilder round-trip ever mangled it, every - managed-path query would silently break. Pin that it survives verbatim (the reason we detect + emit - via the base builder rather than NpgsqlConnectionStringBuilder). */ + on every connection, so a query would silently break if this ever went back to emitting through a + builder round trip instead of appending to the caller's own string (round-1 review on #4285's PR). + Pin that it survives verbatim. */ Assert.Equal("collect,config,public", builder.SearchPath); } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index 3184f4f328..2f42dd038d 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -1737,9 +1737,12 @@ and independent of the pool's connection-open timing relative to PgMigrations' best-effort ALTER DATABASE ... SET search_path. Without this a FRESH bring-your-own store silently collects nothing until the service is restarted; see EnsureStoreSearchPath for the pool-timing root cause. Managed mode already sets it, so - this is a no-op there. */ - storeConnectionString = DarlingStoreConnection.PinSessionTimeZoneUtc(EnsureStoreSearchPath(storeConnectionString)); - await using var postgres = NpgsqlDataSource.Create(storeConnectionString); + this is a no-op there. The pin has to sit OUTSIDE EnsureStoreSearchPath (wrapping its + result rather than the two being one call), so the census below only has to look for the + pin on the Create line itself — no reassignment in between for a future edit to slip + an unpinned read behind. */ + await using var postgres = NpgsqlDataSource.Create( + DarlingStoreConnection.PinSessionTimeZoneUtc(EnsureStoreSearchPath(storeConnectionString))); _postgres = postgres; /* #2936: a failure here is triaged rather than uniformly terminal. A store that is unreachable for a moment — restarting, failing over, still coming up alongside this service — and a sibling @@ -4389,15 +4392,19 @@ internal static string EnsureStoreSearchPath(string connectionString) var builder = new NpgsqlConnectionStringBuilder(connectionString); if (string.IsNullOrWhiteSpace(builder.SearchPath)) { - /* DarlingManagedPostgres is annotated [SupportedOSPlatform("windows")] for its DPAPI / + /* Append, don't rewrite through the builder: round-1 review on #4285's PR found the builder's + writer erases a caller keyword set to an explicit empty value (Password='', ...), because it + emits that as a bare Key=, which its own reader treats as "not set". Appending keeps every + other keyword byte for byte. The constant below holds no ';', '=' or quote character, so it + never needs quoting — a plain "Search Path=" is unambiguous. + DarlingManagedPostgres is annotated [SupportedOSPlatform("windows")] for its DPAPI / bundled-cluster surface, but SearchPath is a platform-neutral compile-time constant with no runtime dependency, and BYO mode runs on any OS — so the CA1416 cross-platform reachability flag is spurious for this const reference. Suppressed narrowly rather than forking the constant, keeping managed and BYO byte-identical (a test pins them equal). */ #pragma warning disable CA1416 - builder.SearchPath = DarlingManagedPostgres.SearchPath; + return connectionString + ";Search Path=" + DarlingManagedPostgres.SearchPath; #pragma warning restore CA1416 - return builder.ConnectionString; } return connectionString; diff --git a/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs b/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs index cc14c03da0..36a230481d 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs @@ -36,14 +36,25 @@ namespace PerformanceMonitor.Darling.Storage; /// public static class DarlingStoreConnection { - /// The pin itself: Npgsql's Timezone connection-string keyword, forced to UTC. + /// + /// The pin itself: Npgsql's Timezone connection-string keyword, forced to UTC — by APPENDING + /// it to the caller's own string, never by rewriting the string through a builder. A builder round trip + /// writes an explicit empty value (Password='', Root Certificate="", ...) as a bare + /// Key=, and its own reader treats that as "not set" — silently dropping the keyword and letting + /// Npgsql fall back to a PG* environment variable or a default file where the caller's empty value + /// meant "never fall back" (round-1 review on #4285's PR). Parsing once first keeps today's exception on a + /// malformed string at this exact statement; the parsed builder is then discarded, so every OTHER keyword + /// in the caller's string survives byte for byte. Npgsql keeps only the LAST value for a repeated keyword, + /// so appending still overwrites any Timezone/TimeZone the caller's string already sets, + /// wherever it appears and in whatever case — measured against trailing ;, trailing spaces, and an + /// odd-case existing key, all of which still parse to a single UTC Timezone after the append. + /// public static string PinSessionTimeZoneUtc(string connectionString) { - var builder = new NpgsqlConnectionStringBuilder(connectionString) - { - Timezone = "UTC", - }; + /* Parse once, so a malformed string fails here exactly as Npgsql's own constructor would — the + discarded builder exists only for that validation, never for re-serializing the string. */ + _ = new NpgsqlConnectionStringBuilder(connectionString); - return builder.ConnectionString; + return connectionString + ";Timezone=UTC"; } } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 3456bc72d3..d0b9d231f5 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -544,17 +544,21 @@ a hint — so fail toward silence rather than toward a warning nobody can act on /// caller clamps it to 5–60). Pure + string-only, so it is unit-tested without a live Postgres. Detection /// uses the base , whose ContainsKey reflects exactly the keys /// present in the string (NpgsqlConnectionStringBuilder overrides ContainsKey to answer for every KNOWN - /// keyword, which can't tell "set" from "settable"). + /// keyword, which can't tell "set" from "settable"). When the keyword is absent, the preference is APPENDED + /// to the caller's own string rather than emitted through a builder round trip — round-1 review on #4285's + /// PR measured the base builder's round trip changing 17 of 24 test strings, because its writer erases a + /// keyword the caller set to an explicit empty value the same way 's + /// remarks describe for PinSessionTimeZoneUtc; appending leaves every other keyword untouched. /// internal static string ApplyConnectionTimeout(string connectionString, int timeoutSeconds) { var builder = new DbConnectionStringBuilder { ConnectionString = connectionString }; if (!builder.ContainsKey("Timeout")) { - builder["Timeout"] = timeoutSeconds; + return connectionString + ";Timeout=" + timeoutSeconds.ToString(CultureInfo.InvariantCulture); } - return builder.ConnectionString; + return connectionString; } ///