Repository navigation
Pin every STORE connection's session time zone to UTC (#4277) - #4285
Conversation
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
Round-1 security review at 36bde40Scope: the security questions only, for 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 valueLocation: The helper parses the string and writes it out again. The writer emits an empty value as Npgsql 10.0.3 treats an empty value and a missing value differently. Its fallbacks use
Failure sequence:
Why Low: it needs an operator-written quoted-empty value, plus a Where it is new: dev already erases these values where a string goes through a builder before Npgsql. Examples are
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 Low 2: the census accepts an indirect pin from anywhere in the fileLocation: This is a gap in the guard, not a hole in the product today.
Fix: at Questions with no findingQ1, every other keyword. The round trip is exact for every non-empty value. The harness set each keyword property of The set includes Passwords with The pin overwrites a caller Q2, new exceptions. None. In Npgsql 10.0.3, The message names only the keyword, in lower case: One case is older than this PR. If a password contains an unquoted 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 No code in the Darling tree reads back the 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 No store code runs 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.
|
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
Round-1 Lows fixedPushed to Low 1: the pin must not rewrite the caller's string
I verified this empirically, using Same bug in the two older helpers
Low 2: the census test's indirect-pin rule
I confirmed New test:
|
Closes #4277.
Why
Every timestamp column in the Darling store is
timestamp without time zoneand holds UTC. A managed store writestimezone = 'UTC'into its ownpostgresql.conf(the v9 block). A bring-your-own store never gets that block. It keeps the zone its owner set, or the zoneinitdbtook from its host.PostgreSQL resolves a comparison between such a column and a
timestamptzvalue in the session's time zone. A barenow()in the SQL is one such value, and so is a boundDateTimewithKind=Utc. So on a store whose session zone is not UTC, a read can shift its window without any error.StoreSqlClockDisciplineTestsrecords 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, inPerformanceMonitor.Darling.Storage, is one new helper. It appends;Timezone=UTCto the caller's connection string and returns it. Npgsql keeps the last value of a repeated keyword, so the pin overwrites anyTimezonethe 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(andNpgsqlConnection(found 25 store sites:DarlingWorker.cs: the worker's store data source, and the viewer-role source for custom alert rules.Mcp/DarlingMcpHostService.csandMcp/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, thepg_hbarule, 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:
runtime.ConnectionStringsites inDarlingWorker.csDarlingServerConnector.csTargets/PostgresTargetProvider.csPgTargetBaselineProvider.Clock.csreads 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 barePassword=, and the builder reads that as "not set". Npgsql then uses thePGPASSWORDenvironment 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.EnsureStoreSearchPathappends;Search Path=collect,config,publicwhen the string has no search path.ViewerDataService.ApplyConnectionTimeoutappends;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
devwithout the pin and ran the full suite once against a store database whose time zone was America/New_York.log_timezonestayed 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
StoreSessionTimeZonePinCensusTestsscans 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 namedMonitoredTargetSiteslist 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.RunCollectionLoopAsyncused to pin a variable two lines before it opened its data source. It now pins inside theNpgsqlDataSource.Createcall. To prove the test, the lane removed the pin atDarlingCliCommands.csline 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.StoreSessionTimeZoneLiveTestsneedsDARLING_TEST_PGand runs in thelive-postgrescollection. It creates its own scratch database, dropped at the end, and runsALTER DATABASE ... SET timezone = 'America/New_York'on it. The test is therefore meaningful on a UTC CI runner too. It checks three things:SHOW timezone= America/New_York, which proves the setup.SHOW timezone= UTC.DarlingModuleMap.RefreshSql's predicate,collection_time >= now() - interval '2 days', includes a row 46 hours old and excludes a row 50 hours old. AnAssert.Containspins 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.
StoreConnectionStringAppendPinTestsruns the three helpers over five kinds of string. They hold a quoted emptyPassword, a quoted emptyRoot 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
TIMEZONEin any case.EnsureStoreSearchPathfinds an existingSEARCHPATHin 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_PublishesTheEffectivePoolSizeItBuildsThePoolOnandDarlingStoreLoginsTests.EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBeforesearched the source for the old call text, such asNpgsqlDataSource.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
Databaseproject sets no session or connection time zone anywhere. DuckDB'sTIMESTAMPcarries no zone, and it compares a column with a boundDateTimeor withnow()as plain values.Test plan
Darling/Darling.Tests/Darling.Tests.csproj: 0 warnings, 0 errors.StoreSessionTimeZonePinCensusTests: failed with one pin removed, passed with it restored.StoreSessionTimeZoneLiveTestsagainst a store database at America/New_York: failed with the pin removed, passed with it restored.devand a freshdarlingtestat 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.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.ee4de7d7: build, Darling PostgreSQL tests and Lite tests all pass.CHANGELOG entry
SECTION: Fixed
ENTRY:
postgresql.conf. The service and the viewer also keep a connection-string setting that has an empty value, such asPassword='', when they add their own settings. Before, they dropped it, and Npgsql then used thePGPASSWORDenvironment variable or a password file if either one was there.REF:
[Pin every STORE connection's session time zone to UTC (#4277) #4285]: Pin every STORE connection's session time zone to UTC (#4277) #4285
🤖 Generated with Claude Code
https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