Repository navigation
Report when darling.json disagrees with a REGISTERED server's registry row (#2552) - #2556
Conversation
…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>
| 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); |
There was a problem hiding this comment.
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.
|
Reviewed the diff ( Overall: the field-by-field comparison logic ( One correctness gap worth a look, left as an inline comment: 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>
|
Addressed, though not the way it was suggested — the suggestion would have traded this defect for #2158's.
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 The paused line drops the claim, drops the remedy, and drops the Pinned by 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 ( 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 |
Doc comment only, no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| /// <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", | ||
| }; |
There was a problem hiding this comment.
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.)
| AddDrift( | ||
| drift, | ||
| "database", | ||
| EffectiveDatabase(file.Database, storeIsPostgres), | ||
| EffectiveDatabase(store.Database, storeIsPostgres), | ||
| StringComparison.OrdinalIgnoreCase, | ||
| true); |
There was a problem hiding this comment.
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.
|
Reviewed this PR (C#/Darling only — confirmed Lite has no analogous store-vs-file drift risk, since Overall the logic is careful and well-tested — the reflection-based invariant test (
No SQL injection, secrets-in-logs, or missing-index-DMV concerns — the credential exclusion ( |
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>
|
Both correct, both fixed — and the second one generalizes to a field the comment did not name.
One detail worth flagging, because "collapse
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 What only the inner comparer decides is what the message says. A PostgreSQL store excluding both All four reverted individually and watched go red, then restored: the |
…gs-drift # Conflicts: # CHANGELOG.md
ReviewWent through the diff ( Correctness — Independently re-derived the claims the field-fold comments make against the actual connection builders in
The pairing logic in Nothing in this diff needs Lite/Darling parity — Verified the PR's "Darling only" claim rather than taking it on faith: Lite's per-server settings live solely in Test coverage — No correctness, security, or parity issues found. Nothing to flag as blocking. |
Closes #2552 — with two corrections to the issue's framing, at the bottom.
What was wrong
StoreConfigProvider.WarnAboutFileOnlyServersAsyncreadSELECT server_id, name FROM config_monitored_serversand compared names and ids only, then returned early oncefileOnly.Count == 0. So for a server the store already had, every per-server setting indarling.jsonwas dead text —host,database,auth,username,encryptMode,trustServerCertificate,excludedDatabases,port,engine, the display name. Read, parsed, validated, logged asLoaded 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 saidf, withcreated_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:
WarnAboutFileOnlyServersAsyncis nowWarnAboutFileVersusStoreAsync, 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.
namehostDarlingWorker.ServerDefinitionEqualsdatabasemaster/postgres); case-sensitive on PostgreSQL, case-insensitive on SQL ServerMonitoredServerConnectionsubstitutes; and PostgreSQL matchespg_database.datnamebyte for byte, soReportingDBandreportingdbare two databases thereauthusernameencryptModeStrict≡Mandatoryon PostgreSQLstrictvsStrictis not drift, nor a typo againstMandatory; and the Npgsql builder branches onOPTIONALalone, so the other two are one connection thereengineMonitoredServer.TargetEngineauroraandpostgresare one engineport0→5432host,1433readOnlyIntent,multiSubnetFailovertrustServerCertificateOptionalSslMode.Preferwithout reading the flag; SQL Server'sOptionalstill validates the certificateexcludedDatabasesNOT INset (DatabaseExclusionFilter) — live on both engines, sincePostgresTargetProvider.BuildDatabaseListPlanuses the same filter againstdatnameto pick the per-database fan-outmonthlyCostUsddecimalcarries its scale, so1200and1200.00are one figurealertDeliveryModeOverride(inherit)password/encryptedPasswordcapture_plansis_enabledThe 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. ForencryptModethe displayed value and the compared key are deliberately split: folding the display too would printstore=Mandatoryfor a row that literally saysStrict, i.e. a message inventing a value neither side holds.The credential
encrypted_passwordis 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 anenv:/file:reference (DarlingSecretSource) or a dev plaintext password while the store row carries a DPAPI blob, andBuildServerFromRowbackfills 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
encryptedPasswordcomparison putsenv:ALPHA_MONITOR_PASSWORDand 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-connectioncaveat.Filtering the store read to
is_enabled = TRUEwas the obvious fix and is the one option that must not be taken: that read also feedsServersOnlyInFile, 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, andCompareServerSettings' 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
monthlyCostUsdfrom 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(storedserver_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 oneserver_id(identical addresses, where the seed'sON CONFLICT DO NOTHINGleft a single row) — a guess would print two entries' settings as one server's drift and could contradict itself on the same line. That isBuildServerFromRow's "exactly one match" discipline, for the same reason.The
--test-connectioncaveat — yes, it belongs, but conditionallyAppended 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'sserversblock now carries the seed-once paragraph theweb.network/mcp.networkblocks 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
capturePlansis not a per-server setting.MonitoredServerhas no such property;capturePlansis a top-level service setting. Theconfig_monitored_servers.capture_planscolumn exists but the seed writes itNULL, so there is nothing to compare. Nothing else in the issue's field list is wrong.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 MCPadd_servers".add_serverscannot change a registered server:PartitionDuplicatesskips an already-monitored server asstatus:"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 rulesadd_serversout explicitly. (There is no MCP edit verb at all;remove_server+add_serversis 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:350as theencrypt_modeseed normalizer. That line isNormalizePlanXmlCompression—plan_xml_compression, whose V62 CHECK is case-sensitive.encrypt_modehas 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 throughMonitoredServerConnection's own fail-closed switch rather than through a seed-side normalizer.Verification
Builds on macOS: service, viewer and
Darling.Testsall clean, no new warnings.Pure logic RUN locally, not deferred to CI.
PerformanceMonitor.Darling.Serviceisnet10.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:
env:reference visible in the rendered linereadOnlyIntent/multiSubnetFailovermonthlyCostUsddropped from the comparisonconnectTimeoutSecondsStrict≡MandatorycollapsetrustServerCertificateinert gatedatabaseandexcludedDatabases)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
Scratchandscratchrenders asstore=Scratch, scratchwith the fix andstore=Scratchwithout, which is wrong information about the server in the one sentence the operator gets. Asserting the rendered value is what makes it fail.Darling.Testscannot RUN on macOS (net10.0-windows); the 32 facts + 3 theories (43 cases with theInlineDatarows) inRegisteredServerSettingDriftTestsbuild 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 thebuildcheck is the arbiter of.Not verified at all: the live path. Nothing here ran against a real seeded store, so the
SELECTshape, theMonitoredServerhydration 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'sengine/portand V18'salert_delivery_mode_override, all of whichReadMonitoredServersAsyncalready 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.