Skip to content

Pin every STORE connection's session time zone to UTC (#4277) - #4285

Merged
erikdarlingdata merged 6 commits into
devfrom
fix/4277-non-utc-store-zone
Sep 25, 2026
Merged

erikdarlingdata merged 6 commits into
devfrom
fix/4277-non-utc-store-zone

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Closes #4277.

Why

Every timestamp column in the Darling store is timestamp without time zone and holds UTC. A managed store writes timezone = 'UTC' into its own postgresql.conf (the v9 block). A bring-your-own store never gets that block. It keeps the zone its owner set, or the zone initdb took from its host.

PostgreSQL resolves a comparison between such a column and a timestamptz value in the session's time zone. A bare now() in the SQL is one such value, and so is a bound DateTime with Kind=Utc. So on a store whose session zone is not UTC, a read can shift its window without any error. StoreSqlClockDisciplineTests records a measurement: on seeded data, the same one-hour predicate returned 66 rows under UTC and 343 under America/New_York. That is five hours of data in a one-hour window. Before this PR, nothing ran the product against a store whose session zone was not UTC.

What changes

The pin

DarlingStoreConnection.PinSessionTimeZoneUtc, in PerformanceMonitor.Darling.Storage, is one new helper. It appends ;Timezone=UTC to the caller's connection string and returns it. Npgsql keeps the last value of a repeated keyword, so the pin overwrites any Timezone the caller set. That is on purpose: the store's columns hold UTC whatever a connection string says.

The helper parses the string once first, so a malformed string still fails where it did before. It never rewrites the string, so every other keyword stays as the caller wrote it. It lives in the Storage project because the Service and the Viewer both reference it.

