Skip to content

Report when darling.json disagrees with a REGISTERED server's registry row (#2552) - #2556

Merged
erikdarlingdata merged 5 commits into
devfrom
fix/2552-server-settings-drift
Aug 23, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
fix/2552-server-settings-drift

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Aug 23, 2026 •

Copy link
Copy Markdown
Owner

Closes #2552 — with two corrections to the issue's framing, at the bottom.

What was wrong

StoreConfigProvider.WarnAboutFileOnlyServersAsync read SELECT server_id, name FROM config_monitored_servers and compared names and ids only, then returned early once fileOnly.Count == 0. So for a server the store already had, every per-server setting in darling.json was dead text — host, database, auth, username, encryptMode, trustServerCertificate, excludedDatabases, port, engine, the display name. Read, parsed, validated, logged as Loaded configuration … 1 server(s), and then not used.

The reproduction in the issue is the loop that makes it expensive: a PostgreSQL target refused a self-signed certificate, the operator applied the documented fix ("trustServerCertificate": true), restarted, and got a byte-identical error — the store row still said f, with created_at == modified_at. A connection failure is exactly the class of problem an operator fixes by editing config and restarting, and this was the one class of edit that produced an unchanged error with no explanation.

#2252/#2254 already warn — well — about the adjacent case, and that warning made this one worse: it teaches "adding a server to the file does not register it", from which the natural inference is that the file still drives the servers the store already knows about.

The store still wins. Store-authoritative is deliberate (#2254) and none of the ordering changed. The defect was that the disagreement was invisible.

What ships

One new WARNING, once per start, naming every registered server whose file entry disagrees with its registry row, every field, and both values:

darling.json disagrees with the registry about 1 monitored server(s), and the registry is what the
service uses: pgtarget: trustServerCertificate (file=true, store=false). The store is authoritative
after the first seed, so editing a registered server's settings in the file changes nothing and a
restart cannot change that — edit them in the Viewer's Manage Servers window (the MCP add_servers
tool cannot: an already-registered server is skipped as a duplicate). Note --test-connection reads
darling.json, so it probes the FILE's settings and can report PASS for a connection the service will
never make.

WarnAboutFileOnlyServersAsync is now WarnAboutFileVersusStoreAsync, one store read serving both causes, with the file-only lines split into their own method so the drift pass is no longer gated on there being anything to say about cause A.

Which fields count, and the fold each one runs through

Every fold below is a real one somewhere else in the service, not a convenience. A check that warns about differences that do not exist is the same silence with extra steps.

field fold why
name blank → host, then case-sensitive the store's spelling is what the Viewer and every alert render
host trim, case-insensitive matches DarlingWorker.ServerDefinitionEquals
database blank → the engine's implicit default (master / postgres); case-sensitive on PostgreSQL, case-insensitive on SQL Server what MonitoredServerConnection substitutes; and PostgreSQL matches pg_database.datname byte for byte, so ReportingDB and reportingdb are two databases there
auth trim, case-insensitive validation already restricts it to integrated/sql
username only when the store row uses SQL auth the only case it reaches the connection string
encryptMode the connect path's fail-closed fold (trim, upper, unknown → Mandatory), plus Strict ≡ Mandatory on PostgreSQL the hazard the issue named — strict vs Strict is not drift, nor a typo against Mandatory; and the Npgsql builder branches on OPTIONAL alone, so the other two are one connection there
engine MonitoredServer.TargetEngine aurora and postgres are one engine
port PostgreSQL only, 0 → 5432 SQL Server carries its port in the host as host,1433
readOnlyIntent, multiSubnetFailover SQL Server only SqlClient concepts the Npgsql builder never sees
trustServerCertificate everywhere except PostgreSQL + a stored Optional that branch picks SslMode.Prefer without reading the flag; SQL Server's Optional still validates the certificate
excludedDatabases trim, drop blanks, de-dupe, order; case follows the engine spliced as a NOT IN set (DatabaseExclusionFilter) — live on both engines, since PostgresTargetProvider.BuildDatabaseListPlan uses the same filter against datname to pick the per-database fan-out
monthlyCostUsd numeric decimal carries its scale, so 1200 and 1200.00 are one figure
alertDeliveryModeOverride null = (inherit) #1236
password / encryptedPassword never read, never compared see below
capture_plans not read no per-server file counterpart to disagree with
is_enabled read, never compared no file counterpart either; it decides which of the two report lines a server lands on — see below

The engine gates read the store's engine (and its stored encryptMode/auth), because the store is what the service connects with; a disagreement about the engine itself is reported on its own line.

Four of those folds are engine-asymmetric, all four found in review by asking what the message claims rather than what the code does. The first two would have been false positives (encryptMode, trustServerCertificate); the last two false negatives (database, excludedDatabases) — the silent direction, which is what #2552 is about, so those were the more expensive half. For encryptMode the displayed value and the compared key are deliberately split: folding the display too would print store=Mandatory for a row that literally says Strict, i.e. a message inventing a value neither side holds.

The credential

encrypted_password is not in the SELECT list. That is structural rather than editorial — there is no blob in memory for a later edit to this file to leak into a log line. It must not be compared even if it were read: a file entry legitimately carries an env:/file: reference (DarlingSecretSource) or a dev plaintext password while the store row carries a DPAPI blob, and BuildServerFromRow backfills exactly that pairing at read time. That is the supported shape, so comparing them would report a working configuration as drift on every start.

Proven by reverting: adding an encryptedPassword comparison puts env:ALPHA_MONITOR_PASSWORD and a DPAPI blob straight into the rendered line, and the guard goes red.

A paused server gets its own sentence (review finding)

A control-plane-disabled server is still registered, so it still pairs and its drift is still real — but "the registry is what the service uses" is not a true sentence about a server nothing is connecting to. It is reported on its own line, at Information, which is the same two-line shape cause A already uses for "never monitored" (warning) versus "deliberately removed" (information). That line drops the claim, the remedy and the --test-connection caveat.

Filtering the store read to is_enabled = TRUE was the obvious fix and is the one option that must not be taken: that read also feeds ServersOnlyInFile, whose question is whether a file entry is registered — and a paused server is — so filtering there would report it as never-monitored and advise re-adding it, which is the #2158 defect. Dropping it from the drift pass silently is rejected more narrowly, since #2552 is a defect about silence being expensive and the drift is exactly what the operator walks back into on re-enable.

The invariant — the category, not the instance

#2552 is not "trustServerCertificate was not compared"; it is "a per-server darling.json setting that nothing compares is silently dead text". So the covered set is derived from both ends rather than listed: MonitoredServer's [JsonPropertyName] keys on one side, and CompareServerSettings' own output on the other — driven with two entries that differ in every field, twice, because two fields are engine-gated and one pass can only see one side of that gate. Field labels are the darling.json keys spelled exactly, so there is no mapping table in between to drift.

Today: 16 keys, 14 compared, 2 excluded (the credential, asserted as such). It fails in both directions — a key nothing compares, and a field label that is not a real key. Proved red by dropping monthlyCostUsd from the comparison, and by adding a new per-server key nothing compares (uncovered=[connectTimeoutSeconds]) — the future defect, caught the day it lands.

Pairing

Same either-or key as ServersOnlyInFile (stored server_id, then the display name case-folded), with one rule on both arms: the match must be unambiguous in both directions or the entry is skipped rather than guessed at. Nothing enforces display-name uniqueness, and two file entries can derive one server_id (identical addresses, where the seed's ON CONFLICT DO NOTHING left a single row) — a guess would print two entries' settings as one server's drift and could contradict itself on the same line. That is BuildServerFromRow's "exactly one match" discipline, for the same reason.

The --test-connection caveat — yes, it belongs, but conditionally

Appended only when a connection-relevant field drifted. That verb probes the file's connection settings, so it can report PASS for a connection the service will never make — which is precisely the reproduction. But it does not exercise the display name, the exclusion list, the cost figure or the delivery override, and a warning must not claim more than it can support.

Docs

darling.sample.json's servers block now carries the seed-once paragraph the web.network / mcp.network blocks have always had. The issue is right that the server list — far more likely to be edited than either — said nothing.

Two corrections to the issue

  1. capturePlans is not a per-server setting. MonitoredServer has no such property; capturePlans is a top-level service setting. The config_monitored_servers.capture_plans column exists but the seed writes it NULL, so there is nothing to compare. Nothing else in the issue's field list is wrong.
  2. The remedy cannot include add_servers. The issue (and the obvious reading of the [QUESTION] Add new server to monitoring #2252 message) suggests "the Viewer's Add/Edit Server dialog, or MCP add_servers". add_servers cannot change a registered server: PartitionDuplicates skips an already-monitored server as status:"duplicate" before it validates anything. Pointing an operator there would send them somewhere that silently does nothing — the exact failure this warning exists to end. The message names the Viewer's Manage Servers window and rules add_servers out explicitly. (There is no MCP edit verb at all; remove_server + add_servers is the only headless path, and it is a heavier hammer than this warning should recommend.)

One thing worth recording separately, not fixed here: the issue cites StoreConfigProvider.cs:350 as the encrypt_mode seed normalizer. That line is NormalizePlanXmlCompression — plan_xml_compression, whose V62 CHECK is case-sensitive. encrypt_mode has no CHECK and the seed writes it raw (line 512). The hazard is real but lives at the connect path, not the seed, which is why the comparison folds through MonitoredServerConnection's own fail-closed switch rather than through a seed-side normalizer.

Verification

  • Builds on macOS: service, viewer and Darling.Tests all clean, no new warnings.

  • Pure logic RUN locally, not deferred to CI. PerformanceMonitor.Darling.Service is net10.0, so a throwaway console harness referencing the real project executed the same 57 assertions against the shipped build: all pass.

  • Every guard proven RED first, reverting only the thing under test, then restored green. Eighteen reverts, each failing exactly the checks that name it and no others:

    reverted went red
    the whole per-field comparison (Editing a registered server's settings in darling.json is silently ignored — only ADDING a server is warned about #2552 as filed) 16 checks, including the field report
    the encryptMode fail-closed fold 6
    the blank-database engine default 4
    the port-0-means-5432 fold 1
    the engine-alias fold 5
    the excludedDatabases set fold 1
    the credential exclusion 2 — with the blob and the env: reference visible in the rendered line
    the engine gate on readOnlyIntent/multiSubnetFailover 2
    the username-only-under-SQL-auth gate 1
    the ambiguous-display-name skip 2
    the id-arm ambiguity guard 1 — two contradictory lines for one server
    the display-budget truncation 1
    the enablement carried onto the drift entry 1
    monthlyCostUsd dropped from the comparison 2, including the invariant naming it
    a new per-server key added and never compared 1, the invariant naming connectTimeoutSeconds
    the PostgreSQL Strict≡Mandatory collapse 1
    the trustServerCertificate inert gate 1
    the engine-sensitive name comparison 2 (database and excludedDatabases)
    the engine-sensitive exclusion-set comparer 1 — and this one had to be rebuilt to bite

    That last row is the one worth reading. Reverting the exclusion set's inner comparer alone left everything green, because the outer comparison catches a case difference either way — a test that passes with and without the fix is worse than none. What only the comparer decides is what the message says: a PostgreSQL store excluding both Scratch and scratch renders as store=Scratch, scratch with the fix and store=Scratch without, which is wrong information about the server in the one sentence the operator gets. Asserting the rendered value is what makes it fail.

  • Darling.Tests cannot RUN on macOS (net10.0-windows); the 32 facts + 3 theories (43 cases with the InlineData rows) in RegisteredServerSettingDriftTests build here and execute in CI. The harness above covers the same assertions against the same code, so the logic is verified — what is not verified locally is that xUnit discovers and runs them, which the build check is the arbiter of.

  • Not verified at all: the live path. Nothing here ran against a real seeded store, so the SELECT shape, the MonitoredServer hydration from the reader and the log line as it actually renders through the service's logger are unexercised outside CI's live-Postgres suite. The SQL selects only columns that exist in the V17 table plus V68's engine/port and V18's alert_delivery_mode_override, all of which ReadMonitoredServersAsync already reads at the same ordinals.

No schema change, no migration rung, no store write. Darling only — Lite has no control-plane store to disagree with.

…y row (#2552)

WarnAboutFileOnlyServersAsync compared server_ids and names and nothing
else, then returned early once every file server was registered. So for a
server the store already had, every per-server setting in darling.json was
dead text: host, database, auth, username, encryptMode,
trustServerCertificate, excludedDatabases, port, engine, the display name.
Read, parsed, validated, logged as loaded, and then not used.

The field report is the loop that makes it expensive: a PostgreSQL target
refused a self-signed certificate, the operator set
"trustServerCertificate": true, restarted, and got a byte-identical error
because the store row still said false. #2252/#2254's warning made it
worse by teaching "adding a server to the file does not register it",
which invites the wrong inference about the servers it already knows.

The store still wins -- store-authoritative is deliberate and none of it
changed. The defect was that the disagreement was invisible.

The comparison runs through the same folds the connect path and the
collectors apply, so it cannot warn about a difference that does not
exist: encryptMode through the connection builder's fail-closed fold,
engine through TargetEngine, a blank database through the engine's
implicit default, PostgreSQL port 0 as 5432, excludedDatabases as the
NOT IN set the collectors splice, monthlyCostUsd numerically. Fields that
are inert on the target are not compared at all, gated on the STORE's
engine because that is what the service connects with.

No credential is compared or printed, by construction: encrypted_password
is not in the SELECT list. It must not be compared anyway -- a file entry
legitimately carries an env:/file: reference or a dev plaintext password
while the store row carries a DPAPI blob, which is the supported shape the
read-time backfill exists to serve.

The remedy names the Viewer's Manage Servers window and rules OUT
add_servers, which skips an already-monitored server as a duplicate before
it validates anything. The --test-connection caveat is appended only when
a connection-relevant field drifted.

darling.sample.json's servers block gains the seed-once paragraph the
web.network / mcp.network blocks have always had.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines +330 to +334
using var command = new NpgsqlCommand(@"
SELECT server_id, name, host, database, auth, username, encrypt_mode, trust_server_certificate,
read_only_intent, multi_subnet_failover, excluded_databases, monthly_cost_usd,
alert_delivery_mode_override, engine, port
FROM config_monitored_servers", connection);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadRegisteredServersForComparisonAsync reads every row in config_monitored_servers with no WHERE is_enabled = TRUE filter, unlike ReadMonitoredServersAsync (line 1299) which the live connect path uses. That means a control-plane-disabled server — one an operator paused from the Viewer — is still included in the drift comparison.

The warning text this feeds says: "the registry is what the service uses ... editing a registered server's settings in the file changes nothing and a restart cannot change that." For a disabled server that's not true — the service isn't connecting to it at all, so neither the file nor the store setting is "in use." An operator who disabled a server and left a stale/divergent darling.json entry for it (plausible, since the file is otherwise vestigial post-seed) will get this warning on every start for a server nobody is monitoring.

Given how much care went into suppressing every other false-positive here (casing, engine aliases, port defaults, cost-figure scale, credential exclusion), this looks like an unintentional gap rather than a deliberate choice — it isn't mentioned in the PR description's "which fields count" table, and there's no test covering it (understandably, since it's a SQL-level filter rather than something CompareServerSettings/DescribeSettingDrift can exercise). Worth adding WHERE is_enabled = TRUE here too, or explicitly deciding disabled servers should still be flagged and adjusting the message wording accordingly.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Reviewed the diff (StoreConfigProvider.cs, the new RegisteredServerSettingDriftTests.cs, darling.sample.json, CHANGELOG.md). This is C#/Darling-only (no T-SQL), so the T-SQL style rules don't apply here, and the PR's claim that Lite has no counterpart to update checks out — Lite has no control-plane store to disagree with.

Overall: the field-by-field comparison logic (CompareServerSettings/DescribeSettingDrift) is careful and well-tested — I checked each fold (encryptMode, database default, port default, engine aliases, excludedDatabases set semantics, trustServerCertificate) against the actual connect-path logic in MonitoredServerConnection.cs and they match. The credential exclusion is structural (not in the SELECT list) as claimed. The id/name pairing's ambiguity guards look correct and are covered by tests (AnAmbiguousDisplayName_IsSkippedRatherThanGuessedAt, TwoFileEntriesDerivingOneServerId_AreSkippedToo, etc).

One correctness gap worth a look, left as an inline comment: ReadRegisteredServersForComparisonAsync's query has no WHERE is_enabled = TRUE, unlike the live connect path's ReadMonitoredServersAsync. So a control-plane-disabled server can still trigger the new drift warning, whose text asserts "the registry is what the service uses" — which isn't true for a server the service isn't connecting to at all. Given how thoroughly this PR suppresses every other false-positive shape, this one looks like an oversight rather than a decision.

No security, SQL-injection, or parity issues found.

…erived

Review finding on #2556: ReadRegisteredServersForComparisonAsync reads
every config_monitored_servers row with no is_enabled filter, so a server
an operator paused from the Viewer was warned about with a claim that is
not true of it -- "the registry is what the service uses" says nothing
useful about a server nothing is connecting to.

The suggested fix, filtering the read to is_enabled = TRUE, is the one
option that must not be taken: that read also feeds ServersOnlyInFile,
whose question is whether a file entry is REGISTERED, and a paused server
is. Filtering there would report it as never-monitored and advise
re-adding it -- the #2158 defect this method was already fixed once to
avoid. Dropping it from the drift pass silently is rejected more narrowly:
#2552 is a defect about silence being expensive, and the drift is exactly
what the operator walks back into when they re-enable.

So the row carries its enablement (a new RegisteredServer record) and the
drift report splits into two lines -- the same two-line shape cause A
already uses for "never monitored" versus "deliberately removed". The
Information line for a paused server drops the claim, the remedy and the
--test-connection caveat.

Also, the category rather than the instance. Field labels are now the
darling.json keys spelled exactly (displayName -> name), which lets a new
pin DERIVE the covered set from both ends: MonitoredServer's
[JsonPropertyName] properties on one side, and CompareServerSettings' own
output when driven with two entries differing in every field on the other
-- twice, because two fields are engine-gated. 16 keys, 14 compared, 2
excluded, and only the credential is excluded. A per-server setting added
to darling.json later is compared or turns the pin red naming itself, so
it cannot go back to being silently dead text.

Proven red by reverting each: dropping monthlyCostUsd from the comparison,
and adding a new per-server key nothing compares.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Addressed, though not the way it was suggested — the suggestion would have traded this defect for #2158's.

WHERE is_enabled = TRUE on that read is the one option that must not be taken. ReadRegisteredServersForComparisonAsync is a shared read: it also builds the storeIds / storeNames sets that feed ServersOnlyInFile, whose question is "is this file entry registered at all" — and a paused server is. Filtering there would report a server the operator deliberately disabled as "NOT monitored and never have been", and advise re-adding it. That is precisely the #2158 failure this method was already fixed once to avoid, on every start, about a server the operator had just paused on purpose.

Suppressing it from the drift pass only was the other candidate, and it is rejected more narrowly: #2552 is a defect about silence being expensive, so answering it by adding a new silence is the wrong direction — and the drift is exactly what the operator walks back into the moment they re-enable.

What was genuinely wrong is the claim, which is the sharp half of the finding: "the registry is what the service uses" is not a true sentence about a server nothing is connecting to. So the row now carries its enablement (a new RegisteredServer record) and the report splits into two lines — the same two-line shape cause A already uses for "never monitored" (warning) versus "deliberately removed" (information):

[Information] darling.json also disagrees with the registry about 1 server(s) that are registered but
DISABLED: alpha: trustServerCertificate (file=true, store=false). Nothing is connecting to them, so
neither value is in force today — but the registry's is the one that would take effect if they were
re-enabled from the Viewer, not the file's.

The paused line drops the claim, drops the remedy, and drops the --test-connection caveat. is_enabled is read but never compared — it has no darling.json counterpart; it only decides which sentence a server belongs under. A store mid-migration with a NULL there reads as enabled, which is the direction that keeps the drift visible.

Pinned by ADisabledServerStillPairs_ButIsReportedSeparately, and proved red by hardcoding the flag to true at the point it is carried onto the drift entry.


And, since the review found this by asking what the message CLAIMS rather than what the code does, I took the same question one level up. #2552 is not "trustServerCertificate was not compared" — it is "a per-server darling.json setting that nothing compares is silently dead text". Fixing the reported field leaves the next one to be found the same way, in the field, by an operator staring at an unchanged error.

So the covered set is now derived from both ends rather than listed. Field labels are the darling.json keys spelled exactly (displayName → name), which lets EveryPerServerDarlingJsonKeyIsEitherComparedOrDeliberatelyExcluded take MonitoredServer's [JsonPropertyName] properties on one side and CompareServerSettings' own output on the other — driven with two entries differing in every field, twice, because two fields are engine-gated and one pass can only see one side of that gate. No mapping table in between, and the shipped function is the evidence rather than a reimplementation that could keep agreeing while the code drifts.

Today: 16 keys, 14 compared, 2 excluded, and the 2 are the credential, asserted as such. It fails in both directions — a key nothing compares, and a field label that is not a real key (which would send an operator to edit something that is not there).

Proved red twice: dropping monthlyCostUsd from the comparison (uncovered=[monthlyCostUsd]), and adding a new per-server key nothing compares (uncovered=[connectTimeoutSeconds]) — which is the future defect, caught the day it lands instead of in the field.

Doc comment only, no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines +731 to +740
/// <summary>
/// The encrypt mode the connection is actually built with — <see cref="MonitoredServerConnection"/>'s
/// fail-closed fold, in canonical casing so the log prints the mode in force rather than what was typed.
/// </summary>
private static string EffectiveEncryptMode(string? mode) => mode?.Trim().ToUpperInvariant() switch
{
"STRICT" => "Strict",
"OPTIONAL" => "Optional",
_ => "Mandatory",
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EffectiveEncryptMode folds encryptMode the same way for both engines, but the two connection builders don't treat the three spellings the same way.

MonitoredServerConnection.BuildConnectionString (SQL Server path) maps to three distinct SqlConnectionEncryptOption values — Strict, Mandatory, Optional really are three different behaviors there, so folding casing/typos while keeping the three canonical buckets is correct.

BuildPostgresConnectionString, though, only branches on == "OPTIONAL":

SslMode = server.EncryptMode?.Trim().ToUpperInvariant() == "OPTIONAL"
    ? SslMode.Prefer
    : server.TrustServerCertificate ? SslMode.Require : SslMode.VerifyFull,

"Strict" and "Mandatory" (and any unrecognized value) collapse to the same SslMode on a Postgres target. So if a registered Postgres server's file entry says "encryptMode": "Strict" while the store row says "Mandatory", this comparison reports encryptMode drift with AffectsConnection = true — pulling in the --test-connection caveat — even though the actual connection the service makes is identical either way.

That's a false positive against this PR's own stated principle ("the comparison runs through the same folds the connect path... applies, because a check that warns about differences that do not exist is the same silence with extra steps"). Worth gating this fold on the store's engine the same way port/readOnlyIntent/multiSubnetFailover already are, e.g. collapsing Strict→Mandatory when store.IsPostgres.

(Same root cause bites trustServerCertificate too: on Postgres with EncryptMode == "Optional", TrustServerCertificate is entirely inert — SslMode.Prefer regardless of its value — yet it's compared unconditionally.)

Comment on lines +626 to +632
AddDrift(
drift,
"database",
EffectiveDatabase(file.Database, storeIsPostgres),
EffectiveDatabase(store.Database, storeIsPostgres),
StringComparison.OrdinalIgnoreCase,
true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, lower-confidence follow-on to the encryptMode comment: database is compared with StringComparison.OrdinalIgnoreCase unconditionally, but BuildPostgresConnectionString passes server.Database to Npgsql verbatim, and PostgreSQL matches the startup packet's database name against pg_database.datname case-sensitively (unlike SQL Server's typically case-insensitive default collation on InitialCatalog). So a Postgres registered server whose file entry differs from the store only in database-name casing ("ReportingDB" vs "reportingdb") would be reported as "not drift" here, even though that casing difference can matter for what actually connects on that engine. Might be worth an engine-gated comparison (case-insensitive for SQL Server, ordinal for Postgres), same shape as the other engine-gated folds in this method.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Reviewed this PR (C#/Darling only — confirmed Lite has no analogous store-vs-file drift risk, since Lite/Services/ServerManager.cs treats servers.json itself as the mutable store with no seed-once/authoritative-store split, so the "Darling only" claim in the PR body checks out).

Overall the logic is careful and well-tested — the reflection-based invariant test (EveryPerServerDarlingJsonKeyIsEitherComparedOrDeliberatelyExcluded) is a nice guard against the exact regression class this PR fixes. Left two inline notes on CompareServerSettings:

  1. EffectiveEncryptMode (the main finding) folds encryptMode identically for both engines, but BuildPostgresConnectionString only distinguishes "Optional" from everything else — "Strict" and "Mandatory" produce the same SslMode on Postgres, unlike SQL Server where they're genuinely distinct SqlConnectionEncryptOption values. So a Postgres server can get a false-positive encryptMode drift line (plus the --test-connection caveat), contradicting the PR's own stated principle that the comparison should fold through the same logic the connect path uses. The same root cause makes trustServerCertificate inert-but-still-compared when a Postgres store row's encryptMode is Optional.
  2. database comparison is case-insensitive for both engines, but Postgres matches database names case-sensitively at connect time (unlike SQL Server's typically CI collation), so a real case-only divergence on a Postgres target could be silently treated as non-drift. Lower confidence / narrower edge case than Add check for updates feature #1.

No SQL injection, secrets-in-logs, or missing-index-DMV concerns — the credential exclusion (encrypted_password never in the SELECT list) is a good structural guarantee. No parity drift found beyond the two notes above.

Two review findings on #2556, both correct, and one of them generalizes to
a field the review did not name.

encryptMode: SQL Server really does have three behaviours (three
SqlConnectionEncryptOption values), but BuildPostgresConnectionString
branches on OPTIONAL alone -- Strict and Mandatory take the same SslMode
arm. So on a PostgreSQL target they are ONE connection, and reporting them
as drift with AffectsConnection=true (pulling in the --test-connection
caveat) reported a connection difference that cannot exist. The comparison
collapses them there. Display and comparison are split for this one field
so the message still prints what each side actually says rather than a
value neither holds.

trustServerCertificate: on a PostgreSQL target whose STORED mode is
Optional, SslMode.Prefer is chosen without ever reading the flag, so it is
inert. Gated on the store's mode, since the store's is the one in force.
It stays live on SQL Server's Optional, where SqlClient still validates
the certificate if the server negotiates encryption.

database, and -- the generalization -- excludedDatabases: PostgreSQL
matches the startup packet's database against pg_database.datname byte for
byte, so ReportingDB and reportingdb are two databases there and folding
case MISSES a real difference. The exclusion list is live on BOTH engines:
the SQL Server collectors splice DatabaseExclusionFilter against d.name,
and PostgresTargetProvider.BuildDatabaseListPlan splices the identical
filter against datname to choose the per-database fan-out, where NOT IN is
case-sensitive. Both are now engine-gated; SQL Server keeps the
case-insensitive comparison ServerDefinitionEquals assumes, with the
collation limit stated.

The first two would have been false positives, the last two false
negatives -- the silent direction this whole change is about.

Each proven red by reverting it alone. The exclusion set's INNER comparer
needed the test rebuilt: the outer comparison catches a case difference
either way, so the first attempt passed with and without the fix. What
only the comparer decides is whether a store excluding both Scratch and
scratch is RENDERED as excluding one, and asserting the rendered value is
what makes it bite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Both correct, both fixed — and the second one generalizes to a field the comment did not name.

encryptMode on PostgreSQL. Confirmed against BuildPostgresConnectionString: it branches on == "OPTIONAL" and everything else takes the same SslMode arm, so Strict and Mandatory are one connection there. Reporting them as drift with AffectsConnection = true — pulling in the --test-connection caveat — was reporting a connection difference the builder cannot express, against this PR's own stated principle. The comparison now collapses them on a PostgreSQL target.

One detail worth flagging, because "collapse Strict→Mandatory" has a trap in it: display and comparison are now split for this one field. Folding the printed value too would have shown store=Mandatory for a store row that literally says Strict — a message inventing a value neither side holds. So the key is collapsed and the words are not:

encryptMode (file=Optional, store=Strict)      ← on a PG target, compared as Optional vs Mandatory

trustServerCertificate — the parenthetical. Also right, and gated on the store's mode, since the store's is the one in force: on PostgreSQL with a stored Optional, SslMode.Prefer is chosen without ever reading the flag. It stays live on SQL Server's Optional, where SqlClient still validates the certificate if the server negotiates encryption — so this is narrower than "Optional means inert".

database case — and excludedDatabases with it. Right, and not as low-confidence as the comment hedged. The generalization is the part worth having: excludedDatabases is live on PostgreSQL too. PostgresTargetProvider.BuildDatabaseListPlan splices the same DatabaseExclusionFilter against pg_database.datname to choose the per-database fan-out, and NOT IN is case-sensitive there — so folding case would treat two genuinely different exclusions as one. Both fields are now engine-gated: Ordinal on PostgreSQL, OrdinalIgnoreCase on SQL Server (matching ServerDefinitionEquals, with the case-sensitive-server-collation limit stated in the doc rather than left implicit).

Worth naming the direction, since it flips: the first two findings were false positives. The last two were false negatives — the silent direction, which is the one #2552 is about, so those were the more expensive half.


One of the four guards nearly shipped without biting, which is the part I want on the record. Reverting the exclusion set's inner comparer alone left everything green: the outer comparison catches "Scratch" vs "scratch" whether or not the set was de-duplicated case-sensitively, so the first test passed with and without the fix — worse than no test, because it converts an open question into false confidence.

What only the inner comparer decides is what the message says. A PostgreSQL store excluding both Scratch and scratch renders as store=Scratch, scratch with it and store=Scratch without — wrong information about the server, in the one sentence the operator gets. Asserting the rendered value is what makes it bite:

======== REVERTED: the engine-sensitive exclusion-set comparer ========
FAIL  both case variants survive the postgres exclusion set
      -> excludedDatabases(file=scratch,store=Scratch,conn=False)

All four reverted individually and watched go red, then restored: the Strict/Mandatory collapse (1), the trustServerCertificate gate (1), the engine-sensitive name comparison (2 — database and excludedDatabases together), and the set comparer (1). Local harness against the shipped build is now 57 assertions, all passing; the xUnit file is 32 facts + 3 theories.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review

Went through the diff (StoreConfigProvider.cs, RegisteredServerSettingDriftTests.cs, darling.sample.json, CHANGELOG.md) against correctness, Lite/Darling parity, security, and performance.

Correctness — Independently re-derived the claims the field-fold comments make against the actual connection builders in MonitoredServerConnection.cs:

  • SQL Server's Encrypt really does take three distinct SqlConnectionEncryptOption values (line 49-54), while BuildPostgresConnectionString's SslMode branches only on == "OPTIONAL" (line 108-110) — confirms Strict/Mandatory are genuinely one bucket on Postgres and the EncryptModeConnectKey collapse is correct, not just asserted.
  • TrustServerCertificate is only read in the non-OPTIONAL branch of the Postgres builder — confirms the inertness gate on stored Optional mode is accurate.
  • Default database (master/postgres) and default Postgres port (5432, if (server.Port > 0)) match EffectiveDatabase/EffectivePostgresPort exactly.
  • PostgresTargetProvider.BuildDatabaseListPlan does splice DatabaseExclusionFilter against pg_database.datname, confirming excludedDatabases is live on both engines as claimed.

The pairing logic in DescribeSettingDrift (id-arm requiring both-sides-unique, name-arm requiring both-sides-unique, "no match → skip rather than guess") is sound and mirrors ServersOnlyInFile's existing either-or discipline. The credential exclusion is structural (encrypted_password never leaves the SELECT list in ReadRegisteredServersForComparisonAsync), so there's no path for a DPAPI blob or env:/file: reference to reach the log line. The disabled-server split (own line, Information, no remedy/--test-connection claim) correctly avoids the #2158 regression the review comment on the branch already flagged.

Nothing in this diff needs OPTION(RECOMPILE) or table/column aliasing per the T-SQL style guide — no T-SQL is touched; this is C# + a Postgres-side read with no interpolated SQL (safe from injection).

Lite/Darling parity — Verified the PR's "Darling only" claim rather than taking it on faith: Lite's per-server settings live solely in servers.json via ServerManager.LoadServers/SaveServers (Lite/Services/ServerManager.cs), with every UI write round-tripping straight back to that one file. DuckDB's servers table (Lite/Database/Schema.cs) only mirrors id/name/enabled plus two collector-observed facts — it holds none of the connection-relevant fields Darling's config_monitored_servers does. There's no seed-once/store-wins pattern in Lite, so there's no equivalent drift for this diagnostic to catch. No Lite-side counterpart is missing.

Test coverage — RegisteredServerSettingDriftTests.cs is unusually thorough (32 facts + 3 theories) and includes a reflection-driven invariant test (EveryPerServerDarlingJsonKeyIsEitherComparedOrDeliberatelyExcluded) that will fail loudly if a future MonitoredServer property is added without being compared or explicitly excluded — good insurance against this exact class of regression recurring.

No correctness, security, or parity issues found. Nothing to flag as blocking.

@erikdarlingdata
erikdarlingdata merged commit cc7120b into dev Aug 23, 2026
6 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/2552-server-settings-drift branch September 12, 2026 20:29
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