Every store connection now passes its connection string through the helper, just before the connection or data source is made. A grep of the non-test Darling projects for NpgsqlDataSource.Create(, NpgsqlDataSourceBuilder( and NpgsqlConnection( found 25 store sites:

  • DarlingWorker.cs: the worker's store data source, and the viewer-role source for custom alert rules.
  • Mcp/DarlingMcpHostService.cs and Mcp/DarlingWebHostService.cs: each host's store data source.
  • DarlingStoreUpgrade.cs, 5 sites: the connections for the managed runtime's TimescaleDB update and upgrade.
  • DarlingCliCommands.cs, 9 sites: every CLI command that opens a store connection.
  • DarlingManagedPostgres.cs, 3 sites: the managed store's startup checks (the database exists, the pg_hba rule, the listener).
  • DarlingStoreLogins.cs, 1 site: the check that a bring-your-own credential works.
  • PerformanceMonitor.Darling.Viewer/ViewerDataService.cs, 1 site: the viewer's store data source.
  • PerformanceMonitor.Darling.Storage/StoreConnectionSelfTest.cs, 2 sites: the self-test that runs after a failed connect.

Four sites stay unpinned on purpose

Four sites connect to a monitored PostgreSQL server, not to the store:

  • the two runtime.ConnectionString sites in DarlingWorker.cs
  • the connect-and-probe in DarlingServerConnector.cs
  • the collector connection factory in Targets/PostgresTargetProvider.cs

PgTargetBaselineProvider.Clock.cs reads a monitored server's own time zone on purpose, so these must keep it.

Two older helpers now append too

Round 1 of review found that a round trip through a connection-string builder drops a keyword set to an empty value. For example, Password='' comes back as a bare Password=, and the builder reads that as "not set". Npgsql then uses the PGPASSWORD environment variable or a password file, which the empty value was there to prevent. The first version of the pin had that problem. Two older helpers that work on the same strings had it too, so they append now as well:

  • DarlingWorker.EnsureStoreSearchPath appends ;Search Path=collect,config,public when the string has no search path.
  • ViewerDataService.ApplyConnectionTimeout appends ;Timeout=<seconds> when the string has no timeout.

Neither one changes what it looks for. Each returns the caller's string unchanged when the keyword is already there.

The run before the fix

The lane built dev without the pin and ran the full suite once against a store database whose time zone was America/New_York. log_timezone stayed UTC, as on CI. Result: 13,982 tests, 0 failed, 49 skipped, 1 not run.

No test failed because most live tests seed their data and compute the expected window from the same DateTime.UtcNow. Both sides shift together, so the test passes on a store that reads a real window wrong. That is why the fix needed a live test seeded with UTC values in SQL (below).

The tests

StoreSessionTimeZonePinCensusTests scans the source of the storage, service, viewer and analysis projects for the same patterns. It finds 29 sites, 25 store and 4 monitored:

  • EveryStoreConnectionStringSourcePinsItsSessionTimeZoneToUtc: every site that is not on the named MonitoredTargetSites list goes through the pin. The pin has to be in the same call that opens the connection. The test does not follow a variable back to an earlier line. DarlingWorker.RunCollectionLoopAsync used to pin a variable two lines before it opened its data source. It now pins inside the NpgsqlDataSource.Create call. To prove the test, the lane removed the pin at DarlingCliCommands.cs line 371 and saw a failure naming that site, then put it back.
  • NoMonitoredTargetConnectionStringIsPinned: each of the four monitored sites is found and is not pinned. A pin on one of them breaks the monitored server's clock read, so this test fails on it.

StoreSessionTimeZoneLiveTests needs DARLING_TEST_PG and runs in the live-postgres collection. It creates its own scratch database, dropped at the end, and runs ALTER DATABASE ... SET timezone = 'America/New_York' on it. The test is therefore meaningful on a UTC CI runner too. It checks three things:

  1. A connection without the pin sees SHOW timezone = America/New_York, which proves the setup.
  2. A connection opened the product's way, through the pin, sees SHOW timezone = UTC.
  3. DarlingModuleMap.RefreshSql's predicate, collection_time >= now() - interval '2 days', includes a row 46 hours old and excludes a row 50 hours old. An Assert.Contains pins that exact SQL text. Under America/New_York the stored time reads as younger than it is, so a wrong session only widens the window. One row well inside the window passes under either zone. So the test puts one row on each side of the 48-hour line.

With the pin removed (the helper returning its input), the live test failed with "expected UTC, got America/New_York". With the pin restored, it passed.

StoreConnectionStringAppendPinTests runs the three helpers over five kinds of string. They hold a quoted empty Password, a quoted empty Root Certificate, a password with ;, quotes and =, a trailing ;, or trailing spaces. Each result must start with the caller's string, and every keyword the caller set must still read back with the same value.

Three more tests cover the rest. The pin overrides an existing TIMEZONE in any case. EnsureStoreSearchPath finds an existing SEARCHPATH in any case. Both helpers still throw on a malformed string. With the three old builder versions put back, 13 of 18 cases failed. With the fix restored, all of them passed.

Two old source tests, updated

ViewerCommandTimeoutTests.TheDataService_PublishesTheEffectivePoolSizeItBuildsThePoolOn and DarlingStoreLoginsTests.EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBefore searched the source for the old call text, such as NpgsqlDataSource.Create(effectiveConnectionString). The pin wraps that argument now, so both tests look for the new text. What each test checks is unchanged.

Lite

Lite needs no pin. Its Database project sets no session or connection time zone anywhere. DuckDB's TIMESTAMP carries no zone, and it compares a column with a bound DateTime or with now() as plain values.

Test plan

  • Build Darling/Darling.Tests/Darling.Tests.csproj: 0 warnings, 0 errors.
  • The run before the fix, described above: 13,982 tests, 0 failed.
  • StoreSessionTimeZonePinCensusTests: failed with one pin removed, passed with it restored.
  • StoreSessionTimeZoneLiveTests against a store database at America/New_York: failed with the pin removed, passed with it restored.
  • After a merge of dev and a fresh darlingtest at America/New_York, the full suite found the two old source tests (14,005 tests, 3 failed). After they were updated, a second full run on a fresh database: 14,005 tests, 0 failed, 49 skipped, 1 not run.
  • After the round-1 fixes (ee4de7d7), with no local PostgreSQL: the changed classes ran 173 tests, 0 failed, 1 skipped (the live test). The full suite ran once: 14,023 tests, 0 failed, 740 skipped (the live classes), 1 not run.
  • GitHub Actions CI on ee4de7d7: build, Darling PostgreSQL tests and Lite tests all pass.

CHANGELOG entry

SECTION: Fixed
ENTRY:

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ

erikdarlingdata and others added 5 commits September 25, 2026 09:20
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
…get 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
…tc-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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round-1 security review at 36bde40

Scope: the security questions only, for DarlingStoreConnection.PinSessionTimeZoneUtc and its 25 call sites. The PR tender already checked that each call site is a pure wrap, so I did not repeat that.

Evidence: I read the diff and the call sites at the PR head. I read the Npgsql 10.0.3 source for password, certificate and startup-parameter handling. I also ran a scratch console app against the Npgsql 10.0.3 package. It connects to no server. For each input, it parses the original string and the pinned string, and it compares every typed property and every stored keyword.

No High or Medium findings. There are two Low findings: one in the helper, and one in the census test.

Low 1: the pin erases a keyword that has an explicit empty value

Location: Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs:40-47

The helper parses the string and writes it out again. The writer emits an empty value as Key= with no quotes, and the parser reads Key= as "not set". So Password='' or Password="" comes back with no Password keyword. The harness shows this for every string keyword that accepts an empty value, for example Passfile, SSL Certificate, SSL Key, SSL Password, Root Certificate, Options and Require Auth. Kerberos Service Name='' becomes postgres, and Encoding='' becomes UTF8.

Npgsql 10.0.3 treats an empty value and a missing value differently. Its fallbacks use ??, so an empty value blocks the fallback and a missing value takes it:

  • Password: NpgsqlDataSource.GetPassword returns "". Under password authentication, the server refuses an empty password. A missing password makes NpgsqlConnector.GetPassword use PGPASSWORD, then a pgpass file (Passfile, PGPASSFILE, or %APPDATA%\postgresql\pgpass.conf).
  • Root Certificate with SSL Mode=VerifyCA or VerifyFull: an empty path fails the connection. A missing value uses PGSSLROOTCERT, then %APPDATA%\postgresql\root.crt, then the system certificate store.
  • SSL Certificate and SSL Key: an empty path fails the connection when Npgsql loads a client certificate. A missing value uses PGSSLCERT and PGSSLKEY, then postgresql.crt and postgresql.key in the same folder. The client then presents that certificate.
  • Options: an empty value sends nothing and ignores PGOPTIONS. A missing value sends PGOPTIONS.
  • Require Auth: an empty value allows all methods and ignores PGREQUIREAUTH. A missing value reads PGREQUIREAUTH, so this change can only make the connection stricter.
  • Username and Database: no change, because Npgsql and PostgreSQL treat empty and missing the same way.

Failure sequence:

  1. An operator writes a bring-your-own store string with Password=''. On dev, the CLI verbs fail to connect under password authentication, because the server rejects the empty password.
  2. The service account has PGPASSWORD set, or a pgpass file that matches the host, port, database and user.
  3. With this PR, the sites listed under "Where it is new" log in with that other credential. No log line reports the change.

PGPASSWORD is not tied to a host. If the server asks for cleartext password authentication, Npgsql sends that value to the host that the string names. Without TLS, the value crosses the network in clear text. Root Certificate='' has the same kind of effect on trust. The trust root changes from "none, so fail" to a file or a store that the string does not name.

Why Low: it needs an operator-written quoted-empty value, plus a PG* variable or a default file in the profile of the process. The service does not set these variables in its own environment. It passes PGPASSWORD only to child libpq tools (DarlingStoreUpgrade.cs:3390). Managed-store strings come from NpgsqlConnectionStringBuilder, so they never contain a quoted-empty value.

Where it is new: dev already erases these values where a string goes through a builder before Npgsql. Examples are EnsureStoreSearchPath when the string has no Search Path, and the viewer's ApplyConnectionTimeout when a connect timeout is set. The PR adds the erasure at these sites, where the raw string reached Npgsql before:

  • the CLI verbs
  • StoreConnectionSelfTest
  • the MCP and web hosts, when they use a configured login or the owner login
  • the worker, when the string sets Search Path
  • the viewer, when no connect timeout is set

Fix: parse once to keep the current error behavior, then append the keyword instead of rewriting the string.

public static string PinSessionTimeZoneUtc(string connectionString)
{
    /* Parse once, so a malformed string fails here exactly as the Npgsql constructor would. */
    _ = new NpgsqlConnectionStringBuilder(connectionString);

    /* Append, do not re-serialize: every caller keyword stays byte for byte. The last Timezone wins,
       so a caller's own Timezone still loses to UTC. */
    return connectionString + ";Timezone=UTC";
}

The harness ran this form on 24 inputs: every quoted-empty string keyword, hostile passwords, a caller Timezone, trailing spaces and a trailing ;;. The parsed result matched the input in all 24, and Timezone was UTC in all 24. The other pattern in the repo, the base DbConnectionStringBuilder in ApplyConnectionTimeout, changed 17 of the same 24 inputs, so it is not a fix. We recommend a unit test that checks that Password='' and Root Certificate='' survive the pin. EnsureStoreSearchPath and ApplyConnectionTimeout have the same erasure today, and they can use the same append pattern.

Low 2: the census accepts an indirect pin from anywhere in the file

Location: Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs:173-194 (ArgumentIsPinned)

This is a gap in the guard, not a hole in the product today.

  1. DarlingWorker.cs has two variables named storeConnectionString: a local in ExecuteAsync (line 1362) and the parameter of RunCollectionLoopAsync (line 1728).
  2. If storeConnectionString = DarlingStoreConnection.PinSessionTimeZoneUtc( appears anywhere in the file, the census counts a bare storeConnectionString argument as pinned.
  3. So a new unpinned NpgsqlDataSource.Create(storeConnectionString) in ExecuteAsync passes the census. An unpinned reassignment between line 1741 and the Create call also passes.

Fix: at DarlingWorker.cs:1741-1742, write the pin inline as NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(EnsureStoreSearchPath(storeConnectionString))). The census remarks say that the two calls cannot be one expression, but this expression keeps the search path under the pin. Nothing reads storeConnectionString after line 1742. Then delete the indirect rule, or limit it to the enclosing member, which the scan already computes with CSharpMemberMap.EnclosingMember.

Questions with no finding

Q1, every other keyword. The round trip is exact for every non-empty value. The harness set each keyword property of NpgsqlConnectionStringBuilder: 59 properties and 140 keyword-value pairs, with every enum value. All 140 parsed back the same, and a second pin produced the same string. Require Auth and Target Session Attributes rejected my first test values, so I tested them with valid values, and they parsed back the same.

The set includes Passfile, SSL Mode, Root Certificate, SSL Certificate, SSL Key, SSL Password, Channel Binding, Kerberos Service Name, Include Realm and GSS Encryption Mode. It also includes Require Auth, SSL Negotiation, Target Session Attributes, Load Balance Hosts and a multi-host Host.

Passwords with ;, ', " or = came back unchanged. Both quote styles came back unchanged. So did leading and trailing spaces, tab, newline, U+0085, U+00A0, U+2028, a backslash, braces and non-ASCII text. The writer can change the quote style, but the parsed value stays the same.

The pin overwrites a caller Timezone, as the remarks on the helper say. The pin also wins over PGTZ, because Npgsql reads Settings.Timezone ?? PGTZ. It wins over Options=-c timezone=... too. PostgreSQL applies the startup options switches before the other startup parameters (process_startup_options in postinit.c).

Q2, new exceptions. None. In Npgsql 10.0.3, new NpgsqlConnection(string) and NpgsqlDataSource.Create(string) parse the string in the constructor with the same builder. The harness shows that the constructor throws for each of the eight bad strings in the test. So every string that the pin rejects failed at the same statement before, and the same try and catch apply. Npgsql rejects the libpq spellings before and after the PR, for example sslmode, sslrootcert, sslcert, target_session_attrs, connect_timeout and hostaddr. It also rejects Integrated Security.

The message names only the keyword, in lower case: Couldn't set sslmode (Parameter 'sslmode'). For a typed keyword with a bad value, the inner exception contains that value, for example Requested value 'X' was not found. A malformed string gives only a character index. In the harness, no message and no ToString() contained the password.

One case is older than this PR. If a password contains an unquoted ;, the text after it becomes the keyword. The message repeats that text in lower case. The CLI has no top-level catch, so such an exception prints to stderr. The PR does not change either behavior.

Q3, where the pinned string goes. Only into Npgsql. At 24 sites the pin sits inline in the constructor argument. The worker assigns the pinned string back to storeConnectionString (DarlingWorker.cs:1741), and no code reads it after Create on the next line.

No code in the Darling tree reads back the ConnectionString of a data source or a connection. The postgres.ConnectionString reads in DarlingCliCommands.cs and DarlingStoreLogins.cs read the config object. With the default Persist Security Info=false, NpgsqlDataSource.ConnectionString omits the password, before and after the PR. The viewer still publishes the unpinned string to ViewerStorePool, as before. The live test reads its connection string from DARLING_TEST_PG, so no credential is committed.

Q4, monitored targets and missed store sessions. The pin reaches no monitored target. I searched the whole repository for the four census patterns. I also searched for CloneWith(, NpgsqlSlimDataSourceBuilder, NpgsqlMultiHostDataSource, target-typed new(...) and .ConnectionString =. There are 29 sites, and all 25 store sites are pinned. The four bare sites are the monitored-target sites that the census names: DarlingServerConnector.cs:449, PostgresTargetProvider.cs:31, and DarlingWorker.cs:3823 and 8622.

No store code runs SET TIME ZONE or sets No Reset On Close. The pin cannot move a host onto the owner login. DarlingStoreLogins.AcceptsLoginAsync fails closed and never falls back to the owner, and any role can set TimeZone at connect.

I did not check server-side sessions on a bring-your-own store, such as TimescaleDB background jobs. No client-side pin can reach them.

Q5, the two re-anchored tests. Both still check what they checked before. DarlingStoreLoginsTests.EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBefore still requires the resolved storeConnectionString, not config.Postgres.ConnectionString, for both hosts. It still counts exactly two assignments, and the inline pin does not change that count.

ViewerCommandTimeoutTests.TheDataService_PublishesTheEffectivePoolSizeItBuildsThePoolOn still requires Publish(effectiveConnectionString) before Create, on the same string. Its new comment says that the pin never changes Max Pool Size. The harness shows this for the values 0, 1 and 3.

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round-1 Lows fixed

Pushed to fix/4277-non-utc-store-zone at commit ee4de7d7 (on top of the round-1 review's head 36bde40a).

Low 1: the pin must not rewrite the caller's string

Darling/PerformanceMonitor.Darling.Storage/DarlingStoreConnection.cs:52 (ee4de7d7). PinSessionTimeZoneUtc now parses once (_ = new NpgsqlConnectionStringBuilder(connectionString);), kept only so a malformed string still throws at this statement. It returns connectionString + ";Timezone=UTC" instead of the builder's re-serialized ConnectionString. Doc comment updated to explain why.

I verified this empirically, using dotnet fsi against the same Npgsql 10.0.3 the review used, not just by reading the review. Plain append survives every case the review's fix recommendation named. A trailing ; and trailing spaces both still parse cleanly after the append (Npgsql collapses ;; and trims). An existing TimeZone/TIMEZONE in any case still loses to the appended Timezone=UTC, because Npgsql keeps only the last value for a repeated keyword. No adjustment to the append was needed for any of these, so the fix is exactly the review's suggested body.

Same bug in the two older helpers

  • DarlingWorker.EnsureStoreSearchPath (DarlingWorker.cs:4387, ee4de7d7). When the search path is absent, it now appends ";Search Path=" + DarlingManagedPostgres.SearchPath" to the caller's string, instead of returning builder.ConnectionString. The constant ("collect,config,public") has no ;, = or quote character, so it never needs quoting. The comment says so, rather than adding quoting logic nothing exercises. Detection (NpgsqlConnectionStringBuilder.SearchPath presence, case- and alias-insensitive) is unchanged, and the already-present branch still returns the input verbatim.
  • ViewerDataService.ApplyConnectionTimeout (ViewerDataService.cs:549, ee4de7d7). When the timeout is absent, it now appends ";Timeout=" + timeoutSeconds.ToString(CultureInfo.InvariantCulture), instead of returning the base DbConnectionStringBuilder's round trip. Detection (DbConnectionStringBuilder.ContainsKey("Timeout")) is unchanged. I updated one stale test comment in ViewerDataServiceTests.cs (ApplyConnectionTimeout_ManagedStyleStringWithoutTimeout_SetsThePreference). It used to explain "the reason we detect + emit via the base builder"; emission no longer goes through any builder.

DarlingWorker.cs:1741-1742 (ee4de7d7). This used to be two statements: assign the pin's result back to storeConnectionString, then create the data source from that variable. It is now one: NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(EnsureStoreSearchPath(storeConnectionString))). A grep over the rest of the method and file confirmed nothing reads storeConnectionString after that point, and the census fix below needs the pin inline on the Create line.

Low 2: the census test's indirect-pin rule

Darling/Darling.Tests/StoreSessionTimeZonePinCensusTests.cs (ee4de7d7). I deleted the indirect-pin branch of ArgumentIsPinned, the identifier-tracking regex that accepted a bare storeConnectionString argument as long as PinSessionTimeZoneUtc was assigned to any same-named variable anywhere in the file. The check is now argument.Contains("PinSessionTimeZoneUtc", StringComparison.Ordinal), direct only. I rewrote the class's "Direct and indirect pins" doc paragraph to match.

I confirmed StoreSourceFiles's scan roots (.Storage, .Service, .Viewer, .Analysis) do not include Darling.Tests. The new test file's own new NpgsqlConnection(...) calls could not have fooled this census.

New test: Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs

A [Theory] matrix (quoted-empty Password, double-quoted-empty Root Certificate, a password holding ; ' " =, trailing ;, trailing spaces) runs against all three helpers. It asserts the output starts with the caller's string, and that every keyword the input set parses to the same value in the output.

Three more targeted facts cover the rest. PinSessionTimeZoneUtc overwrites an odd-case existing TIMEZONE. EnsureStoreSearchPath detects an odd-case existing SEARCHPATH and returns the input unchanged. Both PinSessionTimeZoneUtc and EnsureStoreSearchPath still throw ArgumentException on a malformed string. ApplyConnectionTimeout's detection uses the base DbConnectionStringBuilder, which does not validate Npgsql keyword semantics either before or after this change, so I did not add an unfounded throw claim for it. Timeout's odd-case detection was already pinned by the pre-existing ApplyConnectionTimeout_DetectsAnExistingTimeoutCaseInsensitively, so I did not duplicate it.

Revert-proof, done manually before finalizing: I put all three old round-trip bodies back, rebuilt (0 warnings), and ran *StoreConnectionStringAppendPinTests* alone. 13 of 18 cases failed: every explicit-empty-value and special-character-password case across all three helpers, plus some trailing-;/trailing-space cases. I restored the fix, rebuilt (0 warnings), and reran: all pass.

Tests run

  • Darling.Tests.exe -class "*StoreConnectionStringAppendPinTests*" -class "*StoreSessionTimeZonePinCensusTests*" -class "*ViewerCommandTimeoutTests*" -class "*DarlingStoreLoginsTests*" -class "*DarlingWorkerTests*" -class "*ViewerConnectionTimeoutTests*" -class "*ViewerDataServiceTests*" -class "*HeadlessSelfTestTests*" -class "*StoreSessionTimeZoneLiveTests*": Total 173, Errors 0, Failed 0, Skipped 1. The one skip is the live test; this lane has no DARLING_TEST_PG, as expected, since CI runs it.
  • Full suite once, no rig: Total 14023, Errors 0, Failed 0, Skipped 740, Not Run 1. All skips are the live/rig-gated classes (DARLING_TEST_PG/DARLING_TEST_PGRUNTIME unset, per the brief: no rig for this lane). The single "Not Run" predates this change. None of the six files this PR touches gate a test that way, and it fell outside the tail of the run I inspected. Flagging it here for the coordinator to double check, rather than chasing it further under the token budget.
  • Build: Darling/Darling.Tests/Darling.Tests.csproj -c Debug produced Build succeeded, 0 Warning(s), 0 Error(s), confirmed by the summary line each time, not by the piped exit code.
  • I did not run Installer.Tests, per instructions. I did not stand up a PostgreSQL rig; the brief says it is not needed, since CI runs the live classes.

Not done, left as is

  • DarlingStoreLoginsTests.cs and ViewerCommandTimeoutTests.cs were on the brief's list to check for stale pinned text. I read both call-site patterns they assert (NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(storeConnectionString)) and the effectiveConnectionString equivalent) and confirmed they check source-code shape, not output text. They needed no edit.
  • No CHANGELOG edit, no PR body or title edit, no gh pr ready, per the brief.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant